Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7009 +/- ##
=====================================
Coverage 0.00% 0
=====================================
Files 1 0 -1
Lines 14 0 -14
=====================================
+ Misses 14 0 -14
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughShifts Starknet code to a provider-first model (injecting Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Settings as Settings(chains.rs)
participant Metrics as PrometheusClientMetrics
participant Provider as StarknetProvider
participant Transport as FallbackHttpTransport
participant MetricProv as MetricProvider
participant JsonRPC as JsonRpcClient
participant Component as StarknetComponent
Settings->>Metrics: prepare metrics config
Settings->>Provider: build_starknet_provider(domain, conf, Metrics, &locator)
Provider->>Transport: FallbackHttpTransport::new(urls, Metrics, chain)
loop per URL
Transport->>MetricProv: MetricProvider::new(url, Metrics, cfg)
end
Provider->>JsonRPC: construct JsonRpcClient with Transport
Settings->>Component: Component::new(Provider, conf, locator, ...)
Component->>Provider: rpc_client().clone()
Note over Component,Provider: Contract readers use JsonProvider clone
sequenceDiagram
autonumber
participant Reader as ContractReader
participant Provider as StarknetProvider
participant JsonRPC as JsonRpcClient
participant MetricProv as MetricProvider
participant Prom as Prometheus
Reader->>Provider: rpc_client().clone()
Provider->>JsonRPC: use JsonRpcClient
JsonRPC->>MetricProv: send_request(method, params)
MetricProv->>Prom: record start/time & method
MetricProv->>JsonRPC: dispatch via HTTP transport
JsonRPC-->>MetricProv: response / error
MetricProv->>Prom: record success/failure
MetricProv-->>Reader: return JsonRpcResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
rust/main/chains/hyperlane-starknet/src/provider/metric.rs (2)
79-87: Consider handling JsonRpcResponse success/failure for finer metrics.While the current implementation tracks HTTP-level success, you might want to inspect the actual RPC response to distinguish between successful responses and RPC errors. The transport layer might return OK even when the RPC call failed at the application level.
You could enhance the metrics by checking if the response is
JsonRpcResponse::SuccessorJsonRpcResponse::Error:let result = self.client.send_request(method, params_json).await; let method_string = serde_json::to_string(&method).map_err(Self::Error::Json)?; +let is_success = result.as_ref().map(|r| matches!(r, JsonRpcResponse::Success(_))).unwrap_or(false); self.metrics - .increment_metrics(&self.metrics_config, &method_string, start, result.is_ok()); + .increment_metrics(&self.metrics_config, &method_string, start, is_success);
96-117: Batch request metrics handled properly.The implementation extracts method names from each request and logs metrics for all of them. This gives you per-method visibility even in batch operations, which is quite useful for debugging and monitoring.
🧹 Nitpick comments (14)
rust/main/chains/hyperlane-starknet/src/validator_announce.rs (1)
172-177: Return None when no funds are neededIf balance covers
overall_fee, returningSome(0)is a bit soggy. PreferNoneto signal “no top‑up required.”Apply:
- Some( - estimate - .overall_fee - .saturating_sub(balance.as_u128()) - .into(), - ) + { + let missing = estimate.overall_fee.saturating_sub(balance.as_u128()); + if missing == 0 { None } else { Some(missing.into()) } + }rust/main/chains/hyperlane-starknet/src/interchain_gas.rs (1)
21-26: Avoid needless clone of connYou take
connby value and then clone it. Just move it in.Apply:
- pub fn new(provider: StarknetProvider, conn: ConnectionConf) -> ChainResult<Self> { - Ok(Self { - provider, - conn: conn.clone(), - }) - } + pub fn new(provider: StarknetProvider, conn: ConnectionConf) -> ChainResult<Self> { + Ok(Self { provider, conn }) + }rust/main/chains/hyperlane-starknet/src/indexer.rs (1)
253-261: Guard against reversed ranges (optional)Add a quick check for
start > endto fail fast instead of querying an empty slice of the swamp.Apply:
- if *range.start() > current_block || *range.end() > current_block { + if *range.start() > *range.end() || *range.start() > current_block || *range.end() > current_block { return Err(HyperlaneStarknetError::Other(format!( "range {:?} is not valid for current block {}", range, current_block )) .into()); }rust/main/chains/hyperlane-starknet/src/provider/client.rs (5)
8-8: Alias the metrics ChainInfo to avoid type shadowing with hyperlane_core::ChainInfoTwo different ChainInfo types live in this file; aliasing the metrics one keeps things tidy and reduces head‑scratching.
-use hyperlane_metric::prometheus_metric::{ChainInfo, PrometheusClientMetrics}; +use hyperlane_metric::prometheus_metric::{ChainInfo as MetricChainInfo, PrometheusClientMetrics};
34-45: Provider construction with metrics looks good; minor API polishWiring metrics and chain through the transport is clean. Consider accepting &PrometheusClientMetrics (cloning inside) to make the call‑site cheaper and nudge sharing, but not a blocker.
34-39: Follow‑up to aliasing: update the param type if you adopt the aliasKeeps signatures unambiguous for readers.
- chain: Option<ChainInfo>, + chain: Option<MetricChainInfo>,
80-94: Block fetch path LGTM; minor edge noteNumeric block ids shouldn’t return Pending, but if they ever do, mapping to InvalidBlock is a bit spiky. Optional: log at debug before erroring to aid swamp‑tracking during flaky RPCs.
195-203: Unify rpc_client accessor and error-conversion styleAll good — tidy up: get_chain_metrics (rust/main/chains/hyperlane-starknet/src/provider/client.rs, ~lines 195–203): replace self.rpc_client with self.rpc_client() and change .map_err(HyperlaneStarknetError::from) to .map_err(Into::::into) to match the rest of the file.
rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (4)
5-5: Tiny doc nit: “Clopying” → “Copying”Polish the comment; reads smoother than a mudslide.
55-66: Drop an unnecessary clone on metrics_configNo need to clone right before moving. Lighter footprints, fewer footprints in the swamp.
- let providers = urls.into_iter().map(|url| { - let metrics_config = - PrometheusConfig::from_url(&url, ClientConnectionType::Rpc, chain.clone()); - - MetricProvider::new(url, metrics.clone(), metrics_config.clone()) - }); + let providers = urls.into_iter().map(|url| { + let metrics_config = + PrometheusConfig::from_url(&url, ClientConnectionType::Rpc, chain.clone()); + MetricProvider::new(url, metrics.clone(), metrics_config) + });
89-129: Retry/backoff knobs: make constants or config; consider delegating to FallbackProviderHard‑coding 4 attempts and 100ms backoff makes tuning harder if RPC mud gets thick. Promote to constants at module scope (or a config), and, when feasible, reuse FallbackProvider::call for single‑request paths too so both codepaths share brains.
+// Retry controls for single-request path +const MAX_ATTEMPTS: usize = 4; +const RETRY_BACKOFF_MS: u64 = 100; @@ - // Try up to 4 times - for _ in 0..4 { + // Try up to MAX_ATTEMPTS times + for _ in 0..MAX_ATTEMPTS { if !errors.is_empty() { - sleep(Duration::from_millis(100)).await; + sleep(Duration::from_millis(RETRY_BACKOFF_MS)).await; }If/when trait bounds allow, mirror send_requests by using self.call(...) here to consolidate fallback behavior.
39-51: Embed provider index/URL in FallbackHttpTransportError::Errors for clearer post‑mortemsYou already log provider_index and fallback_count (rust/main/chains/hyperlane-starknet/src/provider/fallback.rs — debug around lines ~100–103; warn push around ~117–121; final Err at ~131). Make the error self‑describing by changing Errors(Vec) → Errors(Vec) where ProviderError contains provider_index and/or provider_url plus the HttpTransportError (minimal option: Errors(Vec<(usize, HttpTransportError)>)). Keeps current logs and gives breadcrumbs when logs aren't available — makes triage less swampy.
rust/main/hyperlane-base/src/settings/chains.rs (2)
841-849: Avoid ref‑of‑clone on locatorPassing &locator.clone() does extra work. Just borrow locator.
- h_starknet::StarknetValidatorAnnounce::new( - provider, - conf, - &locator.clone(), - signer, - ) + h_starknet::StarknetValidatorAnnounce::new( + provider, + conf, + &locator, + signer, + )
1360-1374: Starknet provider builder: small clone cleanup and param naming nit
- metrics.clone() is unnecessary right before move.
- Optional: rename local metrics to client_metrics to avoid shadowing/confusion with the CoreMetrics arg.
-fn build_starknet_provider( +fn build_starknet_provider( chain_conf: &ChainConf, connection_conf: &h_starknet::ConnectionConf, - metrics: &CoreMetrics, + metrics: &CoreMetrics, locator: &ContractLocator, ) -> ChainResult<StarknetProvider> { let middleware_metrics = chain_conf.metrics_conf(); - let metrics = metrics.client_metrics(); + let client_metrics = metrics.client_metrics(); Ok(StarknetProvider::new( locator.domain.clone(), connection_conf, - metrics.clone(), + client_metrics, middleware_metrics.chain.clone(), )) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/main/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
rust/main/chains/hyperlane-starknet/Cargo.toml(1 hunks)rust/main/chains/hyperlane-starknet/src/indexer.rs(3 hunks)rust/main/chains/hyperlane-starknet/src/interchain_gas.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/mailbox.rs(1 hunks)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/provider/client.rs(4 hunks)rust/main/chains/hyperlane-starknet/src/provider/fallback.rs(2 hunks)rust/main/chains/hyperlane-starknet/src/provider/metric.rs(1 hunks)rust/main/chains/hyperlane-starknet/src/provider/mod.rs(1 hunks)rust/main/chains/hyperlane-starknet/src/utils.rs(4 hunks)rust/main/chains/hyperlane-starknet/src/validator_announce.rs(1 hunks)rust/main/hyperlane-base/src/settings/chains.rs(14 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/chains/hyperlane-starknet/src/provider/mod.rsrust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rsrust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rsrust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rsrust/main/chains/hyperlane-starknet/src/mailbox.rsrust/main/chains/hyperlane-starknet/src/validator_announce.rsrust/main/chains/hyperlane-starknet/src/ism/routing_ism.rsrust/main/chains/hyperlane-starknet/src/provider/metric.rsrust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rsrust/main/chains/hyperlane-starknet/src/utils.rsrust/main/chains/hyperlane-starknet/src/provider/client.rsrust/main/chains/hyperlane-starknet/src/interchain_gas.rsrust/main/chains/hyperlane-starknet/src/indexer.rsrust/main/hyperlane-base/src/settings/chains.rsrust/main/chains/hyperlane-starknet/src/provider/fallback.rs
rust/main/{hyperlane-core,hyperlane-base}/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep shared Rust core crates in rust/main/{hyperlane-core,hyperlane-base}
Files:
rust/main/hyperlane-base/src/settings/chains.rs
🧠 Learnings (2)
📚 Learning: 2025-09-02T18:44:06.598Z
Learnt from: CR
PR: hyperlane-xyz/hyperlane-monorepo#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T18:44:06.598Z
Learning: Applies to rust/main/{hyperlane-core,hyperlane-base}/**/src/**/*.rs : Keep shared Rust core crates in rust/main/{hyperlane-core,hyperlane-base}
Applied to files:
rust/main/chains/hyperlane-starknet/Cargo.toml
📚 Learning: 2025-09-02T18:44:06.598Z
Learnt from: CR
PR: hyperlane-xyz/hyperlane-monorepo#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T18:44:06.598Z
Learning: Applies to rust/main/chains/{hyperlane-ethereum,hyperlane-cosmos,hyperlane-sealevel,hyperlane-fuel}/**/src/**/*.rs : Keep chain support implementations within rust/main/chains/{hyperlane-ethereum,hyperlane-cosmos,hyperlane-sealevel,hyperlane-fuel}
Applied to files:
rust/main/chains/hyperlane-starknet/Cargo.tomlrust/main/chains/hyperlane-starknet/src/interchain_gas.rs
🧬 Code graph analysis (13)
rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (8)
rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/interchain_gas.rs (2)
new(21-26)provider(34-36)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
new(28-42)provider(50-52)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
new(47-64)provider(90-92)
rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (7)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
new(34-54)provider(72-74)rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
new(28-42)provider(50-52)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
new(32-45)provider(53-55)
rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (7)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
new(34-54)provider(72-74)rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
new(28-42)provider(50-52)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
new(32-45)provider(53-55)
rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
provider(90-92)new(47-64)rust/main/chains/hyperlane-starknet/src/utils.rs (1)
build_single_owner_account(76-101)
rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
provider(75-77)new(40-57)rust/main/chains/hyperlane-starknet/src/utils.rs (1)
build_single_owner_account(76-101)
rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (8)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
new(34-54)provider(72-74)rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
new(32-45)provider(53-55)rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
new(47-64)provider(90-92)
rust/main/chains/hyperlane-starknet/src/provider/metric.rs (2)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (1)
new(34-54)rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (2)
new(55-69)send_request(77-132)
rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (6)
rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
new(28-42)provider(50-52)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
new(32-45)provider(53-55)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (1)
new(55-69)rust/main/chains/hyperlane-starknet/src/provider/metric.rs (1)
new(27-41)
rust/main/chains/hyperlane-starknet/src/interchain_gas.rs (9)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
new(34-54)provider(72-74)rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
new(31-45)new(139-153)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
new(29-43)provider(51-53)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
new(28-42)provider(50-52)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
new(40-57)provider(75-77)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
new(32-45)provider(53-55)rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
new(47-64)provider(90-92)
rust/main/chains/hyperlane-starknet/src/indexer.rs (2)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (2)
provider(72-74)new(34-54)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
provider(53-55)new(32-45)
rust/main/hyperlane-base/src/settings/chains.rs (8)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (1)
provider(72-74)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
provider(50-52)new(28-42)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
provider(75-77)new(40-57)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
provider(53-55)new(32-45)rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
provider(90-92)new(47-64)
rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (2)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (1)
new(34-54)rust/main/chains/hyperlane-starknet/src/provider/metric.rs (1)
new(27-41)
⏰ 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). (49)
- GitHub Check: infra-test
- GitHub Check: cli-evm-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-evm-e2e-matrix (warp-read)
- GitHub Check: cli-evm-e2e-matrix (core-init)
- GitHub Check: cli-evm-e2e-matrix (warp-send)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-2)
- GitHub Check: cli-evm-e2e-matrix (warp-check-2)
- GitHub Check: cli-evm-e2e-matrix (warp-deploy)
- GitHub Check: cli-evm-e2e-matrix (core-deploy)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-evm-e2e-matrix (warp-init)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-config)
- GitHub Check: cli-evm-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-2)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-submitters)
- GitHub Check: cli-evm-e2e-matrix (warp-check-3)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-1)
- GitHub Check: cli-evm-e2e-matrix (warp-check-1)
- GitHub Check: cli-evm-e2e-matrix (relay)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-1)
- GitHub Check: cli-evm-e2e-matrix (core-read)
- GitHub Check: cli-evm-e2e-matrix (core-check)
- GitHub Check: cli-cosmos-e2e-matrix (core-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (warp-read)
- GitHub Check: cli-evm-e2e-matrix (core-apply)
- GitHub Check: cli-cosmos-e2e-matrix (core-read)
- GitHub Check: cli-cosmos-e2e-matrix (warp-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (core-apply)
- GitHub Check: cli-cosmos-e2e-matrix (core-check)
- GitHub Check: env-test-matrix (mainnet3, inevm, core)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: env-test-matrix (mainnet3, inevm, igp)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: cli-install-test-run
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
🔇 Additional comments (35)
rust/main/chains/hyperlane-starknet/Cargo.toml (1)
31-31: LGTM! Dependency addition looks proper.The new
hyperlane-metricdependency follows the workspace pattern just like the other local dependencies. This'll enable the Prometheus metrics functionality throughout the Starknet provider implementation.rust/main/chains/hyperlane-starknet/src/utils.rs (2)
76-79: Nice refactor to dependency injection!Changed from building providers internally to accepting an injected
JsonProvider. This is a much cleaner approach - lets the caller control the provider lifecycle and configuration.
88-92: Good handling of the execution encoding.The logic properly switches between Legacy and New encoding based on the signer's type. This ensures compatibility with both Cairo 0 and newer account types.
rust/main/chains/hyperlane-starknet/src/provider/metric.rs (4)
17-23: Well-structured metric provider wrapper.The
MetricProviderstruct cleanly encapsulates the HTTP transport with metrics collection. Keeping the metrics and config separate from the client is a solid design choice.
44-56: Clone implementation correctly manages metrics.You're incrementing the provider instance count on clone, which is important for accurate tracking of active providers. The implementation maintains all the necessary state properly.
58-64: Drop implementation ensures proper cleanup.Decrementing the provider count in the destructor prevents metric leaks. This'll keep your Prometheus metrics accurate when providers are cleaned up.
122-132: BlockNumberGetter implementation looks good.The trait implementation properly wraps the JSON-RPC client and maps errors through the appropriate error types. This'll work well with the fallback provider setup.
rust/main/chains/hyperlane-starknet/src/provider/mod.rs (1)
3-3: Module organization is clean and follows conventions.Adding the metric module and re-exporting its contents keeps the provider functionality well-organized. This makes the
MetricProvideravailable throughout the crate without needing deep imports.Also applies to: 7-7
rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (1)
32-45: Constructor properly accepts injected provider.The refactor from building providers internally to accepting an injected
StarknetProvideris consistent with the broader changes across the codebase. The provider's RPC client is properly cloned for the contract reader initialization.rust/main/chains/hyperlane-starknet/src/validator_announce.rs (3)
48-53: Provider-first constructor: solid moveInjecting
StarknetProviderand deriving the account fromprovider.rpc_client()aligns with the rest of the crate’s pattern. Nice and tidy.
61-63: Store-and-expose provider is consistentKeeping the cloned provider and returning it via
HyperlaneChain::provider()matches sibling modules. All good.
163-167: Verify address format passed to get_balanceHeads-up: self.contract.account.address().to_string() may produce a decimal string; confirm provider.get_balance expects a 0x‑prefixed hex address — my earlier rg search returned no matches so I couldn't verify the signature.
Location: rust/main/chains/hyperlane-starknet/src/validator_announce.rs:163-167
Run in repo root to show get_balance signature / provider impl:
rg -n -C2 '\bget_balance\s*\(' -S -g '!**/target/**' rg -n -C2 'impl\s+StarknetProvider' -S -g '!**/target/**' rg -n -C2 'trait\s+StarknetProvider' -S -g '!**/target/**'If get_balance expects hex, adapt the call:
- .get_balance(self.contract.account.address().to_string()) + .get_balance(format!("{:#x}", self.contract.account.address()))If LowerHex isn't implemented, convert the address to bytes and hex-encode.
rust/main/chains/hyperlane-starknet/src/interchain_gas.rs (1)
6-8: Imports look rightNew traits/types wired cleanly. No fuss.
rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
41-47: Constructor refactor: clean and consistentUsing
provider.rpc_client()for account creation matches the new pattern. Nicely layered, like onions.
54-56: Provider plumbed throughStoring the injected provider keeps
HyperlaneChain::provider()trivial and consistent. All good.rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (3)
15-16: Import shuffle OKBringing in
JsonProvideris expected for reader construction.
29-37: Provider-first API matches siblingsDeriving
json_providerfromprovider.rpc_client()and storingproviderkeeps things uniform across ISMs.
40-42: Consistent state initStruct init mirrors the rest; no loose ends.
rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (3)
15-16: Import alignmentSwitch to
JsonProvideris as expected for the reader.
28-36: Constructor consistencyProvider injection +
json_providercloning fits the new pattern. Looks good.
39-41: Init tidyAll fields set cleanly; nothing muddy here.
rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (3)
15-16: Imports: OK
JsonProviderand helpers wired up fine.
29-37: Provider-first constructor: goodReader built from
json_provider, provider retained for traits — consistent with the swamp… err, crate.
40-42: Field init consistentNo footguns spotted.
rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (3)
15-16: Imports look right
JsonProvideradded appropriately.
29-37: Constructor matches the new patternUse of
provider.rpc_client().clone()for the reader and storingprovidermirrors siblings. Ship it.
40-42: Init is straightforwardNo redundant work here.
rust/main/chains/hyperlane-starknet/src/indexer.rs (4)
16-18: Imports updated correctlySwitch to
JsonProvider+StarknetProviderfits with provider-first indexing.
31-39: Mailbox indexer: provider-first wiring LGTMReader now uses
provider.rpc_client().clone(); constructor signature updated. All consistent.
140-147: MerkleTreeHook indexer: same good patternMirrors mailbox indexer changes; no surprises.
1-317: Run Clippy on changed crate (rust/main/chains/hyperlane-starknet)Sandbox couldn't run Clippy: "Unable to proceed. Could not locate working directory.: no /proc/self/exe available. Is /proc mounted?" — can't verify lints here. Run locally or in CI and fix warnings-as-errors:
cd rust/main/chains/hyperlane-starknet
cargo clippy -q --no-deps --all-features -- -D warningsFix reported warnings and push the results or attach the Clippy output.
rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (1)
134-156: Batch path uses FallbackProvider::call correctlyError mapping and boxing look tidy. Nice and layered.
rust/main/hyperlane-base/src/settings/chains.rs (3)
34-35: Importing StarknetProvider here is the right callKeeps construction centralized. No concerns.
312-314: Provider-first Starknet wiring — quick sweep passedFound provider-first constructor at rust/main/chains/hyperlane-starknet/src/interchain_gas.rs:21; no occurrences of old conf/locator-first calls detected.
1-1: Run cargo clippy on the touched crates and fix &*.clone() borrow nitsCouldn't run cargo clippy in the sandbox (no /proc). rg flagged these &*.clone() spots — prefer borrowing (&x) or cloning without the extra &:
- rust/main/hyperlane-base/src/settings/chains.rs:845 — &locator.clone()
- rust/main/agents/relayer/src/relayer.rs:313 — &task_monitor.clone()
- rust/main/agents/scraper/src/agent.rs:273 — &metrics.clone()
- rust/main/agents/scraper/src/agent.rs:274 — &contract_sync_metrics.clone()
- rust/main/agents/scraper/src/agent.rs:315 — &metrics.clone()
- rust/main/agents/scraper/src/agent.rs:316 — &contract_sync_metrics.clone()
- rust/main/agents/scraper/src/agent.rs:358 — &metrics.clone()
- rust/main/agents/scraper/src/agent.rs:359 — &contract_sync_metrics.clone()
- rust/main/hyperlane-base/src/cache/moka/mod.rs:142 — cache.set(&key.clone(), &value, ttl)
- rust/main/hyperlane-base/src/cache/moka/mod.rs:176 — cache.set(&key.clone(), &value, ttl)
- rust/main/hyperlane-base/src/cache/moka/mod.rs:201 — cache.set(&key.clone(), &value, ttl)
- rust/main/hyperlane-base/src/cache/moka/mod.rs:228 — cache.set(&key.clone(), &value, ttl)
Run locally: cargo clippy -p hyperlane-starknet -p hyperlane-base -D warnings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rust/main/hyperlane-base/src/settings/chains.rs (2)
847-856: Drop the needless clone on locator referencePassing
&locator.clone()allocates for nothing. Just pass&locator.- h_starknet::StarknetValidatorAnnounce::new( - provider, - conf, - &locator.clone(), - signer, - ) + h_starknet::StarknetValidatorAnnounce::new( + provider, + conf, + &locator, + signer, + )
1376-1390: Helper reads well; consider avoiding ident shadowing
let metrics = metrics.client_metrics();shadows the param; harmless but a bit muddy. Consider renaming toclient_metricsfor clarity.rust/main/chains/hyperlane-starknet/src/provider/client.rs (1)
8-8: Alias ChainInfo import to reduce type shadowingBoth metric and core define
ChainInfo. You already FQ the core one later; aliasing the metric one keeps the ogre-mind clear.-use hyperlane_metric::prometheus_metric::{ChainInfo, PrometheusClientMetrics}; +use hyperlane_metric::prometheus_metric::{ChainInfo as MetricChainInfo, PrometheusClientMetrics};And update the
newsignature param toOption<MetricChainInfo>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/main/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
rust/main/chains/hyperlane-starknet/src/mailbox.rs(1 hunks)rust/main/chains/hyperlane-starknet/src/provider/client.rs(4 hunks)rust/main/chains/hyperlane-starknet/src/validator_announce.rs(1 hunks)rust/main/hyperlane-base/src/settings/chains.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/main/chains/hyperlane-starknet/src/validator_announce.rs
🧰 Additional context used
📓 Path-based instructions (2)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/chains/hyperlane-starknet/src/mailbox.rsrust/main/chains/hyperlane-starknet/src/provider/client.rsrust/main/hyperlane-base/src/settings/chains.rs
rust/main/{hyperlane-core,hyperlane-base}/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep shared Rust core crates in rust/main/{hyperlane-core,hyperlane-base}
Files:
rust/main/hyperlane-base/src/settings/chains.rs
🧬 Code graph analysis (3)
rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
provider(90-92)new(47-64)rust/main/chains/hyperlane-starknet/src/utils.rs (1)
build_single_owner_account(76-101)
rust/main/chains/hyperlane-starknet/src/provider/client.rs (3)
rust/main/chains/hyperlane-starknet/src/provider/fallback.rs (1)
new(55-69)rust/main/chains/hyperlane-starknet/src/provider/metric.rs (1)
new(27-41)rust/main/chains/hyperlane-ethereum/src/rpc_clients/provider.rs (3)
domain(61-63)provider(65-70)get_chain_metrics(470-503)
rust/main/hyperlane-base/src/settings/chains.rs (7)
rust/main/chains/hyperlane-starknet/src/validator_announce.rs (2)
provider(90-92)new(47-64)rust/main/chains/hyperlane-starknet/src/mailbox.rs (2)
provider(75-77)new(40-57)rust/main/chains/hyperlane-starknet/src/ism/interchain_security_module.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/ism/aggregation_ism.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/ism/routing_ism.rs (2)
provider(50-52)new(28-42)rust/main/chains/hyperlane-starknet/src/ism/multisig_ism.rs (2)
provider(51-53)new(29-43)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
provider(53-55)new(32-45)
⏰ 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). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
🔇 Additional comments (14)
rust/main/hyperlane-base/src/settings/chains.rs (10)
312-314: Provider-first Starknet wiring looks cleanBuilds provider via helper and returns as HyperlaneProvider. Consistent with other chains.
371-374: Mailbox path: provider + signer flow is solidBuilding signer, then provider, then passing into provider-first ctor is spot on.
Also applies to: 372-374
429-431: Merkle tree hook: provider-first constructor is alignedNeat and tidy; matches the new pattern.
500-505: Message indexer: reorg period preservedProvider-first ctor with reorg period forwarded. All good.
Please confirm StarknetMailboxIndexer’s reorg semantics didn’t change with the ctor switch.
572-577: Delivery indexer: consistent migrationSame as message indexer; looks good.
922-925: ISM ctor change looks rightProvider-first and locator forwarded. Good.
977-981: Multisig ISM ctor migration LGTMMatches the new pattern across modules.
1026-1029: Routing ISM wiring is consistentProvider-first + locator. All good, donkey.
1078-1081: Aggregation ISM: clean transitionNo nits here.
637-642: No missing locator in Starknet IGP ctor
TheStarknetInterchainGasPaymaster::newsignature only takes(provider, conn: ConnectionConf), soconf.clone()is correct and no locator is needed.rust/main/chains/hyperlane-starknet/src/mailbox.rs (1)
41-47: Provider-first mailbox ctor is on pointUsing
build_single_owner_account(signer, provider.rpc_client())and storing the passed provider keeps things simple and consistent.Also applies to: 52-55
rust/main/chains/hyperlane-starknet/src/provider/client.rs (3)
34-45: Constructor neatly folds transport + metricsCreating the JsonRpcClient with the metrics-enabled fallback transport is clean and keeps concerns local to the provider.
81-94: Block lookup maps cleanly to BlockInfoGood use of MaybePendingBlockWithTxHashes and error mapping; no footguns spotted.
195-203: Chain metrics now real—niceFetching latest height and reusing get_block_by_height is straightforward. Returning None for base fee is sensible on Starknet.
2 participants
Description
Drive-by changes
Related issues
Backward compatibility
Testing
Summary by CodeRabbit
New Features
Refactor