starknet_committer: add commit e2e tests#11331
starknet_committer: add commit e2e tests#11331ArielElp merged 1 commit intomain-v0.14.1-committerfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
53bc178 to
d2cc76f
Compare
437648a to
fe35926
Compare
03dd814 to
b03fdba
Compare
3165069 to
c8151fc
Compare
b03fdba to
af38e6e
Compare
c8151fc to
ba45006
Compare
af38e6e to
00de040
Compare
ba45006 to
ee507ba
Compare
30f425d to
193b115
Compare
713449c to
701a89d
Compare
2b4d9f6 to
280b616
Compare
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ArielElp).
crates/starknet_committer/src/db/facts_db/types.rs line 71 at r1 (raw file):
}) } }
should do the same thing :)
Suggestion:
#[derive(Clone, Debug, Default, PartialEq)]
pub struct FactsDbInitialRead(pub StateRoots);
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware).
crates/starknet_committer/src/db/facts_db/types.rs line 71 at r1 (raw file):
Previously, nimrod-starkware wrote…
should do the same thing :)
Done.
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp).
crates/starknet_committer/src/db/facts_db/types.rs line 62 at r3 (raw file):
/// Used for reading the roots in facts layout case. #[derive(Clone, Debug, Default, PartialEq)] pub struct FactsDbInitialRead(pub StateRoots);
non-blocking, because StateRoots does have a natural default; however, still consider this, if it's dangerous to accidentally use a default value in production
Suggestion:
#[cfg_attr(any(test, feature = "testing"), derive(Default))]
#[derive(Clone, Debug, PartialEq)]
pub struct FactsDbInitialRead(pub StateRoots);crates/starknet_committer/src/block_committer/commit_test.rs line 60 at r3 (raw file):
#[rstest] #[case([get_first_state_diff(), get_second_state_diff()], EXPECTED_ROOTS_SIMPLE_CASE)] #[case(get_random_state_diffs(), EXPECTED_ROOTS_RANDOM_CASE)]
works, but seems odd; I would consider using expect! to generate the expected roots given the "random" state diffs.
if index layout and facts layout diverge, you will still fail at least one of the tests.
non-blocking
Code quote:
#[case(get_random_state_diffs(), EXPECTED_ROOTS_RANDOM_CASE)]crates/starknet_committer/src/block_committer/commit_test.rs line 101 at r3 (raw file):
expected_roots: StateRoots, ) { let [first_state_diff, second_state_diff] = state_diffs;
non-blocking
Suggestion:
[first_state_diff, second_state_diff]: [StateDiff; 2],
expected_roots: StateRoots,
) {
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp).
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 1 comment and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware).
crates/starknet_committer/src/block_committer/commit_test.rs line 60 at r3 (raw file):
Previously, dorimedini-starkware wrote…
works, but seems odd; I would consider using
expect!to generate the expected roots given the "random" state diffs.
if index layout and facts layout diverge, you will still fail at least one of the tests.
non-blocking
AFAIU it would require a different assertion line in each layout test, and DB layouts shouldn't affect the resulting roots, this would be a protocol change (OS changes + nodes changes etc.)
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @dorimedini-starkware).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @ArielElp).
crates/starknet_committer/src/block_committer/commit_test.rs line 60 at r3 (raw file):
Previously, ArielElp wrote…
AFAIU it would require a different assertion line in each layout test, and DB layouts shouldn't affect the resulting roots, this would be a protocol change (OS changes + nodes changes etc.)
layouts won't, but randomness will.
why different assert line? a single assert_debug_eq should do the trick
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
|
Artifacts upload workflows: |
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp).
crates/starknet_committer/src/block_committer/commit_test.rs line 79 at r6 (raw file):
Self } }
mmmm this isn't enough...?
Suggestion:
impl From<StateRoots> for FactsDbInitialRead {
fn from(roots: StateRoots) -> Self {
Self(roots)
}
}
impl From<StateRoots> for IndexDbReadContext {
fn from(_roots: StateRoots) -> Self {
Self
}
}crates/starknet_committer/src/block_committer/commit_test.rs line 125 at r6 (raw file):
expected_roots: &Expect, ) where Db::InitialReadContext: InitialReadContextFromRoots,
see above
Suggestion:
Db::InitialReadContext: From<StateRoots>,
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware).
crates/starknet_committer/src/block_committer/commit_test.rs line 79 at r6 (raw file):
Previously, dorimedini-starkware wrote…
mmmm this isn't enough...?
Done.
crates/starknet_committer/src/block_committer/commit_test.rs line 125 at r6 (raw file):
Previously, dorimedini-starkware wrote…
see above
Done.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

No description provided.