Merge #86: Backports #78, #80, #81 into v6

* 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]> * make compliant with ibc-go version and fix lint * remove unused make test command * regen protos * add lint check to CI --------- Co-authored-by: Andrew Gouin <[email protected]> Co-authored-by: Max Kupriianov <[email protected]>
strangelove-ventures · May 23, 2023 · c0b7072 · c0b7072
1 parent 6e26c54
commit c0b7072
Show file tree

Hide file tree

Showing 9 changed files with 281 additions and 72 deletions.
diff --git a/.github/workflows/golangci.yaml b/.github/workflows/golangci.yaml
@@ -0,0 +1,27 @@
+name: golangci-lint
+on:
+  push:
+    tags:
+      - v*
+    branches:
+      - master
+      - main
+  pull_request:
+permissions:
+  contents: read
+  # Optional: allow read access to pull request. Use with `only-new-issues` option.
+  # pull-requests: read
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/setup-go@v3
+        with:
+          go-version: 1.19
+      - uses: actions/checkout@v3
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v3
+        with:
+          # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
+          version: latest
diff --git a/Makefile b/Makefile
@@ -176,7 +176,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/go.mod b/go.mod
@@ -8,6 +8,7 @@ replace (
 )
 
 require (
+	cosmossdk.io/errors v1.0.0-beta.7
 	github.com/armon/go-metrics v0.4.1
 	github.com/cosmos/cosmos-sdk v0.46.9
 	github.com/cosmos/ibc-go/v6 v6.1.0
@@ -26,7 +27,6 @@ require (
 )
 
 require (
-	cosmossdk.io/errors v1.0.0-beta.7 // indirect
 	cosmossdk.io/math v1.0.0-beta.4 // indirect
 	filippo.io/edwards25519 v1.0.0-rc.1 // indirect
 	github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect

diff --git a/router/ibc_middleware.go b/router/ibc_middleware.go
@@ -6,6 +6,7 @@ import (
 	"strings"
 	"time"
 
+	errorsmod "cosmossdk.io/errors"
 	"github.com/armon/go-metrics"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -119,6 +120,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.
@@ -136,40 +149,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)
@@ -203,10 +204,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
 	}
 
@@ -254,7 +254,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 
 	var ack channeltypes.Acknowledgement
 	if err := channeltypes.SubModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
-		return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
+		return errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
 	}
 
 	inFlightPacket := im.keeper.GetAndClearInFlightPacket(ctx, packet.SourceChannel, packet.SourcePort, packet.Sequence)

diff --git a/router/keeper/keeper.go b/router/keeper/keeper.go
@@ -6,6 +6,7 @@ import (
 	"strings"
 	"time"
 
+	errorsmod "cosmossdk.io/errors"
 	"github.com/armon/go-metrics"
 	"github.com/cosmos/cosmos-sdk/codec"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
@@ -98,9 +99,9 @@ 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")
+		return errorsmod.Wrap(err, "could not retrieve module from port-id")
 	}
 
 	// for forwarded packets, the funds were moved into an escrow account if the denom originated on this chain.
@@ -113,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,
@@ -181,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,
@@ -223,7 +224,7 @@ func (k *Keeper) ForwardTransferPacket(
 			k.Logger(ctx).Error("packetForwardMiddleware error funding community pool",
 				"error", err,
 			)
-			return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
+			return errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
 		}
 	}
 
@@ -236,7 +237,7 @@ func (k *Keeper) ForwardTransferPacket(
 			k.Logger(ctx).Error("packetForwardMiddleware error marshaling next as JSON",
 				"error", err,
 			)
-			return sdkerrors.Wrapf(sdkerrors.ErrJSONMarshal, err.Error())
+			return errorsmod.Wrapf(sdkerrors.ErrJSONMarshal, err.Error())
 		}
 		memo = string(memoBz)
 	}
@@ -270,7 +271,7 @@ func (k *Keeper) ForwardTransferPacket(
 			"amount", packetCoin.Amount.String(), "denom", packetCoin.Denom,
 			"error", err,
 		)
-		return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
+		return errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
 	}
 
 	// Store the following information in keeper:
@@ -304,11 +305,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_test.go b/router/module_test.go
@@ -18,8 +18,9 @@ import (
 )
 
 var (
-	testDenom  = "uatom"
-	testAmount = "100"
+	testDenom     = "uatom"
+	testAmount    = "100"
+	testAmount256 = "100000000000000000000"
 
 	testSourcePort         = "transfer"
 	testSourceChannel      = "channel-10"
@@ -65,6 +66,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()
@@ -139,6 +170,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()
@@ -229,6 +287,73 @@ 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)