feat(gateway): mock blockifier validator

[yair-starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.60%. Comparing base (d2a047f) to head (de53834).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   83.62%   82.60%   -1.02%     
==========================================
  Files          39       39              
  Lines        1777     1788      +11     
  Branches     1777     1788      +11     
==========================================
- Hits         1486     1477       -9     
- Misses        215      237      +22     
+ Partials       76       74       -2     

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

[elintul]

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @dafnamatsry)

[elintul]

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

a discussion (no related file):
This PR is quite large. Please break it down into smaller PRs.
Perhaps:

• Trait definition
• Breakdown of run_validate
• Generalization of run_validate

And so on.

---

[dafnamatsry]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 67 at r3 (raw file):

    }

    pub fn prepare_validate(

What about tests for this function?

Code quote:

pub fn prepare_validate(

---

crates/gateway/src/stateful_transaction_validator_test.rs line 23 at r3 (raw file):

#[rstest]
#[case::valid_invoke_tx_cairo1(

There is no meaning in having all these test cases anymore. The different external_tx has really no affect on the test.
I guess we only need 2 cases, one in which mock_validator.validate() returns ok, and another in which it returns an error.

We should add tests to check we properly convert to an eccount tx, but that should be in a test file for utils where the conversion is implemented.

[yair-starkware]

Previously, elintul (Elin) wrote…

This PR is quite large. Please break it down into smaller PRs.
Perhaps:

• Trait definition
• Breakdown of run_validate
• Generalization of run_validate

And so on.

Note that it is separated by commits.
Do you still prefer separated PRs?

[yair-starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 67 at r3 (raw file):

Previously, dafnamatsry wrote…

What about tests for this function?

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 23 at r3 (raw file):

Previously, dafnamatsry wrote…

There is no meaning in having all these test cases anymore. The different external_tx has really no affect on the test.
I guess we only need 2 cases, one in which mock_validator.validate() returns ok, and another in which it returns an error.

We should add tests to check we properly convert to an eccount tx, but that should be in a test file for utils where the conversion is implemented.

Done.

[yair-starkware]

Previously, yair-starkware (Yair) wrote…

Note that it is separated by commits.
Do you still prefer separated PRs?

Opened preliminary PR: #381

[elintul]

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

a discussion (no related file):

Previously, yair-starkware (Yair) wrote…

Note that it is separated by commits.
Do you still prefer separated PRs?

Yes,

---

---

crates/gateway/src/stateful_transaction_validator.rs line 30 at r5 (raw file):

#[automock]
pub trait StatefulTransactionValidatorTrait {

Can also be separated to another PR.

Code quote:

pub trait StatefulTransactionValidatorTrait

[yair-starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 30 at r5 (raw file):

Previously, elintul (Elin) wrote…

Can also be separated to another PR.

I don't think it is correct to separate it. The only reason to add this trait is for the mock.

[dafnamatsry]

Reviewed 1 of 6 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul)

[dafnamatsry]

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul)

[elintul]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 31 at r6 (raw file):

#[cfg_attr(test, automock)]
pub trait StatefulTransactionValidatorTrait {
    fn perform_validations(

Suggestion:

validate

---

crates/gateway/src/stateful_transaction_validator.rs line 38 at r6 (raw file):

}

impl StatefulTransactionValidatorTrait for BlockifierStatefulValidator {

WDYT about moving this trait to Blockifier?

[yair-starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 31 at r6 (raw file):

#[cfg_attr(test, automock)]
pub trait StatefulTransactionValidatorTrait {
    fn perform_validations(

done

---

crates/gateway/src/stateful_transaction_validator.rs line 38 at r6 (raw file):

Previously, elintul (Elin) wrote…

WDYT about moving this trait to Blockifier?

Added TODO (will do this when we move to mono repo)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 67 at r6 (raw file):

        Ok(..) => Ok(()),
        Err(e) => Err(e),
    });

@elintul - this code is possible thanks to the mockall attribute.
Notice I didn't have to create any new struct in order to mock the trait.

Code quote:

    let mut mock_validator = MockStatefulTransactionValidatorTrait::new();
    mock_validator.expect_perform_validations().return_once(|_, _| match expected_result {
        Ok(..) => Ok(()),
        Err(e) => Err(e),
    });

[dafnamatsry]

Reviewed 4 of 5 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul)

[giladchase]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul)

---

crates/gateway/src/stateful_transaction_validator.rs line 30 at r5 (raw file):

Previously, yair-starkware (Yair) wrote…

I don't think it is correct to separate it. The only reason to add this trait is for the mock.

If the only usage of a trait is by a testing framework then I'm not sure the trait should be added to production.
If, for example, we suspect we'd want to "switch a backend" or implementation for a validator, then the trait might be justified.

Also, if indeed the trait cannot be justified on it's own, without mockall, then I think mockall can be applied directly onto a struct, without a trait (second code block here)

[yair-starkware]

crates/gateway/src/stateful_transaction_validator.rs line 30 at r5 (raw file):

Previously, giladchase wrote…

If the only usage of a trait is by a testing framework then I'm not sure the trait should be added to production.
If, for example, we suspect we'd want to "switch a backend" or implementation for a validator, then the trait might be justified.

Also, if indeed the trait cannot be justified on it's own, without mockall, then I think mockall can be applied directly onto a struct, without a trait (second code block here)

I agree in theory that the trait is not required, but:

1. What you suggest requires changing the type in blockifier, and it is pretty hard to update dependencies right now
2. Not sure the blockifier should provide the ability to mock itself
3. You can't derive automock in structs that implement traits so I don't want to get frustrated in the future because of the derive

[elintul]

Reviewed 1 of 6 files at r6, 3 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yair-starkware)

[elintul]

Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r9 (raw file):

impl StatefulTransactionValidator {
    pub fn run_validate<TStatefulTransactionValidator: StatefulTransactionValidatorTrait>(

Suggestion:

V

---

crates/gateway/src/stateful_transaction_validator_test.rs line 67 at r6 (raw file):

Previously, yair-starkware (Yair) wrote…

@elintul - this code is possible thanks to the mockall attribute.
Notice I didn't have to create any new struct in order to mock the trait.

Can't we use idiomatic combinators here? Err(e) -> Err(e) looks bad. What's the behavior you want here?
Why not:

mock_validator.expect_validate().return_once(|_, _| {
    expected_result.map_err(|e| e)
});

[yair-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 67 at r6 (raw file):

Previously, elintul (Elin) wrote…

Can't we use idiomatic combinators here? Err(e) -> Err(e) looks bad. What's the behavior you want here?
Why not:

mock_validator.expect_validate().return_once(|_, _| {
    expected_result.map_err(|e| e)
});

It also maps Ok(something( to Ok(()), I think match looks better than expected_result.map(...).map_err(...)

[yair-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r9 (raw file):

impl StatefulTransactionValidator {
    pub fn run_validate<TStatefulTransactionValidator: StatefulTransactionValidatorTrait>(

mistake?

[yair-starkware]

crates/gateway/src/stateful_transaction_validator_test.rs line 67 at r6 (raw file):

Previously, yair-starkware (Yair) wrote…

It also maps Ok(something( to Ok(()), I think match looks better than expected_result.map(...).map_err(...)

nvm, done

[elintul]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r9 (raw file):

Previously, yair-starkware (Yair) wrote…

mistake?

Nope, I think usually generic types can be of 1-2 letters, less cognitive load.

[yair-starkware]

crates/gateway/src/stateful_transaction_validator.rs line 58 at r9 (raw file):

Previously, elintul (Elin) wrote…

Nope, I think usually generic types can be of 1-2 letters, less cognitive load.

done

[elintul]

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

[elintul]

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