Add the sender to the InputOutputCoins transfer events. The SDK did t…

…his in PR cosmos#21460 and I undid it when merging in the v0.50.10 changes since it's more complicated for us than the SDK. So this is bringing us back in line with SDK v0.50.10 with slightly different event ordering. This change also reduces the amount of times subUnlockedCoins is called when there are multiple inputs with the same address.

x/bank/keeper/keeper_test.go

@@ -1638,6 +1638,7 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
 	event2.Attributes = append(
 		event2.Attributes,
 		abci.EventAttribute{Key: banktypes.AttributeKeyRecipient, Value: accAddrs[2].String()},
+		abci.EventAttribute{Key: banktypes.AttributeKeySender, Value: accAddrs[0].String()},
 		abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins.String()},
 	)
 	event3 := sdk.Event{
@@ -1647,11 +1648,12 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
 	event3.Attributes = append(
 		event3.Attributes,
 		abci.EventAttribute{Key: banktypes.AttributeKeyRecipient, Value: accAddrs[3].String()},
+		abci.EventAttribute{Key: banktypes.AttributeKeySender, Value: accAddrs[0].String()},
 		abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins2.String()},
 	)
 	// events are shifted due to the funding account events
 	require.Equal(abci.Event(event1), events[25])
-	require.Equal(abci.Event(event2), events[27])
+	require.Equal(abci.Event(event2), events[28]) // 28 (instead of 27) because we emit all transfer events at the end.
 	require.Equal(abci.Event(event3), events[29])
 }
 

x/bank/keeper/send.go

@@ -11,6 +11,7 @@ import (
 	"cosmossdk.io/math"
 
 	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/internal/conv"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -167,33 +168,43 @@ func (k BaseSendKeeper) InputOutputCoinsProv(ctx context.Context, inputs []types
 
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 
-	// Remove the funds from the inputs first as that's the most common point of failure.
+	// Identify the input accounts and the amounts to remove from each.
+	inputAmounts := make(map[string]sdk.Coins)
+	inputOrder := make([]sdk.AccAddress, 0, len(inputs))
 	for _, input := range inputs {
 		inAddr, err := k.ak.AddressCodec().StringToBytes(input.Address)
 		if err != nil {
 			return err
 		}
 
-		err = k.subUnlockedCoins(ctx, inAddr, input.Coins)
+		key := conv.UnsafeBytesToStr(inAddr)
+		amt, known := inputAmounts[key]
+		if !known {
+			inputOrder = append(inputOrder, inAddr)
+		}
+		inputAmounts[key] = amt.Add(input.Coins...)
+	}
+
+	// Remove the funds from the inputs first as that's the most common point of failure.
+	for _, inAddr := range inputOrder {
+		amt := inputAmounts[conv.UnsafeBytesToStr(inAddr)]
+		err := k.subUnlockedCoins(ctx, inAddr, amt)
 		if err != nil {
 			return err
 		}
 
-		sdkCtx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				sdk.EventTypeMessage,
-				sdk.NewAttribute(types.AttributeKeySender, input.Address),
-			),
-		)
+		sdkCtx.EventManager().EmitEvent(sdk.NewEvent(sdk.EventTypeMessage,
+			sdk.NewAttribute(types.AttributeKeySender, inAddr.String()),
+		))
 	}
 
 	// Create a map of AccAddress (cast to string) to the amount that that address will get.
 	// The keys are the addresses that come back from the send restriction, not necessarily the addresses in the outputs.
 	// Keep track of the order of the output address too since looping over a map is non-deterministic.
-	toOutput := make(map[string]sdk.Coins)
+	outputAmounts := make(map[string]sdk.Coins)
 	outputOrder := make([]sdk.AccAddress, 0, len(outputs))
 	// applySendRestriction will make the call to the send restriction function,
-	// and update the toOutput and outputOrder values accordingly.
+	// and update the outputAmounts and outputOrder values accordingly.
 	applySendRestriction := func(inAddrStr, outAddrStr string, coins sdk.Coins) error {
 		inAddr, err := k.ak.AddressCodec().StringToBytes(inAddrStr)
 		if err != nil {
@@ -208,11 +219,13 @@ func (k BaseSendKeeper) InputOutputCoinsProv(ctx context.Context, inputs []types
 		if err != nil {
 			return err
 		}
-		amt, known := toOutput[string(outAddr)]
+
+		key := conv.UnsafeBytesToStr(outAddr)
+		amt, known := outputAmounts[key]
 		if !known {
 			outputOrder = append(outputOrder, outAddr)
 		}
-		toOutput[string(outAddr)] = amt.Add(coins...)
+		outputAmounts[key] = amt.Add(coins...)
 		return nil
 	}
 
@@ -235,21 +248,13 @@ func (k BaseSendKeeper) InputOutputCoinsProv(ctx context.Context, inputs []types
 		}
 	}
 
-	// Finally, add the coins to the appropriate account(s).
+	// Now, add the coins to the appropriate account(s).
 	for _, outAddr := range outputOrder {
-		amt := toOutput[string(outAddr)]
+		amt := outputAmounts[conv.UnsafeBytesToStr(outAddr)]
 		if err := k.addCoins(ctx, outAddr, amt); err != nil {
 			return err
 		}
 
-		sdkCtx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				types.EventTypeTransfer,
-				sdk.NewAttribute(types.AttributeKeyRecipient, outAddr.String()),
-				sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
-			),
-		)
-
 		// Create account if recipient does not exist.
 		//
 		// NOTE: This should ultimately be removed in favor a more flexible approach
@@ -261,6 +266,43 @@ func (k BaseSendKeeper) InputOutputCoinsProv(ctx context.Context, inputs []types
 		}
 	}
 
+	// Finally, Emit the transfer events. This event strategy differs from the SDK's in a few ways:
+	//  1. All the transfer events are emitted at the end instead of being interspersed with the addCoins events.
+	//  2. There's still a chance that the transfer event does not have a sender but it'll probably be pretty rare.
+	//  3. We allow for there to be multiple inputs, but still do our best to include all three attributes.
+	switch {
+	case len(inputOrder) == 1:
+		sender := inputOrder[0].String()
+		for _, outAddr := range outputOrder {
+			sdkCtx.EventManager().EmitEvent(sdk.NewEvent(types.EventTypeTransfer,
+				sdk.NewAttribute(types.AttributeKeyRecipient, outAddr.String()),
+				sdk.NewAttribute(types.AttributeKeySender, sender),
+				sdk.NewAttribute(sdk.AttributeKeyAmount, outputAmounts[conv.UnsafeBytesToStr(outAddr)].String()),
+			))
+		}
+	case len(outputOrder) == 1:
+		recipient := outputOrder[0].String()
+		for _, inAddr := range inputOrder {
+			sdkCtx.EventManager().EmitEvent(sdk.NewEvent(types.EventTypeTransfer,
+				sdk.NewAttribute(types.AttributeKeyRecipient, recipient),
+				sdk.NewAttribute(types.AttributeKeySender, inAddr.String()),
+				sdk.NewAttribute(sdk.AttributeKeyAmount, inputAmounts[conv.UnsafeBytesToStr(inAddr)].String()),
+			))
+		}
+	default:
+		// There's multiple input addresses AND multiple output addresses.
+		// This can happen when there's multiple inputs (and one output) where the send restriction
+		// changed the destination of at least one, but not all of the transfers. We then go back
+		// to the pre-v0.50.10 strategy of emitting transfer events without a sender.
+		// The senders can still be found in the message->sender events.
+		for _, outAddr := range outputOrder {
+			sdkCtx.EventManager().EmitEvent(sdk.NewEvent(types.EventTypeTransfer,
+				sdk.NewAttribute(types.AttributeKeyRecipient, outAddr.String()),
+				sdk.NewAttribute(sdk.AttributeKeyAmount, outputAmounts[conv.UnsafeBytesToStr(outAddr)].String()),
+			))
+		}
+	}
+
 	return nil
 }