imp: ics20 v2 self review part 3

modules/apps/transfer/ibc_module.go

@@ -181,7 +181,7 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin
 	case types.V1:
 		var datav1 types.FungibleTokenPacketData
 		if err := json.Unmarshal(bz, &datav1); err != nil {
-			return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error())
+			return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V1 transfer packet data: %s", err.Error())
 		}
 
 		if err := datav1.ValidateBasic(); err != nil {
@@ -240,26 +240,28 @@ func (im IBCModule) OnRecvPacket(
 		}
 	}
 
-	ctx.EventManager().EmitEvent(sdk.NewEvent(
-		types.EventTypePacket,
+	eventAttributes := []sdk.Attribute{
 		sdk.NewAttribute(types.AttributeKeySender, data.Sender),
 		sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 		sdk.NewAttribute(types.AttributeKeyTokens, types.Tokens(data.Tokens).String()),
 		sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
-	))
-
-	eventAttributes := []sdk.Attribute{
-		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
 		sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
 	}
+
 	if ackErr != nil {
 		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error()))
 	}
 
-	ctx.EventManager().EmitEvent(sdk.NewEvent(
-		sdk.EventTypeMessage,
-		eventAttributes...,
-	))
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			types.EventTypePacket,
+			eventAttributes...,
+		),
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+		),
+	})
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
 	return ack
@@ -298,11 +300,11 @@ func (im IBCModule) OnAcknowledgementPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyTokens, types.Tokens(data.Tokens).String()),
 			sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
+			sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
 		),
 		sdk.NewEvent(
 			sdk.EventTypeMessage,
 			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-			sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
 		),
 	})
 
@@ -350,9 +352,8 @@ func (im IBCModule) OnTimeoutPacket(
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeTimeout,
-			sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
-			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
-			sdk.NewAttribute(types.AttributeKeyTokens, types.Tokens(data.Tokens).String()),
+			sdk.NewAttribute(types.AttributeKeyReceiver, data.Sender),
+			sdk.NewAttribute(types.AttributeKeyRefundTokens, types.Tokens(data.Tokens).String()),
 			sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
 		),
 		sdk.NewEvent(

modules/apps/transfer/keeper/msg_server.go

@@ -38,10 +38,8 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
 		return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with ics20-1")
 	}
 
-	for _, coin := range coins {
-		if !k.bankKeeper.IsSendEnabledCoin(ctx, coin) {
-			return nil, errorsmod.Wrapf(types.ErrSendDisabled, "transfers are currently disabled for %s", coin.Denom)
-		}
+	if err := k.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil {
+		return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())
 	}
 
 	if k.bankKeeper.BlockedAddr(sender) {

modules/apps/transfer/keeper/msg_server_test.go

@@ -113,6 +113,36 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
 			types.ErrSendDisabled,
 			false,
 		},
+		{
+			"failure: bank send disabled for coin in multi coin transfer",
+			func() {
+				coin2 = sdk.NewCoin("bond", sdkmath.NewInt(100))
+				coins := sdk.NewCoins(coin1, coin2)
+
+				// send some coins of the second denom from bank module to the sender account as well
+				suite.Require().NoError(suite.chainA.GetSimApp().BankKeeper.MintCoins(suite.chainA.GetContext(), types.ModuleName, sdk.NewCoins(coin2)))
+				suite.Require().NoError(suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), sdk.NewCoins(coin2)))
+
+				msg = types.NewMsgTransfer(
+					path.EndpointA.ChannelConfig.PortID,
+					path.EndpointA.ChannelID,
+					coins,
+					suite.chainA.SenderAccount.GetAddress().String(),
+					suite.chainB.SenderAccount.GetAddress().String(),
+					suite.chainB.GetTimeoutHeight(), 0, // only use timeout height
+					"memo",
+				)
+
+				err := suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(),
+					banktypes.Params{
+						SendEnabled: []*banktypes.SendEnabled{{Denom: coin2.Denom, Enabled: false}},
+					},
+				)
+				suite.Require().NoError(err)
+			},
+			types.ErrSendDisabled,
+			true,
+		},
 		{
 			"failure: channel does not exist",
 			func() {

modules/apps/transfer/types/events.go

@@ -13,8 +13,7 @@ const (
 	AttributeKeyDenom          = "denom"
 	AttributeKeyTokens         = "tokens"
 	AttributeKeyRefundReceiver = "refund_receiver"
-	AttributeKeyRefundDenom    = "refund_denom"
-	AttributeKeyRefundAmount   = "refund_amount"
+	AttributeKeyRefundTokens   = "refund_tokens"
 	AttributeKeyAckSuccess     = "success"
 	AttributeKeyAck            = "acknowledgement"
 	AttributeKeyAckError       = "error"

modules/apps/transfer/types/expected_keepers.go

@@ -27,7 +27,7 @@ type BankKeeper interface {
 	SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
 	SendCoinsFromAccountToModule(ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
 	BlockedAddr(addr sdk.AccAddress) bool
-	IsSendEnabledCoin(ctx context.Context, coin sdk.Coin) bool
+	IsSendEnabledCoins(ctx context.Context, coins ...sdk.Coin) error
 	HasDenomMetaData(ctx context.Context, denom string) bool
 	SetDenomMetaData(ctx context.Context, denomMetaData banktypes.Metadata)
 	GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin

modules/apps/transfer/types/msgs.go

@@ -126,7 +126,7 @@ func isValidIBCCoin(coin sdk.Coin) bool {
 	return validateIBCCoin(coin) == nil
 }
 
-// isValidIBCCoin returns true if the token provided is valid,
+// validateIBCCoin returns true if the token provided is valid,
 // and should be used to transfer tokens. The token must
 // have a positive amount.
 func validateIBCCoin(coin sdk.Coin) error {

modules/apps/transfer/types/token.go

@@ -22,14 +22,16 @@ func (t Token) Validate() error {
 		return errorsmod.Wrapf(ErrInvalidAmount, "amount must be strictly positive: got %d", amount)
 	}
 
-	if len(t.Trace) == 0 {
-		return nil
-	}
+	if len(t.Trace) != 0 {
+		trace := strings.Join(t.Trace, "/")
+		identifiers := strings.Split(trace, "/")
 
-	trace := strings.Join(t.Trace, "/")
-	identifiers := strings.Split(trace, "/")
+		if err := validateTraceIdentifiers(identifiers); err != nil {
+			return err
+		}
+	}
 
-	return validateTraceIdentifiers(identifiers)
+	return nil
 }
 
 // GetFullDenomPath returns the full denomination according to the ICS20 specification:

modules/apps/transfer/types/token_test.go

@@ -110,6 +110,33 @@ func TestValidate(t *testing.T) {
 			},
 			ErrInvalidDenomForTransfer,
 		},
+		{
+			"failure: invalid amount string",
+			Token{
+				Denom:  "atom",
+				Amount: "value",
+				Trace:  []string{"transfer/channel-0", "transfer/channel-1"},
+			},
+			ErrInvalidAmount,
+		},
+		{
+			"failure: amount is zero",
+			Token{
+				Denom:  "atom",
+				Amount: "0",
+				Trace:  []string{"transfer/channel-0", "transfer/channel-1"},
+			},
+			ErrInvalidAmount,
+		},
+		{
+			"failure: amount is negative",
+			Token{
+				Denom:  "atom",
+				Amount: "-1",
+				Trace:  []string{"transfer/channel-0", "transfer/channel-1"},
+			},
+			ErrInvalidAmount,
+		},
 		{
 			"failure: invalid identifier in trace",
 			Token{