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

Upgrade to tendermint v0.32, ibc-proto-rs v0.32 #700

Merged

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Jun 2, 2023

Closes: #640
Basecoin update: informalsystems/basecoin-rs#107

Description

Am working on ICS27, needed to upgrade ibc-proto-rs, and respectively tendermint deps. So, come up with this PR ready, though to be merged after tower-abci bump tendermint deps to v0.32

Note:
Since we removed ics23 from ibc-proto-rs and re-exported the types in v0.31.0, we lost certain attributes like Eq, no-std Serialize, and Deserialize on the ics23 proto types. As a result, the serde feature no longer functions in no-std environments and has been excluded from CI's no-std-check


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

@Farhad-Shabani Farhad-Shabani added the A: blocked Admin: blocked by another (internal/external) issue or PR label Jun 2, 2023
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: +0.17 🎉

Comparison is base (2403dc3) 72.28% compared to head (d92b389) 72.45%.

❗ Current head d92b389 differs from pull request most recent head cd887bf. Consider uploading reports for the commit cd887bf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
+ Coverage   72.28%   72.45%   +0.17%     
==========================================
  Files         117      117              
  Lines       15526    15514      -12     
==========================================
+ Hits        11223    11241      +18     
+ Misses       4303     4273      -30     
Impacted Files Coverage Δ
crates/ibc/src/core/ics03_connection/connection.rs 39.34% <0.00%> (+0.23%) ⬆️
crates/ibc/src/core/ics23_commitment/merkle.rs 8.05% <0.00%> (ø)
crates/ibc/src/mock/context.rs 73.90% <ø> (-0.08%) ⬇️
crates/ibc/src/mock/header.rs 66.25% <62.50%> (-6.37%) ⬇️
crates/ibc/src/core/ics23_commitment/specs.rs 93.40% <87.50%> (+34.45%) ⬆️
...s/ibc/src/clients/ics07_tendermint/client_state.rs 70.01% <100.00%> (ø)
...ents/ics07_tendermint/client_state/misbehaviour.rs 83.60% <100.00%> (ø)
...nts/ics07_tendermint/client_state/update_client.rs 82.27% <100.00%> (ø)
crates/ibc/src/core/timestamp.rs 69.23% <100.00%> (-0.47%) ⬇️

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

@Farhad-Shabani Farhad-Shabani removed the A: blocked Admin: blocked by another (internal/external) issue or PR label Jun 12, 2023
@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review June 12, 2023 13:49
@plafer
Copy link
Contributor

plafer commented Jun 14, 2023

Last thing I'm trying to figure out is:

Since we removed ics23 from ibc-proto-rs and re-exported the types in v0.31.0, we lost certain attributes like Eq, no-std Serialize, and Deserialize on the ics23 proto types.

I couldn't find the PR that removed no-std Serialize, and Deserialize, nor information about it. Do you have more context here?

As a result, the serde feature no longer functions in no-std environments and has been excluded from CI's no-std-check

I don't see any file changed in the CI?

@Farhad-Shabani
Copy link
Member Author

Farhad-Shabani commented Jun 14, 2023

I couldn't find the PR that removed no-std Serialize, and Deserialize, nor information about it. Do you have more context here?

As of this PR serde works for std envs only. See this issue as well for more context.

I don't see any file changed in the CI?

here we do not enable serde feature anymore

@plafer plafer changed the title Upgrade to tendermint v0.32, ibc-proto-rs v0.31 Upgrade to tendermint v0.32, ibc-proto-rs v0.32 Jun 14, 2023
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.

🚀

Note that I upgraded ibc-proto-rs to v0.32, and documented that the serde feature no longer works in no_std (2f4c108)

@plafer plafer mentioned this pull request Jun 14, 2023
7 tasks
@Farhad-Shabani Farhad-Shabani merged commit 5d1855b into main Jun 14, 2023
@Farhad-Shabani Farhad-Shabani deleted the farhad/upgrade-to-tendermint-0.32-ibc-proto-rs-0.31 branch June 14, 2023 14:34
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
* Upgrade to tendermint v0.32 and ibc-proto-rs v0.31

* Fix clippy catches

* Bump versions in ci/no-std-check

* Remove irrelevant docstring

* Discard serde & mocks-no-std features from no-std-check

* Add unclog

* fix: remove ibc-proto features

* imp: change ICS* aliases to Raw* for imported specs types

* fix: remove cfg-if dependency from cargo.toml

* dep: bump ibc-proto-rs to 0.32.0

* fix: revert ibc-proto-rs upgrade

* update ibc-proto-rs to v0.32

* document serde feature not working in no_std

* fmt

---------

Signed-off-by: Farhad Shabani <[email protected]>
Co-authored-by: Philippe Laferriere <[email protected]>
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.

Bump ics23 to v0.10 to get prehash_key_before_comparison
2 participants