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

abci: Change hashes' type from Bytes to tendermint::Hash or tendermint::AppHash #1232

Merged
merged 8 commits into from
Nov 16, 2022

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 16, 2022

Fixes: #1095

Both questions in #1095 (comment) are resolved with "Yes".

  • 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/

@@ -19,8 +18,7 @@ pub struct Info {
/// The latest block for which the app has called [`Commit`](super::super::Request::Commit).
pub last_block_height: block::Height,
/// The latest result of [`Commit`](super::super::Request::Commit).
// XXX(hdevalence): fix this, should be apphash?
pub last_block_app_hash: Bytes,
pub last_block_app_hash: AppHash,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mysteriously, this is serialized as an array of bytes rather than a hex string.
This PR does not change it.

@codecov-commenter
Copy link

Codecov Report

Merging #1232 (8c5565b) into main (d102af4) will increase coverage by 0.0%.
The diff coverage is 77.1%.

@@          Coverage Diff          @@
##            main   #1232   +/-   ##
=====================================
  Coverage   64.2%   64.3%           
=====================================
  Files        244     244           
  Lines      21364   21373    +9     
=====================================
+ Hits       13734   13746   +12     
+ Misses      7630    7627    -3     
Impacted Files Coverage Δ
tendermint/src/abci/request/offer_snapshot.rs 0.0% <0.0%> (ø)
tendermint/src/abci/response/init_chain.rs 0.0% <0.0%> (ø)
tendermint/src/genesis.rs 100.0% <ø> (ø)
tendermint/src/abci/response/info.rs 50.0% <50.0%> (ø)
tendermint/src/hash.rs 63.0% <76.9%> (-0.6%) ⬇️
rpc/tests/kvstore_fixtures.rs 98.2% <100.0%> (+<0.1%) ⬆️
testgen/src/vote.rs 84.8% <0.0%> (-0.9%) ⬇️
testgen/src/header.rs 83.5% <0.0%> (-0.6%) ⬇️
tendermint/src/node.rs 63.8% <0.0%> (-0.2%) ⬇️
light-client-verifier/src/types.rs 39.3% <0.0%> (+0.5%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mzabaluev mzabaluev marked this pull request as ready for review November 16, 2022 19:10
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.

I think this makes sense. And for the record, my answer to both questions in #1095 (comment) is yes.

@thanethomson thanethomson merged commit 6ae3f25 into main Nov 16, 2022
@thanethomson thanethomson deleted the mikhail/abci-change-bytes-to-hash branch November 16, 2022 20:52
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.

abci: Change hashes' type from bytes::Bytes to tendermint::Hash
3 participants