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

feat(mempool_test_utils,starknet_integration_tests): prepare funded but undeployed accounts #2986

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

yair-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

graphite-app bot commented Dec 26, 2024

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (12/26/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 3bc6015 to 73dfa81 Compare December 26, 2024 13:57
@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/prepare_state_diff_funding branch from 4911c62 to d58ba8e Compare December 26, 2024 14:01
@yair-starkware yair-starkware changed the title feat(mempool_test_utils,starknet_integration_tests): prepare state diff for testing with funded but undeployed accounts feat(mempool_test_utils,starknet_integration_tests): prepare funded but undeployed accounts Dec 26, 2024
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 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yair-starkware)


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

    }

    // TODO(deploy_account_support): delete method once we have batcher with execution.

Can we delete this here and deploy accounts only with txs now that you have added support for that?

Code quote:

// TODO(deploy_account_support): delete method once we have batcher with execution.

@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 73dfa81 to f9cecc3 Compare December 30, 2024 11:48
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from d58ba8e to e427a44 Compare December 30, 2024 11:48
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from f9cecc3 to 51914aa Compare December 31, 2024 08:11
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from e427a44 to b93ddd4 Compare December 31, 2024 08:11
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, 1 unresolved discussion (waiting on @alonh5)


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

Previously, alonh5 (Alon Haramati) wrote…

Can we delete this here and deploy accounts only with txs now that you have added support for that?

Isn't it used by the integration test as well?

@yair-starkware
Copy link
Contributor Author

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

Previously, yair-starkware (Yair) wrote…

Isn't it used by the integration test as well?

IMO this whole module shouldn't exist.
Needs to be part of the MultiAccountTxGenerator.

@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 51914aa to 1fad30b Compare December 31, 2024 08:40
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from b93ddd4 to 4a533d6 Compare December 31, 2024 08:40
@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 1fad30b to 53d5540 Compare December 31, 2024 08:50
@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_fund_address branch from 53d5540 to 7e8fb71 Compare December 31, 2024 09:51
@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_fund_address branch from 7e8fb71 to 5d87a2d Compare December 31, 2024 13:39
@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_fund_address branch from 5d87a2d to 58e23fe Compare January 1, 2025 09:52
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from cbcaa88 to b3da587 Compare January 1, 2025 09:52
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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yair-starkware)


crates/starknet_integration_tests/src/state_reader.rs line 412 at r1 (raw file):
Yes you're right it's used in the integration test.

Needs to be part of the MultiAccountTxGenerator
Do you think it's easy and we should do it now?


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

    }

    // TODO(deploy_account_support): delete method once we have batcher with execution.

Here you can remove it right?

Code quote:

// TODO(deploy_account_support): delete method once we have batcher with execution.

@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 58e23fe to 826fdb0 Compare January 1, 2025 11:19
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch 2 times, most recently from 8bff41f to 4527d59 Compare January 1, 2025 11:27
@yair-starkware yair-starkware requested a review from alonh5 January 1, 2025 11:27
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, 1 unresolved discussion (waiting on @alonh5)


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

Previously, alonh5 (Alon Haramati) wrote…

Yes you're right it's used in the integration test.

Needs to be part of the MultiAccountTxGenerator
Do you think it's easy and we should do it now?

I don't think it's that easy.
I think we should do it before adding more flows to the e2e tests.


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

Previously, alonh5 (Alon Haramati) wrote…

Here you can remove it right?

The functions are still needed because we can't update the state with the transfer txs yet.
Also, I think it's a useful functionality for easy setups (instead of needing to fund, declare and deploy every contract in every test).
Removed the TODOs.

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 @yair-starkware)


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

Previously, yair-starkware (Yair) wrote…

I don't think it's that easy.
I think we should do it before adding more flows to the e2e tests.

Okay, can you add a TODO for it then?


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

Previously, yair-starkware (Yair) wrote…

The functions are still needed because we can't update the state with the transfer txs yet.
Also, I think it's a useful functionality for easy setups (instead of needing to fund, declare and deploy every contract in every test).
Removed the TODOs.

Yes I only meant remove the TODO here.

@yair-starkware yair-starkware force-pushed the yair/flow_test_fund_address branch from 826fdb0 to 8f67a10 Compare January 2, 2025 09:00
@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from 4527d59 to 4a3e777 Compare January 2, 2025 09:00
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 1 of 1 files at r4, 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/flow_test_fund_address branch from 8f67a10 to 022ce3a Compare January 2, 2025 11:19
@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 changed the base branch from yair/flow_test_fund_address to main January 2, 2025 11:26
@yair-starkware yair-starkware requested a review from alonh5 January 2, 2025 11:32
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: 1 of 5 files reviewed, all discussions resolved (waiting on @alonh5)


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

Previously, alonh5 (Alon Haramati) wrote…

Okay, can you add a TODO for it then?

Done

@yair-starkware yair-starkware force-pushed the yair/prepare_state_diff_funding branch from dce8816 to dc872fc 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 3 of 4 files at r5, 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 added this pull request to the merge queue Jan 2, 2025
Merged via the queue into main with commit 8a22094 Jan 2, 2025
8 checks passed
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