apollo_l1_gas_price,apollo_l1_gas_price_config,apollo_l1_gas_price_types: add Chainlink client - #14942
Conversation
|
Artifacts upload workflows: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 08b8979. Configure here.
|
|
||
| debug!("Caching result for timestamp {timestamp}: {result:?}"); | ||
| cache.put(quantized_timestamp, result.clone()); | ||
| result |
There was a problem hiding this comment.
Resolved failure skips rate fallback
Medium Severity
When a background query finishes with an error, fetch_rate caches that failure and returns it immediately. The cached-failure path falls back to the previous bucket's rate, and the in-flight path does too, so the call that first observes the completed failure can turn a prior Ok into an Err for one proposal before later calls recover via the cache fallback.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 08b8979. Configure here.
…pes: add Chainlink client Adds `ChainlinkOracleClient`, an `ExchangeRateOracleClientTrait` implementation that reads Chainlink's on-chain Starknet price feeds through the sequencer's own batcher, to replace the Pragma HTTP API (now a gated commercial beta returning 401). Lands dark: `ChainlinkOracleConfig` is not attached to `L1GasPriceProviderConfig` and nothing constructs the client outside its own module, so `config_schema.json` is unchanged and there is no behavior change. A later PR adds the config enum that selects it per feed. Reads `latest_round_data()` on the feed proxies (never the aggregators, which rotate behind them) and derives both rates: - STRK/USD: the answer rescaled from 8 to 18 decimals, as USD per STRK. - ETH/STRK: `eth_usd / strk_usd` at 18 decimals, since every Chainlink feed on Starknet is USD-quoted and no direct pair exists. `fetch_rate` keeps the non-blocking spawn-and-cache shape of the HTTP client, so no batcher round trip lands on the proposal build or validate path. Failures are cached for the rest of the time bucket to bound retries, and still fall back to the previous bucket's rate so a transient feed error does not become a hard error for a whole interval. Guards, all rejecting rather than substituting so the existing degradation ladder handles the failure: - staleness on `updated_at`, bounded in both directions. The future bound matters: without it a feed reporting `updated_at = u64::MAX` reads as permanently fresh. Both legs of the derived rate are checked, since one fresh and one stale leg manufactures phantom rate moves worse than freezing both. - absolute sanity bounds per pair and on the derived rate. Consensus only checks that validators agree with each other, and every node reads the same chain state, so a poisoned feed produces unanimous agreement on a wrong value. These bounds are the only thing that notices. - zero answers, out-of-range decimals, and every arithmetic overflow. The rate-of-change bound from the plan is deferred, not dropped: anchoring it to node-local history would make acceptance differ between validators. It needs the previous block's implied rate, which arrives with the block-header fallback work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
08b8979 to
d0678cb
Compare
781e232 to
dd0ee06
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |



Stacked on #14938. Second PR of the L1 price oracle replacement.
What
ChainlinkOracleClient, anExchangeRateOracleClientTraitimplementation that reads Chainlink's on-chain Starknet feeds through our own batcher, replacing the Pragma HTTP API (now a gated commercial beta returning401 No API key).Lands dark.
ChainlinkOracleConfigis not attached toL1GasPriceProviderConfig, and nothing constructs the client outside its own module, soconfig_schema.jsonis untouched and there is no behavior change. The config enum that selects it per feed comes in a later PR, which makes going live a config rollout rather than a deploy.The two rates
Reads
latest_round_data()on the feed proxies (never the aggregators, which rotate behind them viaphase_id):eth_usd / strk_usdat 18 decimals. Every Chainlink feed on Starknet mainnet is USD-quoted; no direct pair exists.answerisu128unsigned on these contracts, so there is no signed-felt decoding.round_idis phase-encoded(phase_id << 128) | round, so it is consumed as aFeltand never narrowed.Non-blocking
fetch_ratekeeps the spawn-and-cache shape of the HTTP client: quantize, check cache, spawn a background query, returnQueryNotReadyError. No batcher round trip lands on the proposal build/validate path. Failures are cached for the rest of the bucket to bound retries (a hostile feed would otherwise cost 4 blocking VM executions per block), and still fall back to the previous bucket's rate so a transient error does not become a hard error for a whole interval.Guards
All reject rather than substitute, so the existing degradation ladder handles them.
updated_at, bounded in both directionsupdated_at = u64::MAXreads as permanently fresh forever. Both legs of the derived rate are checked: one fresh and one stale leg manufactures phantom rate moves worse than freezing both.upgradethe proxy class.Bounds were calibrated against the incoming L2 fee cap and verified not to clip it: the STRK/USD band contains the entire
[min, 10x min]fee range with 58.7x headroom below and 170.5x above. The ETH/STRK floor was raised from 100 to 10,000 STRK/ETH during review, cutting the worst-case L1 undercharge from 1304x to 13x. That leg matters more because it reaches L1 gas pricing viawei_to_friwith no ratchet and no clamp, unlike the SNIP-35 leg's 0.2%/block limit.Deferred, not dropped
The plan's rate-of-change bound is not here. Anchoring it to node-local history would make acceptance differ between validators, so it needs the previous block's implied rate and arrives with the block-header fallback work.
Known dependency before going live
Batcher::call_contractreachescall_view_entry_pointwithinitial_gas = HIGH_GAS_AMOUNTandlimit_steps = false, and the batcher'sLocalComponentServerprocesses one request at a time with no priority preemption. A hostile feed contract could therefore stall block production. That needs a batcher-side bound and is tracked separately; this PR only adds aTODOat the call site. Worth knowing that this would becall_contract's first production consumer anywhere in the tree.Testing
86 tests. Every guard at its exact boundary and one step past, malformed retdata, overflow, the spawn-and-cache and negative-caching behaviour with call counters, the guard counters via a thread-local Prometheus recorder, and truncation on a multi-byte character boundary.
Reviewed through two adversarial rounds (correctness, security, tests, readability). Round 2 caught that round 1's negative-caching fix had introduced an availability regression, and that a
decimals()cache could pin a 100x mis-scale for the process lifetime; both are fixed.🤖 Generated with Claude Code