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(e2e): native token transfer from cosmos to starknet #172

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

rnbguy
Copy link
Member

@rnbguy rnbguy commented Jan 8, 2025

Partially resolves #170 #171

ics20 transfer: cosmos -> starknet -> cosmos

  • mint on starknet
  • burn on starknet

@rnbguy
Copy link
Member Author

rnbguy commented Jan 8, 2025

Current test fails because of cosmos/ibc-rs#1262. The fix is present in ibc-go v9

Update: started using ibc-go v9 9929d13

@rnbguy rnbguy force-pushed the rano/e2e/ics20-packet branch 3 times, most recently from d1f52f6 to 26ae0c1 Compare January 8, 2025 09:29
@rnbguy rnbguy force-pushed the rano/e2e/ics20-packet branch from 26ae0c1 to 9929d13 Compare January 8, 2025 09:33
@rnbguy rnbguy changed the title feat(e2e): relay packets from cosmos to starknet feat(e2e): native token transfer from cosmos to starknet Jan 8, 2025
@rnbguy rnbguy force-pushed the rano/e2e/ics20-packet branch from 19bf4e8 to 63fc99a Compare January 8, 2025 16:08
@rnbguy
Copy link
Member Author

rnbguy commented Jan 8, 2025

Currently, the test fails, as the Starknet doesn't accept packet_ack because of commitment mismatch.

Update: I forgot to add the trace prefix. cd0f882

Comment on lines +30 to 31
fn ibc_token_addresses(self: @TContractState) -> Array<ContractAddress>;
}
Copy link
Member

Choose a reason for hiding this comment

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

If the goal of adding this endpoint is only to obtain the token address of an IBC-created token, it’s actually unnecessary. We can either listen to the CreateTokenEvent that gets emitted or construct the token's key on the relayer side and use the ibc_token_address, same as it's computed by the contract.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed unnecessary. I added it as a hack. I just forgot to add a TODO comment on top of it.

Essentially, the relay_packet method should return any stateful data generated during the relay -- specifically token address of the minted denom. I will wait for Soares to fix this. After that, we can remove this.

Copy link
Member

Choose a reason for hiding this comment

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

Could you open up an issue for this? What exactly is blocking us and what's needed in hermes-sdk so that we can rely on existing facilities here?

Comment on lines +233 to +234
fn from_cosmos_to_cairo_packet<Encoding>(packet: &IbcPacket, encoding: &Encoding) -> CairoPacket
where
Copy link
Member

Choose a reason for hiding this comment

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

This conversion feels quite hacky, doesn’t seem worth investing in, especially since we’ll be implementing #150 soon. I’m becoming more convinced to prioritize #150 first (at least for the types used in this PR), then get back here. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to finish up the handshake and relays, as I already started working on this.

Or, we can merge until here and work on #150 and then resume work on getting packet relay working.

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 have enough time ahead of us, thus not recommend leaving the PR with this function.
Let's get #150 done first. Let me know if I can be assist on that.

)
.await?;

// TODO(rano): how do I get the ics20 token contract address from starknet events
Copy link
Member

Choose a reason for hiding this comment

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

When a token is created, the ICS20 emits a CreateTokenEvent that includes the token's address. We can read the address by retrieving that event.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment is mainly about how to use relay_packet and still get the token address. The hack, that you pointed in #172 (comment), is introduced exactly for this.

@rnbguy
Copy link
Member Author

rnbguy commented Jan 9, 2025

I wanted to perform erc20 token transfer from starknet to cosmos, but I realized, we need to permit the ics20 contract on behalf of the sender. I am not sure how to do that.

I am making this PR final. Let's test starknet -> cosmos -> starknet on a different PR.

@rnbguy rnbguy marked this pull request as ready for review January 9, 2025 13:55
@rnbguy rnbguy force-pushed the rano/e2e/ics20-packet branch from 80c2ea2 to 9881575 Compare January 9, 2025 14:14
@Farhad-Shabani
Copy link
Member

@rnbguy Now that the test is working, we can be confident in the correctness of our implementation on both the relayer and contracts side. My suggestion is to address #172 (comment) and #172 (comment) first, update the PR, and then wrap it up.

@Farhad-Shabani
Copy link
Member

I wanted to perform erc20 token transfer from starknet to cosmos, but I realized, we need to permit the ics20 contract on behalf of the sender. I am not sure how to do that.

The way it works at the moment, users should approve their allowance before initiating a send_transfer like it's done here in the contract's unit tests.

Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

By the way, I approve the PR, leaving that to you how to organize issues/PRs moving forward.

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.

Enhance test_starknet_ics20_contract to fully simulate relaying from Cosmos to Starknet
2 participants