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

Add func convert token to coin ibc #6584

Merged
Show file tree
Hide file tree
Changes from 5 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
18 changes: 7 additions & 11 deletions modules/apps/transfer/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/hashicorp/go-metrics"

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

"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -182,10 +181,11 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
}

// parse the transfer amount
transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
if !ok {
return false, errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount: %s", token.Amount)
coin, err := token.ToCoin()
if err != nil {
return false, err
}
transferAmount := coin.Amount

// This is the prefix that would have been prefixed to the denomination
// on sender chain IF and only if the token originally came from the
Expand All @@ -200,8 +200,6 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
// remove prefix added by sender chain
token.Denom.Trace = token.Denom.Trace[1:]

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

@DimitrisJim DimitrisJim Jun 14, 2024

Choose a reason for hiding this comment

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

ah, we modify the trace here so the hashed denom ends up different (ditto if not Source), we would need to swap out the coins Denomination to have this working 😞 we should do manual conversion here and ooth use ToCoin in forwarding.go (which I think should be possible)

Can take care of fixing these!


if k.bankKeeper.BlockedAddr(receiver) {
return false, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver)
}
Expand Down Expand Up @@ -331,13 +329,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,7 @@ 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 +26,19 @@ func (t Token) Validate() error {

return nil
}

// ToCoin converts a Token to an sdk.Coin.
//
// It takes a Token as a parameter and returns an sdk.Coin and an error. The function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// It takes a Token as a parameter and returns an sdk.Coin and an error. The function
// It takes a Token as a parameter and returns an sdk.Coin if conversion succeeds, otherwise an error. The function

Copy link
Contributor

Choose a reason for hiding this comment

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

tweaked this slightly more, feel free to do a suggestion if you feel its needed

// parses the Amount field of the Token into an sdkmath.Int and creates 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
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
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
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
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
}
49 changes: 49 additions & 0 deletions modules/apps/transfer/types/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"fmt"
"testing"

sdkmath "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"
)

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

func TestToCoin(t *testing.T) {
testCases := []struct {
name string
token Token
coins sdk.Coin
Copy link
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.

Suggested change
coins sdk.Coin
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()
expPass := tc.expError == nil
if expPass {
require.NoError(t, err, tc.name)
require.Equal(t, tc.coins, coin, tc.name)
} else {
require.ErrorContains(t, err, tc.expError.Error(), tc.name)
Copy link
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
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

}
})
}
}
Loading