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

consensus: make Header nonce u64 #1385

Closed
tcoratger opened this issue Sep 26, 2024 · 4 comments
Closed

consensus: make Header nonce u64 #1385

tcoratger opened this issue Sep 26, 2024 · 4 comments
Labels
enhancement New feature or request
Milestone

Comments

@tcoratger
Copy link
Contributor

Component

consensus, eips, genesis

Describe the feature you would like

At the moment, the nonce field in Header consensus struct is B64:

pub nonce: B64,

To match the definition we expect in reth and other repos:

https://github.com/paradigmxyz/reth/blob/3a255a1cabd1f19399b42c1fe9bdcc130ac61c2e/crates/storage/codecs/src/alloy/header.rs#L30

it would be nice to make it u64, would you agree @mattsse ?

Additional context

No response

@tcoratger tcoratger added the enhancement New feature or request label Sep 26, 2024
@Rjected
Copy link
Contributor

Rjected commented Sep 26, 2024

Just noting that to do this correctly, we also need to bring back the Encodable / Decodable implementations from before reth switched to alloy types, otherwise this would break RLP encoding.

@mattsse
Copy link
Member

mattsse commented Sep 26, 2024

technically this is 8bytes, I think we can keep B64, dont think this type is used that often and we can add a helper that converts this to u64

@yash-atreya yash-atreya added this to the 1.0 milestone Sep 26, 2024
@alloy-rs alloy-rs deleted a comment from SlayerPower Oct 7, 2024
@varun-doshi
Copy link

Do we still need a helper to convert it to u64? If so I can take this

@mattsse
Copy link
Member

mattsse commented Dec 30, 2024

I think we can leave this as b64, because for mainnet this field is now useless anyway and b64 would be more beneficial in case OP starts embedding encoded values in there

@mattsse mattsse closed this as completed Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants