Skip to content

Conversation

@LuisUrrutia
Copy link
Contributor

@LuisUrrutia LuisUrrutia commented Aug 21, 2025

Summary

This PR updates the Alloy Ethereum library from version 0.9 to 1.0.24, bringing the relayer up to date with the latest features, improvements, and security fixes in the Alloy ecosystem.

  • Updated provider initialization to use AnyNetwork for better network abstraction
  • Added proper type aliases for EvmProviderType with new provider filler structure
  • Replaced PrimitiveSignature with Signature for signature creation
  • Modified test fixtures to use new receipt structure with AnyReceiptEnvelope

Breaking Changes Addressed

Provider API Changes: Updated to new provider builder pattern with network specification
Signature Types: Migrated from PrimitiveSignature to Signature
Receipt Structure: Adapted to new nested receipt structure with inner field access

Summary by CodeRabbit

  • New Features

    • Dynamic EVM provider with broader network compatibility.
    • Raw JSON-RPC request method for advanced use cases.
    • Standardized block and transaction receipt response types.
  • Bug Fixes

    • More accurate detection of transaction failure status.
  • Refactor

    • Migrated provider and RPC types to newer abstractions; simplified block retrieval.
    • Updated signing paths to use a unified signature type.
  • Tests

    • Updated test scaffolding to align with new block and receipt representations.
  • Chores

    • Upgraded core EVM library to latest compatible version.

@LuisUrrutia LuisUrrutia requested review from a team as code owners August 21, 2025 13:37
@coderabbitai
Copy link

coderabbitai bot commented Aug 21, 2025

Walkthrough

Upgrade alloy to v1.0.24 and adapt code/tests to its API: introduce nested receipt types and aliases, add BlockResponse/TransactionReceipt type aliases, rewire EVM provider to use AnyNetwork and a composite EvmProviderType with new trait methods, and replace PrimitiveSignature with Signature in KMS signers.

Changes

Cohort / File(s) Change Summary
Dependency Update
Cargo.toml
Upgrade alloy from 0.91.0.24 (features = ["full"]).
Receipt handling & tests
src/domain/transaction/evm/status.rs
Import ReceiptResponse/AnyReceiptEnvelope; read status via receipt.inner.status(); test helpers rebuilt to construct nested TransactionReceipt with AnyReceiptEnvelope.
RPC model aliases
src/models/rpc/evm/mod.rs
Added imports AnyRpcBlock, AnyTransactionReceipt and aliases pub type BlockResponse = AnyRpcBlock; and pub type TransactionReceipt = AnyTransactionReceipt;.
Provider surface & wiring
src/services/provider/evm/mod.rs
New composite EvmProviderType (Identity + Fillers + RootProvider<AnyNetwork>); updated init/retry signatures to use it; added raw_request_dyn; get_block_by_numberBlockResponse; get_transaction_receiptOption<TransactionReceipt>; RPC callsites adapted to AnyNetwork and Into conversions.
Gas service tests
src/services/gas/evm_gas_price.rs
Tests updated to construct Block and convert to AnyRpcBlock (AnyRpcBlock::from) and adjust imports; production logic unchanged.
Signer changes (KMS)
src/services/signer/evm/aws_kms_signer.rs, src/services/signer/evm/google_cloud_kms_signer.rs
Replace PrimitiveSignature::from_raw usages with Signature::from_raw and update imports to Signature from alloy::primitives; signing flows otherwise unchanged.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Provider as EvmProviderType
    participant Network as AnyNetwork

    rect #f8f9fa
      Client->>Provider: initialize_provider(config, client)
      Provider->>Network: connect RootProvider<AnyNetwork>
      Provider-->>Client: EvmProviderType
    end

    rect #eef8f3
      Client->>Provider: get_block_by_number()
      Provider->>Network: raw_request_dyn / provider call
      Network-->>Provider: AnyRpcBlock
      Provider-->>Client: BlockResponse (AnyRpcBlock)
    end

    rect #fff4f0
      Client->>Provider: get_transaction_receipt(tx_hash)
      Provider->>Network: raw_request_dyn / provider call
      Network-->>Provider: AnyTransactionReceipt
      Provider-->>Client: TransactionReceipt (AnyTransactionReceipt)
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • zeljkoX

Poem

I nibble code beneath moonlight's sheen,
Alloy updated — receipts nested clean.
Providers hop to AnyNetwork's glade,
Signatures swap, no logic unmade.
A rabbit cheers the tidy cascade 🐇

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 83ec534 and 9a59fe6.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • Cargo.toml (1 hunks)
  • src/services/signer/evm/google_cloud_kms_signer.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/services/signer/evm/google_cloud_kms_signer.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: test
  • GitHub Check: msrv
  • GitHub Check: clippy
  • GitHub Check: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer
  • GitHub Check: Analyze (rust)
🔇 Additional comments (2)
Cargo.toml (2)

30-30: Verify alloy “full” feature necessity and lockfile sync. Manually inspect active features (cargo tree -p alloy -e features) and update the lockfile (cargo update -p alloy --precise 1.0.24). If not all submodules are required, narrow the feature list instead of using "full" in a follow-up.


5-5: Validate Alloy v1.0.24 MSRV
rust-version = "1.86" aligns with rust-toolchain.toml’s channel = "1.86.0"; confirm the Alloy v1.0.24 dependency supports MSRV 1.86 to avoid introducing toolchain constraints.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch plat-6404-update-alloy-package-version-to-latest-one

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbit in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbit in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbit gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbit read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbit help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbit ignore or @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbit summary or @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbit or @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/services/provider/evm/mod.rs (1)

484-539: Fix contract creation handling and propagate gas limit in TransactionRequest conversion

The current implementation always constructs a TxKind::Call with an empty string when tx.to is None, causing contract-creation transactions to fail parsing, and silently drops the gas limit. Based on Alloy 1.0.24’s API:

  • Contract creation should be represented as Some(TxKind::Create) for to: None, not TxKind::Call("") (reth.rs, docs.rs).
  • The gas-limit field on TransactionRequest is named gas: Option<u64>, so we should map tx.gas_limit into that field (docs.rs).

Apply this diff:

@@ impl TryFrom<&EvmTransactionData> for TransactionRequest {
-           to: Some(TxKind::Call(
-               tx.to
-                   .clone()
-                   .unwrap_or("".to_string())
-                   .parse()
-                   .map_err(|_| {
-                       TransactionError::InvalidType("Invalid address format".to_string())
-                   })?,
-           )),
+           to: match &tx.to {
+               Some(addr) => Some(TxKind::Call(
+                   addr.parse().map_err(|_| {
+                       TransactionError::InvalidType("Invalid address format".to_string())
+                   })?,
+               )),
+               None => Some(TxKind::Create),
+           },

@@
            input: TransactionInput::from(tx.data_to_bytes()?),
+           gas: tx.gas_limit.map(|gl| gl.into()),

            nonce: tx
                .nonce
                .map(|n| {
                    Uint::<256, 4>::from(n)
                        .try_into()
                        .map_err(|_| TransactionError::InvalidType("Invalid nonce".to_string()))
                })
                .transpose()?,
@@
            ..Default::default()
        })
  • When tx.to is None, use Some(TxKind::Create) for contract creation.
  • Propagate gas_limit (from EvmTransactionData) into the gas: Option<u64> field on TransactionRequest.
src/domain/transaction/evm/status.rs (1)

60-72: Ensure correct field access on TransactionReceipt wrapper

The alias TransactionReceipt in src/models/rpc/evm/mod.rs is defined as

pub type TransactionReceipt = AnyTransactionReceipt;

where AnyTransactionReceipt is a wrapper struct with two fields—inner (the actual TransactionReceipt<…> carrying block_number) and other. There is no direct block_number on the alias, so the code must drill into inner to access it.

Please update in src/domain/transaction/evm/status.rs (around lines 60–72):

• Replace

- let tx_block_number = receipt
-     .block_number
+ let tx_block_number = receipt
+     .inner
+     .block_number
     .ok_or(TransactionError::UnexpectedError(
         "Transaction receipt missing block number".to_string(),
     ))?;

This ensures you’re reading the block_number from the inner receipt struct of the wrapper.

🧹 Nitpick comments (12)
Cargo.toml (1)

30-30: Alloy 1.0.24 upgrade: verify MSRV and consider trimming features.

  • Good move aligning the codebase to alloy 1.0.x. Please double-check that alloy 1.0.24 supports the declared MSRV (rust-version = "1.86") in this crate, and that CI runs with that toolchain.
  • Using features = ["full"] can significantly increase compile times and dependency surface. If feasible, consider narrowing to only the used crates/features (e.g., primitives, rpc-types, consensus, providers, network). This will reduce build time and shrink the binary.
src/services/signer/evm/aws_kms_signer.rs (2)

116-121: Consider a fallback normalization for legacy branch only if needed.

Legacy transactions typically preserve v as 27/28 in external representations. If Signature::from_raw in alloy 1.0.x rejects 27/28, a guarded fallback keeps this branch robust while preserving 27/28 in the returned signature bytes.

-            let signature = Signature::from_raw(&signed_bytes)
-                .map_err(|e| SignerError::ConversionError(e.to_string()))?;
+            let signature = Signature::from_raw(&signed_bytes).or_else(|_| {
+                // Try with normalized v (0/1) if 27/28 was rejected
+                let mut b = signed_bytes.clone();
+                if b[64] == 27 {
+                    b[64] = 0;
+                } else if b[64] == 28 {
+                    b[64] = 1;
+                }
+                Signature::from_raw(&b)
+            }).map_err(|e| SignerError::ConversionError(e.to_string()))?;

Note: Keep the output bytes for legacy as-is (27/28) to match downstream expectations and existing tests.


316-320: Tests assert EIP-1559 v normalization.

The check for v in {0,1} for typed transactions is aligned with the change. Consider adding a small unit test for the legacy path verifying v in {27,28} to lock this behavior.

Do you want me to draft that test?

src/services/signer/evm/google_cloud_kms_signer.rs (1)

103-110: Optional: guarded fallback for legacy v if alloy rejects 27/28.

Mirror the AWS suggestion so both signers behave consistently under alloy 1.0.x.

-            let signature = Signature::from_raw(&signed_bytes)
-                .map_err(|e| SignerError::ConversionError(e.to_string()))?;
+            let signature = Signature::from_raw(&signed_bytes).or_else(|_| {
+                let mut b = signed_bytes.clone();
+                if b[64] == 27 { b[64] = 0; }
+                else if b[64] == 28 { b[64] = 1; }
+                Signature::from_raw(&b)
+            }).map_err(|e| SignerError::ConversionError(e.to_string()))?;
src/services/gas/evm_gas_price.rs (2)

443-446: Test: construct Block then convert to AnyRpcBlock — LGTM.

The pattern let mut block: Block = Block::default(); block.header.base_fee_per_gas = Some(...); Ok(AnyRpcBlock::from(block)) is clear and mirrors the production usage.

Consider extracting a small helper in the test module to avoid repeating this stub in multiple tests.


472-475: Repeat stub — consider factoring to a helper.

Same suggestion as above; reduces duplication and improves readability across tests.

Example helper outside the selected lines:

fn mock_block_with_base_fee(base_fee: u128) -> AnyRpcBlock {
    let mut block: Block = Block::default();
    block.header.base_fee_per_gas = Some(base_fee as u64);
    AnyRpcBlock::from(block)
}
src/models/rpc/evm/mod.rs (1)

25-26: Public aliases for BlockResponse and TransactionReceipt — good façade.

These type aliases provide a stable public surface while allowing internal library upgrades. Consider adding short doc comments to clarify they map to alloy “Any*” types and may evolve with dependency updates.

-pub type BlockResponse = AnyRpcBlock;
-pub type TransactionReceipt = AnyTransactionReceipt;
+/// Opaque EVM block response type (currently alloy::network::AnyRpcBlock).
+pub type BlockResponse = AnyRpcBlock;
+/// Opaque EVM transaction receipt type (currently alloy::network::AnyTransactionReceipt).
+pub type TransactionReceipt = AnyTransactionReceipt;
src/services/provider/evm/mod.rs (4)

23-30: Consider reordering fillers to minimize RPC round-trips and align with Alloy’s typical fill sequence.

Current order is Identity → Gas → BlobGas → Nonce → ChainId. In practice, resolving ChainId early can help subsequent fillers (Nonce/Gas) avoid redundant calls. A commonly used sequence is: ChainId → Nonce → Gas → BlobGas (wrapped with Identity).

This is optional and code will work as-is, but reordering can reduce latency on some providers.


221-243: Minor: avoid cloning the client when connecting the provider.

ClientBuilder::connect_client takes ownership; the local client isn’t reused. The .clone() is unnecessary.

Apply this diff:

-        let provider = ProviderBuilder::new()
-            .network::<AnyNetwork>()
-            .connect_client(client.clone());
+        let provider = ProviderBuilder::new()
+            .network::<AnyNetwork>()
+            .connect_client(client);

429-432: Use a more accurate error variant for TX hash parse failures.

TX hash parsing failures are being surfaced as InvalidAddress, which is misleading.

Apply this diff to avoid misclassification:

-        let parsed_tx_hash = tx_hash
-            .parse::<alloy::primitives::TxHash>()
-            .map_err(|e| ProviderError::InvalidAddress(e.to_string()))?;
+        let parsed_tx_hash = tx_hash
+            .parse::<alloy::primitives::TxHash>()
+            .map_err(|e| ProviderError::Other(format!("Invalid transaction hash: {}", e)))?;

461-472: Micro: avoid string allocation in raw_request_dyn.

You can pass the method as a borrowed Cow without cloning.

Apply this diff:

-            let method_clone = method.to_string();
-            let params_clone = params.clone();
+            let params_clone = params.clone();
@@
-                let result = provider
-                    .raw_request_dyn(std::borrow::Cow::Owned(method_clone), &params_raw)
+                let result = provider
+                    .raw_request_dyn(std::borrow::Cow::Borrowed(method), &params_raw)
                     .await
                     .map_err(ProviderError::from)?;

Also applies to: 474-477

src/domain/transaction/evm/status.rs (1)

593-620: Tests: avoid magic number for receipt r#type.

Using 0 for legacy type works but is brittle. Prefer the enum/constant provided by Alloy for readability and future-proofing (e.g., TxType::Legacy as u8 if available).

Optional, but recommended for clarity.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 818258a and 45d7685.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml (1 hunks)
  • src/domain/transaction/evm/status.rs (5 hunks)
  • src/models/rpc/evm/mod.rs (2 hunks)
  • src/services/gas/evm_gas_price.rs (3 hunks)
  • src/services/provider/evm/mod.rs (8 hunks)
  • src/services/signer/evm/aws_kms_signer.rs (3 hunks)
  • src/services/signer/evm/google_cloud_kms_signer.rs (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/provider/evm/mod.rs (2)
src/services/provider/retry.rs (2)
  • retry_rpc_call (192-340)
  • from (496-498)
src/services/provider/mod.rs (9)
  • from (45-47)
  • from (51-53)
  • from (57-59)
  • from (100-102)
  • from (106-108)
  • from (112-120)
  • from (125-127)
  • from (135-154)
  • from (159-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: msrv
  • GitHub Check: test
  • GitHub Check: clippy
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer
  • GitHub Check: Analyze (rust)
  • GitHub Check: semgrep/ci
🔇 Additional comments (11)
src/services/signer/evm/aws_kms_signer.rs (2)

3-3: PrimitiveSignature → Signature change looks correct.

Import update matches alloy 1.0.x. No issues spotted here.


65-88: Normalize v before constructing Signature for EIP-1559

Some clients (e.g. Alloy 1.0.x) expect the recovery ID (v) to be 0 or 1 when parsing raw signatures for typed transactions. AWS KMS, however, returns v as 27 or 28. If you call Signature::from_raw(&signed_bytes) with a 27/28 recovery ID, it may fail, forcing you to mutate the bytes post-hoc. Normalizing v to 0/1 before invoking Signature::from_raw ensures the conversion succeeds and lets you drop the later adjustment.

  • File: src/services/signer/evm/aws_kms_signer.rs
  • Lines: 65–88

Apply this diff:

             // Ensure we have the right signature length
             if signed_bytes.len() != 65 {
                 return Err(SignerError::SigningError(format!(
                     "Invalid signature length from AWS KMS: expected 65 bytes, got {}",
                     signed_bytes.len()
                 )));
             }

-            // Construct primitive signature
-            let signature = Signature::from_raw(&signed_bytes)
-                .map_err(|e| SignerError::ConversionError(e.to_string()))?;
-
-            // Extract signature array bytes
-            let mut signature_bytes = signature.as_bytes();
+            // Normalize v for typed tx (expect 0/1) before constructing Signature
+            let mut sig_bytes = signed_bytes.clone();
+            match sig_bytes[64] {
+                27 => sig_bytes[64] = 0,
+                28 => sig_bytes[64] = 1,
+                0 | 1 => {}
+                other => {
+                    return Err(SignerError::SigningError(format!(
+                        "Invalid v value from AWS KMS for EIP-1559: {} (expected 0/1/27/28)",
+                        other
+                    )));
+                }
+            }
+            let signature = Signature::from_raw(&sig_bytes)
+                .map_err(|e| SignerError::ConversionError(e.to_string()))?;
+
+            // Extract signature bytes (already normalized to 0/1)
+            let signature_bytes = signature.as_bytes();

             // Construct a signed transaction
             let signed_tx = unsigned_tx.into_signed(signature);

-            // Adjust v value for EIP-1559 (27/28 -> 0/1)
-            if signature_bytes[64] == 27 {
-                signature_bytes[64] = 0;
-            } else if signature_bytes[64] == 28 {
-                signature_bytes[64] = 1;
-            }
+            // No-op: v already normalized to 0/1 for EIP-1559.
src/services/signer/evm/google_cloud_kms_signer.rs (2)

11-12: PrimitiveSignature → Signature migration acknowledged.

Import update aligns with alloy 1.0.x.


66-77: Ignore the pre‐normalization refactor as proposed
The Signature::from_raw(&bytes) constructor expects the final byte in “Electrum” notation (27 or 28). Normalizing to 0/1 before calling from_raw will cause a ConversionError. If you need to normalize the parity bit earlier, use one of these APIs instead:
Signature::from_bytes_and_parity(&bytes[..64], parity_bool)
Signature::from_raw(&bytes) followed by .with_parity(parity_bool)

As both AWS and GCP signers already correctly adjust v post‐hoc, you can leave the existing mutation in place or switch to one of the above methods—but do not feed 0/1 into from_raw.

Likely an incorrect or invalid review comment.

src/services/gas/evm_gas_price.rs (1)

313-316: Imports updated for AnyRpcBlock and Block/FeeHistory types — looks good.

This aligns test scaffolding with the new alloy RPC types.

src/models/rpc/evm/mod.rs (1)

1-1: New imports for AnyRpcBlock/AnyTransactionReceipt — OK.

Prepares the aliases below without leaking alloy types to all call-sites.

src/services/provider/evm/mod.rs (2)

313-327: Sanity check: Into conversions for estimate_gas/send_transaction/call look correct with Alloy 1.0+.

The switch to Into at call sites matches the updated provider API. No issues spotted here.

Also applies to: 336-351, 442-453


254-278: No changes required: ProviderError already implements From<String>

The repository contains the following implementation, satisfying the requirement for retry_rpc_call:

  • src/services/provider/mod.rs:124–126
    // Add conversion from String to ProviderError
    impl From<String> for ProviderError {
        fn from(error: String) -> Self {
            ProviderError::Other(error)
        }
    }
src/domain/transaction/evm/status.rs (3)

5-5: Correct: import ReceiptResponse to bring status() into scope.

This is required for the trait-provided status() method on the nested receipt. Good catch.


63-66: Correct access to nested receipt status.

receipt.inner.status() aligns with the new AnyReceiptEnvelope-based structure introduced by Alloy 1.0+. This change prevents false positives when checking success.


741-763: Overall test harness updates are coherent with Alloy 1.0+ receipt model.

The switch to AnyReceiptEnvelope, ReceiptWithBloom, and Eip658Value is consistent and the status checks/confirmations logic remains intact. No issues here.

Also applies to: 765-895, 1035-1110, 1151-1173, 1199-1221, 1268-1375

@codecov
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 74.50980% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.0%. Comparing base (efede36) to head (9a59fe6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/services/provider/evm/mod.rs 31.2% 11 Missing ⚠️
src/services/signer/evm/google_cloud_kms_signer.rs 0.0% 2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (74.5%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #430     +/-   ##
=======================================
- Coverage   93.0%   93.0%   -0.1%     
=======================================
  Files        217     217             
  Lines      74164   74160      -4     
=======================================
- Hits       68995   68986      -9     
- Misses      5169    5174      +5     
Flag Coverage Δ
integration 0.5% <0.0%> (-0.1%) ⬇️
properties <0.1% <0.0%> (ø)
unittests 93.0% <74.5%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@zeljkoX zeljkoX left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/services/provider/evm/mod.rs (3)

221-243: initialize_provider returns EvmProviderType but currently builds a RootProvider; wire fillers to guarantee the concrete type.

As noted above, initialize_provider() advertises EvmProviderType but does not compose fillers. Without installing fillers, send_transaction/estimate_gas may rely on upstream-specified fields and break on networks expecting the provider to resolve nonce/chainId/gas.

Apply the same fix as in the previous comment to ensure the returned type is EvmProviderType.


463-477: Bug: passing JSON-RPC params as a JSON string results in double-encoding.

When the caller uses GenericRpcRequest, params arrives as Value::String containing JSON (e.g., "[...]"). Converting that with to_raw_value wraps it in quotes, so the wire payload becomes ""[...]"" instead of a raw JSON array/object — many nodes will reject this.

Detect string params and parse them into RawValue; otherwise, fall back to to_raw_value as you do.

Apply this diff to correctly handle string-encoded params:

-                // Convert params to RawValue and use Cow for method
-                let params_raw = serde_json::value::to_raw_value(&params_clone).map_err(|e| {
-                    ProviderError::Other(format!("Failed to serialize params: {}", e))
-                })?;
-
-                let result = provider
-                    .raw_request_dyn(std::borrow::Cow::Owned(method.to_string()), &params_raw)
+                // Convert params to RawValue without double-encoding
+                let params_raw = match &params_clone {
+                    serde_json::Value::String(s) => {
+                        // Treat the string as pre-serialized JSON
+                        serde_json::from_str::<Box<serde_json::value::RawValue>>(s).map_err(|e| {
+                            ProviderError::Other(format!(
+                                "Invalid JSON in params string: {}",
+                                e
+                            ))
+                        })?
+                    }
+                    _ => serde_json::value::to_raw_value(&params_clone).map_err(|e| {
+                        ProviderError::Other(format!("Failed to serialize params: {}", e))
+                    })?,
+                };
+
+                let result = provider
+                    .raw_request_dyn(std::borrow::Cow::Owned(method.to_string()), &params_raw)
                     .await
                     .map_err(ProviderError::from)?;

483-539: TransactionRequest conversion: handle contract creation, include gas limit, and avoid parsing empty string.

  • Using unwrap_or("") on tx.to will parse an empty address and error out, making it impossible to represent contract-creation txs (to = None). Use TxKind::Create when to is None.
  • Map gas_limit into the TransactionRequest’s gas field.
  • Minor: value is always Some(...); that’s fine given EvmTransactionData.value is non-optional.

Apply this diff to improve correctness:

-            to: Some(TxKind::Call(
-                tx.to
-                    .clone()
-                    .unwrap_or("".to_string())
-                    .parse()
-                    .map_err(|_| {
-                        TransactionError::InvalidType("Invalid address format".to_string())
-                    })?,
-            )),
+            to: Some(match tx.to.as_deref() {
+                Some(addr_str) => TxKind::Call(
+                    addr_str.parse().map_err(|_| {
+                        TransactionError::InvalidType("Invalid address format".to_string())
+                    })?,
+                ),
+                None => TxKind::Create,
+            }),
@@
-            value: Some(Uint::<256, 4>::from(tx.value)),
+            value: Some(Uint::<256, 4>::from(tx.value)),
+            // Respect provided gas limit, if any
+            gas: tx
+                .gas_limit
+                .map(|gl| {
+                    Uint::<256, 4>::from(gl).try_into().map_err(|_| {
+                        TransactionError::InvalidType("Invalid gas limit".to_string())
+                    })
+                })
+                .transpose()?,

If your downstream logic never creates contracts, the Create branch won’t be hit, but supporting it prevents surprising parse errors.

🧹 Nitpick comments (7)
src/services/provider/evm/mod.rs (7)

254-256: Generic retry closure now takes EvmProviderType: good, but prefer opaque provider if possible.

Using the concrete alias couples retry plumbing to filler composition. If you later change fillers, you’ll have to touch signatures across the file. Consider accepting a generic P: Provider<Network = AnyNetwork> (or an impl-trait/boxed provider) in the closure to reduce coupling.

Example (conceptual; outside the selected range):

  • Change F to Fn(P) where P: Provider<Network = AnyNetwork> + Clone + Send + Sync + 'static.

317-325: Unnecessary into() on TransactionRequest for estimate_gas.

If provider.estimate_gas accepts the same TransactionRequest type from alloy::rpc::types, the extra into() is superfluous and can obscure type intent.

-                provider
-                    .estimate_gas(tx_req.into())
+                provider
+                    .estimate_gas(tx_req)

If alloy expects a different request type here, keep the conversion, but add a comment explaining the target type for clarity.


342-345: Same nit: avoid redundant into() for send_transaction if not required.

Keep it only if the method expects a different request flavor.

-                        .send_transaction(tx_req.into())
+                        .send_transaction(tx_req)

409-423: get_block_by_number: OK, but consider exposing a parameter for block tag if needed later.

Hard-wiring Latest is fine for now; if call sites start needing historical data, you’ll end up duplicating logic. Consider a variant that accepts BlockNumberOrTag.


429-433: Error variant for bad tx hash.

Mapping an invalid transaction hash to ProviderError::Other loses structure. If feasible, add a dedicated InvalidTxHash variant, or reuse an existing structured variant to ease upstream error handling and logging.


442-453: call_contract: same redundant into(), and consider passing a block tag.

  • Nit: drop into() if not needed.
  • Optional: expose an overload with a BlockNumberOrTag (default Latest) to support state overrides at specific blocks.
-                provider
-                    .call(tx_req.into())
+                provider
+                    .call(tx_req)

585-613: Unit test depends on external network; consider making it deterministic.

test_reqwest_error_conversion invokes https://www.openzeppelin.com/ with a 1ms timeout. This can fail for reasons other than timeout (DNS), making the test flaky. Prefer a deterministic approach (e.g., request a non-routable IP with a very short connect timeout, or construct a reqwest::Error via a mocked transport).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 45d7685 and 83ec534.

📒 Files selected for processing (1)
  • src/services/provider/evm/mod.rs (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/provider/evm/mod.rs (2)
src/domain/relayer/evm/evm_relayer.rs (1)
  • rpc (450-487)
src/services/provider/mod.rs (9)
  • from (45-47)
  • from (51-53)
  • from (57-59)
  • from (100-102)
  • from (106-108)
  • from (112-120)
  • from (125-127)
  • from (135-154)
  • from (159-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer
🔇 Additional comments (2)
src/services/provider/evm/mod.rs (2)

9-21: Alloy v1 import updates: looks good.

Switching to AnyNetwork and the new rpc/types is consistent with alloy 1.x APIs. No issues spotted here.


23-30: ProviderBuilder::new() already includes the recommended fillers by default

After alloy v0.11, ProviderBuilder::new() instantiates the builder with RecommendedFillers (the exact stack of ChainIdFiller, GasFiller, BlobGasFiller, and NonceFiller) enabled by default. Therefore, your call sequence:

let provider = ProviderBuilder::new()
    .network::<AnyNetwork>()
    .connect_client(client);

already returns a FillProvider< JoinFill<Identity, JoinFill<GasFiller, JoinFill<BlobGasFiller, JoinFill<NonceFiller, ChainIdFiller>>>>, RootProvider<AnyNetwork>, AnyNetwork>—exactly matching the EvmProviderType alias.

No changes are needed to install the fillers. If you ever switch to ProviderBuilder::default() or explicitly call .disable_recommended_fillers(), then you would need to re-enable them via .with_recommended_fillers() or by layering them manually in the same order as defined in the alias.

Likely an incorrect or invalid review comment.

…test-one

* main:
  chore: Updating lock file (#431)
  chore: Updating lock file (#421)
  feat: Implement network inheritance for configs (#428)
…test-one

* main:
  chore: update clippy to check only internal code, not deps (#433)
…test-one

* main:
  refactor: Replace wiremock for mockito (#436)
  fix: clippy issues with deps (#434)
@LuisUrrutia LuisUrrutia merged commit 1725804 into main Aug 28, 2025
24 of 26 checks passed
@LuisUrrutia LuisUrrutia deleted the plat-6404-update-alloy-package-version-to-latest-one branch August 28, 2025 11:29
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants