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 support #167

Merged
merged 19 commits into from
Jan 18, 2021
Merged

Add ibc support #167

merged 19 commits into from
Jan 18, 2021

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Jan 14, 2021

Closes #166

  • Update to newest master with ibc types
  • More CosmosMsg variants in Go types
  • More QueryRequest variants in Go types
  • IBC related types in Go
  • 6 new IBC-related Entry Points exposed to Go
  • Add ibc enabled contract Add Ibc sample contract cosmwasm#714
  • Add integration tests to call ibc-enabled contract

In follow up PR #172:

@ethanfrey ethanfrey changed the base branch from master to 0.14-dev January 14, 2021 22:15
@ethanfrey ethanfrey mentioned this pull request Jan 15, 2021
3 tasks
@ethanfrey ethanfrey marked this pull request as ready for review January 15, 2021 20:37
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 to see IBC support here! 🎸
Some minor notes on naming and types.

types/ibc.go Outdated Show resolved Hide resolved
types/msg.go Outdated Show resolved Hide resolved
types/msg.go Outdated Show resolved Hide resolved
types/msg.go Show resolved Hide resolved
types/msg.go Outdated Show resolved Hide resolved
types/queries.go Show resolved Hide resolved
types/queries.go Show resolved Hide resolved
types/msg.go Show resolved Hide resolved
api/lib.go Outdated Show resolved Hide resolved
api/lib.go Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

Thanks for the review @alpe

I will make some of the requested changes in the data types in cosmwasm and get a new cosmwasm build out before updating the Go side. My key question before I can properly update the Rust types is this:

Okay, so Timeout height is (version, height). Timeout timestamp is in seconds (millisecond? microseconds?) since UNIX 0 ???
And only one of the two can be set per packet? Is exactly one required or is leaving both blank also valid?

@alpe
Copy link
Member

alpe commented Jan 18, 2021

Okay, so Timeout height is (version, height). Timeout timestamp is in seconds (millisecond? microseconds?) since UNIX 0 ???

Timestamp is the absolute timeout. // block timestamp (in nanoseconds) after which the packet times out`` it is the same type used in ConsensusState.return uint64(cs.Timestamp.UnixNano())`

And only one of the two can be set per packet? Is exactly one required or is leaving both blank also valid?

From the packet validation logic I must assume both can be set but one is required

@ethanfrey
Copy link
Member Author

ethanfrey commented Jan 18, 2021

Changes to the ibc types made here: CosmWasm/cosmwasm@62b9fc7

Also, the ibc_enabled flag is now available.

TODO:

api/bindings.h Show resolved Hide resolved
api/bindings.h Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

I will do some cleanup as mentioned before.

@alpe this should be usable for wasmd integration. You will need to copy the updated contracts over.

Note that ibc_test.go in the top level is showing how to test the contract from a high level and can be used as a starting point for wasmd integration tests.

@ethanfrey ethanfrey merged commit af3a20d into 0.14-dev Jan 18, 2021
@ethanfrey ethanfrey deleted the add-ibc-support branch January 18, 2021 19:59
@ethanfrey ethanfrey mentioned this pull request Jan 18, 2021
Src IBCEndpoint `json:"src"`
Dest IBCEndpoint `json:"dest"`
Sequence uint64 `json:"sequence"`
TimeoutHeight *IBCTimeoutHeight `json:"timeout_height,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Naming became a bit odd. This field and the height have the same name. This results into: msg.SendPacket.TimeoutHeight.TimeoutHeight

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy for better names. I copied them both from "somewhere else".

If you want to propose names for all these fields, I can PR that into cosmwasm

type TransferMsg struct {
ChannelID string `json:"channel_id"`
ToAddress string `json:"to_address"`
Amount Coins `json:"amount"`
Copy link
Member

Choose a reason for hiding this comment

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

This should be type Coin. Transfer does not support multiple. See https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/proto/ibc/applications/transfer/v1/tx.proto#L28

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I did this in Rust. Typo here. I will fix.

    Transfer {
        /// exisiting channel to send the tokens over
        channel_id: String,
        /// address on the remote chain to receive these tokens
        to_address: HumanAddr,
        /// packet data only supports one coin
        /// https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/proto/ibc/applications/transfer/v1/transfer.proto#L11-L20
        amount: Coin,
}

Copy link
Member Author

@ethanfrey ethanfrey Jan 18, 2021

Choose a reason for hiding this comment

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

This now resolved in #172
Commit 9404357

// Nanoseconds since UNIX epoch
// See https://golang.org/pkg/time/#Time.UnixNano
TimeoutTimestamp *uint64 `json:"timeout_timestamp,omitempty"`
}
Copy link
Member

Choose a reason for hiding this comment

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

ICS-20 port-id is not fix to transport. It can be configured in the genesis and may vary between chain. Although I don't expect this to happen. To fully support ICS-20 transfers we should have the PortID as a field in the message.

Copy link
Member Author

Choose a reason for hiding this comment

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

To fully support ICS-20 transfers we should have the PortID as a field in the message.

Actually the opposite.
The contract cannot be expected to know the proper port (and would hardcode to transfer). The x/wasm module could read the genesis info "somehow" and then auto-fill with the canonical ICS-20 port for the chain.

At least that is my reasoning. If we could expose this info to a contract, we could just auto-fill.
Or is this info not available to x/wasm?

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.

Expose stargate/ibc related functionality
3 participants