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 compatibility for ICS20 v2 #4008

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented May 30, 2024

Closes: #4006
Closes: #4098

Description

This PR adds the compatibility for ICS20 v2. This includes:

  • Conditional deserialization of PacketData if the channel version is ics20-1 or ics20-2
  • Update channel Version struct to more easily specify ics20-1 or ics20-2
  • Add test to transfer multiple denom in a single message

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:

  • Creating new ibc-proto-rs release with ICS20 v2 protos
  • Adding a new simd to Cosmos Nix to run the Hermes ICS20 v2 test

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

crates/relayer-types/src/applications/transfer/mod.rs Outdated Show resolved Hide resolved
@@ -41,6 +41,7 @@ pub struct MsgTransfer<C = Coin> {
pub timeout_timestamp: Timestamp,
/// optional memo
pub memo: Option<String>,
pub tokens: Vec<C>,
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

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

crates/relayer-types/src/applications/transfer/packet.rs Outdated Show resolved Hide resolved
@@ -52,6 +52,10 @@ impl Version {
})
.unwrap_or(false)
}

pub fn is_ics20_v2(&self) -> bool {
self.0.contains("ics20-2")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.0.contains("ics20-2")
self.0 == "ics20-2"

Copy link
Contributor Author

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"
}

Copy link
Member

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.

Copy link
Contributor Author

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

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.

ICS20 v2 Protobuf encoding Add ICS20 v2 multiple denom compatibility
2 participants