Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces provider health management, adds a Networks API for RPC URL configuration, refactors RPC URL representation from strings to a structured RpcConfig type across the codebase, centralizes provider creation through ProviderConfig, and implements a shared health tracking store for monitoring provider failures and automatic pausing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Network API<br/>(Controller)
participant Repo as Network<br/>Repository
participant Store as RpcHealthStore<br/>(Singleton)
participant Provider as RPC Provider
rect rgb(240, 248, 255)
Note over Client,Provider: Getting Network with Health Status
Client->>API: GET /api/v1/networks/{id}
API->>Repo: fetch_network(id)
Repo-->>API: NetworkRepoModel
API->>API: Convert to NetworkResponse
Note over API: rpc_urls include<br/>weight & health data
API-->>Client: 200 OK NetworkResponse
end
rect rgb(240, 248, 255)
Note over Client,Provider: Updating Network RPC URLs
Client->>API: PATCH /api/v1/networks/{id}<br/>UpdateNetworkRequest
API->>API: Validate non-empty rpc_urls
API->>API: RpcConfig::validate_list()
API->>Repo: fetch_network(id)
Repo-->>API: NetworkRepoModel
API->>API: Merge RPC configs<br/>(child overrides parent)
API->>Repo: save(updated_network)
Repo-->>API: Success
API-->>Client: 200 OK NetworkResponse
end
rect rgb(240, 248, 255)
Note over Client,Provider: Provider Failure Tracking
Provider->>Store: mark_failed(url, threshold=3<br/>pause_duration=60s<br/>failure_expiration=60s)
Store->>Store: Append failure_timestamp
Store->>Store: Prune stale timestamps
alt Failures >= Threshold
Store->>Store: Set paused_until = now + 60s
Store->>Store: Log "Provider paused"
end
Store-->>Provider: Metadata updated
end
rect rgb(240, 248, 255)
Note over Provider,Store: Provider Recovery Check
Provider->>Store: is_paused(url, threshold=3<br/>failure_expiration=60s)
alt Pause expired
Store->>Store: Clear paused_until
Store->>Store: Check if stale failures remain
else Still paused
Store-->>Provider: true (paused)
end
Store-->>Provider: false (available)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes This review involves substantial, multi-system changes with high complexity:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #600 +/- ##
==========================================
+ Coverage 92.30% 92.36% +0.05%
==========================================
Files 250 257 +7
Lines 90865 93113 +2248
==========================================
+ Hits 83875 86005 +2130
- Misses 6990 7108 +118
🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/domain/relayer/solana/solana_relayer.rs (1)
762-804: Fix use-after-move ofrequestinrpcmethodThe destructuring at lines 766-770 moves
request, after which line 800 attempts to accessrequest.jsonrpc, causing a compile error. Capturejsonrpcinto a local variable instead of discarding it, then reuse that variable.Proposed fix
let JsonRpcRequest { - jsonrpc: _, + jsonrpc, id, params, } = request;Then at line 800:
let response = self .rpc_handler .handle_request(JsonRpcRequest { - jsonrpc: request.jsonrpc, + jsonrpc, params: NetworkRpcRequest::Solana(solana_request), id: id.clone(), })src/services/provider/evm/mod.rs (1)
155-191: Fix infinite recursion inget_configs()implementations across all provider traitsThe trait implementations for
EvmProviderTrait,StellarProviderTrait, andSolanaProviderTraitall have the same critical bug: theirget_configs()methods callself.get_configs(), which resolves to the trait method itself rather than the inherent method, causing infinite recursion at runtime.Each provider has a correct inherent method that delegates to the selector (e.g.,
self.selector.get_configs()), but the trait implementations bypass this by calling the trait method recursively.Call the inherent method explicitly to fix this:
Proposed fix
#[async_trait] impl EvmProviderTrait for EvmProvider { fn get_configs(&self) -> Vec<RpcConfig> { - self.get_configs() + EvmProvider::get_configs(self) } }Apply the same pattern to
StellarProviderTraitandSolanaProviderTrait.openapi.json (1)
7104-7142: Align RpcConfig/RpcUrlEntry docs with enforced weight bounds.The
RpcConfigschema correctly documentsweightas 0–100 withmaximum: 100and a default of 100, matching the Rust-side intent.Given that, please double‑check all usages/examples that mention weights via
RpcUrlEntryto ensure they stay within this range (seeUpdateNetworkRequestbelow where an example uses 200). Keeping examples inside the documented bounds avoids confusing clients and mismatches with server-side validation.src/services/provider/solana/mod.rs (1)
353-359: Avoid possible recursion in SolanaProviderTrait::get_configsYou’ve added an inherent method:
impl SolanaProvider { pub fn get_configs(&self) -> Vec<RpcConfig> { self.selector.get_configs() } }and the trait impl currently does:
impl SolanaProviderTrait for SolanaProvider { fn get_configs(&self) -> Vec<RpcConfig> { self.get_configs() } // ... }Because both the trait method and the inherent method have the same name and signature,
self.get_configs()here can resolve back to the trait method, causing infinite recursion.To make the intent explicit and avoid any ambiguity, call the inherent method via fully qualified syntax:
Proposed fix
#[async_trait] #[allow(dead_code)] impl SolanaProviderTrait for SolanaProvider { - fn get_configs(&self) -> Vec<RpcConfig> { - self.get_configs() - } + fn get_configs(&self) -> Vec<RpcConfig> { + SolanaProvider::get_configs(self) + }Also applies to: 642-648
♻️ Duplicate comments (1)
src/domain/relayer/solana/rpc/methods/sign_transaction.rs (1)
560-577: SameSwapInfooption-field change as aboveThis block mirrors the earlier Jupiter mock change; the same reasoning and suggestions apply.
Also applies to: 570-573
🧹 Nitpick comments (36)
src/domain/transaction/evm/status.rs (1)
736-738: Tests now correctly useRpcConfigforrpc_urlsThe test helpers and imports are correctly updated to construct
rpc_urlsasVec<RpcConfig>, which matches the new network model and keeps status logic untouched. One minor nit: sinceRpcConfigis imported (Line 736), you can drop thecrate::models::qualifier in the few places that still usecrate::models::RpcConfig::new(...)for consistency.Also applies to: 803-805, 829-832, 1212-1214
src/models/transaction/repository.rs (1)
1701-1703: Test network configs aligned withRpcConfigThe EVM, Solana, and Stellar test
NetworkConfigCommonfixtures now userpc_urls: Vec<RpcConfig>, which is consistent with the new RPC model and keepsTransactionRepoModel::try_frombehavior intact. If you touch this file again, you could importRpcConfigat the top and drop the repeatedcrate::models::prefix in tests for slightly cleaner code.Also applies to: 1785-1787, 1863-1865, 2125-2127
src/services/jupiter/mod.rs (1)
53-59: Making Jupiter fee fields optional is consistent and safely wiredTurning
SwapInfo.fee_amountandfee_mintintoOption<String>with#[serde(default)]is a good fit for handling routes where Jupiter omits these fields. The mock service and HTTP tests are correctly updated to setSome(...)values, so existing expectations still hold while allowing real responses to leave them out.If you ever need to serialize
SwapInfoback out (beyond tests), consider adding#[serde(skip_serializing_if = "Option::is_none")]on these fields to avoid emittingnullkeys, but that’s not required for current usage.Also applies to: 410-419, 470-479, 684-686, 755-757
src/api/controllers/mod.rs (1)
13-18: Exposenetworkcontroller and keep docs in syncRe‑exporting the
networkcontroller from this module is the right hook for the new Networks API. As a small follow‑up, you might also add a* network – Network configuration endpointsentry to the controller list in the module docs to keep the documentation complete.src/services/gas/fetchers/default.rs (1)
46-48: Tests correctly adapted toRpcConfig, consider importing it onceUsing
crate::models::RpcConfig::new(...)forrpc_urlsmatches the new type and is functionally fine. For readability you might add a singleuse crate::models::RpcConfig;at the top of the test module and useRpcConfig::new(...)everywhere instead of repeating the fully-qualified path.Also applies to: 76-78, 105-107, 135-137, 165-167
src/domain/transaction/evm/utils.rs (1)
229-231: UnifyRpcConfigimports in the test moduleThe switch of
rpc_urlstoRpcConfigis correct. You currently mix a fully-qualified path increate_standard_networkwith function-localuse crate::models::RpcConfig;in the Arbitrum helpers. Consider a singleuse crate::models::RpcConfig;at the top of the test module and usingRpcConfig::new(...)consistently.Also applies to: 245-249, 262-266
src/domain/relayer/solana/rpc/methods/sign_transaction.rs (1)
327-345:SwapInfofee fields updated correctly toOption<String>Using
Some("10".to_string())/Some("…".to_string())matches the new optional fee fields and keeps existing behavior for the “has fee” path. You may eventually want an additional test exercising theNonecase forfee_amount/fee_mintto ensure the validation and routing logic are robust when Jupiter omits these.Also applies to: 337-340
src/repositories/network/network_redis.rs (1)
657-664: Tests updated toRpcConfigcorrectly; consider a shared importThe move from
Vec<String>toVec<RpcConfig>inNetworkConfigCommon.rpc_urlsis correct and keeps these tests aligned with the model. To reduce repetition, you could adduse crate::models::RpcConfig;at the top of the test module and useRpcConfig::new(...)instead ofcrate::models::RpcConfig::new(...)in both helpers.Also applies to: 943-955
src/domain/transaction/evm/price_calculator.rs (1)
704-708: ConsolidateRpcConfigimports in EVM price calculator testsThe mock networks’
rpc_urlsnow correctly useRpcConfig::new(...). Since multiple helpers/tests importRpcConfiglocally, you can simplify by addinguse crate::models::RpcConfig;once at the top of thetestsmodule and dropping the per-functionusestatements.Also applies to: 721-730, 2073-2077
src/config/config_file/network/test_utils.rs (1)
18-23: Helpers moved toRpcConfigcorrectly; reduce repeated local importsAll the helper constructors now build
rpc_urlsasVec<RpcConfig>, which matches the updated config model and keeps the tests in sync. To declutter, you could add a singleuse crate::models::RpcConfig;at the top of this module and drop the per-functionuselines, then keep usingRpcConfig::new(...)in each helper. The JSON factory helpers that still emit URL strings are fine since they’re validating the file format, not the in-memory type.Also applies to: 101-109, 120-128, 139-147, 160-168
src/domain/relayer/stellar/stellar_relayer.rs (1)
4-4: Sanitized Stellar RPC error handling looks solid; consider tightening test expectationsThe switch to
map_provider_error+sanitize_error_descriptionwith an internaltracing::error!log gives you centralized, non‑leaky RPC error responses while keeping full detail in logs. The wiring intocreate_error_responseis correct and type‑safe.You might optionally strengthen
rpc_tests::test_rpc_provider_errorto assert the mapped error code and sanitized description (e.g., that it no longer exposes the underlyingProviderError::Othermessage), so future changes to the sanitization layer are caught by tests rather than accidentally regressing to raw provider strings.Also applies to: 571-590, 3067-3116
src/domain/relayer/solana/rpc/methods/transfer_transaction.rs (1)
540-543: SwapInfo fee fields correctly updated toOption<String>in testsUsing
Some("...".to_string())forfee_amountandfee_mintmatches the newSwapInfosignature and keeps the Jupiter quote mocks in sync with the production type. If you don’t already have coverage elsewhere, consider adding at least one test path where these fields areNoneto exercise the optional case end‑to‑end.Also applies to: 703-706
src/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rs (1)
454-457: Tests aligned with optional SwapInfo fee metadataAll updated mocks now construct
SwapInfo { fee_amount: Some(...), fee_mint: Some(...) }, which is consistent with the new optional fields and keeps sign‑and‑send flows compiling and semantically correct. As with the transfer tests, you may want a negative/Nonecase somewhere in the suite if the runtime logic branches on missing fee metadata.Also applies to: 630-633, 809-812
docs/configuration/index.mdx (1)
68-70: Provider health configuration docs are clear and consistentThe new
PROVIDER_FAILURE_THRESHOLD,PROVIDER_PAUSE_DURATION_SECS, andPROVIDER_FAILURE_EXPIRATION_SECSentries are documented coherently in the table, sample.env, and the “Provider Health Management” section, and the behavior description (failure window, pausing, recovery, and fallback) matches the variable semantics. As a small polish, consider explicitly stating whether recovery on failure expiration can happen beforePROVIDER_PAUSE_DURATION_SECSelapses, to mirror the exact implementation and avoid ambiguity.Also applies to: 105-107, 308-323
src/config/config_file/network/collection.rs (1)
832-875: New mixed-type networks deserialization test looks good; tighten variant checksThe updated
test_deserialize_networks_arraynicely exercises array-based deserialization for both EVM and Solana, includingchain_id,rpc_urls,average_blocktime_ms, andis_testnet. To make this test more robust against accidental type regressions, consider turning theif letblocks intomatches (or adding anelse { panic!(...) }) so a non‑EVM/Non‑Solana variant can’t silently skip the assertions.Example tightening for the EVM assertion
- let evm_network = config - .get_network(ConfigFileNetworkType::Evm, "testnet") - .unwrap(); - if let NetworkFileConfig::Evm(evm_config) = evm_network { - assert_eq!(evm_config.chain_id, Some(11155111)); - assert!(evm_config.common.rpc_urls.is_some()); - assert_eq!(evm_config.common.rpc_urls.as_ref().unwrap().len(), 2); - } + let evm_network = config + .get_network(ConfigFileNetworkType::Evm, "testnet") + .unwrap(); + match evm_network { + NetworkFileConfig::Evm(evm_config) => { + assert_eq!(evm_config.chain_id, Some(11155111)); + assert!(evm_config.common.rpc_urls.is_some()); + assert_eq!(evm_config.common.rpc_urls.as_ref().unwrap().len(), 2); + } + other => panic!("Expected Evm network config, got {:?}", other), + }src/models/network/evm/network.rs (1)
6-7: EvmNetwork RpcConfig migration and tests look correct
rpc_urls: Vec<RpcConfig>pluspublic_rpc_urls(&[RpcConfig])matches the new shared RPC model and the provider trait helpers.- Tests building networks and configs with
RpcConfig::new(...)correctly exercise the updated types and inheritance behavior.If you prefer, you could pull
use crate::models::RpcConfig;up to the top of the tests module instead of scattering function‑local imports, but that’s purely stylistic.Also applies to: 14-16, 160-166, 176-191, 284-315
src/domain/relayer/evm/evm_relayer.rs (1)
59-61: RPC error sanitization and logging look goodForwarding only
(code, message)frommap_provider_errorplus asanitize_error_description-derived description while logging the fullProviderErrorviatracing::error!cleanly separates internal detail from the external JSON‑RPC response. This aligns well with the stated RPC‑sanitization goals, and the plumbing throughcreate_error_responseis correct.Also applies to: 492-508
src/domain/relayer/evm/rpc_utils.rs (1)
12-15: Error‑mapping re‑export and integration tests are consistentRe‑exporting
map_provider_error/sanitize_error_descriptionfromrpc_utilswhile delegating tocrate::utilsis a clean way to avoid duplication and preserve older import paths. The integration tests that buildProviderErrorvariants and assert the resulting JSON‑RPC error codes align with the shared implementation insrc/utils/error_sanitization.rs, so the wiring looks correct.Also applies to: 72-75, 403-483
src/domain/relayer/solana/solana_relayer.rs (1)
836-848: Make insufficient‑funds token‑swap trigger best‑effortOn
SolanaRpcError::InsufficientFunds,check_balance_and_trigger_token_swap_if_needed().await?means any failure in scheduling the token‑swap job bubbles up as aRelayerErrorinstead of returning the mapped JSON‑RPCINSUFFICIENT_FUNDSerror.Consider logging failures from
check_balance_and_trigger_token_swap_if_neededand still returning the JSON‑RPC error so clients always see a consistent insufficiency error, regardless of background job health.src/utils/error_sanitization.rs (1)
40-95: Centralized provider error mapping and sanitization look solidThe mapping and sanitization functions are consistent and well‑covered by tests; the wildcard handling of Solana/selector/transport errors to INTERNAL_ERROR is reasonable for now.
If you add more
ProviderErrorvariants later, it may be worth makingmap_provider_errorexhaustive (no_arm) so the compiler forces you to decide on explicit codes per variant.src/api/routes/network.rs (1)
40-46: Align route‑registration comment with actual orderThe comment says “Register routes with literal segments before routes with path parameters”, but
initcurrently registers the parameterized handlers beforelist_networks. Behavior is fine, but either reorder thecfg.servicecalls or update the comment to avoid confusion for future readers.src/config/config_file/network/common.rs (1)
17-56: RpcConfig deserialization and merge semantics look correctThe custom
deserialize_rpc_urls, the switch toOption<Vec<RpcConfig>>, andmerge_optional_rpc_config_vecsall align with the documented behavior (simple + extended formats, child‑overrides semantics), and the tests thoroughly exercise validation, serialization round‑trips, and parent/child merge behavior.You now have essentially the same
deserialize_rpc_urlslogic in both this module andsrc/models/network/request.rs. If this evolves (e.g., more fields on RpcConfig), consider extracting a shared helper to keep behavior in sync.Also applies to: 80-108, 186-195, 813-985
src/models/network/response.rs (1)
13-113: NetworkResponse mapping from NetworkRepoModel is consistentThe flattening of common fields plus EVM/Stellar‑specific additions in
From<NetworkRepoModel> for NetworkResponsematches the underlying configs, and the EVM tests verify both type‑specific and common fields.Consider adding small tests for Solana and Stellar models to catch regressions in those branches of the
matchas fields evolve.Also applies to: 115-166
src/models/network/request.rs (1)
19-80: Flexible rpc_urls request model and validation look goodThe untagged
RpcUrlEntryfor schema purposes, the customdeserialize_rpc_urls(strings, objects, and mixed), andUpdateNetworkRequest::validatetogether give a nicely flexible yet validated RPC URL update surface; the tests exercise the key shapes and error cases.You might eventually want to (a) share the deserializer with the config/common module to avoid drift, and (b) lean on
DEFAULT_RPC_WEIGHT(via RpcConfig or constants) in tests instead of hard‑coding100u8, so a future default change doesn’t require updating multiple places.Also applies to: 82-130, 132-248
src/services/provider/evm/mod.rs (1)
201-226: New provider initialization logging is fine; consider structured fields
initialize_providernow logs the URL being initialized. To align with structured tracing patterns elsewhere, you might prefer something likeinfo!(%url, "initializing EVM provider");instead of interpolating into the message string, but behavior is otherwise correct.src/models/relayer/rpc_config.rs (1)
112-148: URL validation helpers and tests are solid; minor doc-type mismatch only.The
validate_url_scheme/validate_listhelpers and their tests cover the expected HTTP/HTTPS vs. invalid-scheme cases well, and the switch toeyre::Reportintegrates with the rest of the error stack.Minor nit: the
# Returnsdoc comment onvalidate_liststill saysResult<()>while the signature isResult<(), eyre::Report>. Consider updating or omitting the concrete type in the docs to avoid confusion.Also applies to: 205-287
src/services/provider/stellar/mod.rs (2)
274-331: Exposing RPC configs via get_configs is consistent; cloning is acceptableAdding
fn get_configs(&self) -> Vec<RpcConfig>toStellarProviderTraitand delegating to the inherentStellarProvider::get_configscleanly surfaces the selector’s current config set. Returning an ownedVec<RpcConfig>is reasonable here given the small list size and avoids lifetime complexity.If you ever need cheaper read-only access in hot paths, consider an additional method returning a slice (
&[RpcConfig]) on the concrete provider and using that where cloning isn’t needed.Also applies to: 373-380, 502-507
1005-1008: Test helpers for ProviderConfig are correct but duplicatedBoth
create_test_provider_confighelpers buildProviderConfig::new(configs, timeout, 3, 60, 60)and the updated tests exercise the new constructor paths and error cases correctly.To avoid test-only duplication, you could expose a single helper (e.g. top-level in this module) and reuse it in
concrete_testsviasuper::create_test_provider_config.Also applies to: 1294-1297, 1013-1097, 1081-1097
src/config/server_config.rs (2)
31-80: Provider health config fields and env wiring are consistentThe new
provider_failure_threshold,provider_pause_duration_secs, andprovider_failure_expiration_secsfields are correctly:
- Documented in
from_envdefaults.- Populated via the new getters.
- Exposed as plain public fields for downstream use in
ProviderConfig::from_server_config.The getters’ behavior (env → legacy env → DEFAULT_* with parse fallback) matches the pattern used elsewhere in this module.
Given these values drive RpcHealthStore behavior, you may want to document or clamp pathological cases (e.g., threshold=0 or pause/expiration=0) if those are not intended to be supported configurations.
Also applies to: 100-106, 124-132, 282-312
399-407: Consider extending tests to cover new provider health gettersThe existing tests validate:
- Default values for
provider_failure_thresholdandprovider_pause_duration_secs.from_envequivalence with individual getters (for pre-existing fields).They don’t yet:
- Assert the default for
provider_failure_expiration_secs.- Exercise
get_provider_failure_threshold,get_provider_pause_duration_secs, andget_provider_failure_expiration_secsdirectly or with custom/legacy env vars.Adding a small set of assertions for these getters (including precedence of legacy
RPC_*variables) would lock in the new behavior and guard future refactors.Also applies to: 426-428, 555-608, 712-791
src/services/provider/mod.rs (1)
22-99: ProviderConfig and get_network_provider composition look correct
ProviderConfigcleanly encapsulates RPC configs, timeouts, and health parameters, and:
from_server_config/from_envcorrectly translateServerConfigfields (including ms→s for timeouts and the new provider_* health fields).get_network_providerselects custom vs public RPC configs as intended, errors when none are available, and then constructs aProviderConfigviafrom_envfor all networks before delegating toN::new_provider.Once the
public_rpc_urlsrecursion is fixed, this centralization should make adding future provider knobs straightforward.If you ever need sub-second timeouts, consider switching
ProviderConfig::timeout_secondsto a Duration or carrying both ms and s explicitly to avoid the integer division truncation infrom_server_config.Also applies to: 251-261, 327-346
src/services/provider/rpc_health_store.rs (1)
13-247: RpcHealthStore design and semantics look solidThe new health store:
- Uses a global
Lazy<RpcHealthStore>withArc<RwLock<...>>for safe shared state.- Correctly prunes stale failures on both
mark_failedandis_paused, caps history atthreshold * 2, and updatespaused_untilwith clear logging for first-pause vs extension.- Cleans up metadata entries when pauses expire and no recent failures remain, avoiding unbounded map growth.
- Is thoroughly exercised by tests, including stale vs recent failure mixes, pause extension, and shared-instance behavior.
The only slightly unusual edge case is
threshold == 0, which effectively means “pause on first failure” with zero retained timestamps; that’s consistent but worth documenting if users might accidentally configure it.Also applies to: 249-705
src/services/provider/solana/mod.rs (1)
1025-1036: Test updates for ProviderConfig usage are consistentThe new
create_test_provider_confighelper and the refactored tests that constructSolanaProvidervia:SolanaProvider::new(create_test_provider_config(configs, timeout))or
SolanaProvider::new_with_commitment_and_health(configs, timeout, commitment, 3, 60, 60)are aligned with the new configuration model and correctly assert:
- Success for valid configs (single/multiple URLs).
ProviderError::NetworkConfigurationfor empty or invalid config lists.This keeps Solana tests in sync with the EVM/Stellar provider patterns.
You could reuse
create_test_provider_configin all local tests (including commitment-specific ones) to avoid hard-coding the health parameters in multiple places.Also applies to: 1037-1111, 1140-1154
src/services/provider/rpc_selector.rs (3)
22-22: Import additional tracing macros used in the code.The code uses
tracing::warn!andtracing::debug!(e.g., line 337), but onlyinfois imported. This requires fully-qualified paths elsewhere.🔎 Suggested import fix
-use tracing::info; +use tracing::{debug, info, warn};
167-167: Use more appropriate log level for failures.Marking a provider as failed is a significant event that affects availability. Consider using
warn!instead ofinfo!to ensure it's visible in production logs at standard levels.🔎 Proposed change
- info!("Marking current provider as failed"); + warn!("Marking current provider as failed");
426-429: Consider preserving error details in client initialization.Line 428 converts the error using
e.to_string(), which loses the underlying error type and source chain. For debugging, consider preserving the full error context.💡 Alternative error handling
- initializer(&url).map_err(|e| RpcSelectorError::ClientInitializationError(e.to_string())) + initializer(&url).map_err(|e| { + RpcSelectorError::ClientInitializationError(format!("{:#}", e)) + })Using
{:#}preserves the error chain, providing more context for debugging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
docs/configuration/index.mdxopenapi.jsonsrc/api/controllers/mod.rssrc/api/controllers/network.rssrc/api/controllers/relayer.rssrc/api/routes/docs/mod.rssrc/api/routes/docs/network_docs.rssrc/api/routes/mod.rssrc/api/routes/network.rssrc/api/routes/relayer.rssrc/config/config_file/mod.rssrc/config/config_file/network/collection.rssrc/config/config_file/network/common.rssrc/config/config_file/network/evm.rssrc/config/config_file/network/inheritance.rssrc/config/config_file/network/mod.rssrc/config/config_file/network/test_utils.rssrc/config/server_config.rssrc/constants/relayer.rssrc/domain/relayer/evm/evm_relayer.rssrc/domain/relayer/evm/rpc_utils.rssrc/domain/relayer/solana/dex/jupiter_swap.rssrc/domain/relayer/solana/dex/jupiter_ultra.rssrc/domain/relayer/solana/rpc/methods/fee_estimate.rssrc/domain/relayer/solana/rpc/methods/prepare_transaction.rssrc/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rssrc/domain/relayer/solana/rpc/methods/sign_transaction.rssrc/domain/relayer/solana/rpc/methods/transfer_transaction.rssrc/domain/relayer/solana/rpc/methods/utils.rssrc/domain/relayer/solana/solana_relayer.rssrc/domain/relayer/stellar/gas_abstraction.rssrc/domain/relayer/stellar/stellar_relayer.rssrc/domain/relayer/stellar/token_swap.rssrc/domain/transaction/evm/evm_transaction.rssrc/domain/transaction/evm/price_calculator.rssrc/domain/transaction/evm/status.rssrc/domain/transaction/evm/utils.rssrc/domain/transaction/stellar/test_helpers.rssrc/models/network/evm/network.rssrc/models/network/mod.rssrc/models/network/repository.rssrc/models/network/request.rssrc/models/network/response.rssrc/models/network/solana/network.rssrc/models/network/stellar/network.rssrc/models/relayer/mod.rssrc/models/relayer/rpc_config.rssrc/models/transaction/repository.rssrc/openapi.rssrc/repositories/network/network_in_memory.rssrc/repositories/network/network_redis.rssrc/repositories/relayer/mod.rssrc/services/gas/cache.rssrc/services/gas/evm_gas_price.rssrc/services/gas/fetchers/default.rssrc/services/gas/fetchers/mod.rssrc/services/gas/fetchers/polygon_zkevm.rssrc/services/gas/price_params_handler.rssrc/services/jupiter/mod.rssrc/services/provider/evm/mod.rssrc/services/provider/mod.rssrc/services/provider/retry.rssrc/services/provider/rpc_health_store.rssrc/services/provider/rpc_selector.rssrc/services/provider/solana/mod.rssrc/services/provider/stellar/mod.rssrc/utils/error_sanitization.rssrc/utils/mocks.rssrc/utils/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust_standards.mdc)
**/*.rs: Follow official Rust style guidelines using rustfmt (edition = 2021, max_width = 100)
Follow Rust naming conventions: snake_case for functions/variables, PascalCase for types, SCREAMING_SNAKE_CASE for constants and statics
Order imports alphabetically and group by: std, external crates, local crates
Include relevant doc comments (///) on public functions, structs, and modules. Use comments for 'why', not 'what'. Avoid redundant doc comments
Document lifetime parameters when they're not obvious
Avoid unsafe code unless absolutely necessary; justify its use in comments
Write idiomatic Rust: Prefer Result over panic, use ? operator for error propagation
Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Prefer header imports over function-level imports of dependencies
Prefer borrowing over cloning when possible
Use &str instead of &String for function parameters
Use &[T] instead of &Vec for function parameters
Avoid unnecessary .clone() calls
Use Vec::with_capacity() when size is known in advance
Use explicit lifetimes only when necessary; infer where possible
Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Streams: Use futures::StreamExt for processing streams efficiently
Always use serde for JSON serialization/deserialization with #[derive(Serialize, Deserialize)]
Use type aliases for complex types to improve readability
Implement common traits (Debug, Clone, PartialEq) where appropriate, using derive macros when possible
Implement Display for user-facing types
Prefer defining traits when implementing services to make mocking and testing easier
For tests, prefer existing utils for creating mocks instead of duplicating code
Use assert_eq! and assert! macros appropriately
Keep tests minimal, deterministic, and fast
Use tracing for structured logging, e.g., tracing::info! for request and error logs
When optimizing, prefer clarity first, then performance
M...
Files:
src/api/routes/docs/mod.rssrc/api/controllers/mod.rssrc/domain/transaction/evm/price_calculator.rssrc/constants/relayer.rssrc/domain/relayer/solana/rpc/methods/sign_transaction.rssrc/domain/transaction/evm/evm_transaction.rssrc/config/config_file/network/collection.rssrc/domain/relayer/solana/dex/jupiter_ultra.rssrc/utils/mod.rssrc/domain/relayer/solana/rpc/methods/prepare_transaction.rssrc/services/gas/fetchers/default.rssrc/models/relayer/mod.rssrc/api/controllers/network.rssrc/domain/transaction/stellar/test_helpers.rssrc/models/network/response.rssrc/models/network/mod.rssrc/services/gas/fetchers/polygon_zkevm.rssrc/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rssrc/models/network/stellar/network.rssrc/utils/error_sanitization.rssrc/domain/relayer/stellar/stellar_relayer.rssrc/config/config_file/network/mod.rssrc/services/jupiter/mod.rssrc/api/routes/relayer.rssrc/openapi.rssrc/config/config_file/network/evm.rssrc/domain/relayer/solana/dex/jupiter_swap.rssrc/config/config_file/network/test_utils.rssrc/api/routes/network.rssrc/api/routes/docs/network_docs.rssrc/config/server_config.rssrc/services/gas/cache.rssrc/domain/relayer/evm/rpc_utils.rssrc/domain/relayer/solana/rpc/methods/utils.rssrc/services/provider/retry.rssrc/domain/relayer/solana/rpc/methods/transfer_transaction.rssrc/domain/transaction/evm/utils.rssrc/repositories/network/network_redis.rssrc/api/controllers/relayer.rssrc/services/provider/mod.rssrc/domain/relayer/solana/solana_relayer.rssrc/models/relayer/rpc_config.rssrc/models/network/request.rssrc/config/config_file/network/common.rssrc/domain/transaction/evm/status.rssrc/models/network/repository.rssrc/services/provider/solana/mod.rssrc/services/gas/evm_gas_price.rssrc/services/provider/rpc_health_store.rssrc/config/config_file/network/inheritance.rssrc/domain/relayer/solana/rpc/methods/fee_estimate.rssrc/repositories/relayer/mod.rssrc/domain/relayer/evm/evm_relayer.rssrc/config/config_file/mod.rssrc/models/network/evm/network.rssrc/services/gas/price_params_handler.rssrc/api/routes/mod.rssrc/services/provider/evm/mod.rssrc/services/provider/stellar/mod.rssrc/services/provider/rpc_selector.rssrc/models/transaction/repository.rssrc/domain/relayer/stellar/gas_abstraction.rssrc/services/gas/fetchers/mod.rssrc/repositories/network/network_in_memory.rssrc/domain/relayer/stellar/token_swap.rssrc/models/network/solana/network.rssrc/utils/mocks.rs
🧠 Learnings (8)
📚 Learning: 2025-08-19T14:31:09.686Z
Learnt from: LuisUrrutia
Repo: OpenZeppelin/openzeppelin-relayer PR: 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/config/config_file/network/collection.rssrc/config/config_file/network/test_utils.rssrc/services/gas/price_params_handler.rs
📚 Learning: 2025-08-19T10:41:31.051Z
Learnt from: LuisUrrutia
Repo: OpenZeppelin/openzeppelin-relayer PR: 428
File: config/networks/bsc.json:30-32
Timestamp: 2025-08-19T10:41:31.051Z
Learning: The network inheritance mechanism in OpenZeppelin Relayer configs correctly resolves `from` references within the appropriate scope, preventing false positive collisions between networks with similar names across different blockchain types.
Applied to files:
src/config/config_file/network/collection.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : For tests, prefer existing utils for creating mocks instead of duplicating code
Applied to files:
src/utils/mod.rssrc/domain/transaction/stellar/test_helpers.rssrc/services/provider/retry.rssrc/repositories/relayer/mod.rssrc/domain/relayer/evm/evm_relayer.rssrc/utils/mocks.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Applied to files:
src/utils/mod.rssrc/utils/error_sanitization.rssrc/domain/relayer/evm/rpc_utils.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Prefer defining traits when implementing services to make mocking and testing easier
Applied to files:
src/domain/transaction/stellar/test_helpers.rssrc/services/provider/retry.rssrc/domain/relayer/solana/solana_relayer.rssrc/repositories/relayer/mod.rssrc/domain/relayer/evm/evm_relayer.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Keep tests minimal, deterministic, and fast
Applied to files:
src/services/provider/retry.rssrc/repositories/relayer/mod.rssrc/domain/relayer/evm/evm_relayer.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Use assert_eq! and assert! macros appropriately
Applied to files:
src/models/network/repository.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Use tracing for structured logging, e.g., tracing::info! for request and error logs
Applied to files:
src/services/provider/evm/mod.rs
🧬 Code graph analysis (29)
src/domain/transaction/evm/price_calculator.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/config/config_file/network/collection.rs (4)
src/models/network/repository.rs (3)
config(136-138)common(24-30)common(131-133)src/config/config_file/network/mod.rs (1)
is_testnet(100-106)src/models/network/evm/network.rs (1)
is_testnet(135-137)src/models/network/solana/network.rs (1)
is_testnet(83-85)
src/api/controllers/network.rs (5)
src/models/network/repository.rs (1)
config(136-138)src/api/routes/network.rs (3)
list_networks(13-18)get_network(22-27)update_network(32-38)src/config/config_file/network/collection.rs (1)
networks(138-142)src/models/api_response.rs (2)
paginated(77-85)success(47-55)src/models/network/stellar/network.rs (1)
network_id(83-87)
src/models/network/response.rs (2)
src/models/network/repository.rs (2)
config(136-138)new_evm(71-80)src/models/network/evm/network.rs (1)
required_confirmations(140-142)
src/models/network/stellar/network.rs (3)
src/models/network/evm/network.rs (1)
public_rpc_urls(160-166)src/models/network/solana/network.rs (1)
public_rpc_urls(71-77)src/services/provider/mod.rs (4)
public_rpc_urls(254-254)public_rpc_urls(266-270)public_rpc_urls(280-284)public_rpc_urls(294-298)
src/domain/relayer/stellar/stellar_relayer.rs (2)
src/utils/error_sanitization.rs (2)
map_provider_error(40-56)sanitize_error_description(71-96)src/domain/relayer/evm/rpc_utils.rs (1)
create_error_response(31-47)
src/config/config_file/network/mod.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/openapi.rs (1)
src/api/routes/docs/network_docs.rs (3)
doc_list_networks(69-69)doc_get_network(132-132)doc_update_network(197-197)
src/api/routes/network.rs (2)
src/api/controllers/network.rs (3)
list_networks(33-61)get_network(73-102)update_network(116-201)src/models/network/stellar/network.rs (1)
network_id(83-87)
src/config/server_config.rs (1)
src/models/network/repository.rs (1)
config(136-138)
src/domain/relayer/evm/rpc_utils.rs (1)
src/utils/error_sanitization.rs (2)
map_provider_error(40-56)sanitize_error_description(71-96)
src/services/provider/retry.rs (1)
src/services/provider/rpc_selector.rs (3)
new(79-111)new_with_defaults(123-130)available_provider_count(144-152)
src/services/provider/mod.rs (4)
src/services/provider/evm/mod.rs (2)
new(163-191)new(545-555)src/services/provider/stellar/mod.rs (2)
new(335-371)new(845-860)src/config/server_config.rs (1)
from_env(106-132)src/models/network/evm/network.rs (1)
public_rpc_urls(160-166)
src/domain/relayer/solana/solana_relayer.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/models/relayer/rpc_config.rs (1)
src/config/config_file/network/common.rs (1)
deserialize(81-107)
src/models/network/request.rs (1)
src/models/relayer/rpc_config.rs (1)
validate_list(142-148)
src/config/config_file/network/common.rs (1)
src/models/relayer/rpc_config.rs (2)
deserialize(54-69)with_weight(96-101)
src/models/network/repository.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/services/provider/solana/mod.rs (2)
src/services/provider/rpc_selector.rs (1)
get_configs(158-160)src/services/provider/mod.rs (1)
new(54-68)
src/repositories/relayer/mod.rs (1)
src/repositories/mod.rs (4)
create(59-59)list_all(61-61)update(66-66)delete_by_id(67-67)
src/domain/relayer/evm/evm_relayer.rs (2)
src/domain/relayer/evm/rpc_utils.rs (2)
create_error_response(31-47)create_success_response(60-70)src/utils/error_sanitization.rs (2)
map_provider_error(40-56)sanitize_error_description(71-96)
src/models/network/evm/network.rs (5)
src/models/network/solana/network.rs (1)
public_rpc_urls(71-77)src/models/network/stellar/network.rs (1)
public_rpc_urls(93-99)src/services/provider/mod.rs (5)
public_rpc_urls(254-254)public_rpc_urls(266-270)public_rpc_urls(280-284)public_rpc_urls(294-298)new(54-68)src/models/relayer/rpc_config.rs (1)
new(78-83)src/domain/transaction/evm/utils.rs (3)
None(298-298)None(330-330)None(362-362)
src/services/gas/price_params_handler.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/api/routes/mod.rs (8)
src/api/routes/network.rs (1)
init(41-46)src/api/routes/relayer.rs (1)
init(219-243)src/api/routes/plugin.rs (1)
init(49-53)src/api/routes/metrics.rs (1)
init(124-128)src/api/routes/api_keys.rs (1)
init(44-50)src/api/routes/notification.rs (1)
init(61-67)src/api/routes/signer.rs (1)
init(58-64)src/api/routes/health.rs (1)
init(29-31)
src/services/provider/evm/mod.rs (2)
src/services/provider/rpc_selector.rs (1)
get_configs(158-160)src/services/provider/mod.rs (2)
new(54-68)from_env(95-98)
src/services/provider/stellar/mod.rs (2)
src/services/provider/rpc_selector.rs (1)
get_configs(158-160)src/services/provider/mod.rs (2)
new(54-68)from_env(95-98)
src/services/provider/rpc_selector.rs (1)
src/services/provider/rpc_health_store.rs (2)
instance(38-40)is_paused(188-238)
src/domain/relayer/stellar/token_swap.rs (1)
src/services/provider/mod.rs (1)
new(54-68)
src/models/network/solana/network.rs (3)
src/models/network/evm/network.rs (1)
public_rpc_urls(160-166)src/models/network/stellar/network.rs (1)
public_rpc_urls(93-99)src/services/provider/mod.rs (4)
public_rpc_urls(254-254)public_rpc_urls(266-270)public_rpc_urls(280-284)public_rpc_urls(294-298)
⏰ 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). (10)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: msrv
- GitHub Check: Unit Tests
- GitHub Check: clippy
- GitHub Check: Properties Tests
- 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
| } | ||
|
|
||
| #[test] | ||
| fn test_deserialize_with_inheritance_resolution() { |
There was a problem hiding this comment.
Why this test was removed?
There was a problem hiding this comment.
@LuisUrrutia
Main reason for removing/changing is because i have removed inheritance part for rpc urls.
I think we do not need inheritance from parent for rpc urls.
Example: sepolia inherits mainnet. rpc urls are different and they should not be inherited.
There was a problem hiding this comment.
merging PR now but we can address in a new one if something is needed.
NicoMolinaOZ
left a comment
NicoMolinaOZ
left a comment
There was a problem hiding this comment.
LGTM, just minor comments. Let me know if you need a testing round for this RPC logic, thanks!
Never enough testing. Definitely if you find some time try to test it. Thanks. |
4 participants
Summary
This PR will:
SDK PR: OpenZeppelin/openzeppelin-relayer-sdk#242
Testing Process
Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.