Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FEAT] ICS20-V2 #6352

Merged
merged 36 commits into from
May 27, 2024
Merged

[FEAT] ICS20-V2 #6352

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c07bca9
Adding proto files for ics20-v2 (#6110)
chatton Apr 8, 2024
e66bd89
update amount -> string (#6120)
charleenfei Apr 9, 2024
034f472
Update MsgTransfer to accept sdk.Coins instead of sdk.Coin (#6113)
chatton Apr 9, 2024
4cc6a85
fix: allow base denom with trailing slash (#6148)
crodriguezvega Apr 15, 2024
71f830c
imp: add CurrentVersion, EscrowVersion (#6160)
charleenfei Apr 16, 2024
28ff9b6
chore: add function for converting packet data from v1 to v3 (#6116)
chatton Apr 16, 2024
4e55137
chore: implement required `FungibleTokenPacketData` v3 interface meth…
charleenfei Apr 22, 2024
ca056cf
imp: `getMultiDenomFungibleTokenPacketData`to be used in packet unmar…
charleenfei Apr 29, 2024
147cf17
chore: implement version checking for channel handshake application c…
charleenfei May 6, 2024
9bbfa1a
imp: update transfer authz implementation to account for multi denom …
charleenfei May 6, 2024
4f57916
ics20-v2: backwards compatibility for transfer rpc and packet callbac…
crodriguezvega May 10, 2024
4c333c5
changes to transfer tx CLI to support multiple denoms
crodriguezvega May 14, 2024
0478cb9
import renaming
crodriguezvega May 14, 2024
f39d173
Use type with V2 suffix for package data (#6330)
chatton May 21, 2024
9b39944
Adding additional comments and changing version variable names (#6345)
chatton May 21, 2024
06ca9a5
Merge branch 'main' into merge-main
chatton May 21, 2024
7897ef3
chore: correctly claim capability
chatton May 21, 2024
786a4f1
lint
colin-axner May 21, 2024
5747756
Merge pull request #6346 from cosmos/merge-main
DimitrisJim May 21, 2024
7e2e6df
Merge branch 'main' into feat/ics20-v2
chatton May 22, 2024
a84b0e7
imp: change ics20 events to emit token set (#6348)
colin-axner May 22, 2024
43877df
imp: check length tokens array against maximum allowed (#6349)
crodriguezvega May 22, 2024
8eae033
Modify UnmarshalPacketData interface to allow additional args (#6341)
DimitrisJim May 22, 2024
dbcff45
Refactor packet data unmarshalling to use specific version (#6354)
chatton May 23, 2024
bb69698
Merge branch 'main' into merge-main-2
chatton May 23, 2024
f19a145
chore: fixing tests
chatton May 23, 2024
8f86dda
Merge pull request #6359 from cosmos/merge-main-2
chatton May 23, 2024
d4b06c8
imp: self review comments for ics20-v2 (#6360)
colin-axner May 23, 2024
a9391a4
imp: self review on ics20-v2 part 2 (#6364)
colin-axner May 23, 2024
575403e
chore: move functions from internal/denom back to trace.go (#6368)
DimitrisJim May 23, 2024
50ccd94
imp: ics20 v2 self review part 3 (#6373)
colin-axner May 23, 2024
87eb32e
chore: remove duplicate test case
colin-axner May 23, 2024
e8b9d5a
chore: address minor nits (#6374)
DimitrisJim May 23, 2024
6d40bc6
Refactor msgs_test.go to use expError (#6367)
chatton May 24, 2024
c171059
chore: remove unused chain variable in setup (#6371)
chatton May 24, 2024
9890f53
Merge branch 'main' into feat/ics20-v2
chatton May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 48 additions & 40 deletions e2e/tests/transfer/authz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,18 @@ func (suite *AuthzTransferTestSuite) TestAuthz_MsgTransfer_Succeeds() {
t.Run("broadcast MsgGrant", createMsgGrantFn)

t.Run("broadcast MsgExec for ibc MsgTransfer", func(t *testing.T) {
transferMsg := transfertypes.MsgTransfer{
SourcePort: channelA.PortID,
SourceChannel: channelA.ChannelID,
Token: testvalues.DefaultTransferAmount(chainADenom),
Sender: granterAddress,
Receiver: receiverWalletAddress,
TimeoutHeight: suite.GetTimeoutHeight(ctx, chainB),
}

protoAny, err := codectypes.NewAnyWithValue(&transferMsg)
transferMsg := transfertypes.NewMsgTransfer(
channelA.PortID,
channelA.ChannelID,
testvalues.DefaultTransferCoins(chainADenom),
granterAddress,
receiverWalletAddress,
suite.GetTimeoutHeight(ctx, chainB),
0,
"",
)

protoAny, err := codectypes.NewAnyWithValue(transferMsg)
suite.Require().NoError(err)

msgExec := &authz.MsgExec{
Expand Down Expand Up @@ -175,16 +177,18 @@ func (suite *AuthzTransferTestSuite) TestAuthz_MsgTransfer_Succeeds() {
})

t.Run("exec unauthorized MsgTransfer", func(t *testing.T) {
transferMsg := transfertypes.MsgTransfer{
SourcePort: channelA.PortID,
SourceChannel: channelA.ChannelID,
Token: testvalues.DefaultTransferAmount(chainADenom),
Sender: granterAddress,
Receiver: receiverWalletAddress,
TimeoutHeight: suite.GetTimeoutHeight(ctx, chainB),
}

protoAny, err := codectypes.NewAnyWithValue(&transferMsg)
transferMsg := transfertypes.NewMsgTransfer(
channelA.PortID,
channelA.ChannelID,
testvalues.DefaultTransferCoins(chainADenom),
granterAddress,
receiverWalletAddress,
suite.GetTimeoutHeight(ctx, chainB),
0,
"",
)

protoAny, err := codectypes.NewAnyWithValue(transferMsg)
suite.Require().NoError(err)

msgExec := &authz.MsgExec{
Expand Down Expand Up @@ -255,16 +259,18 @@ func (suite *AuthzTransferTestSuite) TestAuthz_InvalidTransferAuthorizations() {
const invalidSpendAmount = spendLimit + 1

t.Run("broadcast MsgExec for ibc MsgTransfer", func(t *testing.T) {
transferMsg := transfertypes.MsgTransfer{
SourcePort: channelA.PortID,
SourceChannel: channelA.ChannelID,
Token: sdk.Coin{Denom: chainADenom, Amount: sdkmath.NewInt(invalidSpendAmount)},
Sender: granterAddress,
Receiver: receiverWalletAddress,
TimeoutHeight: suite.GetTimeoutHeight(ctx, chainB),
}

protoAny, err := codectypes.NewAnyWithValue(&transferMsg)
transferMsg := transfertypes.NewMsgTransfer(
channelA.PortID,
channelA.ChannelID,
sdk.NewCoins(sdk.Coin{Denom: chainADenom, Amount: sdkmath.NewInt(invalidSpendAmount)}),
granterAddress,
receiverWalletAddress,
suite.GetTimeoutHeight(ctx, chainB),
0,
"",
)

protoAny, err := codectypes.NewAnyWithValue(transferMsg)
suite.Require().NoError(err)

msgExec := &authz.MsgExec{
Expand Down Expand Up @@ -312,16 +318,18 @@ func (suite *AuthzTransferTestSuite) TestAuthz_InvalidTransferAuthorizations() {
invalidWalletAddress := invalidWallet.FormattedAddress()

t.Run("broadcast MsgExec for ibc MsgTransfer", func(t *testing.T) {
transferMsg := transfertypes.MsgTransfer{
SourcePort: channelA.PortID,
SourceChannel: channelA.ChannelID,
Token: sdk.Coin{Denom: chainADenom, Amount: sdkmath.NewInt(spendLimit)},
Sender: granterAddress,
Receiver: invalidWalletAddress,
TimeoutHeight: suite.GetTimeoutHeight(ctx, chainB),
}

protoAny, err := codectypes.NewAnyWithValue(&transferMsg)
transferMsg := transfertypes.NewMsgTransfer(
channelA.PortID,
channelA.ChannelID,
sdk.NewCoins(sdk.Coin{Denom: chainADenom, Amount: sdkmath.NewInt(spendLimit)}),
granterAddress,
invalidWalletAddress,
suite.GetTimeoutHeight(ctx, chainB),
0,
"",
)

protoAny, err := codectypes.NewAnyWithValue(transferMsg)
suite.Require().NoError(err)

msgExec := &authz.MsgExec{
Expand Down
4 changes: 2 additions & 2 deletions e2e/tests/transfer/incentivized_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func (s *IncentivizedTransferTestSuite) TestMsgPayPacketFee_InvalidReceiverAccou
transferAmount := testvalues.DefaultTransferAmount(chainADenom)

t.Run("send IBC transfer", func(t *testing.T) {
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, transferAmount, chainAWallet.FormattedAddress(), testvalues.InvalidAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, sdk.NewCoins(transferAmount), chainAWallet.FormattedAddress(), testvalues.InvalidAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
txResp := s.BroadcastMessages(ctx, chainA, chainAWallet, msgTransfer)
// this message should be successful, as receiver account is not validated on the sending chain.
s.AssertTxSuccess(txResp)
Expand Down Expand Up @@ -323,7 +323,7 @@ func (s *IncentivizedTransferTestSuite) TestMultiMsg_MsgPayPacketFeeSingleSender
})

msgPayPacketFee := feetypes.NewMsgPayPacketFee(testFee, channelA.PortID, channelA.ChannelID, chainAWallet.FormattedAddress(), nil)
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, transferAmount, chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, sdk.NewCoins(transferAmount), chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
resp := s.BroadcastMessages(ctx, chainA, chainAWallet, msgPayPacketFee, msgTransfer)
s.AssertTxSuccess(resp)

Expand Down
8 changes: 4 additions & 4 deletions e2e/tests/transfer/localhost_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (s *LocalhostTransferTestSuite) TestMsgTransfer_Localhost() {

t.Run("channel open init localhost", func(t *testing.T) {
msgChanOpenInit := channeltypes.NewMsgChannelOpenInit(
transfertypes.PortID, transfertypes.Version,
transfertypes.PortID, transfertypes.V2,
channeltypes.UNORDERED, []string{exported.LocalhostConnectionID},
transfertypes.PortID, rlyWallet.FormattedAddress(),
)
Expand All @@ -85,10 +85,10 @@ func (s *LocalhostTransferTestSuite) TestMsgTransfer_Localhost() {

t.Run("channel open try localhost", func(t *testing.T) {
msgChanOpenTry := channeltypes.NewMsgChannelOpenTry(
transfertypes.PortID, transfertypes.Version,
transfertypes.PortID, transfertypes.V2,
channeltypes.UNORDERED, []string{exported.LocalhostConnectionID},
transfertypes.PortID, msgChanOpenInitRes.ChannelId,
transfertypes.Version, localhost.SentinelProof, clienttypes.ZeroHeight(), rlyWallet.FormattedAddress(),
transfertypes.V2, localhost.SentinelProof, clienttypes.ZeroHeight(), rlyWallet.FormattedAddress(),
)

txResp := s.BroadcastMessages(ctx, chainA, rlyWallet, msgChanOpenTry)
Expand All @@ -100,7 +100,7 @@ func (s *LocalhostTransferTestSuite) TestMsgTransfer_Localhost() {
t.Run("channel open ack localhost", func(t *testing.T) {
msgChanOpenAck := channeltypes.NewMsgChannelOpenAck(
transfertypes.PortID, msgChanOpenInitRes.ChannelId,
msgChanOpenTryRes.ChannelId, transfertypes.Version,
msgChanOpenTryRes.ChannelId, transfertypes.V2,
localhost.SentinelProof, clienttypes.ZeroHeight(), rlyWallet.FormattedAddress(),
)

Expand Down
8 changes: 5 additions & 3 deletions e2e/tests/transfer/upgrades_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (

sdkmath "cosmossdk.io/math"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/e2e/testsuite"
"github.com/cosmos/ibc-go/e2e/testsuite/query"
"github.com/cosmos/ibc-go/e2e/testvalues"
Expand Down Expand Up @@ -196,7 +198,7 @@ func (s *TransferChannelUpgradesTestSuite) TestChannelUpgrade_WithFeeMiddleware_
transferAmount := testvalues.DefaultTransferAmount(chainA.Config().Denom)

msgPayPacketFee := feetypes.NewMsgPayPacketFee(testFee, channelA.PortID, channelA.ChannelID, chainAWallet.FormattedAddress(), nil)
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, transferAmount, chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, sdk.NewCoins(transferAmount), chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
resp := s.BroadcastMessages(ctx, chainA, chainAWallet, msgPayPacketFee, msgTransfer)
s.AssertTxSuccess(resp)
})
Expand Down Expand Up @@ -360,7 +362,7 @@ func (s *TransferChannelUpgradesTestSuite) TestChannelUpgrade_WithFeeMiddleware_
s.Require().NoError(err)

s.Require().Equal(channeltypes.OPEN, channel.State, "the channel state is not OPEN")
s.Require().Equal(transfertypes.Version, channel.Version, "the channel version is not ics20-1")
s.Require().Equal(transfertypes.V2, channel.Version, "the channel version is not ics20-2")

errorReceipt, err := query.UpgradeError(ctx, chainA, channelA.PortID, channelA.ChannelID)
s.Require().NoError(err)
Expand All @@ -373,7 +375,7 @@ func (s *TransferChannelUpgradesTestSuite) TestChannelUpgrade_WithFeeMiddleware_
s.Require().NoError(err)

s.Require().Equal(channeltypes.OPEN, channel.State, "the channel state is not OPEN")
s.Require().Equal(transfertypes.Version, channel.Version, "the channel version is not ics20-1")
s.Require().Equal(transfertypes.V2, channel.Version, "the channel version is not ics20-2")

errorReceipt, err := query.UpgradeError(ctx, chainB, channelB.PortID, channelB.ChannelID)
s.Require().NoError(err)
Expand Down
2 changes: 1 addition & 1 deletion e2e/tests/upgrades/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ func (s *UpgradeTestSuite) TestV8ToV8_1ChainUpgrade_ChannelUpgrades() {
transferAmount := testvalues.DefaultTransferAmount(chainA.Config().Denom)

msgPayPacketFee := feetypes.NewMsgPayPacketFee(testFee, channelA.PortID, channelA.ChannelID, chainAWallet.FormattedAddress(), nil)
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, transferAmount, chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
msgTransfer := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, sdk.NewCoins(transferAmount), chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "")
resp := s.BroadcastMessages(ctx, chainA, chainAWallet, msgPayPacketFee, msgTransfer)
s.AssertTxSuccess(resp)
})
Expand Down
8 changes: 6 additions & 2 deletions e2e/testsuite/testsuite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ func (s *E2ETestSuite) ConfigureRelayer(ctx context.Context, chainA, chainB ibc.
pathName := s.generatePathName()

channelOptions := ibc.DefaultChannelOpts()
// TODO: better way to do this.
// For now, set the version to the latest transfer module version
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
channelOptions.Version = transfertypes.V2

if channelOpts != nil {
channelOpts(&channelOptions)
}
Expand Down Expand Up @@ -449,7 +453,7 @@ func (s *E2ETestSuite) GetRelayerExecReporter() *testreporter.RelayerExecReporte
// TransferChannelOptions configures both of the chains to have non-incentivized transfer channels.
func (*E2ETestSuite) TransferChannelOptions() func(options *ibc.CreateChannelOptions) {
return func(opts *ibc.CreateChannelOptions) {
opts.Version = transfertypes.Version
opts.Version = transfertypes.V2
opts.SourcePortName = transfertypes.PortID
opts.DestPortName = transfertypes.PortID
}
Expand All @@ -459,7 +463,7 @@ func (*E2ETestSuite) TransferChannelOptions() func(options *ibc.CreateChannelOpt
func (s *E2ETestSuite) FeeMiddlewareChannelOptions() func(options *ibc.CreateChannelOptions) {
versionMetadata := feetypes.Metadata{
FeeVersion: feetypes.Version,
AppVersion: transfertypes.Version,
AppVersion: transfertypes.V2,
}
versionBytes, err := feetypes.ModuleCdc.MarshalJSON(&versionMetadata)
s.Require().NoError(err)
Expand Down
2 changes: 1 addition & 1 deletion e2e/testsuite/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ func (s *E2ETestSuite) ExecuteGovV1Beta1Proposal(ctx context.Context, chain ibc.
func (s *E2ETestSuite) Transfer(ctx context.Context, chain ibc.Chain, user ibc.Wallet,
portID, channelID string, token sdk.Coin, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string,
) sdk.TxResponse {
msg := transfertypes.NewMsgTransfer(portID, channelID, token, sender, receiver, timeoutHeight, timeoutTimestamp, memo)
msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo)
return s.BroadcastMessages(ctx, chain, user, msg)
}

Expand Down
4 changes: 4 additions & 0 deletions e2e/testvalues/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ func DefaultTransferAmount(denom string) sdk.Coin {
return sdk.Coin{Denom: denom, Amount: sdkmath.NewInt(IBCTransferAmount)}
}

func DefaultTransferCoins(denom string) sdk.Coins {
return sdk.NewCoins(DefaultTransferAmount(denom))
}

func TransferAmount(amount int64, denom string) sdk.Coin {
return sdk.Coin{Denom: denom, Amount: sdkmath.NewInt(amount)}
}
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string)
// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into an InterchainAccountPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (IBCMiddleware) UnmarshalPacketData(bz []byte) (interface{}, error) {
func (IBCMiddleware) UnmarshalPacketData(_ sdk.Context, _, _ string, bz []byte) (interface{}, error) {
var data icatypes.InterchainAccountPacketData
err := data.UnmarshalJSON(bz)
if err != nil {
Expand Down
6 changes: 4 additions & 2 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,13 +1276,15 @@ func (suite *InterchainAccountsTestSuite) TestPacketDataUnmarshalerInterface() {
Memo: "",
}

packetData, err := controller.IBCMiddleware{}.UnmarshalPacketData(expPacketData.GetBytes())
// Context, port identifier and channel identifier are unused for controller.
packetData, err := controller.IBCMiddleware{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

// test invalid packet data
invalidPacketData := []byte("invalid packet data")
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(invalidPacketData)
// Context, port identifier and channel identifier are not used for controller.
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", invalidPacketData)
Comment on lines +1279 to +1287
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for invalid packet data.

The test for invalid packet data correctly checks for an error, which is good practice. However, it would be beneficial to assert the type of error to ensure that it is specifically related to packet data unmarshalling. This can help identify if the correct error handling paths are being triggered.

// Suggested addition to the test to check for specific error type
suite.Require().IsType(icatypes.ErrInvalidPacketData, err, "Expected error type icatypes.ErrInvalidPacketData")

suite.Require().Error(err)
suite.Require().Nil(packetData)
}
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/host/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, pr
// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into an InterchainAccountPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
func (IBCModule) UnmarshalPacketData(_ sdk.Context, _, _ string, bz []byte) (interface{}, error) {
var data icatypes.InterchainAccountPacketData
err := data.UnmarshalJSON(bz)
if err != nil {
Expand Down
6 changes: 4 additions & 2 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,13 +883,15 @@ func (suite *InterchainAccountsTestSuite) TestPacketDataUnmarshalerInterface() {
Memo: "",
}

packetData, err := icahost.IBCModule{}.UnmarshalPacketData(expPacketData.GetBytes())
// Context, port identifier and channel identifier are unused for host.
packetData, err := icahost.IBCModule{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

// test invalid packet data
invalidPacketData := []byte("invalid packet data")
packetData, err = icahost.IBCModule{}.UnmarshalPacketData(invalidPacketData)
// Context, port identifier and channel identifier are unused for host.
packetData, err = icahost.IBCModule{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", invalidPacketData)
suite.Require().Error(err)
suite.Require().Nil(packetData)
}
38 changes: 20 additions & 18 deletions modules/apps/27-interchain-accounts/host/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,16 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(found)

msg := &transfertypes.MsgTransfer{
SourcePort: transferPath.EndpointA.ChannelConfig.PortID,
SourceChannel: transferPath.EndpointA.ChannelID,
Token: sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)),
Sender: interchainAccountAddr,
Receiver: suite.chainA.SenderAccount.GetAddress().String(),
TimeoutHeight: suite.chainB.GetTimeoutHeight(),
TimeoutTimestamp: uint64(0),
}
msg := transfertypes.NewMsgTransfer(
transferPath.EndpointA.ChannelConfig.PortID,
transferPath.EndpointA.ChannelID,
sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100))),
interchainAccountAddr,
suite.chainA.SenderAccount.GetAddress().String(),
suite.chainB.GetTimeoutHeight(),
0,
"",
)

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg}, encoding)
suite.Require().NoError(err)
Expand All @@ -376,15 +377,16 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(found)

msg := &transfertypes.MsgTransfer{
SourcePort: transferPath.EndpointA.ChannelConfig.PortID,
SourceChannel: transferPath.EndpointA.ChannelID,
Token: sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)),
Sender: interchainAccountAddr,
Receiver: "",
TimeoutHeight: suite.chainB.GetTimeoutHeight(),
TimeoutTimestamp: uint64(0),
}
msg := transfertypes.NewMsgTransfer(
transferPath.EndpointA.ChannelConfig.PortID,
transferPath.EndpointA.ChannelID,
sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100))),
interchainAccountAddr,
"",
suite.chainB.GetTimeoutHeight(),
0,
"",
)

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg}, encoding)
suite.Require().NoError(err)
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,11 +472,11 @@ func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string)
// UnmarshalPacketData attempts to use the underlying app to unmarshal the packet data.
// If the underlying app does not support the PacketDataUnmarshaler interface, an error is returned.
// This function implements the optional PacketDataUnmarshaler interface required for ADR 008 support.
func (im IBCMiddleware) UnmarshalPacketData(bz []byte) (interface{}, error) {
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) {
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler)
if !ok {
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
}

return unmarshaler.UnmarshalPacketData(bz)
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz)
Comment on lines +475 to +481
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling when the underlying app does not support packet data unmarshaling.

The error message in UnmarshalPacketData should provide more context about the failure, including the port and channel IDs involved, to aid in debugging.

- return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
+ return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app for port %s and channel %s does not implement PacketDataUnmarshaler", portID, channelID)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) {
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler)
if !ok {
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
}
return unmarshaler.UnmarshalPacketData(bz)
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz)
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) {
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler)
if !ok {
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app for port %s and channel %s does not implement PacketDataUnmarshaler", portID, channelID)
}
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz)

}
6 changes: 4 additions & 2 deletions modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1573,7 +1573,8 @@ func (suite *FeeTestSuite) TestPacketDataUnmarshalerInterface() {
feeModule, ok := cbs.(porttypes.PacketDataUnmarshaler)
suite.Require().True(ok)

packetData, err := feeModule.UnmarshalPacketData(ibcmock.MockPacketData)
// Context, port identifier, channel identifier are not used in current wiring of fee.
packetData, err := feeModule.UnmarshalPacketData(suite.chainA.GetContext(), "", "", ibcmock.MockPacketData)
suite.Require().NoError(err)
suite.Require().Equal(ibcmock.MockPacketData, packetData)
}
Expand All @@ -1582,7 +1583,8 @@ func (suite *FeeTestSuite) TestPacketDataUnmarshalerInterfaceError() {
// test the case when the underlying application cannot be casted to a PacketDataUnmarshaler
mockFeeMiddleware := ibcfee.NewIBCMiddleware(nil, feekeeper.Keeper{})

_, err := mockFeeMiddleware.UnmarshalPacketData(ibcmock.MockPacketData)
// Context, port identifier, channel identifier are not used in mockFeeMiddleware.
_, err := mockFeeMiddleware.UnmarshalPacketData(suite.chainA.GetContext(), "", "", ibcmock.MockPacketData)
expError := errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
suite.Require().ErrorIs(err, expError)
}
Loading
Loading