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

feat(fee): add os resources to versioned constants #1393

Merged

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Jan 29, 2024

  • Move hardcoded os resources json into the versioned constants json, move OsResources into into OsResources module.
  • Delete os_usage.rs: all logic is now called from methods in VersionedConstants(todo: extract into submodules, currently just a big module).
  • Delete os_usage_test.rs: these are not post-deserialize validations of OsResources.
  • the rest are getters and patching things up.

This change is Reviewable

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 717e323 to a32eb89 Compare January 29, 2024 07:02
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

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

Comparison is base (1af90d3) 70.83% compared to head (132cb62) 68.90%.

Files Patch % Lines
crates/blockifier/src/versioned_constants.rs 79.61% 16 Missing and 5 partials ⚠️
...ates/native_blockifier/src/transaction_executor.rs 0.00% 2 Missing ⚠️
crates/blockifier/src/fee/gas_usage.rs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                              Coverage Diff                              @@
##           gilad/use-versioned-constats-for-cairo-os    #1393      +/-   ##
=============================================================================
- Coverage                                      70.83%   68.90%   -1.93%     
=============================================================================
  Files                                             60       58       -2     
  Lines                                           8167     7742     -425     
  Branches                                        8167     7742     -425     
=============================================================================
- Hits                                            5785     5335     -450     
- Misses                                          1946     1957      +11     
- Partials                                         436      450      +14     

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

@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from 7430754 to 1d9a08c Compare January 29, 2024 07:04
@giladchase giladchase changed the title Add os resources to versioned constants feat(fee): add os resources to versioned constants 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 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/resources/versioned_constants.json line 252 at r1 (raw file):

    }
  },
  "starknet_os_constants": {

Suggestion:

os_constants

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

    }

    pub fn resources_for_tx_type(&self, tx_type: &TransactionType) -> &VmExecutionResources {

Suggestion:

os_resources_for_tx_type

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

}

#[derive(Debug, Deserialize, Clone, Default)]

Alphabetize?

Code quote:

Debug, Deserialize, Clone, Default

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

// Serde trick for adding validations via a customr deserializer, without forgoing the derive.
// See: https://github.com/serde-rs/serde/issues/1220.
#[serde(remote = "Self")]

Can you shortly elaborate more?

Code quote:

#[serde(remote = "Self")]

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

            os_resources.execute_syscalls.values().chain(os_resources.execute_txs_inner.values());

        for resources in all_resources {

Suggestion:

        let all_resources =
            os_resources.execute_syscalls.values().chain(os_resources.execute_txs_inner.values());
        for resources in all_resources {

@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/versioned_constants.rs line 136 at r1 (raw file):

Previously, elintul (Elin) wrote…

Can you shortly elaborate more?

Not sure if you meant in docstring or here 😅

Adding remote = X tells serde that the decorated struct's serialization/deserialization will be applied to the struct X whenever X is [de]serialized.

If X is Self, then without any custom [de]serializers, nothing happens, because its just saying "deserialize myself with myself".

However, if remote = Self apparently the implementation ensures that Self's derived deserialization still applies, and the custom deserializer can access it.
(It looks like a "useful coincidence which is not in the official docs AFAICS, but the answer in the issue i linked is from serde's author, so it's legit.)

For our usage (and as stated in the issue), it allowed us to treat the custom deserializer of OsResources as a validator (which fails the deserialization if it does pass), without having to deserialize the fields manually.
In particular, this allowed us to use this line inside the deserializer let os_resources = Self::deserialize(deserializer)?; which is typically impossible to use inside a custom deserializer, because without the remote=self trick the custom deserializer defines the dserializer, so it can't use it inside the definition (here it is using the derived deserizliaer... very confusing indeed).

@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from 143333b to b5def39 Compare January 30, 2024 19:32
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 21 files at r2, 3 of 10 files at r3, all commit messages.
Reviewable status: 8 of 22 files reviewed, 1 unresolved discussion (waiting on @noaov1)

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch 2 times, most recently from 8b38505 to 350ba79 Compare February 1, 2024 05:07
@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from b5def39 to 76e7a03 Compare February 1, 2024 08:44
@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/add-os-resources-to-versioned-constants branch from 76e7a03 to 15eaa96 Compare February 1, 2024 11:15
@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from b93c238 to 458bc11 Compare February 1, 2024 13:42
@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch 3 times, most recently from 6a2bda7 to 5bf0823 Compare February 1, 2024 15:45
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 2 of 10 files at r3, 15 of 20 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/src/versioned_constants.rs line 425 at r5 (raw file):

#[derive(Clone, Debug, Deserialize)]
pub struct ResourcesParams {

How come it appears as added in this PR?

Code quote:

ResourcesParams

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, 2 unresolved discussions (waiting on @elintul and @noaov1)


crates/blockifier/src/versioned_constants.rs line 425 at r5 (raw file):

Previously, elintul (Elin) wrote…

How come it appears as added in this PR?

It's original location, os_usage.rs, has been deleted: that module isn't necessary anymore, since OsResources is now a private member of VersionedConstants.
(We should probably split up VersionedConstants into submodules? it's getting chubby, but probably should do that after we merge everything and agree on the design.)

@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from 5bf0823 to 1b76fae Compare February 4, 2024 16:12
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:

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

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 458bc11 to 406eda9 Compare February 5, 2024 12:35
@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from 1b76fae to dc5e2ed Compare February 5, 2024 15: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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@giladchase giladchase force-pushed the gilad/use-versioned-constats-for-cairo-os branch from 406eda9 to 1af90d3 Compare February 5, 2024 15:56
@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from dc5e2ed to 132cb62 Compare February 5, 2024 15:56
Base automatically changed from gilad/use-versioned-constats-for-cairo-os to main-v0.13.1 February 5, 2024 16:10
@giladchase giladchase dismissed elintul’s stale review February 5, 2024 16:10

The base branch was changed.

- Move hardcoded os resources json into the versioned constants json,
  move `OsResources` into into `OsResources` module.
- Indent versioned_constants.json at 4.
- Delete os_usage.rs: all logic is now called from methods in
  `VersionedConstants`(todo: extract into submodules, currently
  just a big module).
- Delete os_usage_test.rs: these are not post-deserialize validations of
  `OsResources`.
- sorted versioned_constants (where possible, which is everywhere except
  for inside os_constants keys).
@giladchase giladchase force-pushed the gilad/add-os-resources-to-versioned-constants branch from 132cb62 to 1f3261d Compare February 5, 2024 16:11
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 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@giladchase giladchase merged commit 4e20c44 into main-v0.13.1 Feb 5, 2024
8 checks passed
@giladchase giladchase deleted the gilad/add-os-resources-to-versioned-constants branch February 5, 2024 16:21
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.

3 participants