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

primitives: use alloy KECCAK_EMPTY constant #11851

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

tcoratger
Copy link
Contributor

No description provided.

Verified

This commit was signed with the committer’s verified signature.
klkvr Arsenii Kulikov

Verified

This commit was signed with the committer’s verified signature.
klkvr Arsenii Kulikov
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense to trim all of these from the reth traits

we could however keep re-exporting the non chain specific constants from there

but i'd rather have a them in some constant crate eventually

@tcoratger
Copy link
Contributor Author

makes sense to trim all of these from the reth traits

we could however keep re-exporting the non chain specific constants from there

but i'd rather have a them in some constant crate eventually

@mattsse So you mean doing something like?

/// Keccak256 over empty array: `0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470`
pub const KECCAK_EMPTY: B256 = alloy_consensus::constants::KECCAK_EMPTY;

Doesn't this add some overhead? I feel like direct imports from alloy_consensus are pretty smooth everywhere and alloy_consensus seems to be pretty much everywhere already. I'd say that removing the KECCAK_EMPTY declaration in reth entirely is in line with all the cleanup we've done on the primitives crate, right? But maybe I'm wrong :)

@mattsse mattsse added this pull request to the merge queue Oct 18, 2024
Merged via the queue into paradigmxyz:main with commit 0c70f6b Oct 18, 2024
40 checks passed
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.

None yet

2 participants