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

feat(transfer): add unwinding ability #6656

Merged
merged 53 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
013f883
Create ontimeoutpacket test for forwarding
bznein Jun 13, 2024
83f4582
Propagate ack on A
bznein Jun 13, 2024
59b93ed
Refactoring
bznein Jun 13, 2024
ff35ada
Minor changes
bznein Jun 13, 2024
926d8da
Added comments
bznein Jun 13, 2024
8a3dd36
Merge branch 'feat/ics20-v2-path-forwarding' into bznein/6384/ontimeo…
bznein Jun 13, 2024
3f5dc6c
Fix type name.
bznein Jun 14, 2024
10592e1
Gofumpt
bznein Jun 14, 2024
907de8e
Merge branch 'feat/ics20-v2-path-forwarding' into bznein/6384/ontimeo…
bznein Jun 14, 2024
6787c74
Update modules/apps/transfer/keeper/relay_forwarding_test.go
bznein Jun 14, 2024
37375ba
Update modules/apps/transfer/keeper/relay_forwarding_test.go
bznein Jun 14, 2024
65a0d0a
Update modules/apps/transfer/keeper/relay_forwarding_test.go
bznein Jun 14, 2024
ca59adf
Update modules/apps/transfer/keeper/relay_forwarding_test.go
bznein Jun 14, 2024
1492378
Add godoc to test.
bznein Jun 14, 2024
288f5f6
Changed trace construction
bznein Jun 14, 2024
d4e9b59
Merge branch 'feat/ics20-v2-path-forwarding' into bznein/6384/ontimeo…
bznein Jun 17, 2024
a70e56d
Update modules/apps/transfer/keeper/relay_forwarding_test.go
bznein Jun 17, 2024
6a33a57
remove error msg parameter from helper function
bznein Jun 14, 2024
7fb5569
Add test for forwarded packet
bznein Jun 17, 2024
774d971
Merge branch 'feat/ics20-v2-path-forwarding' into bznein/6384/ontimeo…
bznein Jun 17, 2024
ad4e6f6
Construct packet for B ack check.
bznein Jun 18, 2024
3acd732
Merge branch 'feat/ics20-v2-path-forwarding' into bznein/6384/ontimeo…
bznein Jun 18, 2024
139facf
PR feedback
bznein Jun 18, 2024
2a0225e
feat(transfer): add unwind, refactor proto structure. gen-all
DimitrisJim Jun 20, 2024
04127e7
tests(transfer/types): fix test failures in types tests.
DimitrisJim Jun 20, 2024
0e3552a
tests(transfer/keeper): fix test failures in keeper tests.
DimitrisJim Jun 20, 2024
878b64c
cli(transfer): fix cli usage. pending flag for unwind.
DimitrisJim Jun 20, 2024
23cf902
tests(callbacks): fix failing tests in callbacks.
DimitrisJim Jun 20, 2024
acc9a95
tests(transfer/internal): fix failures in internal package.
DimitrisJim Jun 20, 2024
74a6a9a
tests(transfer): fix test failures in top level tranfer package.
DimitrisJim Jun 20, 2024
d68b2e4
tests(ica/host/keeper): fix repr of msg transfer in ica host msg exec…
DimitrisJim Jun 20, 2024
040b4fe
lint(all): lint this bad boy
DimitrisJim Jun 20, 2024
3a59487
chore(transfer/types): amend validation for MsgTransfer's ShouldBeFor…
DimitrisJim Jun 20, 2024
18d4927
nit(self): only pass relevant fields to create packet data; minor com…
DimitrisJim Jun 20, 2024
5f78cc2
Apply suggestions from code review
DimitrisJim Jun 20, 2024
babd20f
Merge branch 'feat/ics20-v2-path-forwarding' into jim/refactor-forwar…
DimitrisJim Jun 20, 2024
f01fb8c
chore(merge): fix merge issues.
DimitrisJim Jun 20, 2024
80524a3
chore(proto): mention optional nature of fields.
DimitrisJim Jun 20, 2024
a343a25
memo: do not drop it
DimitrisJim Jun 20, 2024
ba5bbe0
validation: drop requirement on memo being empty on msg transfer.
DimitrisJim Jun 20, 2024
147b05c
feat(transfer): add unwinding ability, wip.
DimitrisJim Jun 20, 2024
0b8e2b9
Merge branch 'feat/ics20-v2-path-forwarding' into jim/unwind-coins-an…
bznein Jun 21, 2024
5e13d74
Merge branch 'feat/ics20-v2-path-forwarding' into jim/unwind-coins-an…
bznein Jun 21, 2024
4b43f07
Added unwind to allocation forwarding.
bznein Jun 21, 2024
8cb674d
Add tests and move some validation
bznein Jun 24, 2024
c1d1b8a
Merge branch 'feat/ics20-v2-path-forwarding' into jim/unwind-coins-an…
bznein Jun 24, 2024
f1c5968
Missing import
bznein Jun 24, 2024
a07fe59
Fixed validation and added test
bznein Jun 24, 2024
02b8d6f
PR Feedback
bznein Jun 24, 2024
d31590c
Return nil when returning an error.
bznein Jun 24, 2024
a2accc1
Cleaner comment
bznein Jun 24, 2024
1c0fb6c
Merge branch 'feat/ics20-v2-path-forwarding' into jim/unwind-coins-an…
bznein Jun 24, 2024
0e67b51
Add test case for multiple hos
bznein Jun 25, 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
5 changes: 5 additions & 0 deletions modules/apps/transfer/keeper/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ func (k Keeper) TokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
return k.tokenFromCoin(ctx, coin)
}

// UnwindHops is a wrapper around unwindToken for testing purposes.
func (k Keeper) UnwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgTransfer, error) {
return k.unwindHops(ctx, msg)
}

// CreatePacketDataBytesFromVersion is a wrapper around createPacketDataBytesFromVersion for testing purposes
func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, hops)
Expand Down
36 changes: 36 additions & 0 deletions modules/apps/transfer/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
}

if msg.Forwarding.Unwind {
msg, err = k.unwindHops(ctx, msg)
if err != nil {
return nil, err
}
}

sequence, err := k.sendTransfer(
ctx, msg.SourcePort, msg.SourceChannel, coins, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
msg.Memo, msg.Forwarding)
Expand All @@ -59,3 +66,32 @@ func (k Keeper) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams)

return &types.MsgUpdateParamsResponse{}, nil
}

// unwindHops unwinds the hops present in the tokens denomination and returns the message modified to reflect
// the unwound path to take. It ensures only a single token must be present in the tokens list
// and that the token is not native to the chain.
func (k Keeper) unwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgTransfer, error) {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
coins := msg.GetCoins()
token, err := k.tokenFromCoin(ctx, coins[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the length has already been verified in ValidateBasic, but I wonder if we should comment or change the godoc because at first glance I was unsure if this actually ensures that a single token is used or just cuts the list if there are multiple - but I guess we working from the assumption that ValidateBasic catches the error of multiple coins.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add a comment here for clarity and change the godoc :)

if err != nil {
return nil, err
}

if token.Denom.IsNative() {
return nil, errorsmod.Wrap(types.ErrInvalidForwarding, "cannot unwind a native token")
}
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
var unwindHops []types.Hop
for _, trace := range token.Denom.Trace[1:] {
unwindHops = append(unwindHops, types.Hop{PortId: trace.PortId, ChannelId: trace.ChannelId}) //nolint: gosimple
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}

// Update message fields.
msg.SourcePort, msg.SourceChannel = token.Denom.Trace[0].PortId, token.Denom.Trace[0].ChannelId
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
msg.Forwarding.Hops = append(unwindHops, msg.Forwarding.Hops...)
msg.Forwarding.Unwind = false

if err := msg.ValidateBasic(); err != nil {
return nil, err
}
return msg, nil
}
100 changes: 100 additions & 0 deletions modules/apps/transfer/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
abci "github.com/cometbft/cometbft/abci/types"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
Expand Down Expand Up @@ -238,3 +239,102 @@ func (suite *KeeperTestSuite) TestUpdateParams() {
})
}
}

func (suite *KeeperTestSuite) TestUnwindHops() {
var msg *types.MsgTransfer
var path *ibctesting.Path
denom := types.NewDenom(ibctesting.TestCoin.Denom, types.NewTrace(ibctesting.MockPort, "channel-0"), types.NewTrace(ibctesting.MockPort, "channel-1"))
coins := sdk.NewCoins(sdk.NewCoin(denom.IBCDenom(), ibctesting.TestCoin.Amount))
testCases := []struct {
name string
malleate func()
assertResult func(modified *types.MsgTransfer, err error)
}{
{
"success",
func() {
suite.chainA.GetSimApp().TransferKeeper.SetDenom(suite.chainA.GetContext(), denom)
},
func(modified *types.MsgTransfer, err error) {
suite.Require().NoError(err, "got unexpected error from unwindHops")
msg.SourceChannel = denom.Trace[0].PortId
msg.SourcePort = denom.Trace[0].ChannelId
msg.Forwarding = types.NewForwarding(false, types.Hop{PortId: denom.Trace[1].PortId, ChannelId: denom.Trace[1].ChannelId})
suite.Require().Equal(*msg, *modified, "expected msg and modified msg are different")
},
},
{
"success - unwind hops are added to existing hops",
func() {
suite.chainA.GetSimApp().TransferKeeper.SetDenom(suite.chainA.GetContext(), denom)
msg.Forwarding = types.NewForwarding(true, types.Hop{PortId: ibctesting.MockPort, ChannelId: "channel-2"})
},
func(modified *types.MsgTransfer, err error) {
suite.Require().NoError(err, "got unexpected error from unwindHops")
msg.SourceChannel = denom.Trace[0].PortId
msg.SourcePort = denom.Trace[0].ChannelId
msg.Forwarding = types.NewForwarding(false,
types.Hop{PortId: denom.Trace[1].PortId, ChannelId: denom.Trace[1].ChannelId},
types.Hop{PortId: ibctesting.MockPort, ChannelId: "channel-2"},
)
suite.Require().Equal(*msg, *modified, "expected msg and modified msg are different")
},
},
{
"failure: no denom set on keeper",
func() {},
func(modified *types.MsgTransfer, err error) {
suite.Require().ErrorIs(err, types.ErrDenomNotFound)
},
Comment on lines +306 to +308
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

},
{
"failure: validateBasic() fails due to invalid channelID",
func() {
denom.Trace[0].ChannelId = "channel/0"
coins = sdk.NewCoins(sdk.NewCoin(denom.IBCDenom(), ibctesting.TestCoin.Amount))
msg.Tokens = coins
suite.chainA.GetSimApp().TransferKeeper.SetDenom(suite.chainA.GetContext(), denom)
},
func(modified *types.MsgTransfer, err error) {
suite.Require().ErrorContains(err, "invalid source channel ID")
},
},
{
"failure: denom is native",
func() {
denom.Trace = nil
coins = sdk.NewCoins(sdk.NewCoin(denom.IBCDenom(), ibctesting.TestCoin.Amount))
msg.Tokens = coins
suite.chainA.GetSimApp().TransferKeeper.SetDenom(suite.chainA.GetContext(), denom)
},
func(modified *types.MsgTransfer, err error) {
suite.Require().ErrorIs(err, types.ErrInvalidForwarding)
},
},
}

for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewTransferPath(suite.chainA, suite.chainB)
path.Setup()

msg = types.NewMsgTransfer(
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
coins,
suite.chainA.SenderAccount.GetAddress().String(),
suite.chainB.SenderAccount.GetAddress().String(),
clienttypes.ZeroHeight(),
suite.chainA.GetTimeoutTimestamp(),
"memo",
types.NewForwarding(true),
)

tc.malleate()
gotMsg, err := suite.chainA.GetSimApp().TransferKeeper.UnwindHops(suite.chainA.GetContext(), msg)
tc.assertResult(gotMsg, err)
})
}
}
Loading
Loading