-
Notifications
You must be signed in to change notification settings - Fork 34
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
test(starknet_integration_tests): add deploy_tx to the integration test #3803
Conversation
1933ed2
to
3f4152b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs
line 230 at r2 (raw file):
/// Creates a multi-account transaction generator for the integration test. pub fn create_integration_test_tx_generator() -> MultiAccountTransactionGenerator {
Why can't you share the logic of the flow test?
Code quote:
a
ac0f50a
to
4c9f573
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5)
crates/starknet_integration_tests/src/utils.rs
line 230 at r2 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Why can't you share the logic of the flow test?
Currently the flow test is working differently. it has a deployed accounts and undeployed accounts and is checking very specific scenarios on each. So the setup is different.
I think we should consider having only undeployed accounts also in flow_test. But this should be done separately.
2f4d4a7
to
dae32c0
Compare
4c9f573
to
e58d457
Compare
dae32c0
to
25e8ad0
Compare
e58d457
to
65c0136
Compare
25e8ad0
to
e364a14
Compare
65c0136
to
df1764f
Compare
32ff285
to
17e34a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs
line 251 at r4 (raw file):
for (i, account) in [ FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1(RunnableCairo1::Casm)), FeatureContract::AccountWithoutValidations(CairoVersion::Cairo0),
Are we registering 2 accounts but only using 1?
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 46 at r4 (raw file):
// Run the first block scenario to bootstrap the accounts. integration_test_manager .test_and_verify(tx_generator, 0, FirstBlock, SENDER_ACCOUNT, BlockNumber(2))
Make a const for consistency.
Code quote:
BlockNumber(2)
There was a problem hiding this 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, 2 unresolved discussions (waiting on @alonh5)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 46 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Make a const for consistency.
Done.
crates/starknet_integration_tests/src/utils.rs
line 251 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Are we registering 2 accounts but only using 1?
good question, I didn't change this code, I believe we should use both -to test also Cairo0 accounts, and in general we should add multiple accounts.
I can add a monday task to enable this account and also multiple accounts.
18c4198
to
9d9f69c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs
line 251 at r4 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
good question, I didn't change this code, I believe we should use both -to test also Cairo0 accounts, and in general we should add multiple accounts.
I can add a monday task to enable this account and also multiple accounts.
Ok, yeah let's add it to monday.
Until then you can really simplify this function.
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
pub async fn end_to_end_integration(tx_generator: &mut MultiAccountTransactionGenerator) { const EXPECTED_SECOND_BLOCK_NUMBER: BlockNumber = BlockNumber(2);
Can we rename these three consts? Something more along the lines of n blocks to wait, that suits the usage better IMO. WDYT?
There was a problem hiding this 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, 2 unresolved discussions (waiting on @alonh5)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Can we rename these three consts? Something more along the lines of n blocks to wait, that suits the usage better IMO. WDYT?
I agree, I also thought this was confusing but tried to avoid too many changes.
crates/starknet_integration_tests/src/utils.rs
line 251 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Ok, yeah let's add it to monday.
Until then you can really simplify this function.
Done.
9d9f69c
to
d53c8ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
I agree, I also thought this was confusing but tried to avoid too many changes.
Try BLOCK_TO_WAIT_FOR_BOOTSTRAP
, BLOCK_TO_WAIT_FOR_FIRST_ROUND
, BLOCK_TO_WAIT_FOR_LATE_NODE
Also change the trait impl FirstBlock to somthing like Bootstrap/DeployAccount
There was a problem hiding this 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 @alonh5)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Try
BLOCK_TO_WAIT_FOR_BOOTSTRAP
,BLOCK_TO_WAIT_FOR_FIRST_ROUND
,BLOCK_TO_WAIT_FOR_LATE_NODE
Also change the trait impl FirstBlock to somthing like Bootstrap/DeployAccount
Done.
d53c8ca
to
84e57cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
Done.
What about the trait impl?
There was a problem hiding this 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 @alonh5)
crates/starknet_integration_tests/src/end_to_end_integration.rs
line 12 at r5 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
What about the trait impl?
oh sorry, I missed that.
done.
84e57cc
to
ead5c1a
Compare
There was a problem hiding this 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 r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
No description provided.