Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

feat(execution): use gas costs only from VersionedConstants #1392

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Jan 29, 2024

  • Remove from constants module and replace all usages with VersionedConstants#gas_cost(..)
    • Move all comments from the constants module into the const whitelist in VersionedConstants.
  • Add gas cost getter to EntryPointExecutionContext, for readability
    • enclose in closure inside hint_processor.rs for even more brevity.
  • Move Transaction::Initial_gas into VersionedConstants: it is now derived from the constants json.
  • No other logic changes.

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

Attention: 174 lines in your changes are missing coverage. Please review.

Comparison is base (29d2d5f) 70.37% compared to head (1af90d3) 70.83%.
Report is 7 commits behind head on main-v0.13.1.

Files Patch % Lines
...ve_blockifier/src/py_transaction_execution_info.rs 0.00% 57 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs 46.03% 20 Missing and 14 partials ⚠️
crates/blockifier/src/versioned_constants.rs 73.68% 22 Missing and 3 partials ⚠️
crates/native_blockifier/src/py_validator.rs 0.00% 19 Missing ⚠️
crates/blockifier/src/transaction/objects.rs 16.66% 9 Missing and 6 partials ⚠️
...ates/native_blockifier/src/transaction_executor.rs 27.27% 8 Missing ⚠️
...lockifier/src/execution/syscalls/hint_processor.rs 84.09% 7 Missing ⚠️
crates/native_blockifier/src/py_utils.rs 0.00% 4 Missing ⚠️
crates/blockifier/src/context.rs 66.66% 3 Missing ⚠️
crates/native_blockifier/src/errors.rs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@               Coverage Diff                @@
##           main-v0.13.1    #1392      +/-   ##
================================================
+ Coverage         70.37%   70.83%   +0.46%     
================================================
  Files                60       60              
  Lines              7773     8167     +394     
  Branches           7773     8167     +394     
================================================
+ Hits               5470     5785     +315     
- Misses             1874     1946      +72     
- Partials            429      436       +7     

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

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 717e323 to a32eb89 Compare January 29, 2024 07:02
@giladchase giladchase changed the title Use gas costs only from VersionedConstants feat(execution): use gas costs only from VersionedConstants Jan 29, 2024
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/src/versioned_constants.rs line 54 at r1 (raw file):

        let os_consts = &self.starknet_os_constants;

        os_consts.gas_costs["initial_gas_cost"] - os_consts.gas_costs["transaction_gas_cost"]

Suggestion:

        let os_consts = &self.starknet_os_constants;
        os_consts.gas_costs["initial_gas_cost"] - os_consts.gas_costs["transaction_gas_cost"]

crates/blockifier/src/abi/constants.rs line 41 at r1 (raw file):

// Gas Cost.
// See documentation in core/os/constants.cairo.

Is moving those part of an unmerged PR?

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from a32eb89 to 27043f4 Compare January 29, 2024 12:44
Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @noaov1)


crates/blockifier/src/abi/constants.rs line 41 at r1 (raw file):

Previously, elintul (Elin) wrote…

Is moving those part of an unmerged PR?

WDYM by part of an unmerged PR?

I'm removing them because they are now stored in the StarknetOSConstants, and are accessible via versioned_constants.get_gas_cost("name_of_const").
That is, they are no longer consts, since they are dynamically loaded from a json file.

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@giladchase giladchase force-pushed the gilad/add-cairo-constants branch 2 times, most recently from 17854d7 to f367a56 Compare January 31, 2024 17:48
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 27043f4 to 8b38505 Compare January 31, 2024 18:15
@giladchase giladchase changed the title feat(execution): use gas costs only from VersionedConstants feat(execution): use gas costs only from VersionedConstants Jan 31, 2024
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 8b38505 to 350ba79 Compare February 1, 2024 05:07
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 350ba79 to b93c238 Compare February 1, 2024 11:10
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from b93c238 to 458bc11 Compare February 1, 2024 13:42
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@giladchase giladchase force-pushed the gilad/add-cairo-constants branch 4 times, most recently from 755a80c to bd185f3 Compare February 5, 2024 12:27
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 458bc11 to 406eda9 Compare February 5, 2024 12:35
elintul
elintul previously approved these changes Feb 5, 2024
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Base automatically changed from gilad/add-cairo-constants to main-v0.13.1 February 5, 2024 13:33
@giladchase giladchase dismissed elintul’s stale review February 5, 2024 13:33

The base branch was changed.

noaov1
noaov1 previously approved these changes Feb 5, 2024
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 14 files at r1, 6 of 11 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @giladchase)


crates/blockifier/src/test_utils.rs line 215 at r4 (raw file):

        caller_address: ContractAddress::default(),
        call_type: CallType::Call,
        initial_gas: VersionedConstants::create_for_account_testing().gas_cost("initial_gas_cost"),

Suggestion:

VersionedConstants::create_for_testing().gas_cost("initial_gas_cost"),

crates/blockifier/src/transaction/transactions_test.rs line 81 at r4 (raw file):

#[fixture]
fn initial_gas() -> u64 {

Suggestion:

fn tx_initial_gas() -> u64 {

crates/blockifier/src/transaction/transactions_test.rs line 82 at r4 (raw file):

#[fixture]
fn initial_gas() -> u64 {
    VersionedConstants::latest_constants().tx_initial_gas()

?

Suggestion:

    VersionedConstants::create_for_testing().tx_initial_gas()

crates/blockifier/src/transaction/transactions_test.rs line 313 at r4 (raw file):

        validate_gas_consumed: 0,
        execute_gas_consumed: 0,
        inner_call_initial_gas: VersionedConstants::create_for_account_testing().gas_cost("initial_gas_cost"),

Create a fixture?

Code quote:

inner_call_initial_gas: VersionedConstants::create_for_account_testing().gas_cost("initial_gas_cost"),

- Remove from constants module and replace all usages with
  `VersionedConstants#gas_cost(..)`
  - Move all comments from the constants module into the const whitelist
    in `VersionedConstants`.
- Add gas cost getter to `EntryPointExecutionContext`, for readability
  - enclose in closure inside hint_processor.rs for even more brevity.
- Move `Transaction::Initial_gas` into `VersionedConstants`: it is now
  derived from the constants json.
- No other logic changes.
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


crates/blockifier/src/transaction/transactions_test.rs line 88 at r5 (raw file):

fn versioned_constants_for_account_testing() -> VersionedConstants {
    VersionedConstants::create_for_account_testing()
}

Create a fixture for versioned constants?

Code quote:

#[fixture]
fn tx_initial_gas() -> u64 {
    VersionedConstants::create_for_testing().tx_initial_gas()
}

#[fixture]
fn versioned_constants_for_account_testing() -> VersionedConstants {
    VersionedConstants::create_for_account_testing()
}

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/blockifier/src/transaction/transactions_test.rs line 88 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Create a fixture for versioned constants?

Called explicitly later down, complicates this.

@giladchase giladchase merged commit ab9c4a1 into main-v0.13.1 Feb 5, 2024
8 checks passed
@giladchase giladchase deleted the gilad/use-versioned-constats-for-cairo-os branch February 5, 2024 16:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants