apollo_l1_gas_price,apollo_l1_gas_price_config,apollo_node: select oracle source per feed - #14944
Open
asaf-sw wants to merge 1 commit into
Open
Conversation
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 c0801bf. Configure here.
asaf-sw
force-pushed
the
asaf/l1-oracle-05-select-oracle-source
branch
from
August 11, 2026 10:05
c0801bf to
4f1581b
Compare
asaf-sw
force-pushed
the
asaf/l1-oracle-03-chainlink-oracle-client
branch
from
August 11, 2026 10:51
08b8979 to
d0678cb
Compare
asaf-sw
force-pushed
the
asaf/l1-oracle-05-select-oracle-source
branch
from
August 11, 2026 10:51
4f1581b to
131ddb0
Compare
Contributor
Author
|
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. |
…acle source per feed Makes `ChainlinkOracleClient` selectable from node config, per feed. The previous PR landed it dark; this is the go-live switch, so enabling Chainlink becomes a config rollout rather than a code deploy. Both feeds default to `Http`, so there is no production behavior change. The schema diff is 14 additions, no existing parameter's value, privacy, or description moved. The two feeds are selected independently so they can be migrated one at a time. `ChainlinkOracleConfig` is now nested under `L1GasPriceProviderConfig` with `#[validate(nested)]`, which makes the validation added in the previous PR actually reachable from the node config root. That matters: `ChainlinkOracleClient::new` expects a non-zero `max_cache_size` and `lag_interval_seconds`, so without it an operator zero would surface as a node panic instead of a config error. The factory takes the whole `L1GasPriceProviderConfig` and derives each feed's HTTP config and metrics bundle from its rate kind, so a feed cannot be paired with another feed's config or metrics. Review found that the earlier shape, which passed them as independent arguments, allowed a transposition that every test still passed: both feeds default to an identical `ExchangeRateOracleConfig`, and the assertions only checked the client's type. The rate-kind-to-metrics mapping now exists in exactly one place. Selecting `Chainlink` where no batcher client exists fails at startup, naming every offending config key at once, rather than panicking later or silently serving HTTP. Every shipped topology (consolidated, hybrid, distributed) gives the L1 service a batcher client, so this is reachable only by misconfiguration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
asaf-sw
force-pushed
the
asaf/l1-oracle-05-select-oracle-source
branch
from
August 11, 2026 11:09
131ddb0 to
666c587
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Stacked on #14942.
What
Makes
ChainlinkOracleClientselectable from node config, per feed. #14942 landed it dark; this is the go-live switch, so enabling Chainlink is a config rollout, not a code deploy.Both feeds default to
Http. The schema diff is 14 additions, with no existing parameter's value, privacy, or description changed, verified by set-diffing the parsed JSON rather than eyeballing it.The two feeds are independently switchable so they can be migrated one at a time.
Validation is now actually reachable
ChainlinkOracleConfigis nested underL1GasPriceProviderConfigwith#[validate(nested)], which makes the cross-field validation added in #14942 live for the first time (main.rs→load_and_validate_config→validate_node_config→.validate()). That matters:ChainlinkOracleClient::newexpects a non-zeromax_cache_sizeandlag_interval_seconds, so without this an operator zero surfaces as a node panic instead of a config error.The transposition that tests could not see
Review found the original factory shape took
rate_kindand the per-feed HTTP config as independent arguments. Swapping them compiled, ran, and passed all 99 tests — because both feeds default to a byte-identicalExchangeRateOracleConfig, and the assertions only checked the client's concrete type. The same was true of the two metrics constants: swapping those would have published every ETH/STRK reading undersnip35_strk_usd_*and vice versa, silently pointing every dashboard and alert at the wrong series.Fixed structurally rather than with more assertions: the factory takes the whole
L1GasPriceProviderConfigand derives the HTTP config, metrics bundle, and config key fromrate_kind, so the transposition is now unrepresentable. The rate-kind-to-metrics mapping lives in exactly one place (ChainlinkRateKind::metrics()), where it was previously duplicated between the factory andChainlinkOracleClient::new.Tests are behavioural rather than
Debug-scraping: twomockitoservers return distinct prices, and the test asserts both that each rate resolves to its own feed and that it lands on its own Prometheus series. Each fix was then verified by mutation — transposing the configs, the metrics, and the rate kinds each now fails a test named for it.Misconfiguration behaviour
Selecting
Chainlinkwhere no batcher client exists fails at startup, naming every offending config key at once rather than only the first (which would have cost the operator two restart cycles for one mistake). Every shipped topology — consolidated, hybrid, distributed — gives the L1 service a batcher client, so this is reachable only by misconfiguration.An invalid enum value fails loudly: the enum has no
rename_all, so a lowercase"chainlink"is rejected at config load rather than silently falling back toHttpand making a migration look successful while nothing changed. There's a test pinning that.Operator-facing
The bound parameters are in micro units, which was explained only in a Rust doc comment and therefore never reached the schema. An operator would see
value: 20000000described as "micro-USD per ETH" and have to guess it means $20, where being wrong by 10^6 either disables the guard or rejects every reading. Every bound description now anchors the scale with a worked conversion, and both source descriptions state that selectingChainlinkwithout a batcher is a startup failure rather than a fallback.Before flipping a feed
Selecting
Chainlinkin hybrid or distributed routes every rate query through a remote call to the batcher from the L1 pod, and the ETH/STRK branch issues two per uncached bucket. #14945 (batcher view-call resource bounds) should land before any feed is flipped toChainlink; it is offmainrather than in this stack.🤖 Generated with Claude Code