apollo_consensus_orchestrator: add SNIP-35 module tests

[sirandreww-starkware]

No description provided.

[sirandreww-starkware]

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.
Learn more

• apollo_consensus_orchestrator: add SNIP-35 integration TODO doc #13982
• apollo_consensus_orchestrator: tmp dirty tests for SNIP-35 (NOT FOR MERGE) #13857
• apollo_l1_gas_price,apollo_l1_gas_price_config: move exchange_rate_decimals const to config #13881
• apollo_consensus_orchestrator: add SNIP-35 proposer-validator symmetry tests #13856
• apollo_consensus_orchestrator: add SNIP-35 module edge case tests #13855
• apollo_consensus_orchestrator: add SNIP-35 e2e test #13821
• apollo_consensus_manager: wire STRK/USD oracle into consensus manager #13820
• apollo_consensus_orchestrator: add SNIP-35 fee_proposal validation #13819
• 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
• apollo_consensus_orchestrator: add fee_actual floor to EIP-1559 gas price calculation #13824
• apollo_consensus_orchestrator: add SNIP-35 fee_proposals sliding window #13817
• apollo_consensus_orchestrator,apollo_consensus_manager: add strk_exchange_rate_oracle field to deps #13823
• apollo_l1_gas_price_types,apollo_l1_gas_price: rename trait method eth_to_fri_rate to fetch_rate #13987
• apollo_dashboard: add SNIP-35 row #13954
• apollo_consensus_orchestrator: add SNIP-35 metrics #13816
• apollo_consensus_orchestrator: add SNIP-35 module tests #13815 👈 (View in Graphite)
• apollo_consensus_orchestrator: add SNIP-35 module #13854
• apollo_storage: add fee_proposal_fri migrator and bump blocks version to 6.1 #13936 : 2 other dependent PRs (#13814 , #13934 )
• apollo_protobuf: add fee_proposal_fri to sync proto SignedBlockHeader #13957
• starknet_api: add fee_proposal_fri to BlockHeaderWithoutHash #13822
• apollo_protobuf: add fee_proposal_fri to ProposalInit #13812 : 2 other dependent PRs (#13811 , #13935 )
• main

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

[reviewable-StarkWare]

This change is 

[ShahakShama]

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

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 6 at r1 (raw file):

#[test]
fn test_compute_fee_actual_with_10_identical_values() {

This is way too many test cases. IMO you can have a test case with a random list of size 12 you've generated on ipython and pasted here and precompute the result, another 3 cases for when you return None and that's it

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 72 at r1 (raw file):

    // Target: $3e-9/gas = 3_000_000_000 atto-USD. STRK at $0.50 = 500_000_000_000_000_000.
    // floor = 3_000_000_000 * 10^18 / 500_000_000_000_000_000 = 6_000_000_000.
    let target = compute_fee_target(3_000_000_000, 500_000_000_000_000_000, 0, u128::MAX);

The meaning of each parameter here is unclear. See https://github.com/starkware-industries/code-quality/blob/main/style_guide/rust_style_guide.md#unnamed-function-parameters
Same for other tests

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 78 at r1 (raw file):

#[test]
fn test_compute_fee_target_clamp_min() {
    let target = compute_fee_target(1, 10u128.pow(18), 100, u128::MAX);

Set a constant for MIN and use it here and in the assert_eq below. Same for the max test and the zero rate test

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 91 at r1 (raw file):

#[test]
fn test_compute_fee_target_zero_rate() {

Append to the test name the expected result: ..._returns_max

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 117 at r1 (raw file):

#[test]
fn test_compute_fee_proposal_target_within_bounds() {

Test for both directions (can be done in the same test)

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 131 at r1 (raw file):

#[test]
fn test_compute_fee_actual_window_size_below_minimum_returns_none() {

Move this to the other actual fee tests

[sirandreww-starkware]

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

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 6 at r1 (raw file):

Previously, ShahakShama wrote…

This is way too many test cases. IMO you can have a test case with a random list of size 12 you've generated on ipython and pasted here and precompute the result, another 3 cases for when you return None and that's it

Done

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 72 at r1 (raw file):

Previously, ShahakShama wrote…

The meaning of each parameter here is unclear. See https://github.com/starkware-industries/code-quality/blob/main/style_guide/rust_style_guide.md#unnamed-function-parameters
Same for other tests

Done

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 78 at r1 (raw file):

Previously, ShahakShama wrote…

Set a constant for MIN and use it here and in the assert_eq below. Same for the max test and the zero rate test

Done
Added FLOOR_MIN_FRI and FLOOR_MAX_FRI constants at the top of the test module and reused them in the clamp_min, clamp_max, and zero_rate_returns_max rstest cases (both as the input bound and the expected GasPrice)

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 91 at r1 (raw file):

Previously, ShahakShama wrote…

Append to the test name the expected result: ..._returns_max

Done
the case is now case::zero_rate_returns_max in the test_compute_fee_target rstest

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 117 at r1 (raw file):

Previously, ShahakShama wrote…

Test for both directions (can be done in the same test)

Done.

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 131 at r1 (raw file):

Previously, ShahakShama wrote…

Move this to the other actual fee tests

Done.

[ShahakShama]

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

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 11 at r3 (raw file):

#[test]
fn test_compute_fee_actual_random_window() {

What I meant was 12 with window size 10. You can do that in a new test if you prefer. Make sure that one of the first numbers outside of the window would've affected the median if it were in the window (And state it in comments)

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 51 at r3 (raw file):

// Very low STRK price → very high floor, clamped down to FLOOR_MAX_FRI.
#[case::clamp_max(FRI_DECIMALS_SCALE, 1, 0, FLOOR_MAX_FRI, GasPrice(FLOOR_MAX_FRI))]
#[case::zero_rate_returns_max(100, 0, 0, FLOOR_MAX_FRI, GasPrice(FLOOR_MAX_FRI))]

Why 100 and not FRI_DECIMALS_SCALE? It should be a constant anyway

[sirandreww-starkware]

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

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 11 at r3 (raw file):

Previously, ShahakShama wrote…

What I meant was 12 with window size 10. You can do that in a new test if you prefer. Make sure that one of the first numbers outside of the window would've affected the median if it were in the window (And state it in comments)

Done

---

crates/apollo_consensus_orchestrator/src/snip35/test.rs line 51 at r3 (raw file):

Previously, ShahakShama wrote…

Why 100 and not FRI_DECIMALS_SCALE? It should be a constant anyway

Done

[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 ff91417. Configure here.}

[ShahakShama]

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