apollo_l1_gas_price,apollo_l1_gas_price_config,apollo_node: build oracle clients per feed source

[asaf-sw]

Builds each feed's oracle client from the source #14995 made configurable, so enabling Chainlink for a feed becomes a config rollout rather than a code deploy.

Both feeds default to Http, so merging changes nothing at runtime — the same two ExchangeRateOracleClients are built from the same two configs as before.

Also closes a transposition the previous tests could not see: crossing a feed's config with another feed's metrics bundle would have published every ETH/STRK reading under snip35_strk_usd_*, pointing every dashboard and alert at the wrong series. The factory now has no positional argument left to cross, and every assertion is a number rather than a Debug string.

Stacked on #14995. Part of the L1 price oracle replacement split of #14942. Everything the source switch adds is deleted with the HTTP source.

---

Detailed Summary for AI Bots

What

new_with_oracle builds each feed's client from the source #14995 made configurable. Both feeds default to Http, so merging changes nothing at runtime.

The factory is generic over the feed's rate kind, and everything about the feed comes from that kind: its selected source, the HTTP config that source reads, the metrics bundle it publishes on, the Chainlink feeds and bounds it reads, and the config key named when the selection cannot be honoured. new_with_oracle passes no per-feed argument at all, only the whole config and the optional batcher client.

The transposition that tests could not see

new_with_oracle on the base branch pairs each feed's HTTP config with that feed's metrics constant positionally, and no test constructs it at all, so crossing either pair is invisible: every ETH/STRK reading would publish under snip35_strk_usd_* and vice versa, pointing every dashboard and alert at the wrong series.

The factory in #14944 kept that shape one level down. It took the feed's source, HTTP config and source config key as positional arguments independent of the rate kind, so any two of them could still be crossed. Its HTTP cases read the rates and the series back per feed, but its Chainlink cases asserted format!("{client:?}") substrings, starts_with("ChainlinkOracleClient") and pair: EthStrk, over byte-identical default configs, and never read a rate through a Chainlink client.

This PR closes it from both sides. Structurally, a private SourceSelectableFeed impl per rate kind holds that feed's source, HTTP config and config key, so the factory has no argument left to transpose, and ChainlinkRate::metrics() remains the only place a pair maps to a metrics bundle. Behaviourally, every assertion is now a number rather than a Debug string:

• Two mockito servers and a mocked batcher are wired in every case of each_feed_is_served_by_the_source_selected_for_it, and the four rates the two sources serve are distinct. A feed built from the other feed's source, config or rate kind resolves to a number named for that other feed.
• each_feed_publishes_on_its_own_metrics_series runs on each source and reads both rates back off a local Prometheus recorder, so a client holding the other feed's bundle fails on the series whichever source built that client.

Each fix was verified by mutation: transposing the per-feed source, the per-feed HTTP config, the rate kind at a call site, the source config key, and the metrics bundle each fails a test named for it, as do hardcoding one pair's bundle where ChainlinkOracleClient::new reads Kind::metrics() and a MissingBatcherClientError message that stops naming the offending keys. Nothing in this PR asserts on a Debug string.

One deleted test, and why

#14944 had chainlink_clients_sample_on_their_own_feeds_interval, pinning that a Chainlink client samples on its feed's ExchangeRateOracleConfig::lag_interval_seconds. #14983 moved the Chainlink cadence into ChainlinkOracleConfig::sampling_interval_seconds, which both feeds share, so the per-source cadence distinction that test protected is gone by design and the factory threads no interval at all. That also removes the NonZeroU64 conversion #14944 needed, and with it the range(min = 1) that PR had to add to lag_interval_seconds to keep an operator zero from surfacing as a node panic.

Misconfiguration behaviour

Selecting Chainlink where no batcher client exists fails while the components are built, rather than falling back to Http or failing per proposal. MissingBatcherClientError resolves both feeds before reporting either, so one startup names every offending key: an operator who set both feeds wrong learns of both at once instead of paying 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.

components.rs panics on the error, at the composition root where a config mistake should stop the process.

Everything here has a deletion date

The HTTP source removal deletes MissingBatcherClientError, the factory's Http arm, and both *_ORACLE_SOURCE_CONFIG_KEY consts, together with the SourceSelectableFeed selection that exists only to choose between the two sources. All are marked. The error type is worth its lines even so: during the window an operator is actively editing these keys, and without it a misconfigured node dies on an expect inside the factory, with a message that names neither the feed that was misconfigured nor the key that misconfigured it.

The two ChainlinkRate impls get their production callers here: ChainlinkOracleClient was constructed only by tests until now, so Kind::metrics() and the guard counters were reachable only under cfg(test).

Testing

10 new cases, 99 in the crate. The four each_feed_is_served_by_the_source_selected_for_it cases cover both feeds on each source and each feed alone on Chainlink; the two each_feed_publishes_on_its_own_metrics_series cases cover the metrics wiring under each source; http_sources_do_not_need_a_batcher_client and the three chainlink_source_without_a_batcher_client_is_rejected cases cover the batcher requirement, the rejection cases each asserting that the rendered error names every offending config key verbatim.

The Chainlink cases read through the batcher fixtures the chainlink_oracle tests already use, whose two feed answers derive to one rate for ETH/STRK and read as another for STRK/USD. Only the entry point composing them is new; the test_utils module and the three fixture consts the provider test names widened to pub(crate).

🤖 Generated with Claude Code

[reviewable-StarkWare]

This change is 

[asaf-sw]

• apollo_l1_gas_price,apollo_l1_gas_price_config,apollo_node: build oracle clients per feed source #14996 👈 (View in Graphite)
• apollo_l1_gas_price_config,apollo_deployments,apollo_node: add ExchangeRateOracleSource #14995
• apollo_l1_gas_price_config,apollo_deployments,apollo_node: nest the Chainlink config #14994
• apollo_l1_gas_price: bound Chainlink freshness fallback between blocks #14993 : 1 other dependent PR (#14997 )
• apollo_l1_gas_price,apollo_l1_gas_price_types: hold Chainlink query failures for the retry interval #14992
• apollo_l1_gas_price,apollo_l1_gas_price_config: ChainlinkOracleClient query lifecycle #14991 : 1 other dependent PR (#14973 )
• apollo_l1_gas_price,apollo_l1_gas_price_config,apollo_l1_gas_price_types: read a Chainlink feed #14990
• apollo_l1_gas_price: Chainlink retdata decoding and rate arithmetic #14989
• apollo_l1_gas_price_config: Chainlink oracle config #14983
• apollo_l1_gas_price_config,apollo_l1_gas_price_types: source-agnostic rate bounds #14982
• apollo_l1_gas_price,apollo_l1_gas_price_types,apollo_dashboard: fold oracle metrics onto labels #14981 : 1 other dependent PR (#14948 )
• apollo_metrics: labeled gauge lossy and unix-now setters #14998
• main-v0.14.3

This stack of pull requests is managed by Graphite. Learn more about stacking.

[asaf-sw]

Migrated review comment from #14944, closed in favor of this split.

@matanl-starkware wrote on crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 118, on ChainlinkRateKind::EthToFri (original thread):

It's weird to call this enum "ChainLink" when the mode is "Http"; it actually refers to Pragma.

This PR owns the code the comment lands on. The enum is gone, replaced by the RateKind / ChainlinkRate traits and their EthToFri and StrkUsd types, and the factory here is generic over the rate kind for both sources, so the ChainlinkRate bound is what names an HTTP feed's kind.

[asaf-sw]

Migrated review comment from #14944, closed in favor of this split.

@matanl-starkware wrote on crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 270, on config: &L1GasPriceProviderConfig, (original thread):

Pass the relevant struct from the call site (after splitting as suggested before)

This PR owns that signature. build_exchange_rate_oracle_client still takes &L1GasPriceProviderConfig plus the optional batcher client, with the per-feed fields selected inside by the rate kind through SourceSelectableFeed rather than passed positionally. The "splitting as suggested before" it refers to is #14983 and #14982, nested by #14994.

[asaf-sw]

Migrated review comment from #14944, closed in favor of this split.

@matanl-starkware wrote on crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 279 (original thread), on the ChainlinkRateKind::EthToFri arm that selected config.eth_to_strk_oracle_source, &config.eth_to_strk_oracle_config and ETH_TO_STRK_ORACLE_SOURCE_CONFIG_KEY:

To calc the Eth/STRK ratio, the code must also read the STRK/USD from the ChainLink contract.
IMO we should either take both from Http or both from ChainLink. The separation just adds more room for bugs.

This PR owns the per-feed source selection the comment lands on: new_with_oracle builds each feed's client from its own configured source, so the two feeds can sit on different sources. The derived ETH/STRK rate reading both legs off Chainlink, and its freshness across those legs, is #14993, where the same comment is copied.

[matanl-starkware]

@matanl-starkware made 3 comments.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on asaf-sw).

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 73 at r2 (raw file):

// [Temporary comment] Deleted in C1 together with the two config params they name.
const ETH_TO_STRK_ORACLE_SOURCE_CONFIG_KEY: &str =
    "l1_gas_price_provider_config.eth_to_strk_oracle_source";

Is it the same string that appears in the KEYS_TO_BE_REPLACED?
Can you make it a single source of truth?

Code quote:

"l1_gas_price_provider_config.eth_to_strk_oracle_source"

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 91 at r2 (raw file):

    // Every feed that selects `Chainlink`, so that one startup reports the whole misconfiguration.
    source_config_keys: Vec<&'static str>,
}

Seems like an overkill to me. It's OK if the Operator gets 2 different errors.

Code quote:

/// Whether a batcher client is available is decided by the composition root, not by config, so this
/// is raised while the components are built rather than caught by config validation.
// [Temporary comment] Deleted in C1: once Chainlink is the only source, a provider without a
// batcher client cannot serve a rate at all, so there is no source selection left to reject.
#[derive(Debug, Eq, Error, PartialEq)]
#[error(
    "Chainlink is selected as the oracle source by {}, but this service has no batcher client. \
     Chainlink feeds are read through the batcher, so either run the batcher alongside this \
     service or select Http instead.",
    .source_config_keys.join(" and ")
)]
pub struct MissingBatcherClientError {
    // Every feed that selects `Chainlink`, so that one startup reports the whole misconfiguration.
    source_config_keys: Vec<&'static str>,
}

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 123 at r2 (raw file):

        batcher_client: Option<SharedBatcherClient>,
    ) -> Result<Self, MissingBatcherClientError> {
        // Both feeds are resolved before either failure is reported, so an operator who

I don't really think it's necessary.
Why do we even allow the batcher_client to be None under the "new_with_oracle" function?
This flow has no meaning without a client.

Code quote:

Both feeds are resolved before either failure is reported

[asaf-sw]

@asaf-sw+AGNT made 3 comments.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on matanl-starkware).

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 73 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Is it the same string that appears in the KEYS_TO_BE_REPLACED?
Can you make it a single source of truth?

Done, and all three of these collapse into one deletion.

new_with_oracle now requires the batcher client and is infallible:

pub fn new_with_oracle(
    config: L1GasPriceProviderConfig,
    batcher_client: SharedBatcherClient,
) -> Self

components.rs does the expect at the composition root. MissingBatcherClientError is gone, and with it the resolve-both-feeds logic, so there is no second error for an operator to receive either.

The two *_ORACLE_SOURCE_CONFIG_KEY consts existed only to name the offending key in that error message, so they are deleted too. That removes the string duplicated against KEYS_TO_BE_REPLACED rather than trying to share it, which was not reachable anyway: apollo_deployments has no dependency on apollo_l1_gas_price, and its phf_set! holds plain literals for every component key.

Net -111 lines, 98 tests pass in the crate.

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 91 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Seems like an overkill to me. It's OK if the Operator gets 2 different errors.

Agreed, and it is gone entirely rather than reduced to two errors. new_with_oracle requires the batcher client now, so there is no failure to report: see the reply on the config-key thread for the shape.

---

crates/apollo_l1_gas_price/src/l1_gas_price_provider.rs line 123 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

I don't really think it's necessary.
Why do we even allow the batcher_client to be None under the "new_with_oracle" function?
This flow has no meaning without a client.

You are right, there is no meaning to this flow without a client. batcher_client is now SharedBatcherClient rather than Option, and components.rs resolves the Option from get_batcher_shared_client() with an expect at the composition root.

Checked before removing it: get_batcher_shared_client() returns None only for Client::Disabled, and every shipped topology gives the L1 service a batcher client, including distributed, where l1.json has components.batcher.execution_mode = Remote.

This also deleted the two tests that only existed to cover the removed error.

[matanl-starkware]

@matanl-starkware reviewed 7 files and all commit messages, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on asaf-sw).

[asaf-sw]

@asaf-sw reviewed 7 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on asaf-sw).

[graphite-app]

Merge activity

• Aug 23, 12:56 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

[matanl-starkware]

@matanl-starkware made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on asaf-sw).

[gkaempfer]

Security scan complete — no issues detected.

---

Generated by Claude Code