fix(eibc): improve eibc memo error handling (#838)

Co-authored-by: Omri <[email protected]>

ibctesting/delayed_ack_test.go

@@ -106,7 +106,16 @@ func (suite *DelayedAckTestSuite) TestTransferRollappToHubNotFinalized() {
 	suite.Require().True(ok)
 	coinToSendToB := sdk.NewCoin(sdk.DefaultBondDenom, amount)
 
-	msg := types.NewMsgTransfer(rollappEndpoint.ChannelConfig.PortID, rollappEndpoint.ChannelID, coinToSendToB, suite.rollappChain.SenderAccount.GetAddress().String(), suite.hubChain.SenderAccount.GetAddress().String(), timeoutHeight, 0, "")
+	msg := types.NewMsgTransfer(
+		rollappEndpoint.ChannelConfig.PortID,
+		rollappEndpoint.ChannelID,
+		coinToSendToB,
+		suite.rollappChain.SenderAccount.GetAddress().String(),
+		suite.hubChain.SenderAccount.GetAddress().String(),
+		timeoutHeight,
+		0,
+		"",
+	)
 	res, err := suite.rollappChain.SendMsgs(msg)
 	suite.Require().NoError(err) // message committed
 
@@ -115,7 +124,7 @@ func (suite *DelayedAckTestSuite) TestTransferRollappToHubNotFinalized() {
 
 	// relay send
 	err = path.RelayPacket(packet)
-	// expeting error as no AcknowledgePacket expected
+	// expecting error as no AcknowledgePacket expected
 	suite.Require().Error(err) // relay committed
 	found := hubIBCKeeper.ChannelKeeper.HasPacketAcknowledgement(hubEndpoint.Chain.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
 	suite.Require().False(found)

ibctesting/eibc_test.go

@@ -236,7 +236,7 @@ func (suite *EIBCTestSuite) TestEIBCDemandOrderFulfillment() {
 				packet := suite.TransferRollappToHub(path, IBCSenderAccount, fulfiller.String(), tc.fulfillerInitialIBCDenomBalance, memo, false)
 				// Finalize rollapp state - at this state no demand order was fulfilled
 				currentRollappBlockHeight = uint64(suite.rollappChain.GetContext().BlockHeight())
-				_, err := suite.FinalizeRollappState(rollappStateIndex, uint64(currentRollappBlockHeight))
+				_, err := suite.FinalizeRollappState(rollappStateIndex, currentRollappBlockHeight)
 				suite.Require().NoError(err)
 				// Check the fulfiller balance was updated fully with the IBC amount
 				isUpdated := false
@@ -315,7 +315,7 @@ func (suite *EIBCTestSuite) TestEIBCDemandOrderFulfillment() {
 
 			// Finalize rollapp and check fulfiller balance was updated with fee
 			currentRollappBlockHeight = uint64(suite.rollappChain.GetContext().BlockHeight())
-			evts, err := suite.FinalizeRollappState(rollappStateIndex, uint64(currentRollappBlockHeight))
+			evts, err := suite.FinalizeRollappState(rollappStateIndex, currentRollappBlockHeight)
 			suite.Require().NoError(err)
 
 			ack, err := ibctesting.ParseAckFromEvents(evts)

x/delayedack/eibc.go

@@ -1,7 +1,7 @@
 package delayedack
 
 import (
-	"encoding/json"
+	"errors"
 	"fmt"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -13,11 +13,6 @@ import (
 	eibctypes "github.com/dymensionxyz/dymension/v3/x/eibc/types"
 )
 
-const (
-	eibcMemoObjectName = "eibc"
-	PFMMemoObjectName  = "forward"
-)
-
 // eIBCDemandOrderHandler handles the eibc packet by creating a demand order from the packet data and saving it in the store.
 // the rollapp packet can be of type ON_RECV or ON_TIMEOUT.
 // If the rollapp packet is of type ON_RECV, the function will validate the memo and create a demand order from the packet data.
@@ -28,24 +23,14 @@ func (im IBCMiddleware) eIBCDemandOrderHandler(ctx sdk.Context, rollappPacket co
 
 	switch t := rollappPacket.Type; t {
 	case commontypes.RollappPacket_ON_RECV:
-		// Handle eibc demand order if exists - Start by validating the memo
-		memo := make(map[string]interface{})
-		err := json.Unmarshal([]byte(data.Memo), &memo)
-		if err != nil || memo[eibcMemoObjectName] == nil {
-			logger.Debug("Memo is empty or failed to unmarshal", "memo", data.Memo)
+		var err error
+		packetMetaData, err = types.ParsePacketMetadata(data.Memo)
+		if errors.Is(err, types.ErrMemoUnmarshal) || errors.Is(err, types.ErrMemoEibcEmpty) {
+			logger.Debug("skipping demand order creation - no eibc memo provided")
 			return nil
 		}
-
-		// Currently not supporting eibc with PFM: https://github.com/dymensionxyz/dymension/issues/599
-		if memo[PFMMemoObjectName] != nil {
-			err = fmt.Errorf("EIBC packet with PFM is currently not supported")
-			return err
-		}
-		// Unmarshal the packet metadata from the memo
-		err = json.Unmarshal([]byte(data.Memo), packetMetaData)
 		if err != nil {
-			logger.Error("parsing packet metadata from memo", "error", err)
-			return nil
+			return err
 		}
 	case commontypes.RollappPacket_ON_TIMEOUT, commontypes.RollappPacket_ON_ACK:
 		// Calculate the fee by multiplying the fee by the price
@@ -61,7 +46,6 @@ func (im IBCMiddleware) eIBCDemandOrderHandler(ctx sdk.Context, rollappPacket co
 		case commontypes.RollappPacket_ON_ACK:
 			feeMultiplier = im.keeper.ErrAckFee(ctx)
 		}
-
 		fee := amountDec.Mul(feeMultiplier).TruncateInt()
 		if !fee.IsPositive() {
 			logger.Debug("fee is not positive, skipping demand order creation", "fee type", t, "fee", fee.String(), "multiplier", feeMultiplier.String())
@@ -74,22 +58,14 @@ func (im IBCMiddleware) eIBCDemandOrderHandler(ctx sdk.Context, rollappPacket co
 		}
 	}
 
-	// Validate the packet metadata
-	if err := packetMetaData.ValidateBasic(); err != nil {
-		logger.Error("error validating packet metadata", "error", err)
-		return nil
-	}
-	// Create the eibc demand order
 	eibcDemandOrder, err := im.createDemandOrderFromIBCPacket(data, &rollappPacket, *packetMetaData.EIBC)
 	if err != nil {
-		err = fmt.Errorf("create eibc demand order: %s", err)
-		return err
+		return fmt.Errorf("create eibc demand order: %s", err)
 	}
-	// Save the eibc order in the store
+
 	err = im.keeper.SetDemandOrder(ctx, eibcDemandOrder)
 	if err != nil {
-		err = fmt.Errorf("save eibc demand order: %s", err)
-		return err
+		return fmt.Errorf("set eibc demand order: %s", err)
 	}
 	return nil
 }
@@ -105,25 +81,22 @@ func (im IBCMiddleware) createDemandOrderFromIBCPacket(fungibleTokenPacketData t
 	if err := fungibleTokenPacketData.ValidateBasic(); err != nil {
 		return nil, err
 	}
+	if err := eibcMetaData.ValidateBasic(); err != nil {
+		return nil, fmt.Errorf("validate eibc metadata: %w", err)
+	}
 	// Verify the original recipient is not a blocked sender otherwise could potentially use eibc to bypass it
 	if im.keeper.BlockedAddr(fungibleTokenPacketData.Receiver) {
 		return nil, fmt.Errorf("not allowed to receive funds: receiver: %s", fungibleTokenPacketData.Receiver)
 	}
-	// Calculate the demand order price and validate it
-	amountInt, ok := sdk.NewIntFromString(fungibleTokenPacketData.Amount)
-	if !ok || !amountInt.IsPositive() {
-		return nil, fmt.Errorf("convert amount to positive integer: %s", fungibleTokenPacketData.Amount)
-	}
+	// Calculate the demand order price and validate it,
+	amt, _ := sdk.NewIntFromString(fungibleTokenPacketData.Amount) // guaranteed ok and positive by above validation
+
 	// Get the fee from the memo
-	fee := eibcMetaData.Fee
-	feeInt, ok := sdk.NewIntFromString(fee)
-	if !ok || !feeInt.IsPositive() {
-		return nil, fmt.Errorf("convert fee to positive integer: %s", fee)
-	}
-	if amountInt.LT(feeInt) {
-		return nil, fmt.Errorf("fee cannot be larger than amount")
-	}
+	fee, _ := eibcMetaData.FeeInt() // guaranteed ok by above validation
 
+	if amt.LT(fee) {
+		return nil, fmt.Errorf("fee cannot be larger than amount: fee: %s: amt :%s", fee, fungibleTokenPacketData.Amount)
+	}
 	/*
 		   In case of timeout/errack:
 		       fee = fee_multiplier*transfer_amount
@@ -138,11 +111,11 @@ func (im IBCMiddleware) createDemandOrderFromIBCPacket(fungibleTokenPacketData t
 		           demander balance += (transfer_amount - fee)
 		              equivalent to += (1-fee_multiplier)*transfer_amount
 	*/
-	demandOrderPrice := amountInt.Sub(feeInt).String()
+	demandOrderPrice := amt.Sub(fee)
 
+	// Get the denom for the demand order
 	var demandOrderDenom string
 	var demandOrderRecipient string
-	// Get the denom for the demand order
 	switch rollappPacket.Type {
 	case commontypes.RollappPacket_ON_TIMEOUT:
 		fallthrough
@@ -154,15 +127,12 @@ func (im IBCMiddleware) createDemandOrderFromIBCPacket(fungibleTokenPacketData t
 		demandOrderDenom = im.getEIBCTransferDenom(*rollappPacket.Packet, fungibleTokenPacketData)
 		demandOrderRecipient = fungibleTokenPacketData.Receiver // who we tried to send to
 	}
-	// Create the demand order and validate it
-	eibcDemandOrder, err := eibctypes.NewDemandOrder(*rollappPacket, demandOrderPrice, fee, demandOrderDenom, demandOrderRecipient)
-	if err != nil {
-		return nil, fmt.Errorf("create eibc demand order: %s", err)
-	}
-	if err := eibcDemandOrder.Validate(); err != nil {
+
+	order := eibctypes.NewDemandOrder(*rollappPacket, demandOrderPrice, fee, demandOrderDenom, demandOrderRecipient)
+	if err := order.Validate(); err != nil {
 		return nil, fmt.Errorf("validate eibc data: %s", err)
 	}
-	return eibcDemandOrder, nil
+	return order, nil
 }
 
 // getEIBCTransferDenom returns the actual denom that will be credited to the eIBC fulfiller.

x/delayedack/types/errors.go

@@ -7,7 +7,6 @@ import (
 var (
 	ErrCanOnlyUpdatePendingPacket = errorsmod.Register(ModuleName, 1, "can only update pending packet")
 	ErrRollappPacketDoesNotExist  = errorsmod.Register(ModuleName, 2, "rollapp packet does not exist")
-	ErrInvalidEIBCFee             = errorsmod.Register(ModuleName, 3, "invalid eibc fee")
 	ErrEmptyEpochIdentifier       = errorsmod.Register(ModuleName, 4, "empty epoch identifier")
 	ErrMismatchedStateRoots       = errorsmod.Register(ModuleName, 5, "mismatched state roots")
 	ErrMismatchedSequencer        = errorsmod.Register(ModuleName, 6, "mismatched sequencer")

x/delayedack/types/packet_metadata.go

@@ -1,6 +1,11 @@
 package types
 
 import (
+	"encoding/json"
+	"fmt"
+
+	"cosmossdk.io/math"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
@@ -17,9 +22,54 @@ func (p PacketMetadata) ValidateBasic() error {
 }
 
 func (e EIBCMetadata) ValidateBasic() error {
-	_, ok := sdk.NewIntFromString(e.Fee)
-	if !ok {
-		return ErrInvalidEIBCFee
+	_, err := e.FeeInt()
+	if err != nil {
+		return fmt.Errorf("fee: %w", err)
 	}
 	return nil
 }
+
+var ErrEIBCFeeNotPositiveInt = fmt.Errorf("eibc fee is not a positive integer")
+
+func (e EIBCMetadata) FeeInt() (math.Int, error) {
+	i, ok := sdk.NewIntFromString(e.Fee)
+	if !ok || !i.IsPositive() {
+		return math.Int{}, ErrEIBCFeeNotPositiveInt
+	}
+	return i, nil
+}
+
+const (
+	memoObjectKeyEIBC = "eibc"
+	memoObjectKeyPFM  = "forward"
+)
+
+var (
+	ErrMemoUnmarshal         = fmt.Errorf("unmarshal memo")
+	ErrEIBCMetadataUnmarshal = fmt.Errorf("unmarshal eibc metadata")
+	ErrMemoHashPFMandEIBC    = fmt.Errorf("EIBC packet with PFM is currently not supported")
+	ErrMemoEibcEmpty         = fmt.Errorf("memo eIBC field is missing")
+)
+
+func ParsePacketMetadata(input string) (*PacketMetadata, error) {
+	bz := []byte(input)
+
+	memo := make(map[string]any)
+	err := json.Unmarshal(bz, &memo)
+	if err != nil {
+		return nil, ErrMemoUnmarshal
+	}
+	if memo[memoObjectKeyEIBC] == nil {
+		return nil, ErrMemoEibcEmpty
+	}
+	if memo[memoObjectKeyPFM] != nil {
+		// Currently not supporting eibc with PFM: https://github.com/dymensionxyz/dymension/issues/599
+		return nil, ErrMemoHashPFMandEIBC
+	}
+	var metadata PacketMetadata
+	err = json.Unmarshal(bz, &metadata)
+	if err != nil {
+		return nil, ErrEIBCMetadataUnmarshal
+	}
+	return &metadata, nil
+}

x/delayedack/types/packet_metadata_test.go

@@ -0,0 +1,67 @@
+package types
+
+import (
+	"reflect"
+	"testing"
+)
+
+func Test_parsePacketMetadata(t *testing.T) {
+	type args struct {
+		input string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *PacketMetadata
+		wantErr bool
+	}{
+		{
+			"valid",
+			args{
+				`{"eibc":{"fee":"100"}}`,
+			},
+			&PacketMetadata{
+				EIBC: &EIBCMetadata{
+					Fee: "100",
+				},
+			},
+			false,
+		},
+		{
+			"invalid - misquoted fee",
+			args{
+				`{"eibc":{"fee":100}}`,
+			},
+			nil,
+			true,
+		},
+		{
+			"invalid - pfm",
+			args{
+				`{"forward":{}}`,
+			},
+			nil,
+			true,
+		},
+		{
+			"invalid - empty",
+			args{
+				``,
+			},
+			nil,
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParsePacketMetadata(tt.args.input)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parsePacketMetadata() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("parsePacketMetadata() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

x/eibc/keeper/demand_order_test.go

@@ -22,9 +22,8 @@ func (suite *KeeperTestSuite) TestListDemandOrdersByStatus() {
 			ProofHeight: 2,
 			Packet:      &packet,
 		}
-		demandOrder, err := types.NewDemandOrder(*rollappPacket, "150", "50", "stake", demandOrderAddresses[i].String())
-		suite.Require().NoError(err)
-		err = keeper.SetDemandOrder(ctx, demandOrder)
+		demandOrder := types.NewDemandOrder(*rollappPacket, math.NewIntFromUint64(150), math.NewIntFromUint64(50), "stake", demandOrderAddresses[i].String())
+		err := keeper.SetDemandOrder(ctx, demandOrder)
 		suite.Require().NoError(err)
 	}
 	// Get the demand orders with status active

x/eibc/keeper/grpc_query_test.go

@@ -33,8 +33,7 @@ func (suite *KeeperTestSuite) TestQueryDemandOrderById() {
 
 	// Create a demand order with status pending
 	recipientAddress := apptesting.AddTestAddrs(suite.App, suite.Ctx, 1, math.NewInt(1000))[0]
-	demandOrder, err := types.NewDemandOrder(*rollappPacket, "150", "50", "stake", recipientAddress.String())
-	suite.Require().NoError(err)
+	demandOrder := types.NewDemandOrder(*rollappPacket, math.NewIntFromUint64(150), math.NewIntFromUint64(50), "stake", recipientAddress.String())
 	err = keeper.SetDemandOrder(suite.Ctx, demandOrder)
 	suite.Require().NoError(err)
 
@@ -69,12 +68,11 @@ func (suite *KeeperTestSuite) TestQueryDemandOrdersByStatus() {
 		// Use a unique address for each demand order
 		recipientAddress := demandOrderAddresses[i].String()
 
-		demandOrder, err := types.NewDemandOrder(*rollappPacket, "150", "50", "stake", recipientAddress)
-		suite.Require().NoError(err)
+		demandOrder := types.NewDemandOrder(*rollappPacket, math.NewIntFromUint64(150), math.NewIntFromUint64(50), "stake", recipientAddress)
 		// Assert needed type of status for packet
 		demandOrder.TrackingPacketStatus = status
 
-		err = keeper.SetDemandOrder(suite.Ctx, demandOrder)
+		err := keeper.SetDemandOrder(suite.Ctx, demandOrder)
 		suite.Require().NoError(err)
 
 		// Query demand orders by status