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

feat(core)!: use newtype sequencer block hashes #1884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SuperFluffy
Copy link
Member

@SuperFluffy SuperFluffy commented Dec 19, 2024

Summary

Adds a newtype astria_core::sequencerblock::v1::block::Hash to represent Sequencer block hash (an array of 32 bytes).

The standard formatting of the newtype's std::format:Display implementation uses lower case hex, while its alternative selector uses standard base64. Note that the latter was introduced to provide a way to format the hash using the same conventions as pbjson for the protobuf wire types.

use astria_core::sequencerblock::v1::block;
let block_hash = block::Hash::new([42; 32]);
assert_eq!(
    "2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a",
    format!("{block_hash}"),
);
assert_eq!(
    "KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKio=",
    format!("{block_hash:#}"),
};

Background

During work on the Astria Auctioneer it was discovered that representing Sequencer Block Hashes with a distinct type is desirable for two reasons:

  1. to distinguish Sequencer block hashes from other objects represented by an array of 32 bytes ([u8; 32]) that show up in the Astria ecosystem, for example Rollup block hashes.
  2. to have a unified std::format::Display implementation for all Astria block hashes using a hexadecimal representation.

Changes

  • Introduce the astria_core::sequencerblock::v1::block::Hash newtype for [u8; 32] objects representing Astria block hashes, and replace all domain types with it.

Testing

Added doc tests for the Hash constructor and normal and alternative display impl.

Changelogs

Changelogs updated.

Breaking Changelist

  • This is a breaking change in astria-core but not in any of the services (because the return value of public getters was changed).
  • This is not network breaking.

@github-actions github-actions bot added conductor pertaining to the astria-conductor crate sequencer pertaining to the astria-sequencer crate labels Dec 19, 2024
@SuperFluffy SuperFluffy force-pushed the superfluffy/newtype-block-hashes branch from ad21f02 to e4e4167 Compare December 19, 2024 13:53
@SuperFluffy SuperFluffy marked this pull request as ready for review December 19, 2024 13:54
@SuperFluffy SuperFluffy requested a review from a team as a code owner December 19, 2024 13:54
@SuperFluffy SuperFluffy force-pushed the superfluffy/newtype-block-hashes branch from e4e4167 to 3179682 Compare December 19, 2024 13:56
@SuperFluffy SuperFluffy force-pushed the superfluffy/newtype-block-hashes branch from 3179682 to 73dcc88 Compare December 19, 2024 13:57
Copy link
Contributor

@ethanoroshiba ethanoroshiba left a comment

Choose a reason for hiding this comment

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

Couple nits but otherwise LGTM. I don't foresee these changes being breaking since the storage types remain the same, but it may be worthwhile to double check this by starting a sync with main, stopping midway and building with this PR, then continuing with the sync.

///
/// Note that hex based `Display` impl of [`Hash`] does not follow the pbjson
/// convention to display protobuf `bytes` using base64 encoding. To get the
/// display formatting faithful to pbjson convetion use the alternative formatting selector,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// display formatting faithful to pbjson convetion use the alternative formatting selector,
/// display formatting faithful to pbjson convention use the alternative formatting selector,

Comment on lines +56 to +57
// TODO: use the block hash's Display impl for this
state.serialize_field(FIELDS[1], &base64(&*self.0.block_hash))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this can't be done now?

Suggested change
// TODO: use the block hash's Display impl for this
state.serialize_field(FIELDS[1], &base64(&*self.0.block_hash))?;
state.serialize_field(FIELDS[1], &self.0.block_hash.to_string())?;

Copy link
Member Author

@SuperFluffy SuperFluffy Dec 20, 2024

Choose a reason for hiding this comment

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

Ha, no, thanks for pointing that out! I updated the lines without thinking about about them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conductor pertaining to the astria-conductor crate override-freeze sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants