refactor(fee): tx_args holds valid resource bounds

[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): remove resource mapping usage in rpc and adjust to use all resources #580
• 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

[nimrod-starkware]

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/blockifier/src/test_utils/declare.rs line 111 at r1 (raw file):

                    signature: declare_tx_args.signature,
                    sender_address: declare_tx_args.sender_address,
                    resource_bounds: declare_tx_args.resource_bounds.0.try_into().expect("todo"),

will be handled in an upper branch

Code quote:

0.try_into().expect("todo")

---

crates/blockifier/src/test_utils/deploy_account.rs line 99 at r1 (raw file):

        starknet_api::transaction::DeployAccountTransaction::V3(DeployAccountTransactionV3 {
            signature: deploy_tx_args.signature,
            resource_bounds: deploy_tx_args.resource_bounds.0.try_into().expect("todo"),

ditto

Code quote:

.0.try_into().expect("todo"),

---

crates/blockifier/src/test_utils/invoke.rs line 99 at r1 (raw file):

    } else if invoke_args.version == TransactionVersion::THREE {
        starknet_api::transaction::InvokeTransaction::V3(InvokeTransactionV3 {
            resource_bounds: invoke_args.resource_bounds.0.try_into().expect("todo"),

ditto

Code quote:

s.0.try_into().expect("todo")

---

crates/papyrus_common/src/transaction_hash.rs line 226 at r1 (raw file):

) -> Result<Felt, StarknetApiError> {
    let (l1_resource_bounds, l2_resource_bounds) = match resource_bounds {
        ValidResourceBounds::L1Gas(l1_gas_bounds) => (l1_gas_bounds, &ResourceBounds::default()),

WDYT?
Assuming l2_gas previously was always zero.

Code quote:

&ResourceBounds::default())

---

crates/starknet_api/src/transaction_hash.rs line 181 at r1 (raw file):

) -> Result<Felt, StarknetApiError> {
    let (l1_resource_bounds, l2_resource_bounds) = match resource_bounds {
        ValidResourceBounds::L1Gas(l1_gas_bounds) => (l1_gas_bounds, &ResourceBounds::default()),

ditto

Code quote:

ResourceBounds::default()

[ShahakShama]

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

---

crates/papyrus_protobuf/src/converters/transaction.rs line 577 at r23 (raw file):

Previously, nimrod-starkware wrote…

I spoke with Shahak about it, and he said that's what he and Ariel agreed on.

Yes. @dorimedini-starkware the reason it's ok here is that this is a transaction inside a block, and the gateway would've rejected it if l2_gas=0 but l1_data_gas is present

---

crates/papyrus_protobuf/src/converters/transaction.rs line 582 at r25 (raw file):

}

impl TryFrom<protobuf::ResourceLimits> for ResourceBounds {

Unrelated to your PR: ResourceBounds is very confusing. Can we rename it?

---

crates/papyrus_protobuf/src/proto/p2p/proto/transaction.proto line 13 at r24 (raw file):

Previously, dorimedini-starkware wrote…

what's this...? @ShahakShama any BC issues may arise from this addition?

No, because it's appended to the list of fields. If we would've added it before l2_gas then BC issues would've happened

---

crates/papyrus_rpc/src/v0_6/transaction.rs line 172 at r3 (raw file):

Previously, nimrod-starkware wrote…

WDYM?
Should I panic in the case of ValidResourceBounds::AllResources(_)?

No, you should just convert the l1_gas and l2_gas to the output struct and not use the l1_data_gas
This code is called when the user asks: "Give me all the txs of block 1,000,000". If in block 1,000,000 we have a "new style" transaction then we don't want to panic, we'll just give the data that we know how to give and drop the data we don't know how to. That was what the l2_gas A/C preparation was for, right?

In summary, just remove the TODO

---

crates/papyrus_rpc/src/v0_7/transaction.rs line 190 at r25 (raw file):

                Self { l1_gas, l2_gas: ResourceBounds::default() }
            }
            // TODO(Nimrod): Don't allow this conversion.

Same as before, remove this TODO

---

crates/starknet_api/src/transaction.rs line 990 at r3 (raw file):

Previously, dorimedini-starkware wrote…

backwards compatibility

I think you can derive Serialize with serde(untagged) and the result will be the same. Only in deserialization you are doing something unique

---

crates/starknet_api/src/transaction.rs line 1001 at r22 (raw file):

Previously, nimrod-starkware wrote…

Your suggestion won't work..
It will always deserialize to the AllResources variant...
We need to check somewhere that l2 gas is zero in the first variant

That's why I suggested adding serde(deny_unknown_fields). This way it will distinguish between the two by looking if the l1_data_gas field is present. Is that good enough or do you prefer distinguishing through l2_gas being zero specifically?

---

crates/starknet_client/src/reader/objects/transaction.rs line at r3 (raw file):

Previously, dorimedini-starkware wrote…

yes

Approved

[ShahakShama]

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

[nimrod-starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware, @ShahakShama, and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 582 at r25 (raw file):

Previously, ShahakShama wrote…

Unrelated to your PR: ResourceBounds is very confusing. Can we rename it?

Added a TODO to consider renaming it

---

crates/papyrus_rpc/src/v0_6/transaction.rs line 172 at r3 (raw file):

Previously, ShahakShama wrote…

No, you should just convert the l1_gas and l2_gas to the output struct and not use the l1_data_gas
This code is called when the user asks: "Give me all the txs of block 1,000,000". If in block 1,000,000 we have a "new style" transaction then we don't want to panic, we'll just give the data that we know how to give and drop the data we don't know how to. That was what the l2_gas A/C preparation was for, right?

In summary, just remove the TODO

Done.

---

crates/papyrus_rpc/src/v0_7/transaction.rs line 190 at r25 (raw file):

Previously, ShahakShama wrote…

Same as before, remove this TODO

Done.

---

crates/starknet_api/src/transaction.rs line 990 at r3 (raw file):

Previously, ShahakShama wrote…

I think you can derive Serialize with serde(untagged) and the result will be the same. Only in deserialization you are doing something unique

let's do it in a separate PR, i added a TODO

---

crates/starknet_api/src/transaction.rs line 1001 at r22 (raw file):

Previously, ShahakShama wrote…

That's why I suggested adding serde(deny_unknown_fields). This way it will distinguish between the two by looking if the l1_data_gas field is present. Is that good enough or do you prefer distinguishing through l2_gas being zero specifically?

Will do it in a separate PR (added a TODO)

[dorimedini-starkware]

Reviewed 3 of 3 files at r26, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nimrod-starkware, @ShahakShama, and @TzahiTaub)

[ShahakShama]

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

[ShahakShama]

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

---

crates/papyrus_protobuf/src/converters/transaction.rs line 567 at r26 (raw file):

        };
        let Some(l1_data_gas) = value.l1_data_gas else {
            return Err(ProtobufConversionError::MissingField {

Don't return an error here. Use unwrap_or_default and add a TODO to assert this is not None once we remove support for 0.13.2

[nimrod-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @ShahakShama, and @TzahiTaub)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 567 at r26 (raw file):

Previously, ShahakShama wrote…

Don't return an error here. Use unwrap_or_default and add a TODO to assert this is not None once we remove support for 0.13.2

Done.

[nimrod-starkware]

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

---

crates/papyrus_protobuf/src/converters/transaction.rs line 567 at r26 (raw file):

Previously, nimrod-starkware wrote…

Done.

Added your name to the TODO if that's ok :)

[ShahakShama]

Reviewed 1 of 2 files at r27.
Reviewable status: 42 of 44 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

[ShahakShama]

Reviewable status: 42 of 44 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

[nimrod-starkware]

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

---

crates/papyrus_protobuf/src/converters/transaction.rs line 652 at r11 (raw file):

Previously, ShahakShama wrote…

Yes, but you need to add the new fields at the end of the message so that it can work with nodes that didn't do this upgrade (Juno/Pathfinder)

Done.

[nimrod-starkware]

Dismissed @dorimedini-starkware from a discussion.
Reviewable status: 42 of 44 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @TzahiTaub)

k

[nimrod-starkware]

Reviewed 1 of 1 files at r28, all commit messages.
Reviewable status: 43 of 44 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[nimrod-starkware]

Reviewed 1 of 2 files at r27.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[nimrod-starkware]

Merge activity

• Sep 4, 9:01 AM EDT: @nimrod-starkware started a stack merge that includes this pull request via Graphite.
• Sep 4, 9:02 AM EDT: @nimrod-starkware merged this pull request with Graphite.