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

refactor(execution): put all create for testing methods under testing… #164

Merged

Conversation

meship-starkware
Copy link
Contributor

@meship-starkware meship-starkware commented Jul 29, 2024

… config

refactor(execution): put all create for testing methods under testing config


This change is Reviewable

Copy link

graphite-app bot commented Jul 29, 2024

Graphite Automations

"Request reviewers once CI passes" took an action on this PR • (07/29/24)

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

@meship-starkware meship-starkware removed the request for review from dorimedini-starkware July 29, 2024 08:35
@meship-starkware meship-starkware force-pushed the meship/blockifier/crate_for_testing_under_test_cfg branch from 9dbb7dc to 2f47bed Compare July 29, 2024 08:46
Copy link
Contributor Author

@meship-starkware meship-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 2 files reviewed, 1 unresolved discussion (waiting on @noaov1)

a discussion (no related file):
@tdelabro


Copy link
Collaborator

@noaov1 noaov1 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 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware)


crates/blockifier/src/transaction/transactions.rs line 514 at r2 (raw file):

    #[cfg(any(test, feature = "testing"))]
    pub fn create_for_testing(l1_fee: Fee, contract_address: ContractAddress) -> Self {

Please move it to struct_impl

Code quote:

    #[cfg(any(test, feature = "testing"))]
    pub fn create_for_testing(l1_fee: Fee, contract_address: ContractAddress) -> Self {

crates/gateway/src/config.rs line 212 at r2 (raw file):

#[cfg(any(test, feature = "testing"))]
impl ChainInfoConfig {
    pub fn create_for_testing() -> Self {

Same below

Suggestion:

impl ChainInfoConfig {
    #[cfg(any(test, feature = "testing"))]
    pub fn create_for_testing() -> Self {

Copy link
Contributor Author

@meship-starkware meship-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, 3 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/transaction/transactions.rs line 514 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please move it to struct_impl

Do we want all impl L1HandlerTransaction to only appear in tests binary?


crates/gateway/src/config.rs line 212 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Same below

Do you want me to mark the configuration only for the function?

Copy link
Collaborator

@noaov1 noaov1 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, 3 unresolved discussions (waiting on @meship-starkware)


crates/blockifier/src/transaction/transactions.rs line 514 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Do we want all impl L1HandlerTransaction to only appear in tests binary?

No, only the create_for_testing method

Copy link
Contributor Author

@meship-starkware meship-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, 3 unresolved discussions (waiting on @noaov1)


crates/gateway/src/config.rs line 212 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Do you want me to mark the configuration only for the function?

I can't do this because it is a part of the gateway crate and not the blockifier.

@meship-starkware meship-starkware force-pushed the meship/blockifier/crate_for_testing_under_test_cfg branch from 2f47bed to fb4d4f5 Compare July 30, 2024 12:41
Copy link
Contributor Author

@meship-starkware meship-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, 3 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/transaction/transactions.rs line 514 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

No, only the create_for_testing method

Done.

Copy link
Collaborator

@noaov1 noaov1 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 r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware)


crates/gateway/src/config.rs line 212 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I can't do this because it is a part of the gateway crate and not the blockifier.

Can you please elaborate?

@meship-starkware meship-starkware force-pushed the meship/blockifier/crate_for_testing_under_test_cfg branch from fb4d4f5 to 8d43db2 Compare July 30, 2024 13:44
Copy link
Contributor Author

@meship-starkware meship-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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @noaov1)


crates/gateway/src/config.rs line 212 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please elaborate?

Nevermind I did not understand the remark
Done.

Copy link
Collaborator

@noaov1 noaov1 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: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware)


crates/gateway/src/config.rs line 210 at r4 (raw file):

    }
}
impl ChainInfoConfig {

Suggestion:

}

impl ChainInfoConfig {

crates/gateway/src/config.rs line 290 at r4 (raw file):

#[cfg(any(test, feature = "testing"))]
impl StatefulTransactionValidatorConfig {
    pub fn create_for_testing() -> Self {

Suggestion:

impl StatefulTransactionValidatorConfig {
    #[cfg(any(test, feature = "testing"))]
    pub fn create_for_testing() -> Self {

@meship-starkware meship-starkware force-pushed the meship/blockifier/crate_for_testing_under_test_cfg branch from 8d43db2 to 1b3406c Compare July 30, 2024 14:20
Copy link
Contributor Author

@meship-starkware meship-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, 3 unresolved discussions (waiting on @noaov1)


crates/gateway/src/config.rs line 210 at r4 (raw file):

    }
}
impl ChainInfoConfig {

Done.


crates/gateway/src/config.rs line 290 at r4 (raw file):

#[cfg(any(test, feature = "testing"))]
impl StatefulTransactionValidatorConfig {
    pub fn create_for_testing() -> Self {

Done.

Copy link
Collaborator

@noaov1 noaov1 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 r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

@meship-starkware meship-starkware merged commit 0a4d9c2 into main Jul 30, 2024
12 of 13 checks passed
@meship-starkware meship-starkware deleted the meship/blockifier/crate_for_testing_under_test_cfg branch July 30, 2024 14:45
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 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