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 support for tendermint::chain::Id constants #1105

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

tony-iqlusion
Copy link
Collaborator

@tony-iqlusion tony-iqlusion commented Mar 16, 2022

In PeggyJV/ocular#18 we are trying to create a registry of "well-known" chain IDs. I proposed using tendermint::chain::Id for this, but to do so we'd need to have a const initializer support.

This commit changes the internal representation of chain::Id to Cow<'static, str>. This permits a const fn initializer, but avoids adding a lifetime to chain::Id itself.

It also adds chain::Id::new which is defined as a const fn, which can be used to define chain ID constants.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@tony-iqlusion
Copy link
Collaborator Author

If you're happy with the general direction I can add some tests for this specific constructor.

@codecov-commenter
Copy link

Codecov Report

Merging #1105 (fc693c1) into master (e9e12ce) will decrease coverage by 0.0%.
The diff coverage is 47.0%.

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

@@           Coverage Diff            @@
##           master   #1105     +/-   ##
========================================
- Coverage    64.7%   64.7%   -0.1%     
========================================
  Files         240     240             
  Lines       20789   20808     +19     
========================================
+ Hits        13464   13466      +2     
- Misses       7325    7342     +17     
Impacted Files Coverage Δ
tendermint/src/chain/id.rs 63.5% <47.0%> (-4.9%) ⬇️
tendermint/src/evidence.rs 21.6% <0.0%> (-3.8%) ⬇️
abci/src/server.rs 9.2% <0.0%> (-0.2%) ⬇️
light-client/src/peer_list.rs 90.2% <0.0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9e12ce...9360df9. Read the comment docs.

@tony-iqlusion tony-iqlusion force-pushed the tendermint/chain-id-constants branch 2 times, most recently from cd1f6c6 to cc889be Compare March 17, 2022 02:07
@thanethomson
Copy link
Contributor

If you're happy with the general direction I can add some tests for this specific constructor.

Seems good to me!

In PeggyJV/ocular#18 we are trying to create a registry of "well-known"
chain IDs. I proposed using `tendermint::chain::Id` for this, but to do
so we'd need to have a const initializer support.

This commit changes the internal representation of `chain::Id` to
`Cow<'static, str>`. This permits a `const fn` initializer, but avoids
adding a lifetime to `chain::Id` itself.

It also adds `chain::Id::new` which is defined as a `const fn`, which
can be used to define chain ID constants.
@tony-iqlusion tony-iqlusion force-pushed the tendermint/chain-id-constants branch from cc889be to 50ec5ce Compare March 17, 2022 18:47
@tony-iqlusion
Copy link
Collaborator Author

Tests and changelog entry added

Comment on lines +172 to +173
const MAX_LENGTH_CHAIN_ID: &str = "01234567890123456789012345678901234567890123456789";
const OVERLENGTH_CHAIN_ID: &str = "012345678901234567890123456789012345678901234567890";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These need to be constants to test Id::new, which accepts a &'static str as an argument.

Comment on lines +190 to +191
assert!(
matches!(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Soon: assert_matches!

rust-lang/rust#82775

@thanethomson thanethomson added the backport:v0.23 To be backported to v0.23.x label Mar 17, 2022
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

LGTM!

@thanethomson thanethomson merged commit 84012a7 into master Mar 17, 2022
@thanethomson thanethomson deleted the tendermint/chain-id-constants branch March 17, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v0.23 To be backported to v0.23.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants