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

Fix send_transfer #2

Merged
merged 14 commits into from
Sep 11, 2023
Merged

Conversation

plafer
Copy link

@plafer plafer commented Sep 7, 2023

This PR fixes send_transfer() to ensure that ICS-20 token denoms are always unique.

@plafer plafer marked this pull request as ready for review September 7, 2023 19:32
// This applies to all other tokens created on this
// sovereign SDK chain. The token name is not guaranteed to
// be unique, and hence we must use the token address (which
// is guaranteed to be unique) as the ICS-20 denom to ensure
Copy link
Member

Choose a reason for hiding this comment

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

I am seeing, when transferring tokens between two Sovereign chains, we exclusively use the token address as the denom, disregarding the token's name.
Whether the token lives on the source chain, in which case the denom is based on the token address, or if the IBC mints the token on the source chain (which represents a token on the destination chain). In latter, we use a prefixed denom, where the base denom comes from a MsgRecvPacket, which was earlier crafted by relayers based on an emitted TransferEvent from the destination chain...Where the denom is also set as the token address!

Copy link
Author

Choose a reason for hiding this comment

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

when transferring tokens between two Sovereign chains

In the initial version, we will only support sending tokens to Tendermint-based chains, as we will only have a tendermint light client installed.

In latter, we use a prefixed denom, where the base denom comes from a MsgRecvPacket, which was earlier crafted by relayers based on an emitted TransferEvent from the destination chain...Where the denom is also set as the token address!

However, if we were to support Sovereign <-> Sovereign transfers, then the denom of tokens we receive would indeed represent the address of the token on the other chain. Note that the token we mint on our side would have a different address, but its denom would be set to the string representation of the address of the token on the other chain. However, for received tokens, we treat the token name as a black box; we never try to interpret it in any way.

Copy link
Member

Choose a reason for hiding this comment

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

the denom of tokens we receive would indeed represent the address of the token on the other chain

Exactly. And looks like it holds true for bunch of other non-Cosmos chains, which makes string reps of token addresses the most common denom.

By the way, the transfer module stores the "denom → token_address" in the minted_token field. This allows us to map, e.g. the channel-1/port-1/0xb79…4f5ea received from MsgRecvPacket to the corresponding token address on the source chain. So, I'm still a bit puzzled about why can't we work with the MsgTransfer and include the string reps of the token's address as denom? What required us to define SDKTokenTransfer on top of that?

Anyway, as chatted earlier, Let's dive deeper into these details when writing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants