Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 3 additions & 5 deletions modules/apps/transfer/keeper/forwarding.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"errors"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"

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

Expand Down Expand Up @@ -72,11 +71,10 @@ func (k Keeper) revertForwardedPacket(ctx sdk.Context, prevPacket channeltypes.P
// we can iterate over the received tokens of prevPacket by iterating over the sent tokens of failedPacketData
for _, token := range failedPacketData.Tokens {
// parse the transfer amount
transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
if !ok {
return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount)
coin, err := token.ToCoin()
if err != nil {
return err
}
coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)

// check if the token we received originated on the sender
// given that the packet is being reversed, we check the DestinationChannel and DestinationPort
Expand Down
8 changes: 3 additions & 5 deletions modules/apps/transfer/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,13 +331,11 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet,
// NOTE: packet data type already checked in handler.go

for _, token := range data.Tokens {
transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
if !ok {
return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount)
coin, err := token.ToCoin()
if err != nil {
return err
}

coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)

sender, err := sdk.AccAddressFromBech32(data.Sender)
if err != nil {
return err
Expand Down
17 changes: 17 additions & 0 deletions modules/apps/transfer/types/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package types
import (
errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"

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

// Tokens is a slice of Tokens
Expand All @@ -25,3 +27,18 @@ func (t Token) Validate() error {

return nil
}

// ToCoin converts a Token to an sdk.Coin.
//
// The function parses the Amount field of the Token into an sdkmath.Int and returns a new sdk.Coin with
// the IBCDenom of the Token's Denom field and the parsed Amount.
// If the Amount cannot be parsed, an error is returned with a wrapped error message.
func (t Token) ToCoin() (sdk.Coin, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a couple of tests for this just so we have it covered and so that any changes in the future to this function would cover any issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gjermundgaraba I just add unit test.
But I think should more some case and check in this func:

  • Amount is zero
  • Amount is negative

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dont think we need to add cases where the parsing fails. The amount for each token is validated during initial transfer. Just covering the error case with the test case you added seems perfectly fine to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@DimitrisJim I got it. Thank you!

transferAmount, ok := sdkmath.NewIntFromString(t.Amount)
if !ok {
return sdk.Coin{}, errorsmod.Wrapf(ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount)
}

coin := sdk.NewCoin(t.Denom.IBCDenom(), transferAmount)
return coin, nil
}
53 changes: 53 additions & 0 deletions modules/apps/transfer/types/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import (
"testing"

"github.com/stretchr/testify/require"

sdkmath "cosmossdk.io/math"

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

const (
Expand Down Expand Up @@ -149,3 +153,52 @@ func TestValidate(t *testing.T) {
})
}
}

func TestToCoin(t *testing.T) {
testCases := []struct {
name string
token Token
expCoin sdk.Coin
expError error
}{
{
"success: convert token to coin",
Token{
Denom: Denom{
Base: denom,
Trace: []Trace{},
},
Amount: amount,
},
sdk.NewCoin(denom, sdkmath.NewInt(100)),
nil,
},
{
"failure: invalid amount string",
Token{
Denom: Denom{
Base: denom,
Trace: []Trace{},
},
Amount: "value",
},
sdk.Coin{},
ErrInvalidAmount,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
coin, err := tc.token.ToCoin()

require.Equal(t, tc.expCoin, coin, tc.name)

expPass := tc.expError == nil
if expPass {
require.NoError(t, err, tc.name)
} else {
require.ErrorContains(t, err, tc.expError.Error(), tc.name)
Copy link
Copy Markdown
Contributor

@crodriguezvega crodriguezvega Jun 14, 2024

Choose a reason for hiding this comment

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

You could also do here:

require.Equal(t, tc.coins, coin, tc.name)

To check that the coin returned in the failure case is empty. Which means that you could basically do:

require.Equal(t, tc.expCoin, coin, tc.name)
if expPass {
  require.NoError(t, err, tc.name)
} else {
  require.ErrorContains(t, err, tc.expError.Error(), tc.name)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

}
})
}
}