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

Bump ibc-proto to v0.29.0 and add memo field to PacketData #627

Merged
merged 1 commit into from
May 2, 2023
Merged

Bump ibc-proto to v0.29.0 and add memo field to PacketData #627

merged 1 commit into from
May 2, 2023

Conversation

kevinji
Copy link
Contributor

@kevinji kevinji commented Apr 15, 2023

Closes: #559

Description

This PR bumps ibc-proto to v0.29.0, which also introduces the memo field that is added to PacketData for ICS-20, and tendermint to v0.30.0.

Most of the other changes are due to Protobuf::encode_vec no longer returning a Result.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • 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).

@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Patch coverage: 61.70% and project coverage change: -0.19 ⚠️

Comparison is base (89c5298) 73.09% compared to head (5982cfa) 72.90%.

❗ Current head 5982cfa differs from pull request most recent head 43768d7. Consider uploading reports for the commit 43768d7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
- Coverage   73.09%   72.90%   -0.19%     
==========================================
  Files         126      126              
  Lines       15665    15677      +12     
==========================================
- Hits        11450    11429      -21     
- Misses       4215     4248      +33     
Impacted Files Coverage Δ
crates/ibc/src/core/handler.rs 74.64% <ø> (-0.36%) ⬇️
crates/ibc/src/core/ics02_client/error.rs 20.00% <ø> (-0.84%) ⬇️
crates/ibc/src/core/ics03_connection/connection.rs 33.86% <ø> (-2.31%) ⬇️
crates/ibc/src/core/ics04_channel/channel.rs 53.25% <ø> (-0.83%) ⬇️
crates/ibc/src/core/ics04_channel/error.rs 0.00% <ø> (ø)
...s/ibc/src/clients/ics07_tendermint/client_state.rs 68.03% <33.33%> (-0.36%) ⬇️
crates/ibc/src/applications/transfer/memo.rs 35.00% <35.00%> (ø)
...tes/ibc/src/applications/transfer/msgs/transfer.rs 45.53% <50.00%> (-1.14%) ⬇️
crates/ibc/src/applications/transfer/packet.rs 44.00% <50.00%> (-8.64%) ⬇️
...bc/src/clients/ics07_tendermint/consensus_state.rs 81.81% <100.00%> (-0.21%) ⬇️
... and 14 more

... and 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@plafer
Copy link
Contributor

plafer commented Apr 20, 2023

We were holding off upgrading to tendermint-rs v0.30 because the tendermint-abci crate no longer supports Tendermint Core v0.34. I am not sure if any project uses tendermint-abci though...

Though many projects use tower-abci now, and they have a tendermint-30 ready. @erwanor are you waiting on us to upgrade for you to merge that branch? If so then maybe we take the risk to break projects that do use tendermint-abci; we'll have to at some point anyways...

@plafer
Copy link
Contributor

plafer commented Apr 20, 2023

Also @kevinji thank you for the PR! Next time could you have one PR/issue please? Reviewing the "memo field" concern is difficult here because the diff is cluttered with tendermint-rs's upgrade related things. It also makes for a cleaner changelog when each issue has its own PR.

But anyways, this is still very appreciated as is 😁

@kevinji
Copy link
Contributor Author

kevinji commented Apr 20, 2023

@plafer Hey I'm happy to split this PR up but I didn't find an easy way to do so since we needed to bump ibc-proto to pick up the new memo field in the raw types. Would it be better to just add the Memo type in a separate PR without using it yet, and then start using the memo field in the PR that bumps ibc-proto and Tendermint?

@plafer
Copy link
Contributor

plafer commented Apr 20, 2023

Hmmm I don't have a good enough understanding of the specifics yet to say; let's keep it as is for this PR, and I'll see if I can suggest how I would have split it up after reviewing (for future reference)

@romac
Copy link
Member

romac commented Apr 20, 2023

FYI if you end up bumping ibc-proto and tendermint-rs, I just released ibc-proto 0.30.0 which brings with it tendermint-proto 0.31.1.

@kevinji kevinji changed the title Bump ibc-proto to v0.29.0 and add memo field to PacketData Bump ibc-proto to v0.30.0 and add memo field to PacketData Apr 20, 2023
@kevinji
Copy link
Contributor Author

kevinji commented Apr 20, 2023

I just amended the PR so ibc-proto is now bumped to v0.30.

@kevinji kevinji changed the title Bump ibc-proto to v0.30.0 and add memo field to PacketData Bump ibc-proto to v0.29.0 and add memo field to PacketData May 2, 2023
@kevinji
Copy link
Contributor Author

kevinji commented May 2, 2023

Just responded to review comments, sorry for the delay!

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

Looks good, except we're only missing the TimeoutEvent::memo field!

@kevinji
Copy link
Contributor Author

kevinji commented May 2, 2023

Just pushed a new change adding that field.

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

🙏

@plafer plafer merged commit 91c5ea9 into cosmos:main May 2, 2023
@kevinji kevinji deleted the bump-ibc-proto branch May 2, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ICS-20: add memo field
4 participants