Accept JSON or JSON escaped string for next (#63)

Author: agouin

README.md

@@ -81,25 +81,40 @@ Utilizing the packet `memo` field, instructions can be encoded as JSON for multi
 
 In the case of a timeout after 10 minutes for either forward, the packet would be retried up to 2 times, at which case an error ack would be written to issue a refund on the prior chain.
 
-`next` is a string since it could be any `memo` for the next hop, so it must be an escaped JSON string.
+`next` is the `memo` to pass for the next transfer hop. Per `memo` intended usage of a JSON string, it should be either JSON which will be Marshaled retaining key order, or an escaped JSON string which will be passed directly.
 
+`next` as JSON
 ```
 {
   "forward": {
     "receiver": "chain-c-bech32-address",
     "port": "transfer",
-    "channel": "channel-123"
+    "channel": "channel-123",
+    "timeout": "10m",
+    "retries": 2,
+    "next": {
+      "forward": {
+        "receiver": "chain-d-bech32-address",
+        "port": "transfer",
+        "channel":"channel-234",
+        "timeout":"10m",
+        "retries": 2
+      }
+    }
+  }
+}
+```
+
+`next` as escaped JSON string
+```
+{
+  "forward": {
+    "receiver": "chain-c-bech32-address",
+    "port": "transfer",
+    "channel": "channel-123",
     "timeout": "10m",
-    "retries: 2,
-    "next" : "\{
-      \"forward\":\{
-        \"receiver\":\"chain-d-bech32-address\",
-        \"port\":\"transfer\",
-        \"channel\":\"channel-234\",
-        \"timeout\":\"10m\",
-        \"retries\":2
-      \}
-    \}"
+    "retries": 2,
+    "next": "{\"forward\":{\"receiver\":\"chain-d-bech32-address\",\"port\":\"transfer\",\"channel\":\"channel-234\",\"timeout\":\"10m\",\"retries\":2}}"
   }
 }
 ```

go.mod

@@ -13,6 +13,7 @@ require (
 	github.com/golang/protobuf v1.5.2
 	github.com/gorilla/mux v1.8.0
 	github.com/grpc-ecosystem/grpc-gateway v1.16.0
+	github.com/iancoleman/orderedmap v0.2.0
 	github.com/spf13/cobra v1.6.1
 	github.com/stretchr/testify v1.8.1
 	github.com/tendermint/tendermint v0.34.24

go.sum

@@ -300,6 +300,8 @@ github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T
 github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3 h1:aSVUgRRRtOrZOC1fYmY9gV0e9z/Iu+xNVSASWjsuyGU=
 github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3/go.mod h1:5PC6ZNPde8bBqU/ewGZig35+UIZtw9Ytxez8/q5ZyFE=
 github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
+github.com/iancoleman/orderedmap v0.2.0 h1:sq1N/TFpYH++aViPcaKjys3bDClUEU7s5B+z6jq8pNA=
+github.com/iancoleman/orderedmap v0.2.0/go.mod h1:N0Wam8K1arqPXNWjMo21EXnBPOPp36vB07FNRdD2geA=
 github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
 github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
 github.com/improbable-eng/grpc-web v0.15.0 h1:BN+7z6uNXZ1tQGcNAuaU1YjsLTApzkjt2tzCixLaUPQ=

router/keeper/keeper.go

@@ -1,6 +1,7 @@
 package keeper
 
 import (
+	"encoding/json"
 	"fmt"
 	"strings"
 	"time"
@@ -230,7 +231,14 @@ func (k *Keeper) ForwardTransferPacket(
 
 	// set memo for next transfer with next from this transfer.
 	if metadata.Next != nil {
-		memo = *metadata.Next
+		memoBz, err := json.Marshal(metadata.Next)
+		if err != nil {
+			k.Logger(ctx).Error("packetForwardMiddleware error marshaling next as JSON",
+				"error", err,
+			)
+			return sdkerrors.Wrapf(sdkerrors.ErrJSONMarshal, err.Error())
+		}
+		memo = string(memoBz)
 	}
 
 	msgTransfer := transfertypes.NewMsgTransfer(
@@ -356,7 +364,9 @@ func (k *Keeper) RetryTimeout(
 	}
 
 	if data.Memo != "" {
-		metadata.Next = &data.Memo
+		if err := json.Unmarshal([]byte(data.Memo), metadata.Next); err != nil {
+			return fmt.Errorf("error unmarshaling memo json: %w", err)
+		}
 	}
 
 	amount, ok := sdk.NewIntFromString(data.Amount)

router/module.go

@@ -62,7 +62,7 @@ func (AppModuleBasic) RegisterRESTRoutes(clientCtx client.Context, rtr *mux.Rout
 
 // RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the ibc-router module.
 func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) {
-	types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx))
+	_ = types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx))
 }
 
 // GetTxCmd implements AppModuleBasic interface

router/module_test.go

@@ -10,6 +10,7 @@ import (
 	transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
 	channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
 	"github.com/golang/mock/gomock"
+	"github.com/iancoleman/orderedmap"
 	"github.com/strangelove-ventures/packet-forward-middleware/v6/router/keeper"
 	"github.com/strangelove-ventures/packet-forward-middleware/v6/router/types"
 	"github.com/strangelove-ventures/packet-forward-middleware/v6/test"
@@ -35,17 +36,21 @@ func emptyPacket() channeltypes.Packet {
 	return channeltypes.Packet{}
 }
 
-func transferPacket(t *testing.T, receiver string, metadata *types.PacketMetadata) channeltypes.Packet {
+func transferPacket(t *testing.T, receiver string, metadata any) channeltypes.Packet {
 	transferPacket := transfertypes.FungibleTokenPacketData{
 		Denom:    testDenom,
 		Amount:   testAmount,
 		Receiver: receiver,
 	}
 
 	if metadata != nil {
-		memo, err := json.Marshal(metadata)
-		require.NoError(t, err)
-		transferPacket.Memo = string(memo)
+		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)
@@ -171,10 +176,12 @@ func TestOnRecvPacket_ForwardNoFee(t *testing.T) {
 	forwardMiddleware := setup.ForwardMiddleware
 
 	// Test data
-	hostAddr := "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
-	destAddr := "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
-	port := "transfer"
-	channel := "channel-0"
+	const (
+		hostAddr = "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
+		destAddr = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
+		port     = "transfer"
+		channel  = "channel-0"
+	)
 	denom := makeIBCDenom(testDestinationPort, testDestinationChannel, testDenom)
 	senderAccAddr := test.AccAddress()
 	testCoin := sdk.NewCoin(denom, sdk.NewInt(100))
@@ -235,10 +242,12 @@ func TestOnRecvPacket_ForwardWithFee(t *testing.T) {
 	setup.Keepers.RouterKeeper.SetParams(ctx, types.NewParams(sdk.NewDecWithPrec(10, 2)))
 
 	// Test data
-	hostAddr := "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
-	destAddr := "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
-	port := "transfer"
-	channel := "channel-0"
+	const (
+		hostAddr = "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
+		destAddr = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
+		port     = "transfer"
+		channel  = "channel-0"
+	)
 	denom := makeIBCDenom(testDestinationPort, testDestinationChannel, testDenom)
 	senderAccAddr := test.AccAddress()
 	hostAccAddr := test.AccAddressFromBech32(t, hostAddr)
@@ -293,7 +302,7 @@ func TestOnRecvPacket_ForwardWithFee(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestOnRecvPacket_ForwardMultihop(t *testing.T) {
+func TestOnRecvPacket_ForwardMultihopStringNext(t *testing.T) {
 	var err error
 	ctl := gomock.NewController(t)
 	defer ctl.Finish()
@@ -303,12 +312,15 @@ func TestOnRecvPacket_ForwardMultihop(t *testing.T) {
 	forwardMiddleware := setup.ForwardMiddleware
 
 	// Test data
-	hostAddr := "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
-	hostAddr2 := "cosmos1q4p4gx889lfek5augdurrjclwtqvjhuntm6j4m"
-	destAddr := "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
-	port := "transfer"
-	channel := "channel-0"
-	channel2 := "channel-1"
+	const (
+		hostAddr  = "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs"
+		hostAddr2 = "cosmos1q4p4gx889lfek5augdurrjclwtqvjhuntm6j4m"
+		destAddr  = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
+		port      = "transfer"
+		channel   = "channel-0"
+		channel2  = "channel-1"
+	)
+
 	denom := makeIBCDenom(testDestinationPort, testDestinationChannel, testDenom)
 	senderAccAddr := test.AccAddress()
 	senderAccAddr2 := test.AccAddress()
@@ -323,14 +335,131 @@ func TestOnRecvPacket_ForwardMultihop(t *testing.T) {
 	nextBz, err := json.Marshal(nextMetadata)
 	require.NoError(t, err)
 
-	next := string(nextBz)
+	packetOrig := transferPacket(t, hostAddr, &types.PacketMetadata{
+		Forward: &types.ForwardMetadata{
+			Receiver: hostAddr2,
+			Port:     port,
+			Channel:  channel,
+			Next:     types.NewJSONObject(false, nextBz, orderedmap.OrderedMap{}),
+		},
+	})
+	packet2 := transferPacket(t, hostAddr2, nextMetadata)
+	packetFwd := transferPacket(t, destAddr, nil)
+
+	memo1, err := json.Marshal(nextMetadata)
+	require.NoError(t, err)
+
+	msgTransfer1 := transfertypes.NewMsgTransfer(
+		port,
+		channel,
+		testCoin,
+		hostAddr,
+		hostAddr2,
+		keeper.DefaultTransferPacketTimeoutHeight,
+		uint64(ctx.BlockTime().UnixNano())+uint64(keeper.DefaultForwardTransferPacketTimeoutTimestamp.Nanoseconds()),
+		string(memo1),
+	)
+
+	// no memo on final forward
+	msgTransfer2 := transfertypes.NewMsgTransfer(
+		port,
+		channel2,
+		testCoin,
+		hostAddr2,
+		destAddr,
+		keeper.DefaultTransferPacketTimeoutHeight,
+		uint64(ctx.BlockTime().UnixNano())+uint64(keeper.DefaultForwardTransferPacketTimeoutTimestamp.Nanoseconds()),
+		"",
+	)
+
+	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),
+			msgTransfer1,
+		).Return(&apptypes.MsgTransferResponse{Sequence: 0}, nil),
+
+		setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packet2, senderAccAddr2).
+			Return(acknowledgement),
+
+		setup.Mocks.TransferKeeperMock.EXPECT().Transfer(
+			sdk.WrapSDKContext(ctx),
+			msgTransfer2,
+		).Return(&apptypes.MsgTransferResponse{Sequence: 0}, nil),
+
+		setup.Mocks.IBCModuleMock.EXPECT().OnAcknowledgementPacket(ctx, packetFwd, successAck, senderAccAddr2).
+			Return(nil),
+
+		setup.Mocks.IBCModuleMock.EXPECT().OnAcknowledgementPacket(ctx, packet2, 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)
+
+	// chain C with router module receives packet and forwards. ack should be nil so that it is not written yet.
+	ack = forwardMiddleware.OnRecvPacket(ctx, packet2, senderAccAddr2)
+	require.Nil(t, ack)
+
+	// ack returned from chain D to chain C
+	err = forwardMiddleware.OnAcknowledgementPacket(ctx, packetFwd, successAck, senderAccAddr2)
+	require.NoError(t, err)
+
+	// ack returned from chain C to chain B
+	err = forwardMiddleware.OnAcknowledgementPacket(ctx, packet2, successAck, senderAccAddr)
+	require.NoError(t, err)
+}
+
+func TestOnRecvPacket_ForwardMultihopJSONNext(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"
+		hostAddr2 = "cosmos1q4p4gx889lfek5augdurrjclwtqvjhuntm6j4m"
+		destAddr  = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k"
+		port      = "transfer"
+		channel   = "channel-0"
+		channel2  = "channel-1"
+	)
+
+	denom := makeIBCDenom(testDestinationPort, testDestinationChannel, testDenom)
+	senderAccAddr := test.AccAddress()
+	senderAccAddr2 := test.AccAddress()
+	testCoin := sdk.NewCoin(denom, sdk.NewInt(100))
+	nextMetadata := &types.PacketMetadata{
+		Forward: &types.ForwardMetadata{
+			Receiver: destAddr,
+			Port:     port,
+			Channel:  channel2,
+		},
+	}
+	nextBz, err := json.Marshal(nextMetadata)
+	require.NoError(t, err)
+
+	nextJSONObject := new(types.JSONObject)
+	err = json.Unmarshal(nextBz, nextJSONObject)
+	require.NoError(t, err)
 
 	packetOrig := transferPacket(t, hostAddr, &types.PacketMetadata{
 		Forward: &types.ForwardMetadata{
 			Receiver: hostAddr2,
 			Port:     port,
 			Channel:  channel,
-			Next:     &next,
+			Next:     nextJSONObject,
 		},
 	})
 	packet2 := transferPacket(t, hostAddr2, nextMetadata)