-
Notifications
You must be signed in to change notification settings - Fork 245
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(consensus) implement RLP for Account information #789
feat(consensus) implement RLP for Account information #789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the reth struct here, we should be able to derive this implementation. However, we will need to re-order the fields. That seems fine to me
while we're here, I would also like to see a convenience function for trie_hash
(similar to in Encodable2718
pub fn trie_hash_slow(&self) -> B256 {
let mut buf = vec![];
self.encode(&mut buf);
keccak256(buf)
}
bonus points for memoizing the hash in a OnceLock
@Rjected @mattsse do you have any RLP encoding/trie hashing test vectors handy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, I don't think we use this type in reth because we don't store the account's storage root
maybe @rkrasiuk knows if we have some test vectors
but I wonder if we can use derive here?
hmmm, this type is committed in the state MPT right? or am i misunderstanding? |
Thank you very much for your review i added the trie_hash and trie_hash_slow methods |
should i directly derive ? |
yes, please derive and reorder the fields to match reth as linked above |
Thank you very much guys for the review of the PR. I reorder the field similar to reth struct and i derived instead of manual impl |
thank you. last few things:
|
oh and for no_std support in CI, need to add a
|
Thank you very much for you review & your patience !
No problem !
Thank you, when you open issue about it if it's possible to link any example of test like this i'd like to take that new issue :) |
crates/consensus/src/account.rs
Outdated
pub fn trie_hash_slow(&self) -> B256 { | ||
let mut buf = vec![]; | ||
self.encode(&mut buf); | ||
let mut hasher = Keccak256::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
we usually write this using the conveniencealloy_primitives::keccak256
instead of invoking Keccak256::new()
and update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we usually write this using the convenience
alloy_primitives::keccak256
instead of invokingKeccak256::new()
andupdate
Thank you very much, i changed !
this is what i was asking mattsse and rjected about earlier :) I am not sure where to link for testvectors for account encoding |
Yep gotcha, ty very much for all support on this ! |
* Update account.rs * fix clippy * Update account.rs * Update account.rs * Update account.rs * Update account.rs * Update account.rs * Update account.rs * Update account.rs
Closes #780