Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tests_integration): add nonce verification #2071

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

Itay-Tsabary-Starkware
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware commented Nov 14, 2024

commit-id:ab21c59b


Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.591 ms 34.638 ms 34.689 ms]
change: [-4.5598% -2.9563% -1.5448%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.36%. Comparing base (e3165c4) to head (1511a6c).
Report is 497 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2071       +/-   ##
===========================================
+ Coverage   40.10%   77.36%   +37.25%     
===========================================
  Files          26      385      +359     
  Lines        1895    40416    +38521     
  Branches     1895    40416    +38521     
===========================================
+ Hits          760    31266    +30506     
- Misses       1100     6851     +5751     
- Partials       35     2299     +2264     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 69 at r1 (raw file):

fn get_latest_block_number(storage_reader: &StorageReader) -> BlockNumber {
    let txn = storage_reader.begin_ro_txn().unwrap();
    txn.get_state_marker().unwrap().prev().unwrap()

This will panic if no blocks have been written yet, right? Is that what we want?
Same below at get nonce

Code quote:

txn.get_state_marker().unwrap().prev().unwrap()

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 80 at r1 (raw file):

    let txn = storage_reader.begin_ro_txn().unwrap();
    let state_number = StateNumber::unchecked_right_after_block(block_number);
    get_nonce_at(&txn, state_number, None.as_ref(), contract_address).unwrap().unwrap()

Suggestion:

&None

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 83 at r1 (raw file):

}

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after

Suggestion:

if after

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 84 at r1 (raw file):

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after
/// a certain number of attempts the target block number has not been reached.

Suggestion:

the given

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 85 at r1 (raw file):

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after
/// a certain number of attempts the target block number has not been reached.
async fn await_until_storage_contains_block(

Suggestion:

await_block

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 176 at r1 (raw file):

    );

    info!("Verify tx sender account nonce.");

Suggestion:

info!("Verifying tx sender account nonce.");

Copy link
Contributor Author

@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: all files reviewed, 5 unresolved discussions (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 69 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This will panic if no blocks have been written yet, right? Is that what we want?
Same below at get nonce

The setup function that creates the db ensures it is not empty. I prefer not to add error handling here.


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 83 at r1 (raw file):

}

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after

Done.


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 84 at r1 (raw file):

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after
/// a certain number of attempts the target block number has not been reached.

Done.


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 85 at r1 (raw file):

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if the after
/// a certain number of attempts the target block number has not been reached.
async fn await_until_storage_contains_block(

Done.

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@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: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 176 at r1 (raw file):

    );

    info!("Verify tx sender account nonce.");

Done.

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 69 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The setup function that creates the db ensures it is not empty. I prefer not to add error handling here.

No need for error handling. Could you add documentation and also use expect instead of the respective unwrap?

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@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: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 69 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

No need for error handling. Could you add documentation and also use expect instead of the respective unwrap?

Changed to expects, in both location. IMO thier message suffices as documentation, if you think otherwise please lmk what you'd like to be added 🙏

Copy link
Contributor Author

@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: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 69 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Changed to expects, in both location. IMO thier message suffices as documentation, if you think otherwise please lmk what you'd like to be added 🙏

locations

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.162 ms 35.386 ms 35.647 ms]
change: [+1.8951% +2.6293% +3.4332%] (p = 0.00 < 0.05)
Performance has regressed.
Found 18 outliers among 100 measurements (18.00%)
5 (5.00%) high mild
13 (13.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [30.479 ms 30.534 ms 30.593 ms]
change: [+1.3609% +1.6088% +1.8665%] (p = 0.00 < 0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

Copy link
Collaborator

@alonh5 alonh5 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 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

Copy link
Contributor Author

@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 r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@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 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from spr/main/82d3fb9c to main November 19, 2024 11:13
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware merged commit 27a4c29 into main Nov 19, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
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.

3 participants