-
Notifications
You must be signed in to change notification settings - Fork 353
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: Add ICS20 TransferV2 #2317
base: main
Are you sure you want to change the base?
Conversation
a1910c3
to
0031a25
Compare
c02e541
to
2288ae6
Compare
8622495
to
2c6e84d
Compare
2c6e84d
to
40b1383
Compare
bf68aef
to
e934799
Compare
9a7f3f5
to
3714b3a
Compare
3714b3a
to
39179ac
Compare
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.
Looks nice. Some small comments, mostly about the builder.
contracts/ibc-callbacks/src/msg.rs
Outdated
@@ -20,6 +20,8 @@ pub enum ExecuteMsg { | |||
/// Who should receive callbacks for the message | |||
#[serde(default)] | |||
callback_type: CallbackType, | |||
/// IBC channel version | |||
channel_version: String, |
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.
nit: Wouldn't an enum be nicer?
packages/std/src/ibc.rs
Outdated
@@ -4,6 +4,7 @@ | |||
use core::cmp::{Ord, Ordering, PartialOrd}; | |||
use schemars::JsonSchema; | |||
use serde::{Deserialize, Serialize}; | |||
use std::vec; |
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.
Is this needed?
pub struct WithMemo { | ||
memo: String, | ||
pub memo: String, |
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.
Couldn't we keep them pub(crate)
?
/// Adds forwarding data. | ||
/// It is worth to notice that the builder does not allow to add forwarding data along with | ||
/// callbacks. It is discouraged in the IBC docs: | ||
/// https://github.com/cosmos/ibc-go/blob/main/docs/docs/04-middleware/02-callbacks/01-overview.md#known-limitations |
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.
Can we link to the rendered docs instead? https://ibc.cosmos.network/v9/middleware/callbacks/overview/#known-limitations Just a bit nicer to read
/// It is worth to notice that the builder does not allow to add forwarding data along with | ||
/// callbacks. It is discouraged in the IBC docs: |
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.
The IBC docs specifically mention source callbacks. Destination callbacks are fine.
Can we allow that combination forwarding + destination callback in the builder?
4a4172f
to
a1b922e
Compare
No description provided.