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.

Sign up for GitHub

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

Jump to bottom

feat: Migrate trie account type and state root functions from alloy #65

Merged
merged 8 commits into from
Dec 4, 2024
Merged

feat: Migrate trie account type and state root functions from alloy #65

merged 8 commits into from
Dec 4, 2024

Conversation

moricho
Copy link
Contributor

@moricho moricho commented Dec 3, 2024 •
edited
Loading

Migrated trie account type Account and state root functions from alloy-rs/alloy.
Originally I tried to export them form reth to alloy-rs/alloy, but it's better to do that here.

ref: alloy-rs/alloy#1717 (review)

it would be easier if we upstream this to alloy-trie entirely

the trieaccount type + state root functions

could you please open these changes as a pr to alloy-trie
then we import alloy-trie in alloy-genesis and to the from impl there

moricho
moricho commented Dec 3, 2024
View reviewed changes
src/serde/quantity.rs Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Account requires alloy-serde::quantity to serialize the nonce field, but alloy-trie can not depend on it, so I exported it partially here

@moricho moricho mentioned this pull request Dec 3, 2024
Closed
3 tasks
mattsse
mattsse requested changes Dec 3, 2024
View reviewed changes
Copy link
Member

@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.

some nits,

@DaniPopes imo we definitely want this in here, wdyt?

src/account.rs Outdated
#[cfg_attr(any(test, feature = "arbitrary"), derive(arbitrary::Arbitrary))]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))]
pub struct Account {
Copy link
Member

Choose a reason for hiding this comment

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

let's also name this TrieAccount here to avoid conflicts

src/serde/quantity.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

this is too much we only need this for u64, so no need to port the entire trait abstraction, we can just take:

https://github.com/alloy-rs/eips/blob/3e93f339daa4e51aed3767c76dd1c078532d8033/crates/eip7702/src/auth_list.rs#L350

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse Thanks for the review. Fixed at 4f6f859

@moricho moricho requested a review from mattsse December 3, 2024 11:49
mattsse
mattsse requested changes Dec 3, 2024
View reviewed changes
Cargo.toml Outdated
@@ -37,6 +37,7 @@ derive_more = { version = "1", default-features = false, features = [
"from",
"not",
] }
itertools = { version = "0.13", default-features = false, optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

we can actually remove this because we only use one function

    #[cfg(feature = "use_alloc")]
    fn sorted_unstable_by_key<K, F>(self, f: F) -> VecIntoIter<Self::Item>
    where
        Self: Sized,
        K: Ord,
        F: FnMut(&Self::Item) -> K,
    {
        let mut v = Vec::from_iter(self);
        v.sort_unstable_by_key(f);
        v.into_iter()
    }

and we can do that ourselves
then we also don't need to feature gate the state root functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this and handled it internally, while also cleaning up the feature gate: a29f17f

@moricho moricho requested a review from mattsse December 3, 2024 14:34
@moricho
Copy link
Contributor Author

moricho commented Dec 3, 2024

I wonder why alloy-primitives 0.8.14 is used in the test despite specifying 0.8.5 in Cargo.toml 🤔

https://github.com/alloy-rs/trie/actions/runs/12141835115/job/33856483337

error: rustc 1.79.0 is not supported by the following package:
  [email protected] requires rustc 1.81
Either upgrade rustc or select compatible dependency versions with
`cargo update <name>@<current-ver> --precise <compatible-ver>`
where `<compatible-ver>` is the latest version supporting rustc 1.79.0

rkrasiuk
rkrasiuk requested changes Dec 3, 2024
View reviewed changes
Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

please, feature gate any account related code behind ethereum. this crate is unaware of ethereum's state model, it's only MPT implementation

@mattsse
Copy link
Member

mattsse commented Dec 4, 2024 •
edited
Loading

I wonder why alloy-primitives 0.8.14 is used in the test despite specifying 0.8.5 in Cargo.tom

this has to do with the lockfile

fixed on main

@moricho
Copy link
Contributor Author

moricho commented Dec 4, 2024

this has to do with the lockfile

fixed on main

Thanks 🙏

@moricho moricho force-pushed the migrate-trie-accouts branch from 51e382e to d70e843 Compare December 4, 2024 14:26
@moricho
Copy link
Contributor Author

moricho commented Dec 4, 2024

@rkrasiuk Introduced ethereum feature gate: 7f97f9c

Currently, CI is passing except for miri

@moricho moricho requested a review from rkrasiuk December 4, 2024 14:32
mattsse
mattsse requested changes Dec 4, 2024
View reviewed changes
src/root.rs Outdated
Comment on lines 56 to 60
/// Hashes and sorts account keys, then proceeds to calculating the root hash of the state
/// represented as MPT.
/// See [`state_root_unsorted`] for more info.
#[cfg(feature = "ethereum")]
pub fn state_root_ref_unhashed<'a, A: Into<TrieAccount> + Clone + 'a>(
Copy link
Member

Choose a reason for hiding this comment

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

style nit: let's put all of these in a mod ethereum and feature gate this instead

DaniPopes
DaniPopes approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit 0bc031f into alloy-rs:main Dec 4, 2024
19 of 21 checks passed
@moricho moricho deleted the migrate-trie-accouts branch December 4, 2024 15:46
@moricho moricho mentioned this pull request Dec 4, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes DaniPopes approved these changes

@mattsse mattsse mattsse approved these changes

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moricho @mattsse @rkrasiuk @DaniPopes