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

Update to cosmos-sdk 0.46 and tendermint 0.35 #2213

Closed

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented May 13, 2022

Resolves #1560
See also #2276

Description

Generate ibc_proto from a cosmos-sdk 0.46 pre-release revision, to evaluate API changes.
ibc-rs is also checked out from an update-pr-branch WIP branch.

Also updates tendermint-rs to 0.24.0-pre.2.

To be able to do this, ibc-proto-compiler is changed to parse the buf.lock file in cosmos-sdk and fetch the dependency modules listed there using the buf export command. The modules' export directories are added to the include path.


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/).

Reviewer checklist:

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

Use the buf.lock manifest in the newer cosmos-sdk to fetch the
dependency modules and use them in the include path.
Use the dependencies fetched with buf
@mzabaluev mzabaluev added proto I: protocol Internal: related to IBC protocol changes (eg. v2 update) labels May 13, 2022
@mzabaluev
Copy link
Contributor Author

File under "the things we have to do before cosmos/ibc-go#1345 is resolved".

@mzabaluev mzabaluev mentioned this pull request May 13, 2022
9 tasks
Mention the need for buf.
Update the default locations of the git clones.
- The low-level ABCI event types have moved to tendermint_rpc.
- The error code is now NonZeroU32.
- encode_vec is infallible.
This is temporary, until we get a better idea of how to distinguish this
condition from the RPC error.
The subscription events are now coming as ABCI events
rather than a flattened map, so the whole RawObject infrastructure
is no longer necessary.
height,
);
for abci_ev in block_events {
if let Some(ev) = channel_events::try_from_tx(abci_ev) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this can in theory extract any channel events, not only the ones that the previous code extracted specifically by matching a key name. So a WriteAcknowlegement, AcknowledgePacket, or a TimeoutPacket could get into the returned list where previous code would ignore these events. I don't know if it is a problem in practice.

mzabaluev and others added 7 commits May 19, 2022 11:25
Now it points to the tip of carlos/upgrade-sdk-0.46-tendermint-0.35
branch.
No changes to the generated code.
The gaiad test configuration specifies binding to 0.0.0.0, but the
"localhost" URLs cause the client query commands to try IPv6 on
dual-stack machines.
{
tracing::trace!(event = "SendPacket", "tx hash: {}", hash);
}
// TODO: use a helper function to filter to just the interesting
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 keep the tx hash trace if possible.

ALso update the comments for this method.

@mzabaluev
Copy link
Contributor Author

The integration test failures with gaia-rho (currently merged in the master branch) are due to a discrepancy with JSON serialization: tendermint-go has "omitempty" flags on fields of ResponseInfo, while the Rust counterpart in tendermint-proto does not have a corresponding serde(default) attribute. Somehow the value of the app_version field supplied by the RPC server is considered empty and omitted.

@mzabaluev
Copy link
Contributor Author

Works much better with tendermint-rs patched for informalsystems/tendermint-rs#1131: I only get 4 failures of the 12 integrations tests.

modules/Cargo.toml Outdated Show resolved Hide resolved
modules/src/core/ics02_client/events.rs Outdated Show resolved Hide resolved
The tendermint-rpc dependency conflicts with no_std.
The major user of that in modules is the ABCI event processing
infrastructure. It does not really belong in the data structures
crate, which should be chain agnostic.
The tx hash query better belongs with chain-specific stuff,
and in any case it's only used in the relayer.
Its presence in ibc modules complicated dependencies.
@adizere
Copy link
Member

adizere commented Jun 3, 2022

Can we update the

  • name
  • description
  • issue

attached to this PR? Would be great to make it clear to external parties what the work here is for.

As I understand it, the present PR provides compatibility with SDK 0.46 (but is not compat. with pre-0.46).

@mzabaluev mzabaluev changed the title Update proto files to cosmos-sdk 0.46 pre-release and a work-in-progress ibc-go Update to cosmos-sdk 0.46 and tendermint 0.35 RPC Jun 7, 2022
@mzabaluev mzabaluev changed the title Update to cosmos-sdk 0.46 and tendermint 0.35 RPC Update to cosmos-sdk 0.46 and tendermint 0.35 Jun 8, 2022
@mzabaluev
Copy link
Contributor Author

Closing this due to changing situation with cosmos-sdk dependencies. Some of the changes will be repurposed later.

@mzabaluev mzabaluev closed this Jun 30, 2022
@romac romac mentioned this pull request Aug 16, 2022
8 tasks
@romac romac deleted the mikhail/tendermint-rs-0.24-with-cosmos-sdk-prost-build branch November 22, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: protocol Internal: related to IBC protocol changes (eg. v2 update)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate new event structure of tendermint-go v0.35
3 participants