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 Ibc sample contract #714

Merged
merged 34 commits into from
Jan 18, 2021
Merged

Add Ibc sample contract #714

merged 34 commits into from
Jan 18, 2021

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Jan 13, 2021

Addresses #692

It builds on some other items:

#711 to define the entry points we use (MERGED)
#716 to add stargate support and ibc entry points to the vm (MERGED)

  • README of for ibc-reflect contract
  • Implement basic logic
  • unit tests
  • integration tests working (with ibc vm calls)
  • Address open TODOs
  • Update contract APIs (query packet, stargate reflect)
  • Integrate PR review
  • Backport pr reviews on types from Add ibc support wasmvm#167

Note: a unit test brought up CosmWasm/serde-json-wasm#27 but this is not blocked on that (it parses correctly in the vm code, and we never parse this type in contracts).

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice stuff

contracts/ibc-reflect/NOTICE Outdated Show resolved Hide resolved
contracts/ibc-reflect/src/contract.rs Outdated Show resolved Hide resolved
contracts/ibc-reflect/src/contract.rs Outdated Show resolved Hide resolved
Copy link
Member

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Very cool 🎸


#[entry_point]
/// we do nothing
/// TODO: remove the account from the lookup?
Copy link
Member

Choose a reason for hiding this comment

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

If you would propagated this event to the "reflect" contract then it would be able to do some "cleanup" as well. For example if the contract owns tokens for whatever reasons it could return them and would not lock them.

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 think this would never actually be called.

The flow I have is "to-be-defined-sender-contract on chain A" sends a packet to "ibc-reflect on chain B", which dispatches it.

ibc-reflect never sends a packet itself, thus should never get these two callbacks (we could error here I guess?)

// and we are golden
Ok(IbcReceiveResponse {
acknowledgement,
messages: vec![wasm_msg.into()],
Copy link
Member

Choose a reason for hiding this comment

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

How can be ensure that only "authorized" messages are passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we create a channel, there is exactly one contract/module on the other side that can send packets on it, correct? (Please double-check this - this is key to all my authorization reasoning).

When they connect, we give them a fresh address that is theirs. Any messages they send get reflected by that custom address. Thus the channel -> address lookup.

If sending packets on the channel is not as limited as I believe, I need to revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: We really need local (port_id, channel_id) pairs for uniqueness.
However, the receiving contract only has one port, so by fixing the channel_id, we fix the pair.

@ethanfrey ethanfrey force-pushed the ibc-sample-contract branch 4 times, most recently from 5ce3369 to b87385f Compare January 14, 2021 19:22
@ethanfrey ethanfrey marked this pull request as ready for review January 14, 2021 19:24
@ethanfrey
Copy link
Member Author

Okay, I think this is done (and a bit of refactoring for future contracts as well).

Happy for a final review, then I can merge it and use as a demo contract for wasmvm integration

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

👍

packages/std/src/ibc.rs Show resolved Hide resolved
packages/std/src/ibc.rs Outdated Show resolved Hide resolved
contracts/README.md Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

ethanfrey commented Jan 18, 2021

This should be ready for a merge now.

I will start integrating the last commit (62b9fc7) into CosmWasm/wasmvm#167

@webmaster128 webmaster128 merged commit 220e39c into master Jan 18, 2021
@webmaster128 webmaster128 deleted the ibc-sample-contract branch January 18, 2021 13:20
@ethanfrey ethanfrey mentioned this pull request Jan 18, 2021
6 tasks
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.

None yet

3 participants