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: move to alloy pt5 #2000

Merged
merged 39 commits into from
Feb 6, 2025
Merged

chore: move to alloy pt5 #2000

merged 39 commits into from
Feb 6, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

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

PR Type

Enhancement


Description

  • Migrate from ethers to alloy library

  • Update transaction handling and processing

  • Refactor block and transaction structures

  • Improve signature and chain ID handling


Changes walkthrough 📝

Relevant files
Dependencies
3 files
alias.rs
Remove ethers types, add alloy types                                         
+0/-2     
execution_result.rs
Switch from ethers to alloy for hex utility                           
+1/-1     
Cargo.toml
Update dependencies for alloy migration                                   
+2/-1     
Enhancement
13 files
importer_offline.rs
Update transaction processing for alloy                                   
+3/-10   
evm_input.rs
Refactor EvmInput for alloy transactions                                 
+9/-11   
executor.rs
Update executor for alloy transactions                                     
+7/-2     
importer.rs
Adjust transaction index handling                                               
+2/-2     
block.rs
Replace ethers with alloy block types                                       
+6/-6     
external_transaction.rs
Refactor ExternalTransaction for alloy                                     
+6/-20   
mod.rs
Add SignatureComponent to primitives                                         
+3/-0     
signature_component.rs
Implement new SignatureComponent struct                                   
+45/-0   
transaction_input.rs
Refactor TransactionInput for alloy compatibility               
+94/-44 
transaction_mined.rs
Update TransactionMined for alloy transactions                     
+11/-18 
transaction_stage.rs
Adapt TransactionStage for alloy transactions                       
+7/-7     
wei.rs
Add conversion from Wei to u128                                                   
+6/-0     
blockchain_client.rs
Update BlockchainClient for alloy transactions                     
+3/-3     

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 45f2f2d)

    Here are some key observations to aid the review process:

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

    Signature Handling

    The new implementation of signature handling in the try_from_alloy_transaction function might need careful review to ensure it correctly handles all transaction types and edge cases.

    // Get signature components from the envelope
    let signature = value.inner.signature();
    let r = U256::from(signature.r().to_be_bytes::<32>());
    let s = U256::from(signature.s().to_be_bytes::<32>());
    let v = if signature.v() { U64::from(1) } else { U64::from(0) };
    Transaction Type Handling

    The removal of the fill_missing_transaction_type method might affect how different transaction types are handled. Ensure that all transaction types are correctly processed in the new implementation.

    use anyhow::Context;
    use anyhow::Result;
    
    use crate::alias::AlloyTransaction;
    use crate::eth::primitives::BlockNumber;
    use crate::eth::primitives::Hash;
    #[derive(Debug, Clone, derive_more::Deref, serde::Deserialize, serde::Serialize)]
    #[serde(transparent)]
    pub struct ExternalTransaction(#[deref] pub AlloyTransaction);
    
    impl ExternalTransaction {
        /// Returns the block number where the transaction was mined.
        pub fn block_number(&self) -> Result<BlockNumber> {
            Ok(self.0.block_number.context("ExternalTransaction has no block_number")?.into())
        }
    
        /// Returns the transaction hash.
        pub fn hash(&self) -> Hash {
            Hash::from(*self.0.inner.tx_hash())
        }
    }
    
    // -----------------------------------------------------------------------------
    // Conversions: Other -> Self
    // -----------------------------------------------------------------------------
    impl From<AlloyTransaction> for ExternalTransaction {
        fn from(value: AlloyTransaction) -> Self {
            ExternalTransaction(value)
        }
    }
    External Transaction Execution

    The changes in the execute_external_transaction method, particularly the removal of fill_missing_transaction_type, might affect how external transactions are executed. Verify that all transaction types are correctly handled.

        block_number: BlockNumber,
        block_timestamp: UnixTime,
        #[cfg(feature = "metrics")] block_metrics: &mut EvmExecutionMetrics,
    ) -> anyhow::Result<()> {
        // track
        #[cfg(feature = "metrics")]
        let (start, tx_function, tx_contract) = (
            metrics::now(),
            codegen::function_sig(tx.inner.input()),
            codegen::contract_name(&tx.0.to().map_into()),
        );
    
        #[cfg(feature = "tracing")]
        let _span = info_span!("executor::external_transaction", tx_hash = %tx.hash()).entered();
        tracing::info!(%block_number, tx_hash = %tx.hash(), "reexecuting external transaction");
    
        // when transaction externally failed, create fake transaction instead of reexecuting
        let tx_execution = match receipt.is_success() {

    Copy link

    github-actions bot commented Feb 4, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 45f2f2d
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Improve error handling for signatures

    Implement error handling for the signature recovery process in the
    try_from_alloy_transaction function. Currently, if the signature recovery fails, it
    returns an error without providing specific details about the failure.

    src/eth/primitives/transaction_input.rs [137-143]

     true => match value.inner.recover_signer() {
         Ok(signer) => Address::from(signer),
         Err(e) => {
             tracing::warn!(reason = ?e, "failed to recover transaction signer");
    -        return Err(anyhow!("Transaction signer cannot be recovered. Check the transaction signature is valid."));
    +        return Err(anyhow!("Transaction signer cannot be recovered: {}", e));
         }
     },
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves error handling by providing more specific information about the signature recovery failure, which can aid in debugging and troubleshooting.

    Medium
    Handle missing chain_id in conversion

    In the From for AlloyTransaction implementation, consider handling the case where
    value.chain_id is None. Currently, it's directly passed to TxLegacy, which might
    lead to unexpected behavior if chain_id is not provided.

    src/eth/primitives/transaction_input.rs [181-192]

     TxLegacy {
    -    chain_id: value.chain_id.map(Into::into),
    +    chain_id: value.chain_id.map(Into::into).or_else(|| Some(0.into())), // Default to 0 if None
         nonce: value.nonce.into(),
         gas_price: value.gas_price.into(),
         gas_limit: value.gas_limit.into(),
         to: match value.to {
             Some(addr) => TxKind::Call(addr.into()),
             None => TxKind::Create,
         },
         value: value.value.into(),
         input: value.input.clone().into(),
     },
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion addresses a potential issue with missing chain_id values, providing a default value to prevent unexpected behavior. This improves the robustness of the code.

    Low

    Previous suggestions

    Suggestions up to commit ef4109b
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Implement proper error handling

    Remove the TODO comment and implement proper error handling for the From
    implementation. This ensures that all necessary fields are correctly converted and
    prevents potential runtime errors.

    src/eth/primitives/transaction_input.rs [140-164]

    -// TODO: improve before merging
    -impl From<TransactionInput> for alloy_rpc_types_eth::Transaction {
    -    fn from(value: TransactionInput) -> Self {
    -        // Create legacy transaction envelope
    +impl TryFrom<TransactionInput> for alloy_rpc_types_eth::Transaction {
    +    type Error = anyhow::Error;
    +
    +    fn try_from(value: TransactionInput) -> Result<Self, Self::Error> {
             let inner = TxEnvelope::Legacy(Signed::new_unchecked(
                 TxLegacy {
    -                chain_id: value.chain_id.map(Into::into),
    -                nonce: value.nonce.into(),
    -                gas_price: value.gas_price.into(),
    -                gas_limit: value.gas_limit.into(),
    -                to: Into::<TxKind>::into(value.to.map(Into::<alloy_primitives::Address>::into).unwrap_or_default()),
    -                value: value.value.into(),
    -                input: value.input.clone().into(),
    +                chain_id: value.chain_id.map(TryInto::try_into).transpose()?,
    +                nonce: value.nonce.try_into()?,
    +                gas_price: value.gas_price.try_into()?,
    +                gas_limit: value.gas_limit.try_into()?,
    +                to: TxKind::try_from(value.to.map(TryInto::try_into).transpose()?
    +                    .unwrap_or(alloy_primitives::Address::default()))?,
    +                value: value.value.try_into()?,
    +                input: value.input.try_into()?,
                 },
    -            alloy_primitives::PrimitiveSignature::new(SignatureComponent(value.r).into(), SignatureComponent(value.s).into(), value.v.as_u64() == 1),
    +            alloy_primitives::PrimitiveSignature::new(
    +                SignatureComponent(value.r).try_into()?,
    +                SignatureComponent(value.s).try_into()?,
    +                value.v.as_u64() == 1
    +            ),
                 Default::default(),
             ));
     
    -        Self {
    +        Ok(Self {
                 inner,
                 block_hash: None,
                 block_number: None,
                 transaction_index: None,
    -            from: value.signer.into(),
    -            effective_gas_price: Some(value.gas_price.into()),
    -        }
    +            from: value.signer.try_into()?,
    +            effective_gas_price: Some(value.gas_price.try_into()?),
    +        })
         }
     }
    Suggestion importance[1-10]: 9

    Why: This suggestion significantly improves error handling by implementing TryFrom instead of From, and adding proper error checks for all conversions. This is crucial for preventing runtime errors and ensuring robust code behavior.

    9
    Implement proper conversion validations

    Remove the TODO comment and implement proper validation for the conversions in the
    from_external method. This ensures data integrity and prevents potential runtime
    errors.

    src/eth/executor/evm_input.rs [141-155]

    -// TODO: improve before merging(validate conversions)
     pub fn from_external(tx: &ExternalTransaction, receipt: &ExternalReceipt, block_number: BlockNumber, block_timestamp: UnixTime) -> anyhow::Result<Self> {
         Ok(Self {
    -        from: tx.from.into(),
    -        to: tx.inner.to().map_into(),
    -        value: tx.inner.value().into(),
    -        data: tx.inner.input().clone().into(),
    -        nonce: Some(tx.inner.nonce().try_into()?),
    -        gas_limit: if_else!(receipt.is_success(), Gas::MAX, tx.inner.gas_limit().try_into()?),
    -        gas_price: if_else!(receipt.is_success(), Wei::ZERO, tx.inner.gas_price().map_into().unwrap_or(Wei::ZERO)),
    +        from: tx.from.try_into().context("Invalid 'from' address")?,
    +        to: tx.inner.to().map(TryInto::try_into).transpose().context("Invalid 'to' address")?,
    +        value: tx.inner.value().try_into().context("Invalid value")?,
    +        data: tx.inner.input().clone().try_into().context("Invalid input data")?,
    +        nonce: Some(tx.inner.nonce().try_into().context("Invalid nonce")?),
    +        gas_limit: if receipt.is_success() {
    +            Gas::MAX
    +        } else {
    +            tx.inner.gas_limit().try_into().context("Invalid gas limit")?
    +        },
    +        gas_price: if receipt.is_success() {
    +            Wei::ZERO
    +        } else {
    +            tx.inner.gas_price().map(TryInto::try_into).transpose().context("Invalid gas price")?.unwrap_or(Wei::ZERO)
    +        },
             point_in_time: PointInTime::Pending,
             block_number,
             block_timestamp,
    -        chain_id: tx.inner.chain_id().map(|id| id.try_into()).transpose()?,
    +        chain_id: tx.inner.chain_id().map(TryInto::try_into).transpose().context("Invalid chain ID")?,
         })
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical issue by replacing a TODO comment with proper validation for conversions. This enhances data integrity and prevents potential runtime errors, significantly improving the code's robustness and reliability.

    8
    Implement proper input data validation

    Remove the TODO comment and implement proper validation for the tx.inner.input() and
    tx.0.to() conversions. This ensures data integrity and prevents potential runtime
    errors.

    src/eth/executor/executor.rs [361-367]

    -// TODO: improve before merging(validate)
     #[cfg(feature = "metrics")]
     let (start, tx_function, tx_contract) = (
         metrics::now(),
    -    codegen::function_sig(&tx.inner.input()),
    -    codegen::contract_name(&tx.0.to().map_into()),
    +    codegen::function_sig(&tx.inner.input().try_into().context("Invalid input data")?),
    +    codegen::contract_name(&tx.inner.to().map(TryInto::try_into).transpose().context("Invalid 'to' address")?),
     );
    Suggestion importance[1-10]: 7

    Why: This suggestion improves data validation for input and address conversions, reducing the risk of runtime errors. It's important for ensuring data integrity but slightly less critical than the first suggestion.

    7
    Implement proper hash conversion

    Remove the TODO comment and implement proper hash conversion for the hash method.
    This ensures correct hash representation and prevents potential errors.

    src/eth/primitives/external_transaction.rs [27-29]

     pub fn hash(&self) -> Hash {
    -    let tx_hash: B256 = *self.0.inner.tx_hash(); // TODO: improve before merging
    +    let tx_hash: B256 = self.0.inner.tx_hash();
         Hash::from(tx_hash)
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion removes a TODO comment and implements proper hash conversion. While important for correct hash representation, it's less critical than the previous suggestions as it doesn't involve complex validations.

    6

    @gabriel-aranha-cw gabriel-aranha-cw changed the title Chore move to alloy pt5 chore: move to alloy pt5 Feb 4, 2025
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /describe

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /benchmark

    Copy link

    github-actions bot commented Feb 5, 2025

    PR Description updated to latest commit (0091b8c)

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Benchmark:
    Run ID: bench-2837107361

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1920.00, Min: 1042.00, Avg: 1745.09, StdDev: 75.55
    TPS Stats: Max: 1916.00, Min: 1564.00, Avg: 1694.33, StdDev: 83.62

    Plot: View Plot

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /describe

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /flamegraph

    Copy link

    github-actions bot commented Feb 5, 2025

    PR Description updated to latest commit (e9a6a3b)

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Flamegraphs:
    Run ID: bench-2624057345

    Git Info:

    Flamegraphs:
    idle:

    write:

    read:

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

    github-actions bot commented Feb 5, 2025

    Persistent review updated to latest commit 45f2f2d

    @gabriel-aranha-cw gabriel-aranha-cw merged commit 9de7f30 into main Feb 6, 2025
    39 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the chore-move-to-alloy-pt5 branch February 6, 2025 12:36
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-2050046302

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1941.00, Min: 1084.00, Avg: 1763.29, StdDev: 77.15
    TPS Stats: Max: 1968.00, Min: 1539.00, Avg: 1712.28, StdDev: 86.56

    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