-
Notifications
You must be signed in to change notification settings - Fork 334
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
Document ibc #884
Document ibc #884
Conversation
Happy for a review on this. This should be a mostly complete version of the current state. I will be adapting the error handling in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, very helpful.
I'm concerned that this valuable document will never get maintained here (as it happened so often in the past). Why do we not have such things in docs?
Note that neither `counterparty_version` nor `counterparty_endpoint` is set in | ||
`ibc_channel_open` for chain A. Chain B should enforce any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this possible with non-optional type counterparty_endpoint: IbcEndpoint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it just gets "" fields. You are right, we should make this optional if it is not present, but maybe that would confused all calls but one where it is optional (I can use IbcChannelWithOptionalCounterparty
maybe for this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass in the counterparty endpoint on init as well: https://github.com/CosmWasm/wasmd/blob/master/x/wasm/ibc.go#L48
I am not sure if these are set in the sdk before calling us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After digging more in the sdk code, I found that counterparty.port is set here (in ChannelOpenInit
), but counterparty.channel is not set.
In ChannelOpenTry
, the counterparty channel must be set (ignore that comment, the code shows otherwise): https://github.com/cosmos/cosmos-sdk/blob/88e03e4f4056e8614847fd6adf17ce9252b6a89b/x/ibc/core/04-channel/types/msgs.go#L131-L134
Since we collapse these 2 into one call (to limit the number of exports and duplicated code), I am not sure the best way to handle this.
Making channel
optional in all IbcEndpoint
would be overkill and complicate other logic. Using counterparty: IbcEndpointWithOptionalChannel
for a custom type for ibc_channel_open
could work, along with a comment that counterparty_version
and counterparty.channel
are both set or both unset.
Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need counterparty_endpoint.port_id
when counterparty_version
and counterparty_version.channel
is unset? If not, I think
pub counterparty_endpoint: Option<IbcEndpoint>,
would be solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some protocols (like ics27 currently) try to enforce the remote port.
Few if any care about the remote channel_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, okay. Then I don't have a great idea. In this case it starts feeling wrong to use the same type for two different calls because we don't have dynamic typing available here.
is a functioning relayer. (In the absence of a functioning relayer, it will | ||
never get a response). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things in the docs repo are even less connected to our code than md files here. My feeling is that as we hit 1.0 we will no longer have constant breaking changes and the docs will be valid for more than 1-2 months. |
Both { | ||
timestamp_nanos: u64, | ||
block: IbcTimeoutBlock, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this? Both(u64, IbcTimeoutBlock),
?
packages/std/src/ibc.rs
Outdated
pub fn in_secs(block: &BlockInfo, secs: u64) -> Self { | ||
let secs = block.time + secs; | ||
let nanos = secs * 1_000_000_000 + block.time_nanos; | ||
IbcTimeout::TimestampNanos(nanos) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this tight coupling between the two types IbcTimeout
and BlockInfo
. I don't think IbcTimeout
should need to know BlockInfo
exists.
However, I see the point. We need some now + x
constructor, which is a pain as we do not have a nanosecond precision time point type.
Could you move this logic back to the contract and we come up with a better solution later? Otherwise we risk the need for breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think IbcTimeout should need to know BlockInfo exists.
What is the problem here?
BlockInfo
is a super basic primitive. This is to avoid the very typical problem (that I hit a few times) of passing in seconds into the timeout field - both u64.
I think this is the simplest and most straight-forward approach to that. I can pull this into a separate function and not a method if you want, but I would like to keep it in cosmwasm_std
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of types and contracts depend on BlockInfo
(like all the Expiration
times)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem here?
The problem is we throw all of BlockInfo
into IbcTimeout
that does not care about the block info at all (height, chain ID) and simply needs a proper time format. The time in BlockInfo
is not a great Rust type. It is designed for JSON compatibility.
I can create a better solution that as part of this PR if you want. My idea was to unblock this PR by avoiding standard library design discussions here.
A number of types and contracts depend on
BlockInfo
(like all theExpiration
times)
No type in cosmwasm-std depends on BlockInfo
other than Env
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, I will move this to the contracts, but I don't see that as more than a placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see you made the Timepoint. Thanks
As I see we are left with a discussion on how to encode the optional channel for the first call, as well as where to put the helper for ibc packet timeout. Nothing else is an open question, right? |
Add Timestamp type
Created this as a follow-up for the one minor issue left: #888 |
Closes #147