refactor(papyrus_rpc): add l1_data_gas to ResourceBoundsMapping according to V08

[ayeletstarkware]

according to spec
https://github.com/starkware-libs/starknet-specs/blob/28ad3f56c147967b63ecfc78db5f4ba1df993e05/api/starknet_api_openrpc.json#L3484

[reviewable-StarkWare]

This change is 

[ayeletstarkware]

• refactor(papyrus_rpc): change InvokeTransaction TryFrom func to From func #3751
• refactor(papyrus_rpc): change DeployAccountTransaction TryFrom func to From func #3750
• refactor(papyrus_rpc): add l1_data_gas to ResourceBoundsMapping according to V08 #3747 👈 (View in Graphite)
• main

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

[ArniStarkware]

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @ayeletstarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 159 at r1 (raw file):

    pub l1_data_gas: ResourceBounds,
    pub l2_gas: ResourceBounds,
}

Are we sure this is what we want this struct to look like?
I am confused with the pre 0.13.3 Resource bounds vs current resource bounds.

Should this struct fit both cases?

Code quote:

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

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

            l2_gas: value.l2_gas,
        })
    }

If I understand correctly.
This keeps the current functionality.

Suggestion:

    fn from(value: ResourceBoundsMapping) -> Self {
        if value.l2_gas.is_zero() {
            Self::L1Gas(value.l1_gas)
        } else {
            Self::AllResources(AllResourceBounds {
                l1_gas: value.l1_gas,
                l1_data_gas: value.l1_data_gas,
                l2_gas: value.l2_gas,
            })
        }
    }

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 203 at r1 (raw file):

            starknet_api::transaction::fields::ValidResourceBounds::L1Gas(l1_gas) => Self {
                l1_gas,
                l1_data_gas: ResourceBounds::default(),

Non-blocking - As this might be a "for later" problem.

Do we have an empty for this struct? Should we?

Isn't part of the goal of this stack to delete the GasPrice struct? If we would it would replace it in ResourceBounds with non-empty gas-price, and it does not have a default IIUC.

Suggestion:

ResourceBounds::empty()

[ayeletstarkware]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 159 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Are we sure this is what we want this struct to look like?
I am confused with the pre 0.13.3 Resource bounds vs current resource bounds.

Should this struct fit both cases?

According to the spec (attached in the PR's description), this is what is needed.

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

If I understand correctly.
This keeps the current functionality.

The current functionality is relevant only when l2_gas is zero. I am removing this possibility in a separate PR.

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 203 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Non-blocking - As this might be a "for later" problem.

Do we have an empty for this struct? Should we?

Isn't part of the goal of this stack to delete the GasPrice struct? If we would it would replace it in ResourceBounds with non-empty gas-price, and it does not have a default IIUC.

Yes, this is part of refactoring the GasPrice struct to make it nonzero. I believe empty is not an appropriate name because the max_price_per_unit in ResourceBounds is not truly "empty."

[ArniStarkware]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ayeletstarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The current functionality is relevant only when l2_gas is zero. I am removing this possibility in a separate PR.

Then, when this possibility is removed, remove the conditional check and this if {} else {}.

[ArniStarkware]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ayeletstarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 203 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Yes, this is part of refactoring the GasPrice struct to make it nonzero. I believe empty is not an appropriate name because the max_price_per_unit in ResourceBounds is not truly "empty."

This will hunt us later - derive Default for ResourceBounds will soon fail. But again - this is nonblocking.
(I try to be careful about new calls to ::default(), as was discussed in the group sync a few weeks ago).

[ArniStarkware]

crates/papyrus_rpc/src/v0_8/transaction.rs line 199 at r1 (raw file):

impl From<starknet_api::transaction::fields::ValidResourceBounds> for ResourceBoundsMapping {
    fn from(value: starknet_api::transaction::fields::ValidResourceBounds) -> Self {

Import it for better readability.

Code quote:

starknet_api::transaction::fields::ValidResourceBounds

[ayeletstarkware]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Then, when this possibility is removed, remove the conditional check and this if {} else {}.

Is it necessary? ResourceBoundsMapping has three fields, converted to AllResources with three fields. Can V08 ResourceBoundsMapping map directly to L1Gas using just one of its fields? @alonh5 WDYT?

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 199 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Import it for better readability.

Done.

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 203 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This will hunt us later - derive Default for ResourceBounds will soon fail. But again - this is nonblocking.
(I try to be careful about new calls to ::default(), as was discussed in the group sync a few weeks ago).

Added a task to monday to consider that.

[ArniStarkware]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is it necessary? ResourceBoundsMapping has three fields, converted to AllResources with three fields. Can V08 ResourceBoundsMapping map directly to L1Gas using just one of its fields? @alonh5 WDYT?

For the record - I am worried about this conversion:

    let valid_resource_bounds = ValidResourceBounds::L1Gas(ResourceBounds::default());
    // The following assertion fails, as these are not of the same enum variant.
    assert_eq!(
        valid_resource_bounds,
        ValidResourceBounds::from(ResourceBoundsMapping::from(valid_resource_bounds))
    );

[alonh5]

Reviewed 1 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

For the record - I am worried about this conversion:

    let valid_resource_bounds = ValidResourceBounds::L1Gas(ResourceBounds::default());
    // The following assertion fails, as these are not of the same enum variant.
    assert_eq!(
        valid_resource_bounds,
        ValidResourceBounds::from(ResourceBoundsMapping::from(valid_resource_bounds))
    );

I did some research, this is the PR where this was merged. It was previously in v07 and then was changed later to v08 without touching this try_from. I'm guessing it was overlooked.
Let's keep it backward-compatible for now by creating the L1_Gas if both the others are 0, and the AllResources otherwise.

[ayeletstarkware]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ArniStarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 195 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I did some research, this is the PR where this was merged. It was previously in v07 and then was changed later to v08 without touching this try_from. I'm guessing it was overlooked.
Let's keep it backward-compatible for now by creating the L1_Gas if both the others are 0, and the AllResources otherwise.

Nice! done.

[alonh5]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @ayeletstarkware)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 199 at r3 (raw file):

        } else {
            Self::L1Gas(value.l1_gas)
        }

If both are zero you should create L1, otherwise create ALL. (By logic rules what you're currently doing is an OR not an AND).

Suggestion:

        if value.l1_data_gas.is_zero() && value.l2_gas.is_zero() {
            Self::L1Gas(value.l1_gas)
        } else {
            Self::AllResources(AllResourceBounds {
                l1_gas: value.l1_gas,
                l1_data_gas: value.l1_data_gas,
                l2_gas: value.l2_gas,
            })
        }

[ArniStarkware]

crates/papyrus_rpc/src/v0_8/transaction.rs line 199 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

If both are zero you should create L1, otherwise create ALL. (By logic rules what you're currently doing is an OR not an AND).

Also - positive clauses are more readable.

[alonh5]

Reviewed 1 of 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

[ayeletstarkware]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5)

---

crates/papyrus_rpc/src/v0_8/transaction.rs line 199 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Also - positive clauses are more readable.

done.

[alonh5]

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

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)