Skip to content

Conversation

@guy-starkware
Copy link
Contributor

No description provided.

@guy-starkware guy-starkware marked this pull request as ready for review November 2, 2025 11:52
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

@ShahakShama reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @guy-starkware)


crates/apollo_l1_provider/tests/flow_tests.rs line 39 at r1 (raw file):

    // Setup.
    // Must wait at least 1 second, as timestamps are integer seconds.
    const COOLDOWN_MILLIS: u64 = 1000;

Change these to be of Duration type instead


crates/apollo_l1_provider/tests/flow_tests.rs line 40 at r1 (raw file):

    // Must wait at least 1 second, as timestamps are integer seconds.
    const COOLDOWN_MILLIS: u64 = 1000;
    const SMALL_DELAY_MILLIS: u64 = 10;

Rename to WAIT_FOR_L1_MILLIS
(If you accept my comment above, WAIT_FOR_L1_DURATION)


crates/apollo_l1_provider/tests/flow_tests.rs line 96 at r1 (raw file):

    let l1_event = events.pop().unwrap();

    // Convert the L1 event to an Apollo event, so we can get the L2 hash.

If you take my suggestion to extract to a function from previous PR, add this to that function and have the function return the l2 hash


crates/apollo_l1_provider/tests/flow_tests.rs line 142 at r1 (raw file):

    // Set up the L1 scraper and run it as a server.
    let l1_scraper_config = L1ScraperConfig {
        polling_interval_seconds: Duration::from_secs(1),

Use the const: Duration::from_millis(COOLDOWN_MILLIS)


crates/apollo_l1_provider/tests/flow_tests.rs line 175 at r1 (raw file):

    l1_provider_client.start_block(SessionState::Propose, next_block_height).await.unwrap();
    let n_txs = 1;
    let txs = l1_provider_client.get_txs(n_txs, next_block_height).await.unwrap();

Also test that validate returns true


crates/apollo_l1_provider/tests/flow_tests.rs line 178 at r1 (raw file):

    assert!(txs.is_empty());

    // Sleep at least two times the cooldown to make sure we are not failing due to fractional

If the reason is fractional seconds, then you should add a second, not multiply by two
De-facto it's the same now, but if someone changes the constant it will break


crates/apollo_l1_provider/tests/flow_tests.rs line 180 at r1 (raw file):

    // Sleep at least two times the cooldown to make sure we are not failing due to fractional
    // seconds.
    tokio::time::sleep(Duration::from_millis(COOLDOWN_MILLIS * 2)).await;

Use tokio::time::advance and tokio::time::pause instead

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, 7 unresolved discussions (waiting on @ShahakShama)


crates/apollo_l1_provider/tests/flow_tests.rs line 39 at r1 (raw file):

Previously, ShahakShama wrote…

Change these to be of Duration type instead

Done.


crates/apollo_l1_provider/tests/flow_tests.rs line 40 at r1 (raw file):

Previously, ShahakShama wrote…

Rename to WAIT_FOR_L1_MILLIS
(If you accept my comment above, WAIT_FOR_L1_DURATION)

Done.


crates/apollo_l1_provider/tests/flow_tests.rs line 96 at r1 (raw file):

Previously, ShahakShama wrote…

If you take my suggestion to extract to a function from previous PR, add this to that function and have the function return the l2 hash

yeah, that's what I'm thinking.


crates/apollo_l1_provider/tests/flow_tests.rs line 142 at r1 (raw file):

Previously, ShahakShama wrote…

Use the const: Duration::from_millis(COOLDOWN_MILLIS)

Done.


crates/apollo_l1_provider/tests/flow_tests.rs line 175 at r1 (raw file):

Previously, ShahakShama wrote…

Also test that validate returns true

I'll add a calidation attempt after the cooldown as well.


crates/apollo_l1_provider/tests/flow_tests.rs line 178 at r1 (raw file):

Previously, ShahakShama wrote…

If the reason is fractional seconds, then you should add a second, not multiply by two
De-facto it's the same now, but if someone changes the constant it will break

Done.


crates/apollo_l1_provider/tests/flow_tests.rs line 180 at r1 (raw file):

Previously, ShahakShama wrote…

Use tokio::time::advance and tokio::time::pause instead

We use sleep everywhere else in the code. I don't know what the difference is.

@guy-starkware guy-starkware force-pushed the guyn/l1provider/anvil_in_flow_test branch from 14b0114 to a78c5bc Compare November 3, 2025 09:43
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 1fc171a to 47d17eb Compare November 3, 2025 09:43
Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


crates/apollo_l1_provider/tests/flow_tests.rs line 180 at r1 (raw file):

Previously, guy-starkware wrote…

We use sleep everywhere else in the code. I don't know what the difference is.

Fix this at the end of the stack

@guy-starkware guy-starkware force-pushed the guyn/l1provider/anvil_in_flow_test branch from a78c5bc to a820fc0 Compare November 4, 2025 09:45
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 47d17eb to 89e8787 Compare November 4, 2025 09:45
@guy-starkware guy-starkware force-pushed the guyn/l1provider/anvil_in_flow_test branch from a820fc0 to db35f21 Compare November 4, 2025 10:02
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 89e8787 to d5a2109 Compare November 4, 2025 10:02
@guy-starkware guy-starkware force-pushed the guyn/l1provider/anvil_in_flow_test branch from db35f21 to ba543a6 Compare November 4, 2025 14:23
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch 2 times, most recently from 8c98150 to 496e236 Compare November 6, 2025 08:07
@guy-starkware guy-starkware force-pushed the guyn/l1provider/anvil_in_flow_test branch from ba543a6 to 3f99e6b Compare November 6, 2025 08:07
@guy-starkware guy-starkware changed the base branch from guyn/l1provider/anvil_in_flow_test to graphite-base/9902 November 9, 2025 12:59
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 496e236 to 11b1728 Compare November 9, 2025 13:01
@guy-starkware guy-starkware changed the base branch from graphite-base/9902 to guyn/l1provider/anvil_in_flow_test November 9, 2025 13:02
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.

@guy-starkware reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

@graphite-app graphite-app bot changed the base branch from guyn/l1provider/anvil_in_flow_test to graphite-base/9902 November 9, 2025 13:27
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 11b1728 to 56a4b06 Compare November 9, 2025 13:42
@guy-starkware guy-starkware changed the base branch from graphite-base/9902 to guyn/l1provider/anvil_in_flow_test November 9, 2025 13:42
@guy-starkware guy-starkware changed the base branch from guyn/l1provider/anvil_in_flow_test to main-v0.14.1 November 9, 2025 13:52
@guy-starkware guy-starkware force-pushed the guyn/l1provider/basic_flow_test branch from 56a4b06 to bb88bf0 Compare November 9, 2025 13:52
@graphite-app
Copy link

graphite-app bot commented Nov 9, 2025

Merge activity

  • Nov 9, 1:53 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

@guy-starkware guy-starkware added this pull request to the merge queue Nov 10, 2025
Merged via the queue into main-v0.14.1 with commit 779308c Nov 10, 2025
26 of 29 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 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.

4 participants