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

Write a doc on how x/wasm uses IBC #214

Merged
merged 11 commits into from
Aug 19, 2020
Merged

Conversation

ethanfrey
Copy link
Member

This is a proposal of how we use ports and channels.

It is not fully complete and could use feedback. Let's define this well for our best ideas now and adapt
it if the code shows another approach is more advantageous.

Related to CosmWasm/cosmwasm#147 which has a long discussion between Chris and I
in February, also setting forth the version negotiation that has since been implemented in IBC. Please read that for
some of the thinking - that is not final thinking, nor is this document final, but the should cover a lot of the reasoning
behind the design.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added relevant godoc comments.

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Re-reviewed Files changed in the Github PR explorer


For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Sorry, something went wrong.

@ethanfrey ethanfrey requested a review from alpe July 22, 2020 15:05
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
@ethanfrey ethanfrey mentioned this pull request Jul 27, 2020
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #214 into 0.10_to_cosmos-stargate_ce9c2b2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                        @@
##           0.10_to_cosmos-stargate_ce9c2b2     #214   +/-   ##
================================================================
  Coverage                            17.68%   17.68%           
================================================================
  Files                                   32       32           
  Lines                                10593    10593           
================================================================
  Hits                                  1873     1873           
  Misses                                8636     8636           
  Partials                                84       84           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b3040a...1c79245. Read the comment docs.

@ethanfrey
Copy link
Member Author

@alpe I made a lot of progress cleaning up the doc, and think I addressed all your issues as well as integrating ideas from your spike. Can you please review this again and give more feedback?

@ethanfrey ethanfrey changed the base branch from ibc-prototype to 0.9.1_to_cosmos-sdk-0.39-master August 6, 2020 19:58
@ethanfrey
Copy link
Member Author

Okay, rebased this to focus on the stargate tracking branch, so we can look to merge this in, separate from the implementation work (which is a bit of mix of research, spikes, and useful code).

x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated
```go
type IBCSendMsg struct {
ChannelID string
RemotePort string // do we need both? isn't channel enough once it is established???
Copy link
Contributor

Choose a reason for hiding this comment

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

... a single instance of an IBC module may communicate on the same port with any number of other modules and and IBC will correctly route all packets to the relevant module using the (channelID, portID tuple)

https://docs.cosmos.network/master/ibc/overview.html#capabilities

Copy link
Member Author

Choose a reason for hiding this comment

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

@cwgoes can you explain this to me?

I understand connection as IP-level connection (the linked docs refers to channel as like IP connection)

A channel is like TCP/UDP connection which is established with source and remote ports via handshake. Once the channel is established, it has a well-defined port on both sides and simply the channelID should identify which client to use and the source/dest port, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite; the terminology is a little different than TCP/IP.

A connection exists between two chains and has a unique identifier on each chain. Many channels may use one connection.

A channel has a unique identifier per-port, so the combination of port identifier & channel identifier is required to uniquely identify a channel on a single chain. A channel references a single connection, which it will utilise to verify state (that is, it will utilise the client associated with the connection).

Of course, if a contract is always bound to a fixed port, the contract can just identify channels by the channel identifier, and the wasmd runtime can look up the port, presumably.

@ethanfrey
Copy link
Member Author

Thanks for the feedback, I will update the docs

x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Generally looks solid, I tried to answer the outstanding questions

x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated
```go
type IBCSendMsg struct {
ChannelID string
RemotePort string // do we need both? isn't channel enough once it is established???
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite; the terminology is a little different than TCP/IP.

A connection exists between two chains and has a unique identifier on each chain. Many channels may use one connection.

A channel has a unique identifier per-port, so the combination of port identifier & channel identifier is required to uniquely identify a channel on a single chain. A channel references a single connection, which it will utilise to verify state (that is, it will utilise the client associated with the connection).

Of course, if a contract is always bound to a fixed port, the contract can just identify channels by the channel identifier, and the wasmd runtime can look up the port, presumably.

@ethanfrey
Copy link
Member Author

A channel has a unique identifier per-port, so the combination of port identifier & channel identifier is required to uniquely identify a channel on a single chain. A channel references a single connection, which it will utilise to verify state (that is, it will utilise the client associated with the connection).

This is the key point I didn't get. A ChannelID is not globally unique. So if I own portID A, I can open ChannelIDs A, B, C... and not worry if someone else claimed them first?

Now I get it. (PortID, ChannelID) is the globally unique identifier

@cwgoes
Copy link
Contributor

cwgoes commented Aug 7, 2020

A channel has a unique identifier per-port, so the combination of port identifier & channel identifier is required to uniquely identify a channel on a single chain. A channel references a single connection, which it will utilise to verify state (that is, it will utilise the client associated with the connection).

This is the key point I didn't get. A ChannelID is not globally unique. So if I own portID A, I can open ChannelIDs A, B, C... and not worry if someone else claimed them first?

Now I get it. (PortID, ChannelID) is the globally unique identifier

Yes, exactly.

@ethanfrey
Copy link
Member Author

ethanfrey commented Aug 7, 2020

Thank you for the review @cwgoes

I was actually doing a bit of more extension of the detailed callbacks while you wrote that. Will revisit that with my new understanding of why (PortID, ChannelID) are both required.

My last remaining question is how OnChanOpenTry works.

It has a local channelID and a local version string (in addition to the counterparty info). I guess the IBC module will auto-generate the channelID for it when a counterparty initiates a channel (is that true? if not where does it come from?). But shouldn't the module be able to set the version? In fact, I thought the main purpose for this callback was two-fold:

  • Module can ensure that channel has a valid ordering
  • Module checks the counterpartyVersion and either rejects it or accepts it and returns a compatible version we accept locally (maybe an older version, or a few features turned off). The way this callback is written, I would assume the relayer(?) is picking the version string here?

I cannot find much of this in the docs, (and this is sdk-specific stuff, not spec stuff). Maybe you can explain more?


Also, if you want to review the diff (maybe Monday... it is late today), that would be great. It should wrap up all of the loose ends.

@cwgoes
Copy link
Contributor

cwgoes commented Aug 7, 2020

It has a local channelID and a local version string (in addition to the counterparty info). I guess the IBC module will auto-generate the channelID for it when a counterparty initiates a channel (is that true? if not where does it come from?). But shouldn't the module be able to set the version? In fact, I thought the main purpose for this callback was two-fold:

By default, the initiator picks a remote port & channel identifier in addition to a local one - but of course the destination module can choose to reject the proposed identifier (and end the handshake). Per discussions with Agoric (cosmos/ibc#462) we plan to modify the handshake a bit to allow the destination chain to pick the identifier in a different handshake mode; this has not yet been done in the SDK.

The way this callback is written, I would assume the relayer(?) is picking the version string here?

Yes, that's right - the version provided therein is in the message (datagram); if you want the contract to calculate it directly, you can use an architecture similar to ADR 025.

@ethanfrey
Copy link
Member Author

By default, the initiator picks a remote port & channel identifier in addition to a local one

This is a bit tricky, as the initiator must know which channel ids are taken/free on the other side. This may make sense from the point of view of a relayer, but from a contract, one more external piece of info needed.

Yes, that's right - the version provided therein is in the message (datagram); if you want the contract to calculate it directly, you can use an architecture similar to ADR 025.

Very nice ADR025. Thank you for referring me there. Is this possible in the current ibc implementation in the cosmos SDK just by controlling the handler in x/wasm? Or do I need to make some minor changes to the ibc-04-channel module as well?

Also, ADR025 allows the module to perform the version negotiation, but it still requires the initiating party of the channel to specify the channelID on the counterparty, correct?

In fact, more than just the versioning aspect, this "passive relayer" approach is the only one that scales. There can be only 1 active relayer that creates the channel and that seems very centralized (and prone to crashing). On the other hand, there may be N different passive relayers, each relaying some subset of the channels between two chains and providing redundancy. But that is another topic. Looking forward to production quality relayers once stargate gets stabilized.

@cwgoes
Copy link
Contributor

cwgoes commented Aug 9, 2020

Very nice ADR025. Thank you for referring me there. Is this possible in the current ibc implementation in the cosmos SDK just by controlling the handler in x/wasm? Or do I need to make some minor changes to the ibc-04-channel module as well?

Some minor changes may be required, I'm not quite sure - paging @michaelfig, maybe there's room for collaboration here.

Also, ADR025 allows the module to perform the version negotiation, but it still requires the initiating party of the channel to specify the channelID on the counterparty, correct?

Yes; but that will change with cosmos/ibc#462.

@alpe alpe changed the base branch from 0.9.1_to_cosmos-sdk-0.39-master to 0.10_to_cosmos-stargate_ce9c2b2 August 18, 2020 09:17
@alpe alpe force-pushed the 0.10_to_cosmos-stargate_ce9c2b2 branch from fa61594 to 5b3040a Compare August 18, 2020 13:39
Copy link
Contributor

@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.

Thanks for the updates

x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
Msg []byte
// optional fields (or do we need exactly/at least one of these?)
TimeoutHeight uint64
TimeoutTimestamp uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the Sequence here as well to have all fields for an ibc packet
SourcePortID is known to us from the contract context and we can lookup the counterpart with the channelID:portID pair

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 sequence should not be set by the contract, but auto-assigned by the keeper/ibc module.

SourcePortID is known to us from the contract context and we can lookup the counterpart with the channelID:portID pair

Yes, I should add that as a document. The ChannelID (and context-contained PortID) should be enough to uniquely identify the channel and remote counterparty.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 correct. we can use channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel) to get the next sequence to build the package. Do I understand the workflow correct?

  1. the contract returns an IBCSendMsg
  2. in our handler_plugin we create the channeltypes.Packet
  3. we send the packet to channelKeeper.SendPacket
    3a. within the handler_plugin
    3b return it and send it from the keeper
  4. relayer sends packet to other chain....

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds correct.

Now that you mention the details of the HandlerPlugin interface, it doesn't by itself have the ability to query into the channelKeeper to get that info. I don't want to push that responsibility down to the contract and I guess we will have to extend the HandlerPlugin interface to have more capabilities during the transform.

x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
type Version string
```

Packet callbacks:
Copy link
Contributor

Choose a reason for hiding this comment

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

I will comment on the callbacks tomorrow with some more inspiration from the spike. Or we merge this and I do a PR on top

@alpe alpe added the stargate label Aug 18, 2020
Copy link
Member Author

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thanks for the review Alex, I will integrate some of this tomorrow morning.

I would appreciate a PR on top of this with the PacketCallback adjustments you had.

I may merge this first and then merge that second, or the other way around, depending on our timing.

Msg []byte
// optional fields (or do we need exactly/at least one of these?)
TimeoutHeight uint64
TimeoutTimestamp uint64
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 sequence should not be set by the contract, but auto-assigned by the keeper/ibc module.

SourcePortID is known to us from the contract context and we can lookup the counterpart with the channelID:portID pair

Yes, I should add that as a document. The ChannelID (and context-contained PortID) should be enough to uniquely identify the channel and remote counterparty.

x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Outdated Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
x/wasm/IBC.md Show resolved Hide resolved
@alpe alpe merged commit 81b8138 into 0.10_to_cosmos-stargate_ce9c2b2 Aug 19, 2020
@ethanfrey ethanfrey deleted the ibc-port-spec branch August 19, 2020 19:36
zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.5.0 to 1.6.1.
- [Release notes](https://github.com/spf13/viper/releases)
- [Commits](spf13/viper@v1.5.0...v1.6.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants