Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tokenfactory: Add Before send hooks #4382

Merged
merged 4 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions app/keepers/keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ import (
"github.com/cosmos/cosmos-sdk/x/upgrade"
upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
icq "github.com/strangelove-ventures/async-icq/v4"
icqtypes "github.com/strangelove-ventures/async-icq/v4/types"

downtimedetector "github.com/osmosis-labs/osmosis/v14/x/downtime-detector"
downtimetypes "github.com/osmosis-labs/osmosis/v14/x/downtime-detector/types"
"github.com/osmosis-labs/osmosis/v14/x/gamm"
Expand All @@ -44,8 +47,6 @@ import (
ibchooks "github.com/osmosis-labs/osmosis/x/ibc-hooks"
ibchookskeeper "github.com/osmosis-labs/osmosis/x/ibc-hooks/keeper"
ibchookstypes "github.com/osmosis-labs/osmosis/x/ibc-hooks/types"
icq "github.com/strangelove-ventures/async-icq/v4"
icqtypes "github.com/strangelove-ventures/async-icq/v4/types"

icahost "github.com/cosmos/ibc-go/v4/modules/apps/27-interchain-accounts/host"
icahostkeeper "github.com/cosmos/ibc-go/v4/modules/apps/27-interchain-accounts/host/keeper"
Expand Down Expand Up @@ -643,6 +644,12 @@ func (appKeepers *AppKeepers) SetupHooks() {
// e.g. *app.StakingKeeper doesn't appear

// Recall that SetHooks is a mutative call.
appKeepers.BankKeeper.SetHooks(
banktypes.NewMultiBankHooks(
appKeepers.TokenFactoryKeeper.Hooks(*appKeepers.WasmKeeper),
),
)

appKeepers.StakingKeeper.SetHooks(
stakingtypes.NewMultiStakingHooks(
appKeepers.DistrKeeper.Hooks(),
Expand Down
19 changes: 19 additions & 0 deletions proto/osmosis/tokenfactory/v1beta1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ service Query {
option (google.api.http).get =
"/osmosis/tokenfactory/v1beta1/denoms_from_creator/{creator}";
}

// BeforeSendHookAddress defines a gRPC query method for
// getting the address registered for the before send hook.
rpc BeforeSendHookAddress(QueryBeforeSendHookAddressRequest)
returns (QueryBeforeSendHookAddressResponse) {
option (google.api.http).get =
"/osmosis/tokenfactory/v1beta1/denoms/{denom}/before_send_hook";
}
}

// QueryParamsRequest is the request type for the Query/Params RPC method.
Expand Down Expand Up @@ -69,3 +77,14 @@ message QueryDenomsFromCreatorRequest {
message QueryDenomsFromCreatorResponse {
repeated string denoms = 1 [ (gogoproto.moretags) = "yaml:\"denoms\"" ];
}

message QueryBeforeSendHookAddressRequest {
string denom = 1 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
}

// QueryBeforeSendHookAddressResponse defines the response structure for the
// DenomBeforeSendHook gRPC query.
message QueryBeforeSendHookAddressResponse {
string cosmwasm_address = 1
[ (gogoproto.moretags) = "yaml:\"cosmwasm_address\"" ];
}
32 changes: 18 additions & 14 deletions proto/osmosis/tokenfactory/v1beta1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ service Msg {
rpc ChangeAdmin(MsgChangeAdmin) returns (MsgChangeAdminResponse);
rpc SetDenomMetadata(MsgSetDenomMetadata)
returns (MsgSetDenomMetadataResponse);

rpc SetBeforeSendHook(MsgSetBeforeSendHook)
returns (MsgSetBeforeSendHookResponse);
// ForceTransfer is deactivated for now because we need to think through edge
// cases rpc ForceTransfer(MsgForceTransfer) returns
// (MsgForceTransferResponse);
Expand Down Expand Up @@ -51,6 +52,8 @@ message MsgMint {
(gogoproto.moretags) = "yaml:\"amount\"",
(gogoproto.nullable) = false
];
string mintToAddress = 3
[ (gogoproto.moretags) = "yaml:\"mint_to_address\"" ];
}

message MsgMintResponse {}
Expand All @@ -63,6 +66,8 @@ message MsgBurn {
(gogoproto.moretags) = "yaml:\"amount\"",
(gogoproto.nullable) = false
];
string burnFromAddress = 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not related to the send hooks, right? Do we need these mint/burn changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! Srry meant to address this in a different PR must have been pushed together. Fixed

[ (gogoproto.moretags) = "yaml:\"burn_from_address\"" ];
}

message MsgBurnResponse {}
Expand All @@ -79,19 +84,18 @@ message MsgChangeAdmin {
// MsgChangeAdmin message.
message MsgChangeAdminResponse {}

// message MsgForceTransfer {
// string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
// cosmos.base.v1beta1.Coin amount = 2 [
// (gogoproto.moretags) = "yaml:\"amount\"",
// (gogoproto.nullable) = false
// ];
// string transferFromAddress = 3
// [ (gogoproto.moretags) = "yaml:\"transfer_from_address\"" ];
// string transferToAddress = 4
// [ (gogoproto.moretags) = "yaml:\"transfer_to_address\"" ];
// }
// MsgSetBeforeSendHook is the sdk.Msg type for allowing an admin account to
// assign a CosmWasm contract to call with a BeforeSend hook
message MsgSetBeforeSendHook {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
string denom = 2 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
string cosmwasm_address = 3
[ (gogoproto.moretags) = "yaml:\"cosmwasm_address\"" ];
}

// message MsgForceTransferResponse {}
// MsgSetBeforeSendHookResponse defines the response structure for an executed
// MsgSetBeforeSendHook message.
message MsgSetBeforeSendHookResponse {}

// MsgSetDenomMetadata is the sdk.Msg type for allowing an admin account to set
// the denom's bank metadata
Expand All @@ -105,4 +109,4 @@ message MsgSetDenomMetadata {

// MsgSetDenomMetadataResponse defines the response structure for an executed
// MsgSetDenomMetadata message.
message MsgSetDenomMetadataResponse {}
message MsgSetDenomMetadataResponse {}
31 changes: 31 additions & 0 deletions x/tokenfactory/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (

// "strings"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/spf13/cobra"

// "github.com/cosmos/cosmos-sdk/client/flags"
Expand Down Expand Up @@ -45,3 +47,32 @@ func GetCmdDenomsFromCreator() (*osmocli.QueryDescriptor, *types.QueryDenomsFrom
{{.CommandPrefix}} <address>`,
}, &types.QueryDenomsFromCreatorRequest{}
}

// GetCmdDenomAuthorityMetadata returns the authority metadata for a queried denom
func GetCmdDenomBeforeSendHook() *cobra.Command {
cmd := &cobra.Command{
Use: "denom-before-send-hook [denom] [flags]",
Short: "Get the BeforeSend hook for a specific denom",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientQueryContext(cmd)
if err != nil {
return err
}
queryClient := types.NewQueryClient(clientCtx)

res, err := queryClient.BeforeSendHookAddress(cmd.Context(), &types.QueryBeforeSendHookAddressRequest{
Denom: args[0],
})
if err != nil {
return err
}

return clientCtx.PrintProto(res)
},
}

flags.AddQueryFlagsToCmd(cmd)

return cmd
}
32 changes: 32 additions & 0 deletions x/tokenfactory/client/cli/tx.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package cli

import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/client/tx"
"github.com/spf13/cobra"

// "github.com/cosmos/cosmos-sdk/client/flags"
Expand All @@ -17,6 +20,7 @@ func GetTxCmd() *cobra.Command {
NewBurnCmd(),
// NewForceTransferCmd(),
NewChangeAdminCmd(),
NewSetBeforeSendHookCmd(),
)

return cmd
Expand Down Expand Up @@ -49,3 +53,31 @@ func NewChangeAdminCmd() *cobra.Command {
Short: "Changes the admin address for a factory-created denom. Must have admin authority to do so.",
})
}

// NewChangeAdminCmd broadcast MsgChangeAdmin
func NewSetBeforeSendHookCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "set-beforesend-hook [denom] [cosmwasm-address] [flags]",
Short: "Set a cosmwasm contract to be the beforesend hook for a factory-created denom. Must have admin authority to do so.",
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}

txf := tx.NewFactoryCLI(clientCtx, cmd.Flags()).WithTxConfig(clientCtx.TxConfig).WithAccountRetriever(clientCtx.AccountRetriever)

msg := types.NewMsgSetBeforeSendHook(
clientCtx.GetFromAddress().String(),
args[0],
args[1],
)

return tx.GenerateOrBroadcastTxWithFactory(clientCtx, txf, msg)
},
}

flags.AddTxFlagsToCmd(cmd)
return cmd
}
115 changes: 115 additions & 0 deletions x/tokenfactory/keeper/before_send.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package keeper

import (
"encoding/json"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/osmosis-labs/osmosis/v14/x/tokenfactory/types"

wasmKeeper "github.com/CosmWasm/wasmd/x/wasm/keeper"
wasmvmtypes "github.com/CosmWasm/wasmvm/types"
)

func (k Keeper) setBeforeSendHook(ctx sdk.Context, denom string, cosmwasmAddress string) error {
// verify that denom is an x/tokenfactory denom
_, _, err := types.DeconstructDenom(denom)
if err != nil {
return err
}

store := k.GetDenomPrefixStore(ctx, denom)

// delete the store for denom prefix store when cosmwasm address is nil
if cosmwasmAddress == "" {
store.Delete([]byte(types.BeforeSendHookAddressPrefixKey))
return nil
}

_, err = sdk.AccAddressFromBech32(cosmwasmAddress)
if err != nil {
return err
}

store.Set([]byte(types.BeforeSendHookAddressPrefixKey), []byte(cosmwasmAddress))

return nil
}

func (k Keeper) GetBeforeSendHook(ctx sdk.Context, denom string) string {
store := k.GetDenomPrefixStore(ctx, denom)

bz := store.Get([]byte(types.BeforeSendHookAddressPrefixKey))
if bz == nil {
return ""
}

return string(bz)
}

func CWCoinsFromSDKCoins(in sdk.Coins) wasmvmtypes.Coins {
var cwCoins wasmvmtypes.Coins
for _, coin := range in {
cwCoins = append(cwCoins, CWCoinFromSDKCoin(coin))
}
return cwCoins
}

func CWCoinFromSDKCoin(in sdk.Coin) wasmvmtypes.Coin {
return wasmvmtypes.Coin{
Denom: in.GetDenom(),
Amount: in.Amount.String(),
}
}

// Hooks wrapper struct for bank keeper
type Hooks struct {
k Keeper
wasmkeeper wasmKeeper.Keeper
}

var _ types.BankHooks = Hooks{}

// Return the wrapper struct
func (k Keeper) Hooks(wasmkeeper wasmKeeper.Keeper) Hooks {
return Hooks{k, wasmkeeper}
}

// implements BeforeSend hook in the Bank module.
// Calls the stored before send hook for the denom specificed.
func (h Hooks) BeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error {
cwCoins := CWCoinsFromSDKCoins(amount)

for _, coin := range amount {
cosmwasmAddress := h.k.GetBeforeSendHook(ctx, coin.Denom)
if cosmwasmAddress != "" {
cwAddr, err := sdk.AccAddressFromBech32(cosmwasmAddress)
if err != nil {
return err
}

msg := types.SudoMsg{
BeforeSend: types.BeforeSendMsg{
From: from.String(),
To: to.String(),
Amount: cwCoins,
},
}

msgBz, err := json.Marshal(msg)
if err != nil {
return err
}

em := sdk.NewEventManager()

_, err = h.wasmkeeper.Sudo(ctx.WithEventManager(em), cwAddr, msgBz)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to think about gas increase expectations for bank sends with tokenfactory denoms as the contract can consume unbounded gas

Copy link
Member Author

@mattverse mattverse Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think its a possible option to gate usage of gas here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we've been discussing something similar for wasm-hooks: (#4195). For the ibc case, I think non-atomic multimsg is enough, but for send hooks I think we may want to have a hard-cap on how much gas those hooks are allowed to take (something like: https://github.com/cosmos/ibc-go/blob/main/modules/core/keeper/msg_server.go#L451) and have users submit with that limit.

There is no risk to the chain here though, it's more of a UX problem. If a contract (by malice or incompetence) has an infinite loop or just very high gas usage, users may need to submit their tx with increasing gas limits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see I see, sounds fine for now for the UX tradeoff of gas problem, going ahead and merging this for now

fmt.Println(err)
if err != nil {
return err
}
}
}
return nil
}
Loading