apollo_consensus_orchestrator: wire SNIP-35 fee_proposal into build_proposal

[sirandreww-starkware]

No description provided.

[sirandreww-starkware]

• apollo_consensus_orchestrator: add SNIP-35 fee_proposal validation #13819 : 2 dependent PRs (#13820 , #14157 )
• apollo_consensus_orchestrator: include fee_proposal_fri in proposal commitment hash #13949
• apollo_starknet_client: read fee_proposal_fri from feeder gateway #13986
• apollo_consensus_orchestrator: wire SNIP-35 fee_proposal into cende blob #13985
• apollo_consensus_orchestrator: wire SNIP-35 fee_proposal into build_proposal #13818 👈 (View in Graphite)
• apollo_consensus_orchestrator,apollo_consensus_manager: move SNIP-35 bootstrap to pre-consensus #14061
• apollo_node_config: made validation only node hold full state #14122
• main

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

[reviewable-StarkWare]

This change is 

[sirandreww-starkware]

@sirandreww-starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on ShahakShama).

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 34 at r2 (raw file):

/// Target USD cost per L2 gas unit in atto-USD ($3e-9 = 3_000_000_000 atto-USD).
pub(crate) const TARGET_ATTO_USD_PER_L2_GAS: u128 = 3_000_000_000;

add a TODO to make this a dynamic config

Code quote:

TARGET_ATTO_USD_PER_L2_GAS

[ShahakShama]

@ShahakShama reviewed 4 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on sirandreww-starkware).

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 34 at r2 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

add a TODO to make this a dynamic config

Add it

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 31 at r5 (raw file):

pub(crate) const FEE_PROPOSAL_WINDOW_SIZE: usize = 10;

/// Maximum fee_proposal change per block in parts per thousand (SNIP-35: 0.2%).

Add TODO to consider moving to versioned constants

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 37 at r5 (raw file):

pub(crate) const TARGET_ATTO_USD_PER_L2_GAS: u128 = 3_000_000_000;

/// Hard minimum for the oracle-derived floor (FRI).

Get it from versioned constants instead (it's already there)

---

crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs line 408 at r5 (raw file):

        // compute_fee_actual returns None for zero median or insufficient data, falling back to
        // the current l2_gas_price. This ensures proposer and validator use the same fallback.
        let fee_actual = self.compute_fee_actual().unwrap_or(self.l2_gas_price);

Shouldn't you output fee_actual as well for the batcher?

[ShahakShama]

@ShahakShama reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on sirandreww-starkware).

[sirandreww-starkware]

@sirandreww-starkware made 4 comments.
Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on ShahakShama).

---

crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs line 408 at r5 (raw file):

Previously, ShahakShama wrote…

Shouldn't you output fee_actual as well for the batcher?

Decided to take it as an input here and to calculate it outside this function

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 34 at r2 (raw file):

Previously, ShahakShama wrote…

Add it

Done.

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 31 at r5 (raw file):

Previously, ShahakShama wrote…

Add TODO to consider moving to versioned constants

Done.

---

crates/apollo_consensus_orchestrator/src/snip35/mod.rs line 37 at r5 (raw file):

Previously, ShahakShama wrote…

Get it from versioned constants instead (it's already there)

I decided to remove this clamping since it's not actually in the SNIP I just added because I thought it would somehow be "safer"

[cursor]

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 3df8374. Configure here.}

[ShahakShama]

@ShahakShama reviewed 6 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on sirandreww-starkware).

---

crates/apollo_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 1683 at r10 (raw file):

)]
#[tokio::test]
async fn test_compute_snip35_fee_proposal(

Add a test where the rate changes mid run

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 33 at r10 (raw file):

#[rstest]
// Heights [0, 9] needed for fee_actual at height 10; height 5 is missing.

The bahaviour here should be return none or panic?

[sirandreww-starkware]

@sirandreww-starkware made 3 comments.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on ShahakShama).

---

crates/apollo_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 1683 at r10 (raw file):

Previously, ShahakShama wrote…

Add a test where the rate changes mid run

Done.

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 33 at r10 (raw file):

Previously, ShahakShama wrote…

The bahaviour here should be return none or panic?

I tend to think we should warn instead of panic (does not add complexity to the code and prevents unexpected issues from making the node useless).
If we were to change this to a panic, then I think it should be done by expecting on the result instead
What do you say?

[ShahakShama]

@ShahakShama reviewed 3 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on sirandreww-starkware).

---

crates/apollo_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 1683 at r10 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

Done.

No. I meant a test where the oracle returns at first x and then y

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 33 at r10 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

I tend to think we should warn instead of panic (does not add complexity to the code and prevents unexpected issues from making the node useless).
If we were to change this to a panic, then I think it should be done by expecting on the result instead
What do you say?

ok

[ShahakShama]

@ShahakShama resolved 1 discussion.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on sirandreww-starkware).

[ShahakShama]

@ShahakShama reviewed 1 file and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on sirandreww-starkware).