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(ics20): initial version of transfer app #4

Merged
merged 19 commits into from
Jul 31, 2024
Merged

feat(ics20): initial version of transfer app #4

merged 19 commits into from
Jul 31, 2024

Conversation

gjermundgaraba
Copy link
Contributor

Implements one-way transfer (i.e. receive is not implemented here, will be done in a separate PR).

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Amazing work! And so quick. I gave some initial feedback. Looks good overall. The only major change needed is that the sendTransfer flow should be starting in ICS20Transfer.sol, not ICS26Router.sol. You can refer to my comments and the CosmWasm implementation for this.

So the flow goes ICS20Transfer.sol -> ICS26Router.sol -> ICS20Transfer.sol

I actually think what we did is also ok, and could be supported, but this is the main flow Aditya and I decided.

.gitignore Show resolved Hide resolved
src/ICS26Router.sol Show resolved Hide resolved
src/apps/transfer/ICS20Lib.sol Outdated Show resolved Hide resolved
src/apps/transfer/ICS20Transfer.sol Outdated Show resolved Hide resolved
src/apps/transfer/ICS20Transfer.sol Outdated Show resolved Hide resolved
src/apps/transfer/ICS20Transfer.sol Outdated Show resolved Hide resolved

// TODO: Maybe have a "ValidateBasic" type of function that checks the packet data, could be done in unwrapping?

if (data.amount == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we check that the number of decimals make sense. For example, if cosmos is uint64 and eth is uint256. Then we need to normalize the decimals?

Have you thought about how we might handle this?

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 a good question. There are two issues here I suppose.

  • One is the actual uint256 which might cause problems (can be too big for go, I guess?)
  • The other is the decimals variable itself, which is mostly required for frontends to make sense of a token. If the contract implements IERC20Metadata we can get this info, but not sure if we can do much with it. Even if the number of decimals makes sense, the number might still be too big.

One thing we could do is to check the size of the number and if it would not fit into go uint64, we would reject? Not sure what the alternative is, if the Cosmos SDK and ibc-go doesn't support big numbers.

I saw someone mentioning this problem, but not sure where things break: https://x.com/brane_trix/status/1818055801463812494

Copy link
Member

Choose a reason for hiding this comment

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

I think we can handle this situation if we know the decimals variable. Can discuss this later I suppose

src/apps/transfer/ICS20Transfer.sol Outdated Show resolved Hide resolved
test/IntegrationTest.t.sol Outdated Show resolved Hide resolved
src/apps/transfer/ICS20Transfer.sol Outdated Show resolved Hide resolved
@gjermundgaraba gjermundgaraba requested a review from srdtrk July 30, 2024 13:51
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Lgtm!

@srdtrk srdtrk merged commit ea66332 into main Jul 31, 2024
3 checks passed
@srdtrk srdtrk deleted the gjermund/ics20 branch July 31, 2024 04:53
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.

2 participants