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

chore: remove ethers rng utils #2002

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Feb 4, 2025

PR Type

Enhancement


Description

  • Replace ethers_core::rand with rand_core::RngCore

  • Update Dummy trait implementations for primitives

  • Remove dependency on ethers-core for RNG

  • Add rand_core dependency in Cargo.toml


Changes walkthrough 📝

Relevant files
Enhancement
18 files
address.rs
Update Dummy trait implementation for Address                       
+1/-1     
block_header.rs
Modify Dummy trait implementation for BlockHeader               
+1/-1     
block_number.rs
Update Dummy trait implementation for BlockNumber               
+1/-1     
chain_id.rs
Modify Dummy trait implementation for ChainId                       
+1/-1     
code_hash.rs
Update Dummy trait implementation for CodeHash                     
+1/-1     
difficulty.rs
Modify Dummy trait implementation for Difficulty                 
+1/-1     
execution_result.rs
Update Dummy trait implementation for RevertReason             
+1/-1     
gas.rs
Modify Dummy trait implementation for Gas                               
+1/-1     
hash.rs
Update Dummy trait implementation for Hash                             
+1/-1     
log_topic.rs
Modify Dummy trait implementation for LogTopic                     
+1/-1     
miner_nonce.rs
Update Dummy trait implementation for MinerNonce                 
+1/-1     
nonce.rs
Modify Dummy trait implementation for Nonce                           
+1/-1     
size.rs
Update Dummy trait implementation for Size                             
+1/-1     
slot_index.rs
Modify Dummy trait implementation for SlotIndex                   
+1/-1     
slot_value.rs
Update Dummy trait implementation for SlotValue                   
+1/-1     
transaction_input.rs
Modify Dummy trait implementation for TransactionInput     
+1/-1     
unix_time.rs
Update Dummy trait implementation for UnixTime                     
+1/-1     
wei.rs
Modify Dummy trait implementation for Wei                               
+1/-1     
Dependencies
1 files
Cargo.toml
Add rand_core dependency                                                                 
+1/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    github-actions bot commented Feb 4, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit e7ae2fc)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Dependency Change

    The change from ethers_core::rand to rand_core::RngCore might affect the randomness generation. Ensure that the new implementation provides the same level of randomness and security.

    fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(_: &Faker, rng: &mut R) -> Self {
        H160::random_using(rng).into()

    Copy link

    github-actions bot commented Feb 4, 2025

    PR Code Suggestions ✨

    Latest suggestions up to e7ae2fc
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use RngCore-compatible random generation

    Replace rng.next_u64() with a method that uses rand_core::RngCore trait, such as
    rng.gen() or a custom implementation using fill_bytes().

    src/eth/primitives/block_number.rs [74-76]

     impl Dummy<Faker> for BlockNumber {
         fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -        rng.next_u64().into()
    +        let mut bytes = [0u8; 8];
    +        rng.fill_bytes(&mut bytes);
    +        u64::from_le_bytes(bytes).into()
         }
     }
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies the need to update the random number generation method to be compatible with the new rand_core::RngCore trait. The proposed solution using fill_bytes() is more appropriate and ensures correct functionality with the new RNG implementation.

    8
    Ensure RNG compatibility

    Ensure that the H160::random_using method is compatible with the rand_core::RngCore
    trait. If not, consider implementing a custom random generation method for Address
    that uses rand_core::RngCore.

    src/eth/primitives/address.rs [60-62]

     impl Dummy<Faker> for Address {
         fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -        H160::random_using(rng).into()
    +        let mut bytes = [0u8; 20];
    +        rng.fill_bytes(&mut bytes);
    +        H160::from(bytes).into()
         }
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential compatibility issue between H160::random_using and the new rand_core::RngCore trait. The proposed solution ensures proper random generation using the new trait, which is important for maintaining correct functionality.

    7

    Previous suggestions

    Suggestions up to commit e7ae2fc
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure RNG compatibility

    Ensure that the H160::random_using() method is compatible with the new
    rand_core::RngCore trait. If not, consider implementing a custom random generation
    method using the new RNG trait.

    src/eth/primitives/address.rs [60-62]

     fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -    H160::random_using(rng).into()
    +    let mut bytes = [0u8; 20];
    +    rng.fill_bytes(&mut bytes);
    +    H160::from(bytes).into()
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential compatibility issue between H160::random_using() and the new rand_core::RngCore trait. The proposed solution ensures proper random generation using the new RNG trait, which is important for maintaining correct functionality.

    7
    General
    Align random generation with type

    Replace rng.next_u64() with rng.next_u32() to maintain consistency with the as_u32()
    method of BlockNumber.

    src/eth/primitives/block_number.rs [74-76]

     fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -    rng.next_u64().into()
    +    rng.next_u32().into()
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion improves consistency by using rng.next_u32() instead of rng.next_u64(), aligning with the as_u32() method of BlockNumber. This change enhances type consistency and potentially prevents overflow issues.

    6

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review February 4, 2025 20:16
    Copy link

    github-actions bot commented Feb 4, 2025

    Persistent review updated to latest commit e7ae2fc

    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) February 4, 2025 20:20
    @gabriel-aranha-cw gabriel-aranha-cw merged commit 32de341 into main Feb 4, 2025
    40 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the chore-remove-ethers-rng branch February 4, 2025 20:24
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-2405409770

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1957.00, Min: 1359.00, Avg: 1811.82, StdDev: 60.62
    TPS Stats: Max: 1958.00, Min: 1642.00, Avg: 1758.40, StdDev: 76.28

    Plot: View Plot

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    #1963

    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.

    2 participants