diff --git a/CHANGELOG.md b/CHANGELOG.md index eae819fbf93..5194a5e1c02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] ### Features - +* [\#8573](https://github.com/cosmos/ibc-go/pull/8573) Support custom address codecs in transfer, PFM, and rate limiting. * [\#8285](https://github.com/cosmos/ibc-go/pull/8285) Packet forward middleware. * [\#8545](https://github.com/cosmos/ibc-go/pull/8545) Support sending multiple payloads in the same packet for atomic payload execution. * [\#8473](https://github.com/cosmos/ibc-go/pull/8473) Support sending v2 packets on v1 channel identifiers using aliasing. diff --git a/docs/docs/05-migrations/14-v10-to-v11.md b/docs/docs/05-migrations/14-v10-to-v11.md index faedbeef5ab..f740404f9ec 100644 --- a/docs/docs/05-migrations/14-v10-to-v11.md +++ b/docs/docs/05-migrations/14-v10-to-v11.md @@ -24,3 +24,41 @@ Diff examples are shown after the list of overall changes: authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) ``` + +The transfer module, the packet forward middleware, and the rate limiting middleware support custom address codecs. This feature is primarily added to support Cosmos EVM for IBC transfers. In a standard Cosmos SDK app, they are wired as follows: + +```diff + app.TransferKeeper = ibctransferkeeper.NewKeeper( + appCodec, ++ app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), + app.IBCKeeper.ChannelKeeper, + app.MsgServiceRouter(), + app.AccountKeeper, app.BankKeeper, + authtypes.NewModuleAddress(govtypes.ModuleName).String(), + ) +``` + +```diff + app.RateLimitKeeper = ratelimitkeeper.NewKeeper( + appCodec, ++ app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ratelimittypes.StoreKey]), + app.IBCKeeper.ChannelKeeper, + app.IBCKeeper.ClientKeeper, + app.BankKeeper, + authtypes.NewModuleAddress(govtypes.ModuleName).String() + ) +``` + +```diff + app.PFMKeeper = packetforwardkeeper.NewKeeper( + appCodec, ++ app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[packetforwardtypes.StoreKey]), + app.TransferKeeper, + app.IBCKeeper.ChannelKeeper, + app.BankKeeper, + authtypes.NewModuleAddress(govtypes.ModuleName).String() + ) +``` diff --git a/modules/apps/callbacks/testing/simapp/app.go b/modules/apps/callbacks/testing/simapp/app.go index c49cb80e0b6..33c8f7d2dd1 100644 --- a/modules/apps/callbacks/testing/simapp/app.go +++ b/modules/apps/callbacks/testing/simapp/app.go @@ -356,7 +356,9 @@ func NewSimApp( // Create Transfer Keeper // NOTE: the Transfer Keeper's ICS4Wrapper can later be replaced. app.TransferKeeper = ibctransferkeeper.NewKeeper( - appCodec, runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), + appCodec, + app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.MsgServiceRouter(), app.AccountKeeper, app.BankKeeper, diff --git a/modules/apps/packet-forward-middleware/keeper/keeper.go b/modules/apps/packet-forward-middleware/keeper/keeper.go index bf6642ed37f..10488219f0c 100644 --- a/modules/apps/packet-forward-middleware/keeper/keeper.go +++ b/modules/apps/packet-forward-middleware/keeper/keeper.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/go-metrics" + "cosmossdk.io/core/address" corestore "cosmossdk.io/core/store" errorsmod "cosmossdk.io/errors" "cosmossdk.io/log" @@ -39,6 +40,7 @@ var ( type Keeper struct { storeService corestore.KVStoreService cdc codec.BinaryCodec + addressCodec address.Codec transferKeeper types.TransferKeeper channelKeeper types.ChannelKeeper @@ -51,9 +53,11 @@ type Keeper struct { } // NewKeeper creates a new forward Keeper instance -func NewKeeper(cdc codec.BinaryCodec, storeService corestore.KVStoreService, transferKeeper types.TransferKeeper, channelKeeper types.ChannelKeeper, bankKeeper types.BankKeeper, authority string) *Keeper { +func NewKeeper(cdc codec.BinaryCodec, addressCodec address.Codec, storeService corestore.KVStoreService, transferKeeper types.TransferKeeper, channelKeeper types.ChannelKeeper, bankKeeper types.BankKeeper, authority string, +) *Keeper { return &Keeper{ cdc: cdc, + addressCodec: addressCodec, storeService: storeService, transferKeeper: transferKeeper, // Defaults to using the channel keeper as the ICS4Wrapper @@ -102,7 +106,7 @@ func (k *Keeper) moveFundsToUserRecoverableAccount(ctx sdk.Context, packet chann denom := token.GetDenom() coin := sdk.NewCoin(denom.IBCDenom(), amount) - userAccount, err := userRecoverableAccount(inFlightPacket) + userAccount, err := k.userRecoverableAccount(inFlightPacket) if err != nil { return fmt.Errorf("failed to get user recoverable account: %w", err) } @@ -135,11 +139,11 @@ func (k *Keeper) moveFundsToUserRecoverableAccount(ctx sdk.Context, packet chann // If the destination receiver of the original packet is a valid bech32 address for this chain, we use that address. // Otherwise, if the sender of the original packet is a valid bech32 address for another chain, we translate that address to this chain. // Note that for the fallback, the coin type of the source chain sender account must be compatible with this chain. -func userRecoverableAccount(inFlightPacket *types.InFlightPacket) (sdk.AccAddress, error) { +func (k *Keeper) userRecoverableAccount(inFlightPacket *types.InFlightPacket) (sdk.AccAddress, error) { var originalData transfertypes.FungibleTokenPacketData err := transfertypes.ModuleCdc.UnmarshalJSON(inFlightPacket.PacketData, &originalData) if err == nil { // if NO error - sender, err := sdk.AccAddressFromBech32(originalData.Receiver) + sender, err := k.addressCodec.StringToBytes(originalData.Receiver) if err == nil { // if NO error return sender, nil } diff --git a/modules/apps/packet-forward-middleware/keeper/keeper_test.go b/modules/apps/packet-forward-middleware/keeper/keeper_test.go index 0b1e941dcdc..d376e04419b 100644 --- a/modules/apps/packet-forward-middleware/keeper/keeper_test.go +++ b/modules/apps/packet-forward-middleware/keeper/keeper_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "bytes" "context" + "encoding/hex" "fmt" "testing" "time" @@ -21,6 +22,7 @@ import ( clienttypes "github.com/cosmos/ibc-go/v10/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v10/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v10/testing" + ibcmock "github.com/cosmos/ibc-go/v10/testing/mock" ) type KeeperTestSuite struct { @@ -161,62 +163,92 @@ func (s *KeeperTestSuite) TestWriteAcknowledgementForForwardedPacket() { } func (s *KeeperTestSuite) TestForwardTransferPacket() { - s.SetupTest() - path := ibctesting.NewTransferPath(s.chainA, s.chainB) - path.Setup() - - pfmKeeper := keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") + var ( + pfmKeeper *keeper.Keeper + initialSender string + finalReceiver string + ) + tests := []struct { + name string + malleate func() + }{ + { + name: "success: standard cosmos address", + malleate: func() {}, + }, + { + name: "success: with hex address codec", + malleate: func() { + pfmKeeper = keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), ibcmock.TestAddressCodec{}, runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") - ctx := s.chainA.GetContext() - srcPacket := channeltypes.Packet{ - Data: []byte{1}, - Sequence: 1, - SourcePort: path.EndpointA.ChannelConfig.PortID, - SourceChannel: path.EndpointA.ChannelID, - DestinationPort: path.EndpointB.ChannelConfig.PortID, - DestinationChannel: path.EndpointB.ChannelID, - TimeoutHeight: clienttypes.Height{ - RevisionNumber: 10, - RevisionHeight: 100, + initialSender = hex.EncodeToString(s.chainA.SenderAccount.GetAddress().Bytes()) + finalReceiver = hex.EncodeToString(s.chainB.SenderAccount.GetAddress().Bytes()) + }, }, - TimeoutTimestamp: 10101001, } - retries := uint8(2) - timeout := time.Duration(1010101010) - nonRefundable := false + for _, tc := range tests { + s.Run(tc.name, func() { + s.SetupTest() + path := ibctesting.NewTransferPath(s.chainA, s.chainB) + path.Setup() - metadata := pfmtypes.ForwardMetadata{ - Receiver: "first-receiver", - Port: path.EndpointA.ChannelConfig.PortID, - Channel: path.EndpointA.ChannelID, - Timeout: timeout, - Retries: &retries, - Next: nil, - } + pfmKeeper = keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") - initialSender := s.chainA.SenderAccount.GetAddress() - finalReceiver := s.chainB.SenderAccount.GetAddress() + ctx := s.chainA.GetContext() + srcPacket := channeltypes.Packet{ + Data: []byte{1}, + Sequence: 1, + SourcePort: path.EndpointA.ChannelConfig.PortID, + SourceChannel: path.EndpointA.ChannelID, + DestinationPort: path.EndpointB.ChannelConfig.PortID, + DestinationChannel: path.EndpointB.ChannelID, + TimeoutHeight: clienttypes.Height{ + RevisionNumber: 10, + RevisionHeight: 100, + }, + TimeoutTimestamp: 10101001, + } - err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender.String(), finalReceiver.String(), metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable) - s.Require().NoError(err) + retries := uint8(2) + timeout := time.Duration(1010101010) + nonRefundable := false - // Get the inflight packer - inflightPacket, err := pfmKeeper.GetInflightPacket(ctx, srcPacket) - s.Require().NoError(err) + metadata := pfmtypes.ForwardMetadata{ + Receiver: "first-receiver", + Port: path.EndpointA.ChannelConfig.PortID, + Channel: path.EndpointA.ChannelID, + Timeout: timeout, + Retries: &retries, + Next: nil, + } - s.Require().Equal(inflightPacket.RetriesRemaining, int32(retries)) + initialSender = s.chainA.SenderAccount.GetAddress().String() + finalReceiver = s.chainB.SenderAccount.GetAddress().String() - // Call the same function again with inflight packet. Num retries should decrease. - err = pfmKeeper.ForwardTransferPacket(ctx, inflightPacket, srcPacket, initialSender.String(), finalReceiver.String(), metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable) - s.Require().NoError(err) + tc.malleate() - // Get the inflight packer - inflightPacket2, err := pfmKeeper.GetInflightPacket(ctx, srcPacket) - s.Require().NoError(err) + err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable) + s.Require().NoError(err) - s.Require().Equal(inflightPacket.RetriesRemaining, inflightPacket2.RetriesRemaining) - s.Require().Equal(int32(retries-1), inflightPacket.RetriesRemaining) + // Get the inflight packer + inflightPacket, err := pfmKeeper.GetInflightPacket(ctx, srcPacket) + s.Require().NoError(err) + + s.Require().Equal(inflightPacket.RetriesRemaining, int32(retries)) + + // Call the same function again with inflight packet. Num retries should decrease. + err = pfmKeeper.ForwardTransferPacket(ctx, inflightPacket, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable) + s.Require().NoError(err) + + // Get the inflight packer + inflightPacket2, err := pfmKeeper.GetInflightPacket(ctx, srcPacket) + s.Require().NoError(err) + + s.Require().Equal(inflightPacket.RetriesRemaining, inflightPacket2.RetriesRemaining) + s.Require().Equal(int32(retries-1), inflightPacket.RetriesRemaining) + }) + } } func (s *KeeperTestSuite) TestForwardTransferPacketWithNext() { @@ -224,7 +256,7 @@ func (s *KeeperTestSuite) TestForwardTransferPacketWithNext() { path := ibctesting.NewTransferPath(s.chainA, s.chainB) path.Setup() - pfmKeeper := keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") + pfmKeeper := keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") ctx := s.chainA.GetContext() srcPacket := channeltypes.Packet{ Data: []byte{1}, @@ -283,7 +315,7 @@ func (s *KeeperTestSuite) TestRetryTimeoutErrorGettingNext() { path := ibctesting.NewTransferPath(s.chainA, s.chainB) path.Setup() - pfmKeeper := keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") + pfmKeeper := keeper.NewKeeper(s.chainA.GetSimApp().AppCodec(), s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(pfmtypes.StoreKey)), &transferMock{}, s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().BankKeeper, "authority") ctx := s.chainA.GetContext() // Create a transfer detail with invalid memo that will cause GetPacketMetadataFromPacketdata to fail diff --git a/modules/apps/rate-limiting/keeper/keeper.go b/modules/apps/rate-limiting/keeper/keeper.go index 6f641c8104b..4f8313c5e5d 100644 --- a/modules/apps/rate-limiting/keeper/keeper.go +++ b/modules/apps/rate-limiting/keeper/keeper.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "cosmossdk.io/core/address" corestore "cosmossdk.io/core/store" "cosmossdk.io/log" @@ -19,6 +20,7 @@ import ( type Keeper struct { storeService corestore.KVStoreService cdc codec.BinaryCodec + addressCodec address.Codec ics4Wrapper porttypes.ICS4Wrapper channelKeeper types.ChannelKeeper @@ -29,13 +31,14 @@ type Keeper struct { } // NewKeeper creates a new rate-limiting Keeper instance -func NewKeeper(cdc codec.BinaryCodec, storeService corestore.KVStoreService, channelKeeper types.ChannelKeeper, clientKeeper types.ClientKeeper, bankKeeper types.BankKeeper, authority string) *Keeper { +func NewKeeper(cdc codec.BinaryCodec, addressCodec address.Codec, storeService corestore.KVStoreService, channelKeeper types.ChannelKeeper, clientKeeper types.ClientKeeper, bankKeeper types.BankKeeper, authority string) *Keeper { if strings.TrimSpace(authority) == "" { panic(errors.New("authority must be non-empty")) } return &Keeper{ cdc: cdc, + addressCodec: addressCodec, storeService: storeService, // Defaults to using the channel keeper as the ICS4Wrapper // This can be overridden later with WithICS4Wrapper (e.g. by the middleware stack wiring) diff --git a/modules/apps/rate-limiting/keeper/keeper_test.go b/modules/apps/rate-limiting/keeper/keeper_test.go index 643be32174f..963d95c8333 100644 --- a/modules/apps/rate-limiting/keeper/keeper_test.go +++ b/modules/apps/rate-limiting/keeper/keeper_test.go @@ -10,6 +10,7 @@ import ( "github.com/cosmos/ibc-go/v10/modules/apps/rate-limiting/keeper" ratelimittypes "github.com/cosmos/ibc-go/v10/modules/apps/rate-limiting/types" ibctesting "github.com/cosmos/ibc-go/v10/testing" + ibcmock "github.com/cosmos/ibc-go/v10/testing/mock" ) type KeeperTestSuite struct { @@ -44,6 +45,22 @@ func (s *KeeperTestSuite) TestNewKeeper() { instantiateFn: func() { keeper.NewKeeper( s.chainA.GetSimApp().AppCodec(), + s.chainA.GetSimApp().AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(ratelimittypes.StoreKey)), + s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + s.chainA.GetSimApp().IBCKeeper.ClientKeeper, // Add clientKeeper + s.chainA.GetSimApp().BankKeeper, + s.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), + ) + }, + panicMsg: "", + }, + { + name: "success: custom address codec", + instantiateFn: func() { + keeper.NewKeeper( + s.chainA.GetSimApp().AppCodec(), + ibcmock.TestAddressCodec{}, runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(ratelimittypes.StoreKey)), s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().IBCKeeper.ClientKeeper, // Add clientKeeper @@ -58,6 +75,7 @@ func (s *KeeperTestSuite) TestNewKeeper() { instantiateFn: func() { keeper.NewKeeper( s.chainA.GetSimApp().AppCodec(), + s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(ratelimittypes.StoreKey)), s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().IBCKeeper.ClientKeeper, // clientKeeper diff --git a/modules/apps/rate-limiting/keeper/msg_server.go b/modules/apps/rate-limiting/keeper/msg_server.go index ac9d8a05ba6..37985cfde84 100644 --- a/modules/apps/rate-limiting/keeper/msg_server.go +++ b/modules/apps/rate-limiting/keeper/msg_server.go @@ -6,6 +6,7 @@ import ( errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/ibc-go/v10/modules/apps/rate-limiting/types" @@ -24,6 +25,11 @@ var _ types.MsgServer = msgServer{} // Adds a new rate limit. Fails if the rate limit already exists or the channel value is 0 func (k msgServer) AddRateLimit(goCtx context.Context, msg *types.MsgAddRateLimit) (*types.MsgAddRateLimitResponse, error) { + _, err := k.addressCodec.StringToBytes(msg.Signer) + if err != nil { + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address: %s", msg.Signer) + } + if k.authority != msg.Signer { return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Signer) } @@ -38,6 +44,11 @@ func (k msgServer) AddRateLimit(goCtx context.Context, msg *types.MsgAddRateLimi // Updates an existing rate limit. Fails if the rate limit doesn't exist func (k msgServer) UpdateRateLimit(goCtx context.Context, msg *types.MsgUpdateRateLimit) (*types.MsgUpdateRateLimitResponse, error) { + _, err := k.addressCodec.StringToBytes(msg.Signer) + if err != nil { + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address: %s", msg.Signer) + } + if k.authority != msg.Signer { return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Signer) } @@ -52,6 +63,11 @@ func (k msgServer) UpdateRateLimit(goCtx context.Context, msg *types.MsgUpdateRa // Removes a rate limit. Fails if the rate limit doesn't exist func (k msgServer) RemoveRateLimit(goCtx context.Context, msg *types.MsgRemoveRateLimit) (*types.MsgRemoveRateLimitResponse, error) { + _, err := k.addressCodec.StringToBytes(msg.Signer) + if err != nil { + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address: %s", msg.Signer) + } + if k.authority != msg.Signer { return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Signer) } @@ -68,6 +84,11 @@ func (k msgServer) RemoveRateLimit(goCtx context.Context, msg *types.MsgRemoveRa // Resets the flow on a rate limit. Fails if the rate limit doesn't exist func (k msgServer) ResetRateLimit(goCtx context.Context, msg *types.MsgResetRateLimit) (*types.MsgResetRateLimitResponse, error) { + _, err := k.addressCodec.StringToBytes(msg.Signer) + if err != nil { + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address: %s", msg.Signer) + } + if k.authority != msg.Signer { return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Signer) } diff --git a/modules/apps/rate-limiting/keeper/msg_server_test.go b/modules/apps/rate-limiting/keeper/msg_server_test.go index 0a74fa6827b..27c38dc1ce2 100644 --- a/modules/apps/rate-limiting/keeper/msg_server_test.go +++ b/modules/apps/rate-limiting/keeper/msg_server_test.go @@ -5,6 +5,7 @@ import ( sdkmath "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" @@ -13,6 +14,7 @@ import ( "github.com/cosmos/ibc-go/v10/modules/apps/rate-limiting/types" transfertypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v10/modules/core/04-channel/types" + ibctesting "github.com/cosmos/ibc-go/v10/testing" ) var ( @@ -113,8 +115,12 @@ func (s *KeeperTestSuite) TestMsgServer_AddRateLimit() { // Verify that signer == authority required invalidSignerMsg := addRateLimitMsg - invalidSignerMsg.Signer = "" + invalidSignerMsg.Signer = s.chainA.SenderAccount.GetAddress().String() s.addRateLimitWithError(invalidSignerMsg, govtypes.ErrInvalidSigner) + + // Verify that valid signer required + invalidSignerMsg.Signer = ibctesting.InvalidID + s.addRateLimitWithError(invalidSignerMsg, sdkerrors.ErrInvalidAddress) } func (s *KeeperTestSuite) TestMsgServer_UpdateRateLimit() { @@ -150,9 +156,14 @@ func (s *KeeperTestSuite) TestMsgServer_UpdateRateLimit() { // Attempt to update a rate limit that has invalid authority invalidSignerMsg := updateRateLimitMsg - invalidSignerMsg.Signer = "" + invalidSignerMsg.Signer = s.chainA.SenderAccount.GetAddress().String() _, err = msgServer.UpdateRateLimit(s.chainA.GetContext(), &invalidSignerMsg) s.Require().ErrorIs(err, govtypes.ErrInvalidSigner) + + // Verify that valid signer required + invalidSignerMsg.Signer = ibctesting.InvalidID + _, err = msgServer.UpdateRateLimit(s.chainA.GetContext(), &invalidSignerMsg) + s.Require().ErrorIs(err, sdkerrors.ErrInvalidAddress) } func (s *KeeperTestSuite) TestMsgServer_RemoveRateLimit() { @@ -182,9 +193,14 @@ func (s *KeeperTestSuite) TestMsgServer_RemoveRateLimit() { // Attempt to Remove a rate limit that has invalid authority invalidSignerMsg := removeRateLimitMsg - invalidSignerMsg.Signer = "" + invalidSignerMsg.Signer = s.chainA.SenderAccount.GetAddress().String() _, err = msgServer.RemoveRateLimit(s.chainA.GetContext(), &invalidSignerMsg) s.Require().ErrorIs(err, govtypes.ErrInvalidSigner) + + // Verify that valid signer required + invalidSignerMsg.Signer = ibctesting.InvalidID + _, err = msgServer.RemoveRateLimit(s.chainA.GetContext(), &invalidSignerMsg) + s.Require().ErrorIs(err, sdkerrors.ErrInvalidAddress) } func (s *KeeperTestSuite) TestMsgServer_ResetRateLimit() { @@ -219,7 +235,12 @@ func (s *KeeperTestSuite) TestMsgServer_ResetRateLimit() { // Attempt to Remove a rate limit that has invalid authority invalidSignerMsg := resetRateLimitMsg - invalidSignerMsg.Signer = "" + invalidSignerMsg.Signer = s.chainA.SenderAccount.GetAddress().String() _, err = msgServer.ResetRateLimit(s.chainA.GetContext(), &invalidSignerMsg) s.Require().ErrorIs(err, govtypes.ErrInvalidSigner) + + // Verify that valid signer required + invalidSignerMsg.Signer = ibctesting.InvalidID + _, err = msgServer.ResetRateLimit(s.chainA.GetContext(), &invalidSignerMsg) + s.Require().ErrorIs(err, sdkerrors.ErrInvalidAddress) } diff --git a/modules/apps/rate-limiting/types/msgs.go b/modules/apps/rate-limiting/types/msgs.go index f4f5977656d..c296690f196 100644 --- a/modules/apps/rate-limiting/types/msgs.go +++ b/modules/apps/rate-limiting/types/msgs.go @@ -2,6 +2,7 @@ package types import ( "regexp" + "strings" errorsmod "cosmossdk.io/errors" sdkmath "cosmossdk.io/math" @@ -34,9 +35,8 @@ func NewMsgAddRateLimit(denom, channelOrClientID string, maxPercentSend sdkmath. } func (msg *MsgAddRateLimit) ValidateBasic() error { - _, err := sdk.AccAddressFromBech32(msg.Signer) - if err != nil { - return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address (%s)", err) + if strings.TrimSpace(msg.Signer) == "" { + return errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") } if msg.Denom == "" { @@ -89,9 +89,8 @@ func NewMsgUpdateRateLimit(denom, channelOrClientID string, maxPercentSend sdkma } func (msg *MsgUpdateRateLimit) ValidateBasic() error { - _, err := sdk.AccAddressFromBech32(msg.Signer) - if err != nil { - return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address (%s)", err) + if strings.TrimSpace(msg.Signer) == "" { + return errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") } if msg.Denom == "" { @@ -141,9 +140,8 @@ func NewMsgRemoveRateLimit(denom, channelOrClientID string) *MsgRemoveRateLimit } func (msg *MsgRemoveRateLimit) ValidateBasic() error { - _, err := sdk.AccAddressFromBech32(msg.Signer) - if err != nil { - return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address (%s)", err) + if strings.TrimSpace(msg.Signer) == "" { + return errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") } if msg.Denom == "" { @@ -174,9 +172,8 @@ func NewMsgResetRateLimit(denom, channelOrClientID string) *MsgResetRateLimit { } func (msg *MsgResetRateLimit) ValidateBasic() error { - _, err := sdk.AccAddressFromBech32(msg.Signer) - if err != nil { - return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid signer address (%s)", err) + if strings.TrimSpace(msg.Signer) == "" { + return errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") } if msg.Denom == "" { diff --git a/modules/apps/rate-limiting/types/msgs_test.go b/modules/apps/rate-limiting/types/msgs_test.go index 1bf1c194cfa..067d2c37994 100644 --- a/modules/apps/rate-limiting/types/msgs_test.go +++ b/modules/apps/rate-limiting/types/msgs_test.go @@ -37,7 +37,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass bool }{ { - name: "valid add msg with channel id", + name: "success: valid add msg with channel id", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -49,7 +49,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: true, }, { - name: "valid add msg with client id", + name: "success: valid add msg with client id", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -61,7 +61,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: true, }, { - name: "invalid authority", + name: "success: invalid authority", msg: &types.MsgAddRateLimit{ Signer: "invalid", Denom: "uatom", @@ -70,10 +70,22 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { MaxPercentRecv: sdkmath.NewInt(10), DurationHours: 24, }, + expPass: true, // Note: validate basic only checks the signer is not empty, not if it's a valid authority + }, + { + name: "success: empty authority", + msg: &types.MsgAddRateLimit{ + Signer: "", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, expPass: false, }, { - name: "denom can't be empty", + name: "failure: denom can't be empty", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "", @@ -85,7 +97,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "invalid client ID", + name: "failure: invalid client ID", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -97,7 +109,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "invalid channel ID", + name: "failure: invalid channel ID", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -109,7 +121,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "max percent send > 100", + name: "failure: max percent send > 100", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -121,7 +133,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "max percent recv > 100", + name: "failure: max percent recv > 100", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -133,7 +145,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "send and recv both zero", + name: "failure: send and recv both zero", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -145,7 +157,7 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { expPass: false, }, { - name: "duration is zero hours", + name: "failure: duration is zero hours", msg: &types.MsgAddRateLimit{ Signer: s.authority, Denom: "uatom", @@ -167,3 +179,315 @@ func (s *MsgsTestSuite) TestMsgAddRateLimit() { } } } + +func (s *MsgsTestSuite) TestMsgUpdateRateLimit() { + testCases := []struct { + name string + msg *types.MsgUpdateRateLimit + expPass bool + }{ + { + name: "success: valid add msg with channel id", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: true, + }, + { + name: "success: valid add msg with client id", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validClientID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: true, + }, + { + name: "success: invalid authority", + msg: &types.MsgUpdateRateLimit{ + Signer: "invalid", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: true, // Note: validate basic only checks the signer is not empty, not if it's a valid authority + }, + { + name: "success: empty authority", + msg: &types.MsgUpdateRateLimit{ + Signer: "", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: denom can't be empty", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: invalid client ID", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "invalid-client-id", + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: invalid channel ID", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "channel", + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: max percent send > 100", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(101), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: max percent recv > 100", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(101), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: send and recv both zero", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.ZeroInt(), + MaxPercentRecv: sdkmath.ZeroInt(), + DurationHours: 24, + }, + expPass: false, + }, + { + name: "failure: duration is zero hours", + msg: &types.MsgUpdateRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + MaxPercentSend: sdkmath.NewInt(10), + MaxPercentRecv: sdkmath.NewInt(10), + DurationHours: 0, + }, + expPass: false, + }, + } + + for i, tc := range testCases { + err := tc.msg.ValidateBasic() + if tc.expPass { + s.Require().NoError(err, "valid test case %d failed: %s", i, tc.name) + } else { + s.Require().Error(err, "invalid test case %d passed: %s", i, tc.name) + } + } +} + +func (s *MsgsTestSuite) TestMsgRemoveRateLimit() { + testCases := []struct { + name string + msg *types.MsgRemoveRateLimit + expPass bool + }{ + { + name: "success: valid add msg with channel id", + msg: &types.MsgRemoveRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: true, + }, + { + name: "success: valid add msg with client id", + msg: &types.MsgRemoveRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validClientID, + }, + expPass: true, + }, + { + name: "success: invalid authority", + msg: &types.MsgRemoveRateLimit{ + Signer: "invalid", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: true, // Note: validate basic only checks the signer is not empty, not if it's a valid authority + }, + { + name: "success: empty authority", + msg: &types.MsgRemoveRateLimit{ + Signer: "", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: false, + }, + { + name: "failure: denom can't be empty", + msg: &types.MsgRemoveRateLimit{ + Signer: s.authority, + Denom: "", + ChannelOrClientId: s.validChannelID, + }, + expPass: false, + }, + { + name: "failure: invalid client ID", + msg: &types.MsgRemoveRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "invalid-client-id", + }, + expPass: false, + }, + { + name: "failure: invalid channel ID", + msg: &types.MsgRemoveRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "channel", + }, + expPass: false, + }, + } + + for i, tc := range testCases { + err := tc.msg.ValidateBasic() + if tc.expPass { + s.Require().NoError(err, "valid test case %d failed: %s", i, tc.name) + } else { + s.Require().Error(err, "invalid test case %d passed: %s", i, tc.name) + } + } +} + +func (s *MsgsTestSuite) TestMsgResetRateLimit() { + testCases := []struct { + name string + msg *types.MsgResetRateLimit + expPass bool + }{ + { + name: "success: valid add msg with channel id", + msg: &types.MsgResetRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: true, + }, + { + name: "success: valid add msg with client id", + msg: &types.MsgResetRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: s.validClientID, + }, + expPass: true, + }, + { + name: "success: invalid authority", + msg: &types.MsgResetRateLimit{ + Signer: "invalid", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: true, // Note: validate basic only checks the signer is not empty, not if it's a valid authority + }, + { + name: "success: empty authority", + msg: &types.MsgResetRateLimit{ + Signer: "", + Denom: "uatom", + ChannelOrClientId: s.validChannelID, + }, + expPass: false, + }, + { + name: "failure: denom can't be empty", + msg: &types.MsgResetRateLimit{ + Signer: s.authority, + Denom: "", + ChannelOrClientId: s.validChannelID, + }, + expPass: false, + }, + { + name: "failure: invalid client ID", + msg: &types.MsgResetRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "invalid-client-id", + }, + expPass: false, + }, + { + name: "failure: invalid channel ID", + msg: &types.MsgResetRateLimit{ + Signer: s.authority, + Denom: "uatom", + ChannelOrClientId: "channel", + }, + expPass: false, + }, + } + + for i, tc := range testCases { + err := tc.msg.ValidateBasic() + if tc.expPass { + s.Require().NoError(err, "valid test case %d failed: %s", i, tc.name) + } else { + s.Require().Error(err, "invalid test case %d passed: %s", i, tc.name) + } + } +} diff --git a/modules/apps/transfer/keeper/export_test.go b/modules/apps/transfer/keeper/export_test.go index 1618ea76596..3dbedb85bb7 100644 --- a/modules/apps/transfer/keeper/export_test.go +++ b/modules/apps/transfer/keeper/export_test.go @@ -1,6 +1,8 @@ package keeper import ( + "cosmossdk.io/core/address" + sdk "github.com/cosmos/cosmos-sdk/types" internaltypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/internal/types" @@ -17,6 +19,11 @@ func (k *Keeper) IterateDenomTraces(ctx sdk.Context, cb func(denomTrace internal k.iterateDenomTraces(ctx, cb) } +// SetAddressCodec is a setter for the address codec for testing purposes. +func (k *Keeper) SetAddressCodec(addressCodec address.Codec) { + k.addressCodec = addressCodec +} + // GetAllDenomTraces returns the trace information for all the denominations. func (k *Keeper) GetAllDenomTraces(ctx sdk.Context) []internaltypes.DenomTrace { var traces []internaltypes.DenomTrace diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 8f7b7ea4bb2..b0cfd050e6c 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "cosmossdk.io/core/address" corestore "cosmossdk.io/core/store" "cosmossdk.io/log" sdkmath "cosmossdk.io/math" @@ -27,6 +28,7 @@ import ( type Keeper struct { storeService corestore.KVStoreService cdc codec.BinaryCodec + addressCodec address.Codec ics4Wrapper porttypes.ICS4Wrapper channelKeeper types.ChannelKeeper @@ -40,15 +42,7 @@ type Keeper struct { } // NewKeeper creates a new IBC transfer Keeper instance -func NewKeeper( - cdc codec.BinaryCodec, - storeService corestore.KVStoreService, - channelKeeper types.ChannelKeeper, - msgRouter types.MessageRouter, - authKeeper types.AccountKeeper, - bankKeeper types.BankKeeper, - authority string, -) *Keeper { +func NewKeeper(cdc codec.BinaryCodec, addressCodec address.Codec, storeService corestore.KVStoreService, channelKeeper types.ChannelKeeper, msgRouter types.MessageRouter, authKeeper types.AccountKeeper, bankKeeper types.BankKeeper, authority string) *Keeper { // ensure ibc transfer module account is set if addr := authKeeper.GetModuleAddress(types.ModuleName); addr == nil { panic(errors.New("the IBC transfer module account has not been set")) @@ -60,6 +54,7 @@ func NewKeeper( return &Keeper{ cdc: cdc, + addressCodec: addressCodec, storeService: storeService, ics4Wrapper: channelKeeper, // default ICS4Wrapper is the channel keeper channelKeeper: channelKeeper, @@ -87,6 +82,11 @@ func (k *Keeper) GetAuthority() string { return k.authority } +// GetAddressCodec returns the address codec used by the keeper. +func (k *Keeper) GetAddressCodec() address.Codec { + return k.addressCodec +} + // Logger returns a module-specific logger. func (*Keeper) Logger(ctx sdk.Context) log.Logger { return ctx.Logger().With("module", "x/"+exported.ModuleName+"-"+types.ModuleName) diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index f4bc2d64b85..6861cf10476 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -57,6 +57,7 @@ func (s *KeeperTestSuite) TestNewKeeper() { {"success", func() { keeper.NewKeeper( s.chainA.GetSimApp().AppCodec(), + s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(types.StoreKey)), s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().MsgServiceRouter(), @@ -68,6 +69,7 @@ func (s *KeeperTestSuite) TestNewKeeper() { {"failure: transfer module account does not exist", func() { keeper.NewKeeper( s.chainA.GetSimApp().AppCodec(), + s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(types.StoreKey)), s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().MsgServiceRouter(), @@ -79,6 +81,7 @@ func (s *KeeperTestSuite) TestNewKeeper() { {"failure: empty authority", func() { keeper.NewKeeper( s.chainA.GetSimApp().AppCodec(), + s.chainA.GetSimApp().AccountKeeper.AddressCodec(), runtime.NewKVStoreService(s.chainA.GetSimApp().GetKey(types.StoreKey)), s.chainA.GetSimApp().IBCKeeper.ChannelKeeper, s.chainA.GetSimApp().MsgServiceRouter(), diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index a563b16ff00..50af401910e 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -27,7 +27,7 @@ func (k *Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types return nil, types.ErrSendDisabled } - sender, err := sdk.AccAddressFromBech32(msg.Sender) + sender, err := k.addressCodec.StringToBytes(msg.Sender) if err != nil { return nil, err } @@ -47,7 +47,7 @@ func (k *Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types return nil, err } - packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, sender.String(), msg.Receiver, msg.Memo) + packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, msg.Sender, msg.Receiver, msg.Memo) if err := packetData.ValidateBasic(); err != nil { return nil, errorsmod.Wrapf(err, "failed to validate %s packet data", types.V1) @@ -65,7 +65,7 @@ func (k *Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types sequence, err = k.transferV2Packet(ctx, msg.Encoding, msg.SourceChannel, msg.TimeoutTimestamp, packetData) } else { // if a V1 channel exists for the source channel, then use IBC V1 protocol - sequence, err = k.transferV1Packet(ctx, msg.SourceChannel, token, msg.TimeoutHeight, msg.TimeoutTimestamp, packetData) + sequence, err = k.transferV1Packet(ctx, msg.SourceChannel, token, msg.TimeoutHeight, msg.TimeoutTimestamp, sender, packetData) // telemetry for transfer occurs here, in IBC V2 this is done in the onSendPacket callback telemetry.ReportTransfer(msg.SourcePort, msg.SourceChannel, channel.Counterparty.PortId, channel.Counterparty.ChannelId, token) } @@ -78,8 +78,8 @@ func (k *Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types return &types.MsgTransferResponse{Sequence: sequence}, nil } -func (k *Keeper) transferV1Packet(ctx sdk.Context, sourceChannel string, token types.Token, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, packetData types.FungibleTokenPacketData) (uint64, error) { - if err := k.SendTransfer(ctx, types.PortID, sourceChannel, token, sdk.MustAccAddressFromBech32(packetData.Sender)); err != nil { +func (k *Keeper) transferV1Packet(ctx sdk.Context, sourceChannel string, token types.Token, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, sender sdk.AccAddress, packetData types.FungibleTokenPacketData) (uint64, error) { + if err := k.SendTransfer(ctx, types.PortID, sourceChannel, token, sender); err != nil { return 0, err } diff --git a/modules/apps/transfer/keeper/msg_server_test.go b/modules/apps/transfer/keeper/msg_server_test.go index fa62b78f70c..66785fdedf7 100644 --- a/modules/apps/transfer/keeper/msg_server_test.go +++ b/modules/apps/transfer/keeper/msg_server_test.go @@ -61,6 +61,13 @@ func (s *KeeperTestSuite) TestMsgTransfer() { }, types.ErrSendDisabled, }, + { + "failure: zero amount", + func() { + msg.Token = sdk.NewInt64Coin(sdk.DefaultBondDenom, 0) + }, + types.ErrInvalidAmount, + }, { "failure: invalid sender", func() { diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 126e0e6256f..308fa160144 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -127,7 +127,7 @@ func (k *Keeper) OnRecvPacket( return types.ErrReceiveDisabled } - receiver, err := sdk.AccAddressFromBech32(data.Receiver) + receiver, err := k.addressCodec.StringToBytes(data.Receiver) if err != nil { return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "failed to decode receiver address: %s", data.Receiver) } @@ -195,7 +195,7 @@ func (k *Keeper) OnRecvPacket( if err := k.BankKeeper.SendCoins( ctx, moduleAddr, receiver, sdk.NewCoins(voucher), ); err != nil { - return errorsmod.Wrapf(err, "failed to send coins to receiver %s", receiver.String()) + return errorsmod.Wrapf(err, "failed to send coins to receiver %s", data.Receiver) } } @@ -252,7 +252,7 @@ func (k *Keeper) refundPacketTokens( ) error { // NOTE: packet data type already checked in handler.go - sender, err := sdk.AccAddressFromBech32(data.Sender) + sender, err := k.addressCodec.StringToBytes(data.Sender) if err != nil { return err } diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index a28be1761e0..db469165358 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + "encoding/hex" "errors" "fmt" "strings" @@ -47,19 +48,19 @@ func (s *KeeperTestSuite) TestSendTransfer() { expError error }{ { - "successful transfer of native token", + "success: transfer of native token", func() {}, nil, }, { - "successful transfer of native token with memo", + "success: transfer of native token with memo", func() { memo = "memo" //nolint:goconst }, nil, }, { - "successful transfer of IBC token", + "success: transfer of IBC token", func() { // send IBC token back to chainB denom := types.NewDenom(ibctesting.TestCoin.Denom, types.NewHop(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) @@ -70,7 +71,7 @@ func (s *KeeperTestSuite) TestSendTransfer() { nil, }, { - "successful transfer of IBC token with memo", + "success: transfer of IBC token with memo", func() { // send IBC token back to chainB denom := types.NewDenom(ibctesting.TestCoin.Denom, types.NewHop(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) @@ -82,7 +83,7 @@ func (s *KeeperTestSuite) TestSendTransfer() { nil, }, { - "successful transfer of entire balance", + "success: transfer of entire balance", func() { coin = sdk.NewCoin(coin.Denom, types.UnboundedSpendLimit()) var ok bool @@ -92,7 +93,7 @@ func (s *KeeperTestSuite) TestSendTransfer() { nil, }, { - "successful transfer of entire spendable balance with vesting account", + "success: transfer of entire spendable balance with vesting account", func() { // create vesting account vestingAccPrivKey := secp256k1.GenPrivKey() @@ -325,17 +326,27 @@ func (s *KeeperTestSuite) TestOnRecvPacket_ReceiverIsNotSource() { expError error }{ { - "successful receive", + "success: receive", func() {}, nil, }, { - "successful receive with memo", + "success: receive with memo", func() { packetData.Memo = "memo" }, nil, }, + { + "success: receive with hex receiver address", + func() { + s.chainB.GetSimApp().TransferKeeper.SetAddressCodec(ibcmock.TestAddressCodec{}) + + receiver := sdk.MustAccAddressFromBech32(packetData.Receiver) + packetData.Receiver = hex.EncodeToString(receiver.Bytes()) + }, + nil, + }, { "failure: mint zero coin", func() { diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 95d8dcaa1ad..977b9760f69 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -35,9 +35,8 @@ func NewMsgUpdateParams(signer string, params Params) *MsgUpdateParams { // ValidateBasic implements sdk.Msg func (msg MsgUpdateParams) ValidateBasic() error { - _, err := sdk.AccAddressFromBech32(msg.Signer) - if err != nil { - return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + if strings.TrimSpace(msg.Signer) == "" { + return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "missing sender address") } return nil @@ -120,9 +119,8 @@ func (msg MsgTransfer) ValidateBasic() error { return errorsmod.Wrap(ibcerrors.ErrInvalidCoins, msg.Token.String()) } - _, err := sdk.AccAddressFromBech32(msg.Sender) - if err != nil { - return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + if strings.TrimSpace(msg.Sender) == "" { + return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "missing sender address") } if strings.TrimSpace(msg.Receiver) == "" { return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "missing recipient address") diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 4bc0aa91d6b..ad81198047e 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -114,7 +114,6 @@ func TestMsgUpdateParamsValidateBasic(t *testing.T) { expError error }{ {"success: valid signer and valid params", types.NewMsgUpdateParams(ibctesting.TestAccAddress, types.DefaultParams()), nil}, - {"failure: invalid signer with valid params", types.NewMsgUpdateParams(invalidAddress, types.DefaultParams()), ibcerrors.ErrInvalidAddress}, {"failure: empty signer with valid params", types.NewMsgUpdateParams(emptyAddr, types.DefaultParams()), ibcerrors.ErrInvalidAddress}, } diff --git a/modules/apps/transfer/v2/ibc_module.go b/modules/apps/transfer/v2/ibc_module.go index ffda9a4c392..7220ffa1b14 100644 --- a/modules/apps/transfer/v2/ibc_module.go +++ b/modules/apps/transfer/v2/ibc_module.go @@ -52,12 +52,12 @@ func (im IBCModule) OnSendPacket(ctx sdk.Context, sourceChannel string, destinat return err } - sender, err := sdk.AccAddressFromBech32(data.Sender) + sender, err := im.keeper.GetAddressCodec().StringToBytes(data.Sender) if err != nil { return err } - if !signer.Equals(sender) { + if !bytes.Equal(sender, signer) { return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "sender %s is different from signer %s", sender, signer) } @@ -76,7 +76,7 @@ func (im IBCModule) OnSendPacket(ctx sdk.Context, sourceChannel string, destinat return err } - events.EmitTransferEvent(ctx, sender.String(), data.Receiver, data.Token, data.Memo) + events.EmitTransferEvent(ctx, data.Sender, data.Receiver, data.Token, data.Memo) telemetry.ReportTransfer(payload.SourcePort, sourceChannel, payload.DestinationPort, destinationChannel, data.Token) diff --git a/modules/light-clients/08-wasm/testing/simapp/app.go b/modules/light-clients/08-wasm/testing/simapp/app.go index cde1ba8e5c4..af3134dd446 100644 --- a/modules/light-clients/08-wasm/testing/simapp/app.go +++ b/modules/light-clients/08-wasm/testing/simapp/app.go @@ -460,7 +460,9 @@ func newSimApp( // Create Transfer Keeper app.TransferKeeper = ibctransferkeeper.NewKeeper( - appCodec, runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), + appCodec, + app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.MsgServiceRouter(), app.AccountKeeper, app.BankKeeper, diff --git a/simapp/app.go b/simapp/app.go index 875bc6f5db6..d5c90bf4be2 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -367,7 +367,9 @@ func NewSimApp( // Transfer Keeper app.TransferKeeper = ibctransferkeeper.NewKeeper( - appCodec, runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), + appCodec, + app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.MsgServiceRouter(), app.AccountKeeper, app.BankKeeper, @@ -375,9 +377,9 @@ func NewSimApp( ) // Packet Forward Middleware keeper - app.PFMKeeper = packetforwardkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[packetforwardtypes.StoreKey]), app.TransferKeeper, app.IBCKeeper.ChannelKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) + app.PFMKeeper = packetforwardkeeper.NewKeeper(appCodec, app.AccountKeeper.AddressCodec(), runtime.NewKVStoreService(keys[packetforwardtypes.StoreKey]), app.TransferKeeper, app.IBCKeeper.ChannelKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) - app.RateLimitKeeper = ratelimitkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[ratelimittypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ClientKeeper, app.BankKeeper, govAuthority) + app.RateLimitKeeper = ratelimitkeeper.NewKeeper(appCodec, app.AccountKeeper.AddressCodec(), runtime.NewKVStoreService(keys[ratelimittypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ClientKeeper, app.BankKeeper, govAuthority) // Create IBC Router ibcRouter := porttypes.NewRouter() diff --git a/testing/mock/address_codec.go b/testing/mock/address_codec.go new file mode 100644 index 00000000000..0565fa59196 --- /dev/null +++ b/testing/mock/address_codec.go @@ -0,0 +1,27 @@ +package mock + +import ( + "errors" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +type TestAddressCodec struct{} + +func (t TestAddressCodec) StringToBytes(text string) ([]byte, error) { + hexBytes, err := sdk.AccAddressFromHexUnsafe(text) + if err == nil { + return hexBytes, nil + } + + bech32Bytes, err := sdk.AccAddressFromBech32(text) + if err == nil { + return bech32Bytes, nil + } + + return nil, errors.New("invalid address format") +} + +func (t TestAddressCodec) BytesToString(bz []byte) (string, error) { + return sdk.AccAddress(bz).String(), nil +} diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 87d3a232264..c65d894a895 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -362,15 +362,16 @@ func NewSimApp( // Create Transfer Keeper app.TransferKeeper = ibctransferkeeper.NewKeeper( - appCodec, runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), + appCodec, app.AccountKeeper.AddressCodec(), + runtime.NewKVStoreService(keys[ibctransfertypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.MsgServiceRouter(), app.AccountKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) - app.RateLimitKeeper = ratelimitkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[ratelimittypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ClientKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) - app.PFMKeeper = packetforwardkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[packetforwardtypes.StoreKey]), app.TransferKeeper, app.IBCKeeper.ChannelKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) + app.RateLimitKeeper = ratelimitkeeper.NewKeeper(appCodec, app.AccountKeeper.AddressCodec(), runtime.NewKVStoreService(keys[ratelimittypes.StoreKey]), app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ClientKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) + app.PFMKeeper = packetforwardkeeper.NewKeeper(appCodec, app.AccountKeeper.AddressCodec(), runtime.NewKVStoreService(keys[packetforwardtypes.StoreKey]), app.TransferKeeper, app.IBCKeeper.ChannelKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String()) // Mock Module Stack