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

imp (api)!: convert coins to token only once in MsgTransfer #7110

Merged
merged 2 commits into from
Aug 8, 2024
Merged

imp (api)!: convert coins to token only once in MsgTransfer #7110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e352fe
test not working
bznein Aug 8, 2024
47b2229
refactored test and removed unneeded caese
bznein Aug 8, 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
4 changes: 2 additions & 2 deletions modules/apps/transfer/keeper/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ func (k Keeper) TokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
}

// UnwindHops is a wrapper around unwindHops for testing purposes.
func (k Keeper) UnwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgTransfer, error) {
return k.unwindHops(ctx, msg)
func (k Keeper) UnwindHops(msg *types.MsgTransfer, tokens []types.Token) (*types.MsgTransfer, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an API-breaking change but since I assume the whole feature branch will this should not be an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct me if I am wrong, but since this function is in a _test.go file, then it is not part of the public API, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point, I wasn't too sure about it tbh!

return k.unwindHops(msg, tokens)
}

// GetForwardedPacket is a wrapper around getForwardedPacket for testing purposes.
Expand Down
36 changes: 14 additions & 22 deletions modules/apps/transfer/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
return nil, err
}

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

coins := msg.GetCoins()
tokens := make([]types.Token, 0, len(coins))
for _, coin := range coins {
Expand All @@ -42,6 +35,13 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
tokens = append(tokens, token)
}

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

appVersion, found := k.ics4Wrapper.GetAppVersion(ctx, msg.SourcePort, msg.SourceChannel)
if !found {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "application version not found for source port: %s and source channel: %s", msg.SourcePort, msg.SourceChannel)
Expand Down Expand Up @@ -109,8 +109,8 @@ func (k Keeper) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams)
// unwindHops unwinds the hops present in the tokens denomination and returns the message modified to reflect
// the unwound path to take. It assumes that only a single token is present (as this is verified in ValidateBasic)
// in the tokens list and ensures that the token is not native to the chain.
func (k Keeper) unwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgTransfer, error) {
unwindHops, err := k.getUnwindHops(ctx, msg.GetCoins())
func (k Keeper) unwindHops(msg *types.MsgTransfer, tokens []types.Token) (*types.MsgTransfer, error) {
unwindHops, err := k.getUnwindHops(tokens)
if err != nil {
return nil, err
}
Expand All @@ -130,30 +130,22 @@ func (k Keeper) unwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgT
// getUnwindHops returns the hops to be used during unwinding. If coins consists of more than
// one coin, all coins must have the exact same trace, else an error is returned. getUnwindHops
// also validates that the coins are not native to the chain.
func (k Keeper) getUnwindHops(ctx sdk.Context, coins sdk.Coins) ([]types.Hop, error) {
func (Keeper) getUnwindHops(tokens []types.Token) ([]types.Hop, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function can technically be moved out of the keeper now, since it does need to read state to convert coins to token anymore. Happy to do it if it seems the best thing to do

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me!

// Sanity: validation for MsgTransfer ensures coins are not empty.
if len(coins) == 0 {
if len(tokens) == 0 {
return nil, errorsmod.Wrap(types.ErrInvalidForwarding, "coins cannot be empty")
}

token, err := k.tokenFromCoin(ctx, coins[0])
if err != nil {
return nil, err
}
token := tokens[0]

if token.Denom.IsNative() {
return nil, errorsmod.Wrap(types.ErrInvalidForwarding, "cannot unwind a native token")
}

unwindTrace := token.Denom.Trace
for _, coin := range coins[1:] {
token, err := k.tokenFromCoin(ctx, coin)
if err != nil {
return nil, err
}

for _, t := range tokens[1:] {
// Implicitly ensures coin we're iterating over is not native.
if !slices.Equal(token.Denom.Trace, unwindTrace) {
if !slices.Equal(t.Denom.Trace, unwindTrace) {
return nil, errorsmod.Wrap(types.ErrInvalidForwarding, "cannot unwind tokens with different traces.")
}
}
Expand Down
17 changes: 9 additions & 8 deletions modules/apps/transfer/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,13 +327,6 @@ func (suite *KeeperTestSuite) TestUnwindHops() {
suite.Require().Equal(*msg, *modified, "expected msg and modified msg are different")
},
},
{
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 don't believe we need this test anymore, because this would have failed during the conversion we don't perform anymore

"failure: no denom set on keeper",
func() {},
func(modified *types.MsgTransfer, err error) {
suite.Require().ErrorIs(err, types.ErrDenomNotFound)
},
},
{
"failure: validateBasic() fails due to invalid channelID",
func() {
Expand Down Expand Up @@ -398,7 +391,15 @@ func (suite *KeeperTestSuite) TestUnwindHops() {
)

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

tokens := make([]types.Token, 0, len(coins))
for _, c := range msg.GetCoins() {
token, err := suite.chainA.GetSimApp().TransferKeeper.TokenFromCoin(suite.chainA.GetContext(), c)
suite.Require().NoError(err)
tokens = append(tokens, token)
}

gotMsg, err := suite.chainA.GetSimApp().TransferKeeper.UnwindHops(msg, tokens)
tc.assertResult(gotMsg, err)
})
}
Expand Down
Loading