Skip to content

Conversation

guy-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

guy-starkware commented Jul 17, 2025

@guy-starkware guy-starkware force-pushed the guyn/l1price/rate_from_provider branch from 6de36bf to c8cac00 Compare July 17, 2025 09:42
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch 2 times, most recently from 491a603 to 8a33db8 Compare July 17, 2025 10:46
@guy-starkware guy-starkware force-pushed the guyn/l1price/rate_from_provider branch from c8cac00 to c6217b6 Compare July 17, 2025 10:46
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from 8a33db8 to aa6280b Compare July 20, 2025 10:13
@guy-starkware guy-starkware force-pushed the guyn/l1price/rate_from_provider branch from c6217b6 to 07e6f76 Compare July 20, 2025 10:13
@guy-starkware guy-starkware changed the base branch from guyn/l1price/rate_from_provider to graphite-base/8062 July 20, 2025 13:19
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from aa6280b to a4a9be0 Compare July 20, 2025 13:39
@guy-starkware guy-starkware changed the base branch from graphite-base/8062 to guyn/l1price/rate_from_provider July 20, 2025 13:39
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from a4a9be0 to bfd8a52 Compare July 20, 2025 13:40
Copy link

github-actions bot commented Jul 20, 2025

Benchmark movements: tree_computation_flow performance improved 😺 tree_computation_flow time: [34.732 ms 34.760 ms 34.791 ms] change: [-4.1332% -2.4877% -1.0279%] (p = 0.00 < 0.05) Performance has improved. Found 4 outliers among 100 measurements (4.00%) 2 (2.00%) high mild 2 (2.00%) high severe

Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)


crates/apollo_l1_gas_price/src/eth_to_strk_oracle.rs line 51 at r2 (raw file):

}

#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Validate)]

What does this do?

Code quote:

#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Validate)]

Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)


crates/apollo_l1_gas_price/src/eth_to_strk_oracle.rs line 51 at r2 (raw file):

Previously, matan-starkware wrote…

What does this do?

All the configs under NodeConfig have #[validate] on them... even this one that doesn't have any actual validation.

I think that for uniformity this is a good thing, and if we add validation on the individual fields we won't have to change anything else.

@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from bfd8a52 to 6e47a2b Compare July 28, 2025 14:48
@guy-starkware guy-starkware force-pushed the guyn/l1price/rate_from_provider branch from 56efeb0 to be7c574 Compare July 28, 2025 14:48
@guy-starkware guy-starkware changed the base branch from guyn/l1price/rate_from_provider to graphite-base/8062 July 29, 2025 14:19
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from 6e47a2b to e5a2bfe Compare July 30, 2025 14:07
@guy-starkware guy-starkware changed the base branch from graphite-base/8062 to guyn/l1price/rate_from_provider July 30, 2025 14:07
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from e5a2bfe to b1633a6 Compare July 31, 2025 07:36
@guy-starkware guy-starkware force-pushed the guyn/l1price/rate_from_provider branch from f8fc9e2 to a6db0fd Compare August 3, 2025 08:30
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch 3 times, most recently from 286636d to 02f4044 Compare August 3, 2025 09:29
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 31 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware, @guy-starkware, and @matan-starkware)


crates/apollo_l1_gas_price/src/eth_to_strk_oracle.rs line 51 at r2 (raw file):

Previously, guy-starkware wrote…

All the configs under NodeConfig have #[validate] on them... even this one that doesn't have any actual validation.

I think that for uniformity this is a good thing, and if we add validation on the individual fields we won't have to change anything else.

Please keep Validate, this enables recursively validating the subconfigs 🙏 (or manually implement)


crates/apollo_deployments/resources/app_configs/consensus_manager_config.json line 28 at r3 (raw file):

  "consensus_manager_config.eth_to_strk_oracle_config.lag_interval_seconds": 900,
  "consensus_manager_config.eth_to_strk_oracle_config.max_cache_size": 100,
  "consensus_manager_config.eth_to_strk_oracle_config.query_timeout_sec": 3,

I'd expect the addition of the prefix-truncated values in one of the (other) config files in this directory. Is that related to the error you're experiencing?

Code quote:

  "consensus_manager_config.eth_to_strk_oracle_config.lag_interval_seconds": 900,
  "consensus_manager_config.eth_to_strk_oracle_config.max_cache_size": 100,
  "consensus_manager_config.eth_to_strk_oracle_config.query_timeout_sec": 3,

@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch 5 times, most recently from 35eaccf to 458c76c Compare August 4, 2025 12:38
@guy-starkware guy-starkware changed the title apollo_node: remove eth/strk config from consensus_manager and make it top level apollo_node: remove eth/strk config from consensus_manager and move it into l1_gas_price_provider_config Aug 4, 2025
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from 458c76c to d983e34 Compare August 4, 2025 12:55
@guy-starkware guy-starkware changed the title apollo_node: remove eth/strk config from consensus_manager and move it into l1_gas_price_provider_config apollo_node: move eth/strk config from consensus_manager to l1_gas_price_provider_config Aug 4, 2025
@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from d983e34 to 8d1ef88 Compare August 4, 2025 15:58
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 33 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware, @Itay-Tsabary-Starkware, and @matan-starkware)


crates/apollo_deployments/resources/app_configs/consensus_manager_config.json line 28 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I'd expect the addition of the prefix-truncated values in one of the (other) config files in this directory. Is that related to the error you're experiencing?

I've moved this into the l1_gas_price_provider_config and have no more problems.


crates/apollo_l1_gas_price/src/eth_to_strk_oracle.rs line 51 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please keep Validate, this enables recursively validating the subconfigs 🙏 (or manually implement)

Will do.

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 1 of 29 files at r3.
Reviewable status: 2 of 33 files reviewed, 3 unresolved discussions (waiting on @asmaastarkware, @guy-starkware, and @matan-starkware)


crates/apollo_deployments/resources/testing_secrets.json line 3 at r5 (raw file):

{
    "base_layer_config.node_url": "http://anvil-service.anvil.svc.cluster.local:8545",
    "l1_gas_price_provider_config.eth_to_strk_oracle_config.url_header_list": "http://dummy-eth2strk-oracle-service.dummy-eth2strk-oracle.svc.cluster.local/eth_to_strk_oracle?timestamp=:9000",

Please alphabetize the fields in this file 🙏

Code quote:

    "l1_gas_price_provider_config.eth_to_strk_oracle_config.url_header_list": "http://dummy-eth2strk-oracle-service.dummy-eth2strk-oracle.svc.cluster.local/eth_to_strk_oracle?timestamp=:9000",

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 33 files reviewed, 6 unresolved discussions (waiting on @asmaastarkware, @guy-starkware, and @matan-starkware)


crates/apollo_deployments/resources/app_configs/consensus_manager_config.json line 28 at r3 (raw file):

Previously, guy-starkware wrote…

I've moved this into the l1_gas_price_provider_config and have no more problems.

GG EZ


crates/apollo_deployments/src/test_utils.rs line 21 at r5 (raw file):

    )]
    l1_gas_price_provider_config_config_eth_to_strk_oracle_config_url_header_list:
        Option<Vec<UrlAndHeaders>>,

ditto ^

Code quote:

    #[serde(
        rename = "l1_gas_price_provider_config.eth_to_strk_oracle_config.url_header_list",
        serialize_with = "serialize_optional_list_with_url_and_headers_wrapper"
    )]
    l1_gas_price_provider_config_config_eth_to_strk_oracle_config_url_header_list:
        Option<Vec<UrlAndHeaders>>,

crates/apollo_deployments/src/test_utils.rs line 58 at r5 (raw file):

                    headers: Default::default(),
                }],
            ),

ditto ^

Code quote:

            l1_gas_price_provider_config_config_eth_to_strk_oracle_config_url_header_list: Some(
                vec![UrlAndHeaders {
                    url: Url::parse("https://arbitrary.eth_to_strk_oracle.url").unwrap(),
                    headers: Default::default(),
                }],
            ),

crates/apollo_deployments/resources/app_configs/l1_gas_price_provider_config.json line 8 at r5 (raw file):

  "l1_gas_price_provider_config.eth_to_strk_oracle_config.lag_interval_seconds": 900,
  "l1_gas_price_provider_config.eth_to_strk_oracle_config.max_cache_size": 100,
  "l1_gas_price_provider_config.eth_to_strk_oracle_config.query_timeout_sec": 3

ditto ^

Code quote:

  "l1_gas_price_provider_config.eth_to_strk_oracle_config.lag_interval_seconds": 900,
  "l1_gas_price_provider_config.eth_to_strk_oracle_config.max_cache_size": 100,
  "l1_gas_price_provider_config.eth_to_strk_oracle_config.query_timeout_sec": 3

crates/apollo_node/src/components.rs line 449 at r5 (raw file):

                l1_gas_price_provider_config.clone(),
                Arc::new(eth_to_strk_oracle_client),
            ))

Please wrap this in a fn that takes l1_gas_price_provider_config as an arg and returns L1GasPriceProvider.
The top level module should not be aware of this internally-used client (I understand why you'd want it for dependency injection, but this should be abstracted away from the top level module).

Code quote:

            Some(L1GasPriceProvider::new(
                l1_gas_price_provider_config.clone(),
                Arc::new(eth_to_strk_oracle_client),
            ))

crates/apollo_integration_tests/src/integration_test_manager.rs line 903 at r5 (raw file):

        spawn_local_success_recorder(base_layer_ports.get_next_port());
    let (eth_to_strk_oracle_url, _join_handle_eth_to_strk_oracle) =
        spawn_local_eth_to_strk_oracle(base_layer_ports.get_next_port());

Could you please add a todo on my name here (tsabary) to move these to the start of the test, and to properly propagate their values when relevant?

Code quote:

    // All nodes use the same recorder_url and eth_to_strk_oracle_url.
    let (recorder_url, _join_handle) =
        spawn_local_success_recorder(base_layer_ports.get_next_port());
    let (eth_to_strk_oracle_url, _join_handle_eth_to_strk_oracle) =
        spawn_local_eth_to_strk_oracle(base_layer_ports.get_next_port());

@guy-starkware guy-starkware force-pushed the guyn/l1price/eth_strk_config branch from 8d1ef88 to 6dd9769 Compare August 5, 2025 10:14
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 33 files reviewed, 6 unresolved discussions (waiting on @asmaastarkware, @Itay-Tsabary-Starkware, and @matan-starkware)


crates/apollo_deployments/resources/testing_secrets.json line 3 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please alphabetize the fields in this file 🙏

Ok.


crates/apollo_deployments/resources/app_configs/l1_gas_price_provider_config.json line 8 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

ditto ^

Done.


crates/apollo_deployments/src/test_utils.rs line 58 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

ditto ^

Done.


crates/apollo_integration_tests/src/integration_test_manager.rs line 903 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could you please add a todo on my name here (tsabary) to move these to the start of the test, and to properly propagate their values when relevant?

sure.


crates/apollo_node/src/components.rs line 449 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please wrap this in a fn that takes l1_gas_price_provider_config as an arg and returns L1GasPriceProvider.
The top level module should not be aware of this internally-used client (I understand why you'd want it for dependency injection, but this should be abstracted away from the top level module).

Good idea.

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 29 files at r3, 5 of 10 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: 17 of 33 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware, @guy-starkware, and @matan-starkware)

Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 6 of 29 files at r3, 16 of 17 files at r4, 10 of 10 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

@guy-starkware guy-starkware merged commit 2db1eba into guyn/l1price/rate_from_provider Aug 6, 2025
30 of 32 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants