Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @Yael-Starkware)

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

        let bytecode_size = contract_class.sierra_program.len();
        let serialized_class = serde_json::to_string(&contract_class)
            .expect("Unexpected error serializing contract class.");

I use expect here as I do not understand how the serialization may fail.

Should I use some safer mechanism?

Code quote:

        let serialized_class = serde_json::to_string(&contract_class)
            .expect("Unexpected error serializing contract class.");

crates/gateway/src/stateless_transaction_validator.rs line 141 at r1 (raw file):

    raw_class_size: usize,
    max_raw_class_size: usize,
) -> Result<(), DeclareTransactionError> {

We expect to use this function for validations after the compilation as well. We will move it when time comes.

Code quote:

fn validate_class_size(
    bytecode_language: String,
    bytecode_size: usize,
    max_bytecode_size: usize,
    raw_class_size: usize,
    max_raw_class_size: usize,
) -> Result<(), DeclareTransactionError> {

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

Not sure what can be a failure reason, one thing I found online is a limit on recursion depth. Not sure if this is relevant for this case but I would prefer the safe error handling here.

crates/gateway/src/stateless_transaction_validator.rs line 141 at r1 (raw file):

why check the same thing twice?

crates/gateway/src/stateless_transaction_validator_test.rs line 28 at r1 (raw file):

        max_signature_length: 1,
        max_bytecode_size: 3,
        max_raw_class_size: 408,

does these numbers have a meaning or are they random?

crates/gateway/src/stateless_transaction_validator_test.rs line 174 at r1 (raw file):

    };
    let mut sierra_program = vec![stark_felt!(1_u128), stark_felt!(3_u128), stark_felt!(0_u128)];
    sierra_program.extend(vec![stark_felt!(12_u128); config_max_bytecode_size]);

Suggestion:

    let sierra_program = vec![stark_felt!(1_u128); onfig_max_bytecode_size+3];

crates/gateway/src/stateless_transaction_validator_test.rs line 191 at r1 (raw file):

            assert_eq!(bytecode_size, sierra_program_length);
            assert_eq!(max_bytecode_size, config_max_bytecode_size);
        }

nice! I wasn't aware of this syntax

Code quote:

        ) => {
            assert_eq!(bytecode_language, "Sierra");
            assert_eq!(bytecode_size, sierra_program_length);
            assert_eq!(max_bytecode_size, config_max_bytecode_size);
        }

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

Done.

crates/gateway/src/stateless_transaction_validator.rs line 141 at r1 (raw file):

On the other case - it will validate the Casm class size. In this PR this function is used to validate the Sierra class size.

crates/gateway/src/stateless_transaction_validator_test.rs line 28 at r1 (raw file):

These are the minimal numbers for irrelevant cases to pass.
Fixed.

crates/gateway/src/stateless_transaction_validator_test.rs line 174 at r1 (raw file):

    };
    let mut sierra_program = vec![stark_felt!(1_u128), stark_felt!(3_u128), stark_felt!(0_u128)];
    sierra_program.extend(vec![stark_felt!(12_u128); config_max_bytecode_size]);

Done.

Reviewed 2 of 3 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

would be prettier to use "?" instead of map_err.

crates/gateway/src/stateless_transaction_validator_test.rs line 28 at r1 (raw file):

not sure I understand, these number are not test specific?
I'm asking because in test_declare_contract_class_size_too_long you have 3 felts in the sierra program , which equals the default max_bytecode_size that you defined here.
If these are test specific I think they shouldn't be in the default setting, but if they are general to any test then it's fine.

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

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

#[derive(Debug, Error)]
#[cfg_attr(test, derive(PartialEq))]
pub enum DeclareTransactionError {

@dafnamatsry WDYT?

Suggestion:

StatelessDeclareTransactionValidatorError

crates/gateway/src/starknet_api_test_utils.rs line 55 at r3 (raw file):

            };
            external_declare_tx(declare_tx_args!(resource_bounds, signature, contract_class))
        }

Used in the positive flow test.

Code quote:

        TransactionType::Declare => {
            let contract_class = ContractClass {
                sierra_program: vec![stark_felt!(1_u32); 3],
                ..ContractClass::default()
            };
            external_declare_tx(declare_tx_args!(resource_bounds, signature, contract_class))
        }

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

This means I need to have a transparent error.
I don't want StatelessTransactionValidatorError to expose the inner serde json error.

Besides - serde_json::Error does not implement PartialEq.

But of you insist once more - I will make it happen.

crates/gateway/src/stateless_transaction_validator_test.rs line 28 at r1 (raw file):

Replaced with non-test-specific values - but with a healthy margin.

Tests unrelated to these values should not fail on these values.

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @dafnamatsry)

Reviewed 1 of 4 files at r1, 1 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @ArniStarkware)

crates/gateway/src/config.rs line 75 at r3 (raw file):

    pub max_bytecode_size: usize,
    pub max_raw_class_size: usize,
}

Suggestion:

pub struct StatelessTransactionValidatorConfig {
    // If true, validates that the resource bounds are not zero.
    pub validate_non_zero_l1_gas_fee: bool,
    pub validate_non_zero_l2_gas_fee: bool,

    pub max_calldata_length: usize,
    pub max_signature_length: usize,
    
    // Declare txs specific config.
    pub max_bytecode_size: usize,
    pub max_raw_class_size: usize,
}

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

I don't think we need a specific type of Declare txs error. It can be under the StatelessTransactionValidatorError.
It complicates the code, and it is also confusing because it doesn't really include all the possible error (declare txs can also have errors that are general to all tx types).

crates/gateway/src/errors.rs line 62 at r3 (raw file):

        "Declared contract class {bytecode_language} bytecode size is {bytecode_size}. It must be \
         less then {max_bytecode_size}."
    )]

Please use the same string that we have now in the pythonic GW.

Code quote:

    #[error(
        "Declared contract class {bytecode_language} bytecode size is {bytecode_size}. It must be \
         less then {max_bytecode_size}."
    )]

crates/gateway/src/errors.rs line 71 at r3 (raw file):

        "Declared contract class {bytecode_language} size is {contract_class_object_size}. It \
         must be less then {max_contract_class_object_size}."
    )]

Same.

Code quote:

    #[error(
        "Declared contract class {bytecode_language} size is {contract_class_object_size}. It \
         must be less then {max_contract_class_object_size}."
    )]

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

I actually think that expect is more suitable here, since it is a bug of ours if it fails here.

crates/gateway/src/stateless_transaction_validator.rs line 141 at r1 (raw file):

In the Casm class validation we also have bytecode and raw sizes?
In any case, I would prefer to have a dedicated validate_casm_contract_class as it may have additional specific validations to Casm.

crates/gateway/src/stateless_transaction_validator.rs line 30 at r3 (raw file):

        self.validate_tx_size(tx)?;

        if let ExternalTransaction::Declare(ExternalDeclareTransaction::V3(declare_tx)) = tx {

Suggestion:

if let ExternalTransaction::Declare(declare_tx)

crates/gateway/src/stateless_transaction_validator.rs line 107 at r3 (raw file):

    fn validate_declare_tx(
        &self,
        tx: &ExternalDeclareTransactionV3,

Suggestion:

ExternalDeclareTransaction

crates/gateway/src/stateless_transaction_validator.rs line 111 at r3 (raw file):

        self.validate_class_length(&tx.contract_class)?;

        Ok(())

Suggestion:

        self.validate_class_length(&tx.contract_class)

crates/gateway/src/stateless_transaction_validator.rs line 132 at r3 (raw file):

            raw_class_size,
            self.config.max_raw_class_size,
        )?)

Please move the checks here and delete validate_class_size.

Code quote:

        Ok(validate_class_size(
            "Sierra".to_string(),
            bytecode_size,
            self.config.max_bytecode_size,
            raw_class_size,
            self.config.max_raw_class_size,
        )?)

crates/gateway/src/stateless_transaction_validator_test.rs line 25 at r3 (raw file):

        validate_non_zero_l2_gas_fee: true,

        max_calldata_length: 1,

Suggestion:

        validate_non_zero_l2_gas_fee: true,
        max_calldata_length: 1,

crates/gateway/src/stateless_transaction_validator_test.rs line 168 at r3 (raw file):

        config: StatelessTransactionValidatorConfig {
            validate_non_zero_l1_gas_fee: false,
            validate_non_zero_l2_gas_fee: false,

Why not use the default?

Code quote:

            validate_non_zero_l1_gas_fee: false,
            validate_non_zero_l2_gas_fee: false,

crates/gateway/src/stateless_transaction_validator_test.rs line 174 at r3 (raw file):

    };
    let sierra_program = vec![stark_felt!(1_u128); config_max_bytecode_size + 1];
    let sierra_program_length = sierra_program.len();

Suggestion:

    let sierra_program_length = config_max_bytecode_size + 1;
    let sierra_program = vec![stark_felt!(1_u128); sierra_program_length];
    

crates/gateway/src/stateless_transaction_validator_test.rs line 191 at r3 (raw file):

            assert_eq!(max_bytecode_size, config_max_bytecode_size);
        }
    )

Suggestion:

    assert_matches!(
        tx_validator.validate(&tx).unwrap_err(),
        StatelessTransactionValidatorError::DeclareTransactionError(
            DeclareTransactionError::BytecodeSizeTooLarge {
                bytecode_language,
                bytecode_size,
                max_bytecode_size
            }
        ) if (bytecode_language, bytecode_size, max_bytecode_size) ==
        ("Sierra", sierra_program_length, config_max_bytecode_size);
    )

crates/gateway/src/stateless_transaction_validator_test.rs line 200 at r3 (raw file):

        config: StatelessTransactionValidatorConfig {
            validate_non_zero_l1_gas_fee: false,
            validate_non_zero_l2_gas_fee: false,

Same.

Code quote:

            validate_non_zero_l1_gas_fee: false,
            validate_non_zero_l2_gas_fee: false,

crates/gateway/src/stateless_transaction_validator_test.rs line 207 at r3 (raw file):

    let contract_class =
        ContractClass { sierra_program: vec![stark_felt!(1_u128); 3], ..Default::default() };
    let contract_class_len = serde_json::to_string(&contract_class).unwrap().len();

Suggestion:

contract_class_length

crates/gateway/src/stateless_transaction_validator_test.rs line 219 at r3 (raw file):

            assert_eq!(max_contract_class_object_size, config_max_raw_class_size);
        }
    )

Same.

Code quote:

    assert_matches!(
        tx_validator.validate(&tx).unwrap_err(),
        StatelessTransactionValidatorError::DeclareTransactionError(
            DeclareTransactionError::ContractClassObjectSizeTooLarge { bytecode_language, contract_class_object_size, max_contract_class_object_size }
        ) => {
            assert_eq!(bytecode_language, "Sierra");
            assert_eq!(contract_class_object_size, contract_class_len);
            assert_eq!(max_contract_class_object_size, config_max_raw_class_size);
        }
    )

Reviewable status: 4 of 5 files reviewed, 15 unresolved discussions (waiting on @ArniStarkware)

crates/gateway/src/errors.rs line 51 at r4 (raw file):

    #[error(transparent)]
    DeclareTransactionError(#[from] DeclareTransactionError),
    // TODO(Arni): Cosider a transparant error for serde_json::Error.

Please remove.

Code quote:

// TODO(Arni): Cosider a transparant error for serde_json::Error.

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

crates/gateway/src/config.rs line 75 at r3 (raw file):

    pub max_bytecode_size: usize,
    pub max_raw_class_size: usize,
}

Done.

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

Done.

crates/gateway/src/errors.rs line 62 at r3 (raw file):

Done.

crates/gateway/src/errors.rs line 71 at r3 (raw file):

Done.

crates/gateway/src/errors.rs line 51 at r4 (raw file):

Done.

crates/gateway/src/stateless_transaction_validator.rs line 120 at r1 (raw file):

Done.

crates/gateway/src/stateless_transaction_validator.rs line 141 at r1 (raw file):

Done.

We will extract that to a function if need be.

crates/gateway/src/stateless_transaction_validator.rs line 30 at r3 (raw file):

        self.validate_tx_size(tx)?;

        if let ExternalTransaction::Declare(ExternalDeclareTransaction::V3(declare_tx)) = tx {

Done.

crates/gateway/src/stateless_transaction_validator.rs line 107 at r3 (raw file):

    fn validate_declare_tx(
        &self,
        tx: &ExternalDeclareTransactionV3,

Done.

crates/gateway/src/stateless_transaction_validator.rs line 111 at r3 (raw file):

        self.validate_class_length(&tx.contract_class)?;

        Ok(())

Done.

crates/gateway/src/stateless_transaction_validator.rs line 132 at r3 (raw file):

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 25 at r3 (raw file):

        validate_non_zero_l2_gas_fee: true,

        max_calldata_length: 1,

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 168 at r3 (raw file):

Done. (was just fixed in a different PR).

crates/gateway/src/stateless_transaction_validator_test.rs line 174 at r3 (raw file):

    };
    let sierra_program = vec![stark_felt!(1_u128); config_max_bytecode_size + 1];
    let sierra_program_length = sierra_program.len();

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 191 at r3 (raw file):

            assert_eq!(max_bytecode_size, config_max_bytecode_size);
        }
    )

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 200 at r3 (raw file):

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 207 at r3 (raw file):

    let contract_class =
        ContractClass { sierra_program: vec![stark_felt!(1_u128); 3], ..Default::default() };
    let contract_class_len = serde_json::to_string(&contract_class).unwrap().len();

Done.

crates/gateway/src/stateless_transaction_validator_test.rs line 219 at r3 (raw file):

Done.

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)

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

crates/gateway/src/errors.rs line 63 at r5 (raw file):

    },
    #[error("Error serializing object: {object_name}")]
    SerializationError { object_name: String },

Is it used?

Code quote:

SerializationError

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

crates/gateway/src/errors.rs line 63 at r5 (raw file):

No :). Removed.

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