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

test(starknet_integration_tests): test deploy account txs in flow test #2807

Open
wants to merge 1 commit into
base: yair/prepare_state_diff_funding
Choose a base branch
from

Conversation

yair-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

graphite-app bot commented Dec 19, 2024

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (12/19/24)

1 assignee was added to this PR based on Yair's automation.

@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 7fa4fd1 to bc8d70b Compare December 19, 2024 09:27
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 596f94d to 48eb196 Compare December 19, 2024 09:27
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from bc8d70b to 3a11954 Compare December 19, 2024 10:22
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 48eb196 to 0f12509 Compare December 19, 2024 10:22
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 3a11954 to cdf0e7c Compare December 19, 2024 10:24
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 0f12509 to ce5cda7 Compare December 19, 2024 10:24
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from cdf0e7c to c65e8d4 Compare December 19, 2024 10:35
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from ce5cda7 to f99f19f Compare December 19, 2024 10:35
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from c65e8d4 to 8287610 Compare December 19, 2024 10:50
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 8287610 to 3bc6015 Compare December 26, 2024 13:11
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from f99f19f to 09398c8 Compare December 26, 2024 13:56
@yair-starkware yair-starkware changed the base branch from yair/flow_test_fund_address to yair/prepare_state_diff_funding December 26, 2024 13:56
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from a41d373 to 4911c62 Compare December 26, 2024 13:57
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch 2 times, most recently from 026a6a8 to 930556d Compare December 26, 2024 13:59
@yair-starkware yair-starkware changed the title wip test(starknet_integration_tests): test deploy account txs in flow test Dec 26, 2024
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 4911c62 to d58ba8e Compare December 26, 2024 14:01
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from d6828e7 to c699a77 Compare December 31, 2024 08:40
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 4a533d6 to b22ca41 Compare December 31, 2024 08:50
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from c699a77 to 0132a60 Compare December 31, 2024 08:50
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from b22ca41 to bc8f41c Compare December 31, 2024 09:51
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch 2 times, most recently from be95926 to 68911d0 Compare December 31, 2024 10:34
Copy link
Contributor Author

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


crates/starknet_integration_tests/src/state_reader.rs line at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you explain the changes in this file? Was this one of the bugs you found?

Yes.
If you register multiple contracts with the same class, the class will be declared multiple times, and it will fail.
These changes make classes be declared only once.


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 102 at r2 (raw file):

Can you create a function for this and remove the Boxes? You can get the undeployed account with its ID.

Done

do we need an extra block or can we have the funding and the deployment in the same block?

We need another block, otherwise the GW will reject the deployment tx because there is not enough balance (once we remove the initial balance from the test setup)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 128 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Remove.

Done.


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 279 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You should add Code Spell Checker extension to vscode.

Fixed (the code moved)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 293 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you instead use the ID to get the account? Why do we need to check that it's the last?

Technically, nothing is coupling the account IDs in the tests to their index in the generator.
This is another thing we need to refactor in the utility.
What happens if someone registers more accounts in the middle?


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 302 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

If there isn't anything else we want to test, we can just have one function for both.

Done.


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 280 at r2 (raw file):

fn fund_new_account(
    funding_acount: &mut AccountTransactionGenerator,
    to: &Contract,

Same

@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 68911d0 to 45bc2f0 Compare December 31, 2024 12:28
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from bc8f41c to cbcaa88 Compare December 31, 2024 13:39
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 45bc2f0 to adb62c7 Compare December 31, 2024 13:40
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from cbcaa88 to b3da587 Compare January 1, 2025 09:52
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from adb62c7 to 4b89baf Compare January 1, 2025 09:52
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from b3da587 to 8bff41f Compare January 1, 2025 11:19
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 4b89baf to 2ca3bd1 Compare January 1, 2025 11:19
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 8bff41f to 4527d59 Compare January 1, 2025 11:27
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 2ca3bd1 to 3ec6d5c Compare January 1, 2025 11:27
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 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yair-starkware)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 293 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Technically, nothing is coupling the account IDs in the tests to their index in the generator.
This is another thing we need to refactor in the utility.
What happens if someone registers more accounts in the middle?

So it will fail here but it won't be clear why. You should assert the the account id is equal to UNDEPLOYED_ACCOUNT_ID when registering it, and then use UNDEPLOYED_ACCOUNT_ID to get it (as you did in create_funding_txs for example).


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 113 at r5 (raw file):

type ExpectedContentId = Felt;

fn create_test_scenarios() -> Vec<(CreateRpcTxsFn, TestTxHashesFn, ExpectedContentId)> {

Add height also instead of ziping them together?
Also document this function.

Suggestion:

create_test_blocks

@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 4527d59 to 4a3e777 Compare January 2, 2025 09:00
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 3ec6d5c to 1c462fc Compare January 2, 2025 09:00
@yair-starkware yair-starkware requested a review from alonh5 January 2, 2025 09:56
Copy link
Contributor Author

@yair-starkware yair-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: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 293 at r2 (raw file):

You should assert the the account id is equal to UNDEPLOYED_ACCOUNT_ID when registering it

Already doing it: https://github.com/starkware-libs/sequencer/blob/93e65241d683e55739d80c20b1f26385ee9bcfdf/crates/starknet_integration_tests/src/utils.rs#L188

and then use UNDEPLOYED_ACCOUNT_ID to get it

Done

Copy link
Contributor Author

@yair-starkware yair-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: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @alonh5)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 113 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Add height also instead of ziping them together?
Also document this function.

Done.

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 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 4a3e777 to dce8816 Compare January 2, 2025 11:26
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from 1c462fc to f000089 Compare January 2, 2025 11:26
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from dce8816 to dc872fc Compare January 2, 2025 11:34
@yair-starkware yair-starkware force-pushed the yair/flow_test_deploy_account branch from f000089 to f039a1b Compare January 2, 2025 11:34
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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants