build(fee): define valid resources bounds enum

[nimrod-starkware]

This change is 

[nimrod-starkware]

• refactor(fee): tx_args holds valid resource bounds #508 : 2 dependent PRs (#509 , #537 )
• refactor(fee): block context creator may fail #507
• refactor(fee): transaction info creator may fail #506
• build(fee): current tx_info holds validResourceBounds #505
• refactor(fee): rename ResourceBoundsMapping => DeprecatedResourceBoundsMapping #504
• build(fee): define valid resources bounds enum #503 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @nimrod-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 8.57143% with 32 lines in your changes missing coverage. Please review.

Project coverage is 76.53%. Comparing base (cee1f5b) to head (df36dc5).
Report is 3 commits behind head on main.

Files	Patch %	Lines
crates/starknet_api/src/transaction.rs	9.09%	30 Missing ⚠️
...lockifier/src/execution/syscalls/hint_processor.rs	0.00%	1 Missing ⚠️
...tes/gateway/src/stateless_transaction_validator.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
- Coverage   76.58%   76.53%   -0.06%     
==========================================
  Files         348      348              
  Lines       36771    36804      +33     
  Branches    36771    36804      +33     
==========================================
+ Hits        28162    28168       +6     
- Misses       6284     6314      +30     
+ Partials     2325     2322       -3     

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

[dorimedini-starkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

a discussion (no related file):
you added todo!s here, so please open a python side PR (I am concerned about the OS test mainly)

---

---

crates/starknet_api/src/transaction.rs line 870 at r1 (raw file):

    #[serde(rename = "L2_GAS")]
    L2Gas,
    L1DataGas,

Suggestion:

    #[serde(rename = "L2_GAS")]
    L2Gas,
    #[serde(rename = "L1_DATA_GAS")]
    L1DataGas,

---

crates/starknet_api/src/transaction.rs line 891 at r1 (raw file):

    pub fn is_zero(&self) -> bool {
        self.max_amount == 0 && self.max_price_per_unit == 0
    }

name is a bit ambiguous - if one of the two fields is zero, the effective bound is zero.
consider another name / a docstring?

Code quote:

    pub fn is_zero(&self) -> bool {
        self.max_amount == 0 && self.max_price_per_unit == 0
    }

---

crates/starknet_api/src/transaction.rs line 953 at r1 (raw file):

pub enum ValidResourceBounds {
    L1Gas(ResourceBounds), // Pre 0.13.3. L2 bounds are signed but never used.

Suggestion:

L1Gas(ResourceBounds), // Pre 0.13.3. Only L1 gas. L2 bounds are signed but never used.

---

crates/starknet_api/src/transaction.rs line 961 at r1 (raw file):

    pub l2_gas: ResourceBounds,
    pub l1_data_gas: ResourceBounds,
}

define and implement this, to enforce the connection between AllResourceBounds and the Resource enum

Suggestion:

pub struct AllResourceBounds {
    pub l1_gas: ResourceBounds,
    pub l2_gas: ResourceBounds,
    pub l1_data_gas: ResourceBounds,
}

impl AllResourceBounds {
    fn get_bound(&self, resource: Resource) -> ResourceBounds { ... }
}

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

    fn try_from(
        raw_resource_bounds: HashMap<Resource, ResourceBounds>,
    ) -> Result<Self, Self::Error> {

suggestion: while the ResourceBoundsMapping is still alive, make it clear that it converts to the new type.
may be useful in refactoring.
WDYT? (non-blocking, you may have other design plans)

Suggestion:

impl TryFrom<ResourceBoundsMapping> for ValidResourceBounds {
    type Error = StarknetApiError;
    fn try_from(
        raw_resource_bounds: ResourceBoundsMapping,
    ) -> Result<Self, Self::Error> {

---

crates/starknet_api/src/transaction.rs line 984 at r1 (raw file):

                            "{:?}",
                            raw_resource_bounds
                        )))

more information please

Suggestion:

                        Err(StarknetApiError::InvalidResourceMappingInitializer(format!(
                            "Missing DA gas bound, but L2 gas bound is not zero: {:?}",
                            raw_resource_bounds
                        )))

---

crates/starknet_api/src/transaction.rs line 984 at r1 (raw file):

                            "{:?}",
                            raw_resource_bounds
                        )))

I prefer inlining the variable in the formatted string (non-blocking), it is more readable when you have several variables in the string

Suggestion:

                        Err(StarknetApiError::InvalidResourceMappingInitializer(format!(
                            "{raw_resource_bounds:?}"
                        )))

[dorimedini-starkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

you added todo!s here, so please open a python side PR (I am concerned about the OS test mainly)

you can open such a PR for the top of your stack if you want, maybe better

---

[nimrod-starkware]

Reviewable status: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/starknet_api/src/transaction.rs line 870 at r1 (raw file):

    #[serde(rename = "L2_GAS")]
    L2Gas,
    L1DataGas,

Done.

---

crates/starknet_api/src/transaction.rs line 891 at r1 (raw file):

Previously, dorimedini-starkware wrote…

name is a bit ambiguous - if one of the two fields is zero, the effective bound is zero.
consider another name / a docstring?

added a docstring

---

crates/starknet_api/src/transaction.rs line 953 at r1 (raw file):

pub enum ValidResourceBounds {
    L1Gas(ResourceBounds), // Pre 0.13.3. L2 bounds are signed but never used.

Done.

---

crates/starknet_api/src/transaction.rs line 961 at r1 (raw file):

Previously, dorimedini-starkware wrote…

define and implement this, to enforce the connection between AllResourceBounds and the Resource enum

Done.

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

Previously, dorimedini-starkware wrote…

suggestion: while the ResourceBoundsMapping is still alive, make it clear that it converts to the new type.
may be useful in refactoring.
WDYT? (non-blocking, you may have other design plans)

I want to use ResourceBoundsMapping as little as possible (it will be easier to remove it later)...

---

crates/starknet_api/src/transaction.rs line 984 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I prefer inlining the variable in the formatted string (non-blocking), it is more readable when you have several variables in the string

Done

---

crates/starknet_api/src/transaction.rs line 984 at r1 (raw file):

Previously, dorimedini-starkware wrote…

more information please

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

Previously, nimrod-starkware wrote…

I want to use ResourceBoundsMapping as little as possible (it will be easier to remove it later)...

but this TryFrom is coupled to this type. enforcing this coupling will keep code stable in intermediate phases, and the final search for usages will remind you to update this to the relevant input type / move the logic to the general tx deserialization code / delete this TryFrom entirely

[nimrod-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

Previously, dorimedini-starkware wrote…

but this TryFrom is coupled to this type. enforcing this coupling will keep code stable in intermediate phases, and the final search for usages will remind you to update this to the relevant input type / move the logic to the general tx deserialization code / delete this TryFrom entirely

Done

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

Previously, nimrod-starkware wrote…

Done

pushed?

[nimrod-starkware]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/starknet_api/src/transaction.rs line 967 at r1 (raw file):

Previously, dorimedini-starkware wrote…

pushed?

now pushed, sorry :)

[dorimedini-starkware]

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

[dorimedini-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @TzahiTaub)

[dorimedini-starkware]

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

[nimrod-starkware]

Merge activity

• Aug 25, 3:32 AM EDT: @nimrod-starkware started a stack merge that includes this pull request via Graphite.
• Aug 25, 3:32 AM EDT: @nimrod-starkware merged this pull request with Graphite.