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.

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

imp: getMultiDenomFungibleTokenPacketDatato be used in packet unmarshalling/conversion #6226

Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
dbe408a
chore: adding proto files for ics20-v2
chatton Apr 8, 2024
845b269
chore: add newline
chatton Apr 8, 2024
69b2643
chore: modify MsgTransfer to accept coins instead of coin
chatton Apr 8, 2024
8f4eaaa
Merge branch 'feat/ics20-v2' into cian/issue#5799-update-msg-transfer
chatton Apr 8, 2024
d8bfd45
chore: reverted unintentional comment changes
chatton Apr 8, 2024
e32a13d
chore: reverted unintentional comment changes
chatton Apr 8, 2024
e1b52c3
chore: adding test for conversion fn
chatton Apr 8, 2024
0e0e478
chore: fix e2e linter
chatton Apr 8, 2024
00e6c10
chore: adding docs
chatton Apr 8, 2024
8b5eb77
chore: addressing PR feedback
chatton Apr 8, 2024
62d76e9
chore: remove duplicate import
chatton Apr 8, 2024
2cc1804
chore: addressing PR feedback
chatton Apr 9, 2024
4b42e80
Merge branch 'cian/issue#5799-update-msg-transfer' into cian/issue#61…
chatton Apr 9, 2024
3da8e01
chore: moved extration logic into internal package
chatton Apr 9, 2024
dee5a30
chore: commented out failing test
chatton Apr 9, 2024
f7e5792
chore: adding link to issue
chatton Apr 9, 2024
bd34b69
Merge branch 'feat/ics20-v2' into cian/issue#6114-add-conversion-func…
chatton Apr 9, 2024
880b24d
chore: remove duplicate import
chatton Apr 9, 2024
8cad136
chore: fix linting errors
chatton Apr 9, 2024
5ebba40
FungibleTokenPacketData interface methods + tests
charleenfei Apr 9, 2024
867746f
linter
charleenfei Apr 9, 2024
24ed388
wip: token validation
charleenfei Apr 11, 2024
a666243
Merge branch 'cian/issue#6114-add-conversion-function' into charly/is…
charleenfei Apr 11, 2024
a85fa71
update trace identifier validation in Token + tests
charleenfei Apr 15, 2024
e10863e
rm Printf
charleenfei Apr 15, 2024
81c4d50
update with pr review
charleenfei Apr 15, 2024
2504c10
add CurrentVersion, EscrowVersion, update tests
charleenfei Apr 15, 2024
3f9027b
pr review
charleenfei Apr 15, 2024
c417be5
fix e2e tests
charleenfei Apr 16, 2024
15d949f
Merge branch 'feat/ics20-v2' into cian/issue#6114-add-conversion-func…
charleenfei Apr 16, 2024
555d2f2
pr review
charleenfei Apr 16, 2024
602237d
update e2e test version
charleenfei Apr 16, 2024
3b421b3
linter
charleenfei Apr 16, 2024
365b6a4
update hardcoded transfer channel version from interchaintest
charleenfei Apr 16, 2024
ab12fcb
update hardcoded transfer channel version from interchaintest
charleenfei Apr 16, 2024
a5cbcac
Merge branch 'charly/add_versions' into charly/msg_transfer
charleenfei Apr 16, 2024
88d0b4f
Merge branch 'feat/ics20-v2' into charly/msg_transfer
charleenfei Apr 16, 2024
b6301a6
Merge branch 'cian/issue#6114-add-conversion-function' into charly/ms…
charleenfei Apr 16, 2024
a7b5a18
Merge branch 'charly/issue#5795-implement-required-fungibletokenpacke…
charleenfei Apr 16, 2024
390f41f
wip packet unmarshaller
charleenfei Apr 16, 2024
f886b8b
Merge branch 'feat/ics20-v2' into charly/issue#5796-add-getfungibleto…
charleenfei Apr 22, 2024
42077f2
wip
charleenfei Apr 22, 2024
9302bed
wip testing
charleenfei Apr 25, 2024
c0db0de
update test
charleenfei Apr 25, 2024
6ca22ce
linter
charleenfei Apr 25, 2024
c305a87
rm unnecessary version changes
charleenfei Apr 25, 2024
bc122a1
rm unnecessary artifacts
charleenfei Apr 25, 2024
00f94ea
update callbacks test
charleenfei Apr 25, 2024
72f670d
update comment
crodriguezvega Apr 28, 2024
af8ee7a
pr review
charleenfei Apr 29, 2024
1982ed5
Merge branch 'charly/issue#5796-add-getfungibletokenpacketdatav2-and-…
charleenfei Apr 29, 2024
3a8a670
rename getMultiDenomFungibleTokenPacketData to unmarshalPacketDataByt…
crodriguezvega Apr 29, 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
18 changes: 16 additions & 2 deletions modules/apps/callbacks/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
multidenom "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types/v3"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channelkeeper "github.com/cosmos/ibc-go/v8/modules/core/04-channel/keeper"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
Expand Down Expand Up @@ -950,14 +951,27 @@ func (s *CallbacksTestSuite) TestUnmarshalPacketData() {
unmarshalerStack, ok := transferStack.(types.CallbacksCompatibleModule)
s.Require().True(ok)

expPacketData := transfertypes.FungibleTokenPacketData{
initialPacketData := transfertypes.FungibleTokenPacketData{
Denom: ibctesting.TestCoin.Denom,
Amount: ibctesting.TestCoin.Amount.String(),
Sender: ibctesting.TestAccAddress,
Receiver: ibctesting.TestAccAddress,
Memo: fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address":"%s"}}`, ibctesting.TestAccAddress, ibctesting.TestAccAddress),
}
data := expPacketData.GetBytes()
data := initialPacketData.GetBytes()

expPacketData := multidenom.FungibleTokenPacketData{
Tokens: []*multidenom.Token{
{
Denom: initialPacketData.Denom,
Amount: initialPacketData.Amount,
Trace: []string{""},
},
},
Sender: initialPacketData.Sender,
Receiver: initialPacketData.Receiver,
Memo: initialPacketData.Memo,
}

packetData, err := unmarshalerStack.UnmarshalPacketData(data)
s.Require().NoError(err)
Expand Down
31 changes: 27 additions & 4 deletions modules/apps/transfer/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
convertinternal "github.com/cosmos/ibc-go/v8/modules/apps/transfer/internal/convert"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
multidenom "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types/v3"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (can be resolved later on when we do a walkthrough as well): I would probably alias the import with a different name (maybe v3types as you do in another file?), since ICS20 v2 will not only feature multi denom, but also path forwarding, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i used multidenom for now, as i wasn't sure if we will change the v3 naming (referring to FungibleTokenDataPacket) for ics20v2

channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
Expand Down Expand Up @@ -170,6 +172,27 @@ func (IBCModule) OnChanCloseConfirm(
return nil
}

func (IBCModule) getMultiDenomFungibleTokenPacketData(bz []byte) (multidenom.FungibleTokenPacketData, error) {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
// TODO: remove support for this function parsing v1 packet data
// TODO: explicit check for packet data type against app version

var datav1 types.FungibleTokenPacketData
if err := json.Unmarshal(bz, &datav1); err == nil {
if len(datav1.Denom) != 0 {
return convertinternal.PacketDataV1ToV3(datav1), nil
}
}

var data multidenom.FungibleTokenPacketData
if err := json.Unmarshal(bz, &data); err == nil {
if len(data.Tokens) != 0 {
return data, nil
}
}
Comment on lines +179 to +191
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up using protobuf for encoding the packet data in ICS20 v2, would that at least mitigate the problems of the brute force approach? Like if the packet data is protobuf-encoded, the json unmarshalling should fail, right? And also the other way around?


return multidenom.FungibleTokenPacketData{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data")
}

// OnRecvPacket implements the IBCModule interface. A successful acknowledgement
// is returned if the packet data is successfully decoded and the receive application
// logic returns without error.
Expand Down Expand Up @@ -351,11 +374,11 @@ func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, pr
// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into a FungibleTokenPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
var packetData types.FungibleTokenPacketData
if err := json.Unmarshal(bz, &packetData); err != nil {
func (im IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
ftpd, err := im.getMultiDenomFungibleTokenPacketData(bz)
if err != nil {
return nil, err
}

return packetData, nil
return ftpd, nil
}
84 changes: 75 additions & 9 deletions modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
multidenom "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types/v3"
connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
Expand Down Expand Up @@ -474,8 +475,8 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() {
sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()

data []byte
expPacketData types.FungibleTokenPacketData
data []byte
initialPacketData interface{}
)

testCases := []struct {
Expand All @@ -484,30 +485,87 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() {
expPass bool
}{
{
"success: valid packet data with memo",
"success: valid packet data single denom -> multidenom conversion with memo",
func() {
expPacketData = types.FungibleTokenPacketData{
initialPacketData = types.FungibleTokenPacketData{
Denom: ibctesting.TestCoin.Denom,
Amount: ibctesting.TestCoin.Amount.String(),
Sender: sender,
Receiver: receiver,
Memo: "some memo",
}
data = expPacketData.GetBytes()

data = initialPacketData.(types.FungibleTokenPacketData).GetBytes()
},
true,
},
{
"success: valid packet data without memo",
"success: valid packet data single denom -> multidenom conversion without memo",
func() {
expPacketData = types.FungibleTokenPacketData{
initialPacketData = types.FungibleTokenPacketData{
Denom: ibctesting.TestCoin.Denom,
Amount: ibctesting.TestCoin.Amount.String(),
Sender: sender,
Receiver: receiver,
Memo: "",
}
data = expPacketData.GetBytes()

data = initialPacketData.(types.FungibleTokenPacketData).GetBytes()
},
true,
},
{
"success: valid packet data single denom with trace -> multidenom conversion with trace",
func() {
initialPacketData = types.FungibleTokenPacketData{
Denom: "transfer/channel-0/atom",
Amount: ibctesting.TestCoin.Amount.String(),
Sender: sender,
Receiver: receiver,
Memo: "",
}

data = initialPacketData.(types.FungibleTokenPacketData).GetBytes()
},
true,
},
{
"success: valid packet data multidenom with memo",
func() {
initialPacketData = multidenom.FungibleTokenPacketData{
Tokens: []*multidenom.Token{
{
Denom: "atom",
Amount: ibctesting.TestCoin.Amount.String(),
Trace: []string{"transfer/channel-0"},
},
},
Sender: sender,
Receiver: receiver,
Memo: "some memo",
}

data = initialPacketData.(multidenom.FungibleTokenPacketData).GetBytes()
},
true,
},
{
"success: valid packet data multidenom without memo",
func() {
initialPacketData = multidenom.FungibleTokenPacketData{
Tokens: []*multidenom.Token{
{
Denom: ibctesting.TestCoin.Denom,
Amount: ibctesting.TestCoin.Amount.String(),
Trace: []string{""},
},
},
Sender: sender,
Receiver: receiver,
Memo: "",
}

data = initialPacketData.(multidenom.FungibleTokenPacketData).GetBytes()
},
true,
},
Expand All @@ -529,7 +587,15 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)
if _, ok := initialPacketData.(types.FungibleTokenPacketData); ok {
// Note: testing of the denom trace parsing/conversion should be done as part of testing internal conversion functions
suite.Require().Equal(initialPacketData.(types.FungibleTokenPacketData).Amount, packetData.(multidenom.FungibleTokenPacketData).Tokens[0].Amount)
suite.Require().Equal(initialPacketData.(types.FungibleTokenPacketData).Sender, packetData.(multidenom.FungibleTokenPacketData).Sender)
suite.Require().Equal(initialPacketData.(types.FungibleTokenPacketData).Receiver, packetData.(multidenom.FungibleTokenPacketData).Receiver)
suite.Require().Equal(initialPacketData.(types.FungibleTokenPacketData).Memo, packetData.(multidenom.FungibleTokenPacketData).Memo)
} else {
suite.Require().Equal(initialPacketData.(multidenom.FungibleTokenPacketData), packetData.(multidenom.FungibleTokenPacketData))
}
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
} else {
suite.Require().Error(err)
suite.Require().Nil(packetData)
Expand Down
8 changes: 4 additions & 4 deletions modules/apps/transfer/internal/convert/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ func PacketDataV1ToV3(packetData v1types.FungibleTokenPacketData) v3types.Fungib
func extractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) {
v1DenomTrace := v1types.ParseDenomTrace(v1Denom)

splitPath := strings.Split(v1DenomTrace.Path, "/")

// if the path slice is empty, then the base denom is the full native denom.
if len(splitPath) == 0 {
return v1DenomTrace.BaseDenom, nil
if v1DenomTrace.Path == "" {
return v1DenomTrace.BaseDenom, []string{""}
}

splitPath := strings.Split(v1DenomTrace.Path, "/")

// this condition should never be reached.
if len(splitPath)%2 != 0 {
panic("pathSlice length is not even")
Expand Down
13 changes: 13 additions & 0 deletions modules/apps/transfer/internal/convert/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@ func TestConvertPacketV1ToPacketV3(t *testing.T) {
}, sender, receiver, ""),
nil,
},
{
"success with empty trace",
v1types.NewFungibleTokenPacketData("atom", "1000", sender, receiver, ""),
v3types.NewFungibleTokenPacketData(
[]*v3types.Token{
{
Denom: "atom",
Amount: "1000",
Trace: []string{""},
},
}, sender, receiver, ""),
nil,
},
{
"success: base denom with '/'",
v1types.NewFungibleTokenPacketData("transfer/channel-0/atom/withslash", "1000", sender, receiver, ""),
Expand Down
Loading