feat: stateless validator add non zero fee check

[ArniStarkware]

This change is 

[dafnamatsry]

Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

crates/gateway/src/transaction_validator.rs line 20 at r1 (raw file):

    fn default() -> Self {
        Self {
            fee_resource: Resource::L1Gas,

Please document, and consider making it a boolean.

---

crates/gateway/src/transaction_validator.rs line 53 at r1 (raw file):

            },
        };
        let resource_bounds = resource_bounds_mapping.0[&resource];

Can it panic?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.64%. Comparing base (b595276) to head (bc2d4cc).

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #29       +/-   ##
===========================================
+ Coverage   48.48%   64.64%   +16.16%     
===========================================
  Files           5        5               
  Lines          66       99       +33     
  Branches       66       99       +33     
===========================================
+ Hits           32       64       +32     
- Misses         34       35        +1     

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

[ArniStarkware]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/gateway/src/transaction_validator.rs line 20 at r1 (raw file):

Previously, dafnamatsry wrote…

Please document, and consider making it a boolean.

Done.

---

crates/gateway/src/transaction_validator.rs line 53 at r1 (raw file):

Previously, dafnamatsry wrote…

Can it panic?

Yes, it can panic. Now, we are handling this case (Which we need to validate).

Done.

[dafnamatsry]

Reviewed 2 of 6 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 5 of 7 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

crates/gateway/src/transaction_validator.rs line 18 at r4 (raw file):

pub struct TransactionValidatorConfig {
    // TODO(Arni, 1/5/2024): Consider squashing this config into a single field. Is it possible to
    //  use more than one gas for fee in a single flow?

Please ask Elin, so we can remove this TODO.

Code quote:

    // TODO(Arni, 1/5/2024): Consider squashing this config into a single field. Is it possible to
    //  use more than one gas for fee in a single flow?

---

crates/gateway/src/transaction_validator.rs line 57 at r4 (raw file):

            {
                continue;
            }

Once you explicitly filter just 2 Resource variants, then I don't see the point of iterating with a for loop.
Moreover, lets' say we now add a third resource, then we check here that it is not zero, which can be a bug.

I would suggest something like:

if self.config.validate_non_zero_l1_gas_fee {
  validate_non_zero(resource_mapping, Resource::L1Gas);
}
same for L2...

Code quote:

        for resource in Resource::iter() {
            if (resource == Resource::L1Gas && !self.config.validate_non_zero_l1_gas_fee)
                || (resource == Resource::L2Gas && !self.config.validate_non_zero_l2_gas_fee)
            {
                continue;
            }

---

crates/gateway/src/transaction_validator.rs line 71 at r4 (raw file):

                    }
                }
            }

Suggestion:

            if let Some(resource_bounds) = resource_bounds_mapping.0.get(&resource) {
                if resource_bounds.max_amount == 0 || resource_bounds.max_price_per_unit == 0 {
                    return Err(TransactionValidatorError::ZeroFee {
                        resource,
                        resource_bounds: *resource_bounds,
                    });
                }
            } else {
                return Err(TransactionValidatorError::MissingResource { resource });
            }

---

crates/gateway/src/transaction_validator_test.rs line 30 at r4 (raw file):

    Ok(())
)]
#[case::malformed_resource_bounds(

Suggestion:

missing_l1_gas_resource_bounds

---

crates/gateway/src/transaction_validator_test.rs line 35 at r4 (raw file):

    Err(TransactionValidatorError::MissingResource { resource: Resource::L1Gas })
)]
#[case::malformed_l2_gas_resource_bounds(

Suggestion:

missing_l2_gas_resource_bounds

---

crates/gateway/src/transaction_validator_test.rs line 39 at r4 (raw file):

        validate_non_zero_l1_gas_fee: false,
        validate_non_zero_l2_gas_fee: true,
    },

Consider extracting this to a constant as well...

Code quote:

    TransactionValidatorConfig{
        validate_non_zero_l1_gas_fee: false,
        validate_non_zero_l2_gas_fee: true,
    },

---

crates/gateway/src/transaction_validator_test.rs line 43 at r4 (raw file):

    Err(TransactionValidatorError::MissingResource { resource: Resource::L2Gas })
)]
#[case::invalid_resource_bounds(

Suggestion:

zero_l1_gas_resource_bounds

---

crates/gateway/src/transaction_validator_test.rs line 50 at r4 (raw file):

    })
)]
#[case::invalid_l2_gas_resource_bounds(

Suggestion:

zero_l2_gas_resource_bounds

[ArniStarkware]

Reviewable status: 1 of 7 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/gateway/src/transaction_validator.rs line 57 at r4 (raw file):

Previously, dafnamatsry wrote…

Once you explicitly filter just 2 Resource variants, then I don't see the point of iterating with a for loop.
Moreover, lets' say we now add a third resource, then we check here that it is not zero, which can be a bug.

I would suggest something like:

if self.config.validate_non_zero_l1_gas_fee {
  validate_non_zero(resource_mapping, Resource::L1Gas);
}
same for L2...

Done.

---

crates/gateway/src/transaction_validator.rs line 71 at r4 (raw file):

                    }
                }
            }

Done.

---

crates/gateway/src/transaction_validator_test.rs line 30 at r4 (raw file):

    Ok(())
)]
#[case::malformed_resource_bounds(

Done.

---

crates/gateway/src/transaction_validator_test.rs line 35 at r4 (raw file):

    Err(TransactionValidatorError::MissingResource { resource: Resource::L1Gas })
)]
#[case::malformed_l2_gas_resource_bounds(

Done.

---

crates/gateway/src/transaction_validator_test.rs line 39 at r4 (raw file):

Previously, dafnamatsry wrote…

Consider extracting this to a constant as well...

Done.

---

crates/gateway/src/transaction_validator_test.rs line 43 at r4 (raw file):

    Err(TransactionValidatorError::MissingResource { resource: Resource::L2Gas })
)]
#[case::invalid_resource_bounds(

Done.

---

crates/gateway/src/transaction_validator_test.rs line 50 at r4 (raw file):

    })
)]
#[case::invalid_l2_gas_resource_bounds(

Done.

[ArniStarkware]

Reviewable status: 0 of 7 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry, @noaov1, and @Yael-Starkware)

---

crates/gateway/src/transaction_validator.rs line 18 at r4 (raw file):

Previously, dafnamatsry wrote…

Please ask Elin, so we can remove this TODO.

@noaov1 agrees. It is planned that, in the near future, most txs will have both an L1 and L2 gas-cost component.

We would like to have both fields in that case.
Removed the TODO.

[dafnamatsry]

Reviewed 1 of 6 files at r2, 3 of 5 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yael-Starkware)