Merge PR #84: Backports #78, #80, #81 into v4

* Fix same bool being used for all 3 (#81) * fix: middleware panic upon receiving amount that is not int64; added test (#78) resolves #77 * Fix: Allows timeout field to accept both uint64 and string durations (#80) * timeout accetps both string and time.duration * feedback * handle invalid unmarshalls * Update router/ibc_middleware.go Co-authored-by: Andrew Gouin <[email protected]> * add test case for empty valid json --------- Co-authored-by: Andrew Gouin <[email protected]> * fix tests * remove unused protos * regen-protos * fix lint --------- Co-authored-by: Andrew Gouin <[email protected]> Co-authored-by: Max Kupriianov <[email protected]>
strangelove-ventures · May 23, 2023 · 17bd5de · 17bd5de
1 parent 07ccf2c
commit 17bd5de
Show file tree

Hide file tree

Showing 9 changed files with 311 additions and 117 deletions.
diff --git a/Makefile b/Makefile
@@ -177,7 +177,7 @@ go.sum: go.mod
 ###############################################################################
 
 test: test-unit
-test-all: test-unit test-ledger-mock test-race test-cover
+test-all: test-unit test-ledger-mock test-race
 
 TEST_PACKAGES=./...
 TEST_TARGETS := test-unit test-unit-amino test-unit-proto test-ledger-mock test-race test-ledger test-race

diff --git a/router/ibc_middleware.go b/router/ibc_middleware.go
@@ -118,6 +118,18 @@ func getDenomForThisChain(port, channel, counterpartyPort, counterpartyChannel,
 	return transfertypes.ParseDenomTrace(prefixedDenom).IBCDenom()
 }
 
+// getBoolFromAny returns the bool value is any is a valid bool, otherwise false.
+func getBoolFromAny(value any) bool {
+	if value == nil {
+		return false
+	}
+	boolVal, ok := value.(bool)
+	if !ok {
+		return false
+	}
+	return boolVal
+}
+
 // OnRecvPacket checks the memo field on this packet and if the metadata inside's root key indicates this packet
 // should be handled by the swap middleware it attempts to perform a swap. If the swap is successful
 // the underlying application's OnRecvPacket callback is invoked, an ack error is returned otherwise.
@@ -135,40 +147,28 @@ func (im IBCMiddleware) OnRecvPacket(
 		"sequence", packet.Sequence,
 		"src-channel", packet.SourceChannel, "src-port", packet.SourcePort,
 		"dst-channel", packet.DestinationChannel, "dst-port", packet.DestinationPort,
-		"amount", data.Amount, "denom", data.Denom,
+		"amount", data.Amount, "denom", data.Denom, "memo", data.Memo,
 	)
 
-	m := &types.PacketMetadata{}
-	err := json.Unmarshal([]byte(data.Memo), m)
-	if err != nil || m.Forward == nil {
+	d := make(map[string]interface{})
+	err := json.Unmarshal([]byte(data.Memo), &d)
+	if err != nil || d["forward"] == nil {
 		// not a packet that should be forwarded
 		im.keeper.Logger(ctx).Debug("packetForwardMiddleware OnRecvPacket forward metadata does not exist")
 		return im.app.OnRecvPacket(ctx, packet, relayer)
 	}
+	m := &types.PacketMetadata{}
+	err = json.Unmarshal([]byte(data.Memo), m)
+	if err != nil {
+		return channeltypes.NewErrorAcknowledgement(fmt.Errorf("packetForwardMiddleware error parsing forward metadata, %s", err))
+	}
 
 	metadata := m.Forward
 
-	var processed, nonrefundable, disableDenomComposition bool
 	goCtx := ctx.Context()
-	p := goCtx.Value(types.ProcessedKey{})
-	nr := goCtx.Value(types.NonrefundableKey{})
-	ddc := goCtx.Value(types.DisableDenomCompositionKey{})
-
-	if p != nil {
-		if pb, ok := p.(bool); ok {
-			processed = pb
-		}
-	}
-	if nr != nil {
-		if nrb, ok := p.(bool); ok {
-			nonrefundable = nrb
-		}
-	}
-	if ddc != nil {
-		if ddcb, ok := p.(bool); ok {
-			disableDenomComposition = ddcb
-		}
-	}
+	processed := getBoolFromAny(goCtx.Value(types.ProcessedKey{}))
+	nonrefundable := getBoolFromAny(goCtx.Value(types.NonrefundableKey{}))
+	disableDenomComposition := getBoolFromAny(goCtx.Value(types.DisableDenomCompositionKey{}))
 
 	if err := metadata.Validate(); err != nil {
 		return channeltypes.NewErrorAcknowledgement(err)
@@ -202,10 +202,9 @@ func (im IBCMiddleware) OnRecvPacket(
 
 	token := sdk.NewCoin(denomOnThisChain, amountInt)
 
-	var timeout time.Duration
-	if metadata.Timeout.Nanoseconds() > 0 {
-		timeout = metadata.Timeout
-	} else {
+	timeout := time.Duration(metadata.Timeout)
+
+	if timeout.Nanoseconds() <= 0 {
 		timeout = im.forwardTimeout
 	}
 

diff --git a/router/keeper/keeper.go b/router/keeper/keeper.go
@@ -99,7 +99,7 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(
 	ack channeltypes.Acknowledgement,
 ) error {
 	// Lookup module by channel capability
-	_, cap, err := k.channelKeeper.LookupModuleByChannel(ctx, inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId)
+	_, chanCap, err := k.channelKeeper.LookupModuleByChannel(ctx, inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId)
 	if err != nil {
 		return sdkerrors.Wrap(err, "could not retrieve module from port-id")
 	}
@@ -114,7 +114,7 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(
 			ackResult := fmt.Sprintf("packet forward failed after point of no return: %s", ack.GetError())
 			newAck := channeltypes.NewResultAcknowledgement([]byte(ackResult))
 
-			return k.ics4Wrapper.WriteAcknowledgement(ctx, cap, channeltypes.Packet{
+			return k.ics4Wrapper.WriteAcknowledgement(ctx, chanCap, channeltypes.Packet{
 				Data:               inFlightPacket.PacketData,
 				Sequence:           inFlightPacket.RefundSequence,
 				SourcePort:         inFlightPacket.PacketSrcPortId,
@@ -182,7 +182,7 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(
 		}
 	}
 
-	return k.ics4Wrapper.WriteAcknowledgement(ctx, cap, channeltypes.Packet{
+	return k.ics4Wrapper.WriteAcknowledgement(ctx, chanCap, channeltypes.Packet{
 		Data:               inFlightPacket.PacketData,
 		Sequence:           inFlightPacket.RefundSequence,
 		SourcePort:         inFlightPacket.PacketSrcPortId,
@@ -302,11 +302,13 @@ func (k *Keeper) ForwardTransferPacket(
 	store.Set(key, bz)
 
 	defer func() {
-		telemetry.SetGaugeWithLabels(
-			[]string{"tx", "msg", "ibc", "transfer"},
-			float32(token.Amount.Int64()),
-			[]metrics.Label{telemetry.NewLabel(coretypes.LabelDenom, token.Denom)},
-		)
+		if token.Amount.IsInt64() {
+			telemetry.SetGaugeWithLabels(
+				[]string{"tx", "msg", "ibc", "transfer"},
+				float32(token.Amount.Int64()),
+				[]metrics.Label{telemetry.NewLabel(coretypes.LabelDenom, token.Denom)},
+			)
+		}
 
 		telemetry.IncrCounterWithLabels(
 			[]string{"ibc", types.ModuleName, "send"},

diff --git a/router/module.go b/router/module.go
@@ -39,10 +39,10 @@ func (AppModuleBasic) Name() string {
 }
 
 // RegisterLegacyAminoCodec implements AppModuleBasic interface
-func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {}
+func (AppModuleBasic) RegisterLegacyAminoCodec(_ *codec.LegacyAmino) {}
 
 // RegisterInterfaces registers module concrete types into protobuf Any.
-func (AppModuleBasic) RegisterInterfaces(registry codectypes.InterfaceRegistry) {}
+func (AppModuleBasic) RegisterInterfaces(_ codectypes.InterfaceRegistry) {}
 
 // DefaultGenesis returns default genesis state as raw bytes for the ibc
 // router module.
@@ -51,7 +51,7 @@ func (AppModuleBasic) DefaultGenesis(cdc codec.JSONCodec) json.RawMessage {
 }
 
 // ValidateGenesis performs genesis state validation for the router module.
-func (AppModuleBasic) ValidateGenesis(cdc codec.JSONCodec, config client.TxEncodingConfig, bz json.RawMessage) error {
+func (AppModuleBasic) ValidateGenesis(cdc codec.JSONCodec, _ client.TxEncodingConfig, bz json.RawMessage) error {
 	var gs types.GenesisState
 	if err := cdc.UnmarshalJSON(bz, &gs); err != nil {
 		return fmt.Errorf("failed to unmarshal %s genesis state: %w", types.ModuleName, err)
@@ -61,7 +61,7 @@ func (AppModuleBasic) ValidateGenesis(cdc codec.JSONCodec, config client.TxEncod
 }
 
 // RegisterRESTRoutes implements AppModuleBasic interface
-func (AppModuleBasic) RegisterRESTRoutes(clientCtx client.Context, rtr *mux.Router) {}
+func (AppModuleBasic) RegisterRESTRoutes(_ client.Context, _ *mux.Router) {}
 
 // RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the ibc-router module.
 func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) {
@@ -92,7 +92,7 @@ func NewAppModule(k *keeper.Keeper) AppModule {
 }
 
 // RegisterInvariants implements the AppModule interface
-func (AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {}
+func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route implements the AppModule interface
 func (am AppModule) Route() sdk.Route {
@@ -134,30 +134,30 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
 func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion }
 
 // BeginBlock implements the AppModule interface
-func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {}
+func (am AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {}
 
 // EndBlock implements the AppModule interface
-func (am AppModule) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) []abci.ValidatorUpdate {
+func (am AppModule) EndBlock(_ sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
 	return []abci.ValidatorUpdate{}
 }
 
 // AppModuleSimulation functions
 
 // GenerateGenesisState creates a randomized GenState of the router module.
-func (AppModule) GenerateGenesisState(simState *module.SimulationState) {}
+func (AppModule) GenerateGenesisState(_ *module.SimulationState) {}
 
 // ProposalContents doesn't return any content functions for governance proposals.
 func (AppModule) ProposalContents(_ module.SimulationState) []simtypes.WeightedProposalContent {
 	return nil
 }
 
 // RandomizedParams creates randomized ibc-router param changes for the simulator.
-func (AppModule) RandomizedParams(r *rand.Rand) []simtypes.ParamChange {
+func (AppModule) RandomizedParams(_ *rand.Rand) []simtypes.ParamChange {
 	return nil
 }
 
 // RegisterStoreDecoder registers a decoder for router module's types
-func (am AppModule) RegisterStoreDecoder(sdr sdk.StoreDecoderRegistry) {}
+func (am AppModule) RegisterStoreDecoder(_ sdk.StoreDecoderRegistry) {}
 
 // WeightedOperations returns the all the router module operations with their respective weights.
 func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.WeightedOperation {

diff --git a/router/module_test.go b/router/module_test.go
@@ -17,8 +17,9 @@ import (
 )
 
 var (
-	testDenom  = "uatom"
-	testAmount = "100"
+	testDenom     = "uatom"
+	testAmount    = "100"
+	testAmount256 = "100000000000000000000"
 
 	testSourcePort         = "transfer"
 	testSourceChannel      = "channel-10"
@@ -64,6 +65,36 @@ func transferPacket(t *testing.T, receiver string, metadata any) channeltypes.Pa
 	}
 }
 
+func transferPacket256(t *testing.T, receiver string, metadata any) channeltypes.Packet {
+	t.Helper()
+	transferPacket := transfertypes.FungibleTokenPacketData{
+		Denom:    testDenom,
+		Amount:   testAmount256,
+		Receiver: receiver,
+	}
+
+	if metadata != nil {
+		if mStr, ok := metadata.(string); ok {
+			transferPacket.Memo = mStr
+		} else {
+			memo, err := json.Marshal(metadata)
+			require.NoError(t, err)
+			transferPacket.Memo = string(memo)
+		}
+	}
+
+	transferData, err := transfertypes.ModuleCdc.MarshalJSON(&transferPacket)
+	require.NoError(t, err)
+
+	return channeltypes.Packet{
+		SourcePort:         testSourcePort,
+		SourceChannel:      testSourceChannel,
+		DestinationPort:    testDestinationPort,
+		DestinationChannel: testDestinationChannel,
+		Data:               transferData,
+	}
+}
+
 func TestOnRecvPacket_EmptyPacket(t *testing.T) {
 	ctl := gomock.NewController(t)
 	defer ctl.Finish()
@@ -138,6 +169,33 @@ func TestOnRecvPacket_NoForward(t *testing.T) {
 	require.Equal(t, "test", string(expectedAck.GetResult()))
 }
 
+func TestOnRecvPacket_NoMemo(t *testing.T) {
+	ctl := gomock.NewController(t)
+	defer ctl.Finish()
+	setup := test.NewTestSetup(t, ctl)
+	ctx := setup.Initializer.Ctx
+	cdc := setup.Initializer.Marshaler
+	forwardMiddleware := setup.ForwardMiddleware
+
+	// Test data
+	senderAccAddr := test.AccAddress()
+	packet := transferPacket(t, "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k", "{}")
+
+	// Expected mocks
+	gomock.InOrder(
+		setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packet, senderAccAddr).
+			Return(channeltypes.NewResultAcknowledgement([]byte("test"))),
+	)
+
+	ack := forwardMiddleware.OnRecvPacket(ctx, packet, senderAccAddr)
+	require.True(t, ack.Success())
+
+	expectedAck := &channeltypes.Acknowledgement{}
+	err := cdc.UnmarshalJSON(ack.Acknowledgement(), expectedAck)
+	require.NoError(t, err)
+	require.Equal(t, "test", string(expectedAck.GetResult()))
+}
+
 func TestOnRecvPacket_RecvPacketFailed(t *testing.T) {
 	ctl := gomock.NewController(t)
 	defer ctl.Finish()
@@ -228,6 +286,72 @@ func TestOnRecvPacket_ForwardNoFee(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func TestOnRecvPacket_ForwardAmountInt256(t *testing.T) {
+	var err error
+	ctl := gomock.NewController(t)
+	defer ctl.Finish()
+	setup := test.NewTestSetup(t, ctl)
+	ctx := setup.Initializer.Ctx
+	cdc := setup.Initializer.Marshaler
+	forwardMiddleware := setup.ForwardMiddleware
+
+	// Test data
+	const (
+		hostAddr = "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
+		destAddr = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
+		port     = "transfer"
+		channel  = "channel-0"
+	)
+	denom := makeIBCDenom(testDestinationPort, testDestinationChannel, testDenom)
+	senderAccAddr := test.AccAddress()
+
+	amount256, ok := sdk.NewIntFromString(testAmount256)
+	require.True(t, ok)
+
+	testCoin := sdk.NewCoin(denom, amount256)
+	packetOrig := transferPacket256(t, hostAddr, &types.PacketMetadata{
+		Forward: &types.ForwardMetadata{
+			Receiver: destAddr,
+			Port:     port,
+			Channel:  channel,
+		},
+	})
+	packetFwd := transferPacket256(t, destAddr, nil)
+
+	acknowledgement := channeltypes.NewResultAcknowledgement([]byte("test"))
+	successAck := cdc.MustMarshalJSON(&acknowledgement)
+
+	// Expected mocks
+	gomock.InOrder(
+		setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr).
+			Return(acknowledgement),
+
+		setup.Mocks.TransferKeeperMock.EXPECT().Transfer(
+			sdk.WrapSDKContext(ctx),
+			transfertypes.NewMsgTransfer(
+				port,
+				channel,
+				testCoin,
+				hostAddr,
+				destAddr,
+				keeper.DefaultTransferPacketTimeoutHeight,
+				uint64(ctx.BlockTime().UnixNano())+uint64(keeper.DefaultForwardTransferPacketTimeoutTimestamp.Nanoseconds()),
+			),
+		).Return(&transfertypes.MsgTransferResponse{Sequence: 0}, nil),
+
+		setup.Mocks.IBCModuleMock.EXPECT().OnAcknowledgementPacket(ctx, packetFwd, successAck, senderAccAddr).
+			Return(nil),
+	)
+
+	// chain B with router module receives packet and forwards. ack should be nil so that it is not written yet.
+	ack := forwardMiddleware.OnRecvPacket(ctx, packetOrig, senderAccAddr)
+	require.Nil(t, ack)
+
+	// ack returned from chain C
+	err = forwardMiddleware.OnAcknowledgementPacket(ctx, packetFwd, successAck, senderAccAddr)
+	require.NoError(t, err)
+}
+
 func TestOnRecvPacket_ForwardWithFee(t *testing.T) {
 	var err error
 	ctl := gomock.NewController(t)