refactor(gateway): prepare run_validate for mocking

[yair-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.68%. Comparing base (c7fa63e) to head (762177e).
Report is 7 commits behind head on main.

Files	Patch %	Lines
crates/gateway/src/gateway.rs	0.00%	0 Missing and 2 partials ⚠️
...ates/gateway/src/stateful_transaction_validator.rs	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   83.21%   83.68%   +0.46%     
==========================================
  Files          36       38       +2     
  Lines        1692     1765      +73     
  Branches     1692     1765      +73     
==========================================
+ Hits         1408     1477      +69     
- Misses        209      212       +3     
- Partials       75       76       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, all discussions resolved

---

crates/gateway/src/stateful_transaction_validator.rs line 52 at r1 (raw file):

        optional_class_info: Option<ClassInfo>,
        deploy_account_tx_hash: Option<TransactionHash>,
        mut validator: TStatefulTransactionValidator,

Now the blockifier validator is created outside the function so it can be mocked

Code quote:

mut validator: TStatefulTransactionValidator,

[elintul]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @yair-starkware)

a discussion (no related file):
Trait is unrelated to this PR, right?

---

[yair-starkware]

Previously, elintul (Elin) wrote…

Trait is unrelated to this PR, right?

Moved to the following PR

[elintul]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

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

    // TODO(Yael, 19/5/2024): pass the relevant deploy_account_hash.
    let blockifier_validator = stateful_tx_validator.prepare_validate(state_reader_factory)?;

Keep names behavioral and future-proved (type of validator might change and the name will become outdated).

Suggestion:

validator

---

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

type ConcreteBlockifierStatefulValidator =
    BlockifierStatefulValidator<CachedState<Box<dyn MempoolStateReader>>>;

Out of curiosity: where did the type Box<dyn MempoolStateReader>> come from?

Suggestion:

type BlockifierStatefulValidator =
    BlockifierStatefulValidator<CachedState<Box<dyn MempoolStateReader>>>;

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

    }

    pub fn prepare_validate(

Suggestion:

fn instantiate_validator(

---

crates/gateway/src/stateful_transaction_validator.rs line 73 at r2 (raw file):

        Ok(BlockifierStatefulValidator::create(
            state.into(),

What changed here?

Code quote:

.into()

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

    let result =
        stateful_validator.run_validate(&external_tx, optional_class_info, None, validator);

Suggestion:

    let validator = stateful_validator.prepare_validate(&state_reader_factory).unwrap();
    let deploy_account_tx_hash = None;
    let result =
        stateful_validator.run_validate(&external_tx, optional_class_info, deploy_account_tx_hash, validator);

---

crates/gateway/src/stateful_transaction_validator_test.rs line 96 at r2 (raw file):

#[test]
fn test_prepare_validate() {

We're using this function in the test above, not sure a separate one is needed.

Code quote:

test_prepare_validate

[dafnamatsry]

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 107 at r2 (raw file):

        },
    };
    let blockifier_validator = stateful_validator.prepare_validate(&state_reader_factory);

I think maybe we can mock the factory as well. That way we can assert we create the state reader from the latest block.
Can be done in a separate PR.

Code quote:

&state_reader_factory

[dafnamatsry]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yair-starkware)

[elintul]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yair-starkware)

[elintul]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 107 at r2 (raw file):

Previously, dafnamatsry wrote…

I think maybe we can mock the factory as well. That way we can assert we create the state reader from the latest block.
Can be done in a separate PR.

+1

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @elintul)

---

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

Previously, elintul (Elin) wrote…

Keep names behavioral and future-proved (type of validator might change and the name will become outdated).

done

---

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

Previously, elintul (Elin) wrote…

Out of curiosity: where did the type Box<dyn MempoolStateReader>> come from?

Renamed.
This is the return type of the StateReaderFactory

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

    }

    pub fn prepare_validate(

done

---

crates/gateway/src/stateful_transaction_validator.rs line 73 at r2 (raw file):

Previously, elintul (Elin) wrote…

What changed here?

Had a mistake in the alias, good thing you noticed

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

    let result =
        stateful_validator.run_validate(&external_tx, optional_class_info, None, validator);

In the next PR I remove the test case of deploy account

---

crates/gateway/src/stateful_transaction_validator_test.rs line 96 at r2 (raw file):

Previously, elintul (Elin) wrote…

We're using this function in the test above, not sure a separate one is needed.

I don't think it hurts, but I can remove

---

crates/gateway/src/stateful_transaction_validator_test.rs line 107 at r2 (raw file):

Previously, elintul (Elin) wrote…

+1

There's already a factory for testing, I will think about what should be done

[elintul]

Reviewed 1 of 3 files at r3, all commit messages.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

done

Can it be private?

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

In the next PR I remove the test case of deploy account

WDYM?

[yair-starkware]

Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @elintul)

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

Previously, elintul (Elin) wrote…

Can it be private?

No, it is called from process_tx which is in and outer scope.
I think some modules in the gateway crate can be private.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

Previously, elintul (Elin) wrote…

WDYM?

Actually I am not sure what this field does, but I don't think it is in the scope of this task to change it

[yair-starkware]

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

No, it is called from process_tx which is in and outer scope.
I think some modules in the gateway crate can be private.

an*

[elintul]

Reviewed 2 of 3 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Renamed.
This is the return type of the StateReaderFactory

Why do we alias GenericBlockifierStatefulValidator?

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Actually I am not sure what this field does, but I don't think it is in the scope of this task to change it

I agree, but it's a small change (that increases readability) and this line has already changed.

[elintul]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

an*

Perhaps pub(crate)?

[yair-starkware]

crates/gateway/src/stateful_transaction_validator_test.rs line 107 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

There's already a factory for testing, I will think about what should be done

done: #401

[yair-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 45 at r2 (raw file):

Previously, elintul (Elin) wrote…

Perhaps pub(crate)?

I will change the module scope in another PR

---

crates/gateway/src/stateful_transaction_validator_test.rs line 91 at r2 (raw file):

Previously, elintul (Elin) wrote…

I agree, but it's a small change (that increases readability) and this line has already changed.

@Yael-Starkware is deleting this argument in another PR

[yair-starkware]

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

Previously, elintul (Elin) wrote…

Why do we alias GenericBlockifierStatefulValidator?

So the function argument will be simpler to read

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

So the function argument will be simpler to read

I meant the other alias, of GenericBlockifierStatefulValidator; seems unnecessary

[dafnamatsry]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 107 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

done: #401

Thanks :)

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

[yair-starkware]

crates/gateway/src/stateful_transaction_validator.rs line 25 at r2 (raw file):

Previously, elintul (Elin) wrote…

I meant the other alias, of GenericBlockifierStatefulValidator; seems unnecessary

Removed

[elintul]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)