Skip to content

Conversation

@LuisUrrutia
Copy link
Contributor

@LuisUrrutia LuisUrrutia commented Aug 19, 2025

Summary

  • Add network tag constants for better maintainability
  • Update lacks_mempool() to include optimism-based and arbitrum-based networks
  • Rename optimism tag to optimism-based for consistency
  • Update docs and tests to reflect new tag structure

Summary by CodeRabbit

  • New Features

    • Networks tagged as optimism-based or arbitrum-based (or explicitly no-mempool) are treated as lacking a mempool, improving rollup network handling.
  • Refactor

    • Legacy "optimism" tag deprecated in favor of "optimism-based"; update custom network tags for compatibility.
  • Documentation

    • Docs updated to use "optimism-based", add deprecation note for "optimism", and clarify no-mempool/OP Stack semantics.
  • Tests

    • Test suites updated to use the new tag names.

@LuisUrrutia LuisUrrutia requested review from a team as code owners August 19, 2025 10:12
@coderabbitai
Copy link

coderabbitai bot commented Aug 19, 2025

Walkthrough

Renamed the optimism tag to optimism-based (with optimism deprecated), added a new network_tags constants module and re-export, updated EVM network tag checks and added lacks_mempool() using those constants, and updated config, docs, and tests to use the new constants.

Changes

Cohort / File(s) Summary of Changes
Config tag rename
config/networks/optimism.json
Renamed "optimism""optimism-based" for optimism-related network entries; no other structural changes.
Docs update
docs/modules/ROOT/pages/network_configuration.adoc
Added deprecation note for optimism, introduced optimism-based in Special Network Tags, updated examples to use optimism-based, and clarified that arbitrum-based/optimism-based imply no mempool.
Constants module
src/constants/mod.rs, src/constants/network_tags.rs
Added internal network_tags module and pub use network_tags::*;; introduced constants: NO_MEMPOOL_TAG, ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG, deprecated OPTIMISM_TAG, ROLLUP_TAG, and LACKS_MEMPOOL_TAGS.
EVM network logic & tests
src/models/network/evm/network.rs
Replaced literal tag checks with constants; is_optimism, is_rollup, is_arbitrum use constants; added pub fn lacks_mempool(&self) -> bool which checks membership in LACKS_MEMPOOL_TAGS; tests updated to reference constants and cover deprecated optimism.
Test updates (utils, services, price calc)
src/domain/transaction/evm/utils.rs, src/services/gas/network_extra_fee.rs, src/domain/transaction/evm/price_calculator.rs
Tests replaced inline tag strings with imported constants (ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG, NO_MEMPOOL_TAG, ROLLUP_TAG, etc.); no runtime-signature changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller as Caller
  participant EvmNetwork as EvmNetwork
  Note over EvmNetwork: Tags may include rollup, no-mempool, arbitrum-based, optimism-based\n(LACKS_MEMPOOL_TAGS aggregates mempool-less tags)

  Caller->>EvmNetwork: lacks_mempool()
  activate EvmNetwork
  EvmNetwork->>EvmNetwork: iterate network.tags\ncheck tag ∈ LACKS_MEMPOOL_TAGS
  alt any match
    EvmNetwork-->>Caller: true
  else none match
    EvmNetwork-->>Caller: false
  end
  deactivate EvmNetwork
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • zeljkoX

Poem

I hop through tags with whiskers bright,
Swapping strings for constants by moonlight.
"optimism-based" leads the hop, not the old,
Arbi, OP, and no-mempool stories told.
Carrot cheers — the tags march bold. 🥕🐇


📜 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 b867a27 and 0148ef0.

📒 Files selected for processing (1)
  • src/models/network/evm/network.rs (7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/models/network/evm/network.rs:1-1
Timestamp: 2025-08-19T14:31:09.686Z
Learning: In the OpenZeppelin relayer codebase, test helper functions may use network names (like "optimism") as string literals for creating mock networks, which are different from network tag constants (like OPTIMISM_BASED_TAG). These should not be flagged as inconsistencies when reviewing tag-related changes.
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/constants/network_tags.rs:5-6
Timestamp: 2025-08-28T11:19:51.373Z
Learning: In the OpenZeppelin relayer codebase, when deprecating constants that are still used internally (like OPTIMISM_TAG in LACKS_MEMPOOL_TAGS), use doc comments instead of #[deprecated] attribute to avoid clippy failures during the transition period.
📚 Learning: 2025-08-19T14:31:09.686Z
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/models/network/evm/network.rs:1-1
Timestamp: 2025-08-19T14:31:09.686Z
Learning: In the OpenZeppelin relayer codebase, test helper functions may use network names (like "optimism") as string literals for creating mock networks, which are different from network tag constants (like OPTIMISM_BASED_TAG). These should not be flagged as inconsistencies when reviewing tag-related changes.

Applied to files:

  • src/models/network/evm/network.rs
📚 Learning: 2025-08-28T11:19:51.373Z
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/constants/network_tags.rs:5-6
Timestamp: 2025-08-28T11:19:51.373Z
Learning: In the OpenZeppelin relayer codebase, when deprecating constants that are still used internally (like OPTIMISM_TAG in LACKS_MEMPOOL_TAGS), use doc comments instead of #[deprecated] attribute to avoid clippy failures during the transition period.

Applied to files:

  • src/models/network/evm/network.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: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer
  • GitHub Check: clippy
  • GitHub Check: test
  • GitHub Check: msrv
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: Analyze (rust)
🔇 Additional comments (15)
src/models/network/evm/network.rs (15)

1-3: LGTM: Centralized tag imports and LACKS_MEMPOOL_TAGS usage.
Clean, future-proof.


97-102: LGTM: Backward-compatible optimism detection.
Checks both OPTIMISM_BASED_TAG and deprecated OPTIMISM_TAG.


104-106: LGTM: Rollup detection via constant.
Removes raw string dependency.


117-120: LGTM: Mempool detection via LACKS_MEMPOOL_TAGS.
Straightforward and maintainable.


122-124: LGTM: Arbitrum detection via ARBITRUM_BASED_TAG.
Matches new semantics.


184-186: LGTM: Positive case for optimism-based tag.


190-191: LGTM: Negative case without optimism tags.
Use of "mainnet" literal in tests is OK per retrieved learnings.


195-198: LGTM: Compatibility test for deprecated optimism tag.


201-204: LGTM: Mempool test with deprecated optimism tag.


208-209: LGTM: Rollup detection test.


220-221: LGTM: lacks_mempool with explicit no-mempool tag.


226-227: LGTM: lacks_mempool negative control.


232-235: LGTM: Arbitrum-like network semantics.
Asserts align with ARBITRUM_BASED_TAG implying no mempool.


241-245: LGTM: Optimism-like network semantics.
OPTIMISM_BASED_TAG implies no mempool.


273-274: LGTM: TryFrom path uses constants and asserts mempool behavior.

Also applies to: 291-292

✨ 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-6876-avoid-no-mempool-for-optimism-based-and-arbitrum-based-tags

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)
config/networks/optimism.json (1)

78-80: Add “optimism-based” tag to OP Stack networks
The following networks in config/networks/optimism.json only have "rollup" (and in some cases "deprecated") but lack the "optimism-based" tag that both is_optimism() and lacks_mempool() rely on. Without it, Base, World Chain, UniChain (and their Sepolia variants) won’t be recognized as OP Stack.

Networks to update:

  • optimism & optimism-sepolia
  • base & base-sepolia
  • world-chain & world-chain-sepolia
  • unichain & unichain-sepolia

Example diff for each entry:

       "tags": [
-        "rollup"
+        "rollup",
+        "optimism-based"
       ],

Apply the same addition to all deprecated and testnet variants above.

src/models/network/evm/network.rs (1)

1-269: Replace hard-coded tag literal with constant

Only one raw tag literal remains outside the constants module. Please update it to use the ARBITRUM_BASED_TAG constant:

Location:
src/domain/transaction/evm/price_calculator.rs:1918
Current:

tags: vec!["arbitrum-based".to_string()], // This makes is_arbitrum() return true

Update to:

use crate::constants::ARBITRUM_BASED_TAG;
// …
tags: vec![ARBITRUM_BASED_TAG.to_string()],

All other occurrences of "optimism-based", "arbitrum-based", and "no-mempool" are defined and referenced via the network_tags constants.

🧹 Nitpick comments (9)
docs/modules/ROOT/pages/network_configuration.adoc (1)

179-181: Clarify “no-mempool” semantics vs. tag-based detection.

Given the new lacks_mempool() behavior (treating arbitrum-based and optimism-based as “no mempool”), add a note here to prevent confusion about whether the explicit no-mempool tag is required.

Suggested wording update:

-|`no-mempool`
-|Indicates networks that lack a traditional mempool (e.g., Arbitrum)
+|`no-mempool`
+|Indicates networks that lack a traditional mempool (e.g., Arbitrum). Note: The relayer also treats networks tagged as `arbitrum-based` or `optimism-based` as lacking a mempool, even if `no-mempool` is not present.
src/services/gas/network_extra_fee.rs (1)

66-71: Verify OP Stack coverage for Optimism extra fee routing.

get_network_extra_fee_calculator_service() depends on network.is_optimism(), which now keys off the OPTIMISM_BASED_TAG. If Base/World/Unichain configs don’t include this tag, they won’t receive OptimismExtraFeeService even though they’re OP Stack.

  • Ensure OP Stack networks in config are tagged as “optimism-based” (see review on config/networks/optimism.json).
  • Consider adding an additional test case here for a Base (or World Chain) network to assert that OptimismExtraFeeService is selected when the tag is present:
@@
     #[test]
     fn test_get_network_extra_fee_calculator_service_for_optimism() {
@@
     }
+
+    #[test]
+    fn test_get_network_extra_fee_calculator_service_for_op_stack_chain() {
+        let provider = MockEvmProviderTrait::new();
+        // Simulate a Base-like OP Stack network by name, with the optimism-based tag
+        let network = create_test_evm_network("base", true);
+        let service = get_network_extra_fee_calculator_service(network, provider);
+        assert!(
+            matches!(service, NetworkExtraFeeCalculator::Optimism(_)),
+            "OP Stack networks with the optimism-based tag should use Optimism extra fee"
+        );
+    }
src/constants/network_tags.rs (1)

1-5: Consider adding a single source-of-truth list for “no mempool” semantics

To avoid duplicating the set of tags that imply “no mempool” across modules, define a constant array here and consume it from call sites (e.g., EvmNetwork::lacks_mempool). Keeps the policy in one place.

You can add this alongside the existing constants:

// Tags that imply lack of a public mempool
pub const LACKS_MEMPOOL_TAGS: [&str; 3] = [NO_MEMPOOL_TAG, ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG];

Then use it downstream with .contains(&t.as_str()).

src/models/network/evm/network.rs (6)

95-98: Avoid temporary allocation in is_optimism()

No need to allocate a String just to check membership. Iterate and compare against &str.

Apply this diff:

-        self.tags.contains(&OPTIMISM_BASED_TAG.to_string())
+        self.tags.iter().any(|t| t == OPTIMISM_BASED_TAG)

100-102: Avoid temporary allocation in is_rollup()

Match the pattern used elsewhere and avoid to_string().

Apply this diff:

-        self.tags.contains(&ROLLUP_TAG.to_string())
+        self.tags.iter().any(|t| t == ROLLUP_TAG)

104-107: Docstring is out of date with the new unified policy

The implementation treats arbitrum-based and optimism-based as lacking mempool, but the docs only mention “no-mempool”.

Apply this diff:

-    /// Returns whether this network lacks mempool support.
-    ///
-    /// Returns true if the network has no mempool or if it has the "no-mempool" tag.
+    /// Returns whether this network lacks mempool-like behavior (no public/pending pool).
+    ///
+    /// Returns true if any of these tags are present:
+    /// - "no-mempool"
+    /// - "arbitrum-based"
+    /// - "optimism-based"

108-111: Reduce repetition and improve readability in lacks_mempool()

Use a small fixed set instead of chaining ||. Easier to extend and reads clearer.

Apply this diff:

-        self.tags.iter().any(|tag| {
-            tag == NO_MEMPOOL_TAG || tag == ARBITRUM_BASED_TAG || tag == OPTIMISM_BASED_TAG
-        })
+        self.tags
+            .iter()
+            .any(|t| [NO_MEMPOOL_TAG, ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG].contains(&t.as_str()))

113-115: Avoid temporary allocation in is_arbitrum()

Align with the other helpers.

Apply this diff:

-        self.tags.contains(&ARBITRUM_BASED_TAG.to_string())
+        self.tags.iter().any(|t| t == ARBITRUM_BASED_TAG)

209-214: Strengthen “arbitrum-like” test to actually exercise ARBITRUM_BASED_TAG

The current test name suggests Arbitrum semantics but doesn’t include ARBITRUM_BASED_TAG; it relies on NO_MEMPOOL_TAG instead. Include the tag and assert is_arbitrum() to validate both behaviors.

Apply this diff:

-        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG, NO_MEMPOOL_TAG]);
-        assert!(network.is_rollup());
-        assert!(network.lacks_mempool());
-        assert!(!network.is_optimism());
+        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG, ARBITRUM_BASED_TAG]);
+        assert!(network.is_rollup());
+        assert!(network.is_arbitrum());
+        assert!(network.lacks_mempool());
+        assert!(!network.is_optimism());

Optionally, add a separate negative case ensuring is_arbitrum() is false when the tag is absent.

📜 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 7e7fb4f and 5d16238.

📒 Files selected for processing (7)
  • config/networks/optimism.json (2 hunks)
  • docs/modules/ROOT/pages/network_configuration.adoc (2 hunks)
  • src/constants/mod.rs (1 hunks)
  • src/constants/network_tags.rs (1 hunks)
  • src/domain/transaction/evm/utils.rs (3 hunks)
  • src/models/network/evm/network.rs (6 hunks)
  • src/services/gas/network_extra_fee.rs (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/models/network/evm/network.rs (1)
src/services/gas/evm_gas_price.rs (3)
  • network (143-143)
  • network (156-158)
  • network (306-308)
⏰ 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: clippy
  • GitHub Check: test
  • GitHub Check: msrv
  • 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 (18)
config/networks/optimism.json (2)

26-27: Renaming tag to "optimism-based" is correct and consistent.

This matches the new OPTIMISM_BASED_TAG constant and improves maintainability by avoiding string literals.


53-54: Renaming in optimism-sepolia is also correct.

Consistent with constants and the docs changes.

docs/modules/ROOT/pages/network_configuration.adoc (2)

173-175: Docs update for “optimism-based” tag looks good.

Clear description and aligns with the new constant and config changes.


326-327: Example updated to use “optimism-based” — LGTM.

Keeps examples aligned with the new tag taxonomy.

src/constants/mod.rs (1)

38-39: Re-exporting network tag constants is a good call.

This avoids stringly-typed checks across the codebase and centralizes tag values. The private module + public re-exports pattern is appropriate here.

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

76-77: Good use of the OPTIMISM_BASED_TAG constant in tests.

Removes string literals and aligns with the production checks in is_optimism().

Also applies to: 88-89

src/domain/transaction/evm/utils.rs (2)

129-129: Switching tests to use constants for tags is the right move.

Reduces brittleness and centralizes tag definitions.


155-155: LGTM: Tests use ARBITRUM_BASED_TAG and ROLLUP_TAG for Arbitrum chains.

Keeps the Arbitrum detection aligned with the refactored tag-based methods.

Also applies to: 169-169

src/constants/network_tags.rs (1)

1-5: Centralized, well-named network tag constants — LGTM

Good move extracting these tags into a single module. This should reduce stringly-typed drift across the codebase and make future changes easier.

src/models/network/evm/network.rs (9)

1-1: Imports of tag constants — LGTM

Using re-exported constants from crate::constants keeps call sites clean and consistent.


172-176: is_optimism() positive test — LGTM


178-182: is_optimism() negative test — LGTM


184-188: is_rollup() positive test — LGTM


196-200: lacks_mempool() with explicit no-mempool tag — LGTM


202-206: lacks_mempool() negative test — LGTM


216-222: Optimism-like test — LGTM

Covers both tag detection and unified mempool policy.


250-250: try_from() path populating tags with constants — LGTM


269-269: Unified lacks_mempool expectation through try_from() — LGTM

@codecov
Copy link

codecov bot commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.0%. Comparing base (efede36) to head (0148ef0).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #427     +/-   ##
=======================================
- Coverage   93.0%   93.0%   -0.1%     
=======================================
  Files        217     217             
  Lines      74164   74177     +13     
=======================================
+ Hits       68995   69007     +12     
- Misses      5169    5170      +1     
Flag Coverage Δ
integration 0.5% <0.0%> (-0.1%) ⬇️
properties <0.1% <0.0%> (ø)
unittests 93.0% <100.0%> (-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

@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: 1

🧹 Nitpick comments (4)
src/domain/transaction/evm/price_calculator.rs (2)

744-749: Use NO_MEMPOOL_TAG constant instead of hardcoded string

Small consistency nit: since the repo now centralizes network tag strings, prefer the NO_MEMPOOL_TAG constant over the raw "no-mempool" string in the test helper.

Apply this diff:

-    use crate::constants::ARBITRUM_BASED_TAG;
+    use crate::constants::{ARBITRUM_BASED_TAG, NO_MEMPOOL_TAG};
@@
-            tags: vec!["no-mempool".to_string()], // This makes lacks_mempool() return true
+            tags: vec![NO_MEMPOOL_TAG.to_string()], // This makes lacks_mempool() return true

Also applies to: 701-701


245-247: Redundant is_arbitrum() check after unified mempool detection

With lacks_mempool() now including "arbitrum-based" and "optimism-based", the || is_arbitrum() is redundant.

Apply this diff:

-        if self.gas_price_service.network().lacks_mempool()
-            || self.gas_price_service.network().is_arbitrum()
-        {
+        if self.gas_price_service.network().lacks_mempool() {
src/models/network/evm/network.rs (2)

104-114: Docstring and unified mempool detection look good; consider avoiding hardcoded tag names in docs

Implementation correctly uses LACKS_MEMPOOL_TAGS. To prevent future drift, consider phrasing the doc as “Returns true if any tag in LACKS_MEMPOOL_TAGS is present” without enumerating string literals.


184-185: Minor: test tag sets mix constants and raw strings

Not a blocker, but for uniformity you could use constants everywhere you have them. Otherwise, current tests read fine.

Also applies to: 202-203, 208-210

📜 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 5d16238 and 20da299.

📒 Files selected for processing (4)
  • docs/modules/ROOT/pages/network_configuration.adoc (2 hunks)
  • src/constants/network_tags.rs (1 hunks)
  • src/domain/transaction/evm/price_calculator.rs (2 hunks)
  • src/models/network/evm/network.rs (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/constants/network_tags.rs
  • docs/modules/ROOT/pages/network_configuration.adoc
⏰ 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: test
  • GitHub Check: clippy
  • GitHub Check: msrv
  • 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 (6)
src/domain/transaction/evm/price_calculator.rs (2)

701-701: Good move: replace string literal with centralized tag constant

Importing and using ARBITRUM_BASED_TAG in tests improves consistency with the new tag constants.


1919-1924: LGTM: test uses ARBITRUM_BASED_TAG to trigger is_arbitrum()

Using the constant in the test network’s tags correctly exercises is_arbitrum() and integrates with the unified mempool detection.

src/models/network/evm/network.rs (4)

1-1: Centralized tag imports are a win for maintainability

Importing ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG, ROLLUP_TAG, and LACKS_MEMPOOL_TAGS aligns code and tests on a single source of truth.


97-102: LGTM: predicate helpers now rely on constants

  • is_optimism() checks OPTIMISM_BASED_TAG
  • is_rollup() checks ROLLUP_TAG
  • is_arbitrum() checks ARBITRUM_BASED_TAG

This is clear and robust to future string renames.

Also applies to: 116-118


159-159: Nice: tests import NO_MEMPOOL_TAG

Switching tests to constants reduces brittleness when tags change.


178-180: Tests updated to constants are consistent with the new tagging scheme

  • Optimism-like tests use OPTIMISM_BASED_TAG
  • TryFrom path uses ROLLUP_TAG + OPTIMISM_BASED_TAG
  • lacks_mempool() assertions align with LACKS_MEMPOOL_TAGS

All good.

Also applies to: 223-227, 255-256, 274-274

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: 1

🧹 Nitpick comments (3)
src/models/network/evm/network.rs (3)

104-114: lacks_mempool: doc + logic are consistent; minor nit to reference constants

  • The doc accurately reflects the new composite rule.
  • Implementation via LACKS_MEMPOOL_TAGS reads well and is future-proof.

Nit: consider referencing the constants (NO_MEMPOOL_TAG, ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG) in the doc to prevent drift if the underlying tag strings change.


95-117: Optional: add a helper to reduce repetition for tag checks

You repeat the same iterator pattern across is_optimism, is_rollup, and is_arbitrum. A small helper improves readability and reduces duplication.

Apply this diff within the selected lines:

 pub fn is_optimism(&self) -> bool {
-        self.tags.iter().any(|t| t == OPTIMISM_BASED_TAG)
+        self.has_tag(OPTIMISM_BASED_TAG)
 }
 
 pub fn is_rollup(&self) -> bool {
-        self.tags.iter().any(|t| t == ROLLUP_TAG)
+        self.has_tag(ROLLUP_TAG)
 }
 
 pub fn is_arbitrum(&self) -> bool {
-        self.tags.iter().any(|t| t == ARBITRUM_BASED_TAG)
+        self.has_tag(ARBITRUM_BASED_TAG)
 }

And add this helper inside the impl (outside the selected range):

impl EvmNetwork {
    #[inline]
    fn has_tag(&self, tag: &str) -> bool {
        self.tags.iter().any(|t| t == tag)
    }
}

190-190: Nit: keep tests minimal to the behavior under test

NO_MEMPOOL_TAG isn’t needed to validate is_rollup(). Consider simplifying the setup to reduce coupling to mempool semantics.

-        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG, NO_MEMPOOL_TAG]);
+        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG]);
📜 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 20da299 and 7d172cd.

📒 Files selected for processing (2)
  • src/domain/transaction/evm/price_calculator.rs (3 hunks)
  • src/models/network/evm/network.rs (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/domain/transaction/evm/price_calculator.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). (9)
  • GitHub Check: msrv
  • GitHub Check: clippy
  • GitHub Check: test
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: semgrep/ci
  • GitHub Check: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer
  • GitHub Check: Analyze (rust)
🔇 Additional comments (13)
src/models/network/evm/network.rs (13)

1-1: Good move: centralize tags via constants

Importing ARBITRUM_BASED_TAG, OPTIMISM_BASED_TAG, ROLLUP_TAG, and LACKS_MEMPOOL_TAGS improves consistency and avoids string drift across the codebase.


97-97: Rename aligned: use OPTIMISM_BASED_TAG

is_optimism now checks OPTIMISM_BASED_TAG. This matches the new semantics and avoids legacy "optimism" string usage.


101-101: Rollup detection via constant is correct

Using ROLLUP_TAG directly is clear and robust vs. raw strings.


117-117: Arbitrum detection via ARBITRUM_BASED_TAG looks good

Matches the updated tag taxonomy and works with the lacks_mempool aggregation.


158-158: Tests import NO_MEMPOOL_TAG from constants

Good to avoid hardcoding raw strings in tests as well.


178-178: Test uses OPTIMISM_BASED_TAG

Accurately reflects the rename; assertion is coherent with is_optimism().


184-184: Negative test without optimism-based tag

Correctly ensures absence of tag leads to false. Including a non-relevant tag like "mainnet" is fine.


202-203: Mempool absence via NO_MEMPOOL_TAG is covered

This test complements the aggregated lacks_mempool logic; looks good.


208-209: Negative path for lacks_mempool is valid

Ensures that a plain rollup tag doesn’t imply mempool absence by itself. Good coverage.


214-219: Arbitrum-like network test reflects new semantics

  • Uses ARBITRUM_BASED_TAG (fixing the prior mismatch flagged in earlier review threads).
  • Asserts lacks_mempool() without NO_MEMPOOL_TAG, validating the aggregated rule.

223-227: Optimism-like network test validates aggregated lacks_mempool

Correct: presence of OPTIMISM_BASED_TAG implies lacks_mempool() is true.


255-255: try_from test uses new constants in tags

Good end-to-end validation through repository model -> EvmNetwork conversion.


273-274: Final assertion aligns with aggregated mempool rule

lacks_mempool() being true with OPTIMISM_BASED_TAG is consistent with the new policy.

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.

Just a note on a brreaking change. Probably we should revert tag name change. Other than that 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: 5

🧹 Nitpick comments (4)
src/constants/network_tags.rs (1)

8-13: Make LACKS_MEMPOOL_TAGS a const slice to ease future removals.

Avoid array length churn when dropping deprecated tags later.

-pub const LACKS_MEMPOOL_TAGS: [&str; 4] = [
-    NO_MEMPOOL_TAG,
-    ARBITRUM_BASED_TAG,
-    OPTIMISM_BASED_TAG,
-    OPTIMISM_TAG,
-];
+pub const LACKS_MEMPOOL_TAGS: &[&str] = &[
+    NO_MEMPOOL_TAG,
+    ARBITRUM_BASED_TAG,
+    OPTIMISM_BASED_TAG,
+    OPTIMISM_TAG, // deprecated; kept for compatibility
+];
src/models/network/evm/network.rs (3)

99-101: is_optimism: backward-compat path is fine; plan for removal.

The dual check (optimism-based OR deprecated optimism) looks good. Consider adding a TODO referencing the future removal of OPTIMISM_TAG to keep cleanup visible.


115-117: LGTM: simple membership against the shared list.

If this becomes hot-path, consider caching a HashSet<&'static str> behind a once_cell for O(1) membership. Not required now.


206-207: Nit: keep tests focused—drop unrelated tags.

This rollup test doesn’t need NO_MEMPOOL_TAG. Removing it keeps intent clear.

-        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG, NO_MEMPOOL_TAG]);
+        let network = create_test_evm_network_with_tags(vec![ROLLUP_TAG]);
📜 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 7d172cd and ff250c7.

📒 Files selected for processing (3)
  • docs/modules/ROOT/pages/network_configuration.adoc (2 hunks)
  • src/constants/network_tags.rs (1 hunks)
  • src/models/network/evm/network.rs (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/modules/ROOT/pages/network_configuration.adoc
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/models/network/evm/network.rs:1-1
Timestamp: 2025-08-19T14:31:09.686Z
Learning: In the OpenZeppelin relayer codebase, test helper functions may use network names (like "optimism") as string literals for creating mock networks, which are different from network tag constants (like OPTIMISM_BASED_TAG). These should not be flagged as inconsistencies when reviewing tag-related changes.
📚 Learning: 2025-08-19T14:31:09.686Z
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/models/network/evm/network.rs:1-1
Timestamp: 2025-08-19T14:31:09.686Z
Learning: In the OpenZeppelin relayer codebase, test helper functions may use network names (like "optimism") as string literals for creating mock networks, which are different from network tag constants (like OPTIMISM_BASED_TAG). These should not be flagged as inconsistencies when reviewing tag-related changes.

Applied to files:

  • src/constants/network_tags.rs
  • src/models/network/evm/network.rs
🧬 Code graph analysis (1)
src/models/network/evm/network.rs (1)
src/services/gas/evm_gas_price.rs (3)
  • network (143-143)
  • network (156-158)
  • network (306-308)
⏰ 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 (3)
src/models/network/evm/network.rs (3)

1-3: LGTM: imports consolidated via constants module.

Using shared tag constants improves consistency and avoids string drift.


105-105: LGTM: is_rollup uses the canonical constant.


121-121: LGTM: arbitrum detection via ARBITRUM_BASED_TAG.

…d-and-arbitrum-based-tags

* main:
  refactor: Replace wiremock for mockito (#436)
  fix: clippy issues with deps (#434)
  chore: update clippy to check only internal code, not deps (#433)
  chore: Updating lock file (#431)
  chore: Updating lock file (#421)
  feat: Implement network inheritance for configs (#428)
  docs: Add more Stellar transaction submission examples (#424)
@LuisUrrutia LuisUrrutia merged commit 63c04dc into main Aug 28, 2025
25 of 26 checks passed
@LuisUrrutia LuisUrrutia deleted the plat-6876-avoid-no-mempool-for-optimism-based-and-arbitrum-based-tags branch August 28, 2025 12:23
@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.

3 participants