-
Notifications
You must be signed in to change notification settings - Fork 329
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 compatibility for ICS20 v2 #4008
base: master
Are you sure you want to change the base?
Conversation
@@ -41,6 +41,7 @@ pub struct MsgTransfer<C = Coin> { | |||
pub timeout_timestamp: Timestamp, | |||
/// optional memo | |||
pub memo: Option<String>, | |||
pub tokens: Vec<C>, |
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.
Let's add a doc comment here.
I also wonder if we can't just keep this tokens
field only instead of having both token
and tokens
?
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.
If I'm not mistaken, a channel with version ics20-1
will require the token
field since the previous proto didn't have the tokens
field https://github.com/cosmos/ibc-go/blob/v8.3.1/proto/ibc/applications/transfer/v1/tx.proto#L51.
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.
Sure, but this is a domain type, so we can instead opt to only keep a single field here (ie. tokens
), which would only ever contain a single Coin
for v1 and potentially multiple for v2. That is, as long as we can still tell somehow which version we are dealing with, maybe from the context?
Or are we using the fact that token
is Some
or None
to figure out which version it is?
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.
Oh this is a good point. I tried to keep the domain type as close to the proto struct as possible since the crate could be used by someone else.
We don't use the fact that token
is Some
or None
to determine the version, but the implementation From<MsgTransfer> for RawMsgTransfer
might be tricky.
impl From<MsgTransfer> for RawMsgTransfer { |
If there is only 1 token in the domain type's
tokens
, we would need to find a way to determine if we are building a v1 RawMsgTransfer
which will have the token: Some(token)
or the v2 with tokens: vec!(token)
. But maybe ibc-go has some type of conversion since there is already a conversion for the PacketData and this isn't an issue, https://github.com/cosmos/ibc-go/blob/main/modules/apps/transfer/internal/convert/convert.go
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 about making the MsgTransfer
domain type an enum, with one variant for v1 and one for v2?
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 updated MsgTransfer
to be an enum in this commit 8ac128e
@@ -52,6 +52,10 @@ impl Version { | |||
}) | |||
.unwrap_or(false) | |||
} | |||
|
|||
pub fn is_ics20_v2(&self) -> bool { | |||
self.0.contains("ics20-2") |
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.
self.0.contains("ics20-2") | |
self.0 == "ics20-2" |
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.
This might fail to catch an ICS29 channel with ICS20 v2 since the format would be:
{
"fee_version": "ics29-1",
"app_version": "ics20-2"
}
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 right it can contain json, then we should probably try to deserialize the version as a JSON Value
and look at the app_version
field, and otherwise fall back to comparing by equality.
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.
Used JSON deserialization to verify the ics20 versions in this commit f68c5e8
* Deserialize PacketData for ICS20 v2 with Protobuf * Add memo field filter test for ICS20 v2 * Set ICS20 v2 memo filter test behind feature * Add changelog entry * Update comment when creating new Link from opts
Closes: #4006
Closes: #4098
Description
This PR adds the compatibility for ICS20 v2. This includes:
ics20-1
orics20-2
ics20-1
orics20-2
Current status of the PR - 30.05.2024
This PR is blocked until a tagged release of ibc-go with ICS20 v2 feature, required for:
simd
to Cosmos Nix to run the Hermes ICS20 v2 testPR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.