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

feat: backwards compatibility for calldata_factor #1529

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Feb 18, 2024

If os resources' inner tx is missing constant and calldata_factor keys, then:

  • assume it is a flat ExecutionResources instance, and put its contents inside a constant key.
  • initialize calldata_factor as default (all zeros).

Python: https://reviewable.io/reviews/starkware-industries/starkware/34026


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2024

Codecov Report

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

Comparison is base (9c0c99a) 69.19% compared to head (b8ae6a9) 69.16%.

Files Patch % Lines
crates/blockifier/src/versioned_constants.rs 45.00% 3 Missing and 8 partials ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           main-v0.13.1    #1529      +/-   ##
================================================
- Coverage         69.19%   69.16%   -0.04%     
================================================
  Files                59       59              
  Lines              8159     8173      +14     
  Branches           8159     8173      +14     
================================================
+ Hits               5646     5653       +7     
- Misses             2053     2056       +3     
- Partials            460      464       +4     

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

@giladchase giladchase force-pushed the ori/starknet/add_rounding_constants/move_to_os_constants branch from 36a91f8 to 8496617 Compare February 18, 2024 08:08
@giladchase giladchase force-pushed the gilad/os-resources-calldata-factor branch from 6ac6afa to 71b3720 Compare February 18, 2024 08:22
elintul
elintul previously approved these changes Feb 18, 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 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @noaov1)


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

}

impl TryFrom<OSConstantsRawJSON> for OSConstants {

Throughout the file.

Suggestion:

OsConstantsRawJson

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

}

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

Alphabetize?
Throughout this file.

Code quote:

derive

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

            Ok(Self {
                constant: serde_json::from_value(
                    raw_json_data.raw_resource_params_as_dict["constant"].clone(),

Any way to avoid these, given that we have ownership?

Code quote:

.clone()

crates/blockifier/src/versioned_constants_test.rs line 44 at r1 (raw file):

        "validate_max_n_steps": 1,
        "os_constants": {},
        "os_resources": {

Do we have a test of loading the 0.13.1 versioned constants?


crates/blockifier/src/versioned_constants_test.rs line 69 at r1 (raw file):

    // Calldata-factor was initialized as 0, and did not affect the expected result, even if
    // calldata_length is nonzero.

Try to refrain from mentioning variables and functions by name in documentation, if possible; it causes false-positives when searching throughout the codebase.

Suggestion:

    // Calldata factor was initialized as 0, and did not affect the expected result, even if
    // calldata length is nonzero.

Base automatically changed from ori/starknet/add_rounding_constants/move_to_os_constants to main-v0.13.1 February 18, 2024 09:10
@giladchase giladchase dismissed elintul’s stale review February 18, 2024 09:10

The base branch was changed.

@giladchase giladchase force-pushed the gilad/os-resources-calldata-factor branch from 71b3720 to d98ee70 Compare February 18, 2024 10:16
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 512 at r1 (raw file):

Previously, elintul (Elin) wrote…

Any way to avoid these, given that we have ownership?

Done.
Also added proper error handling: return error if the user gave json with constant but not with calldata_factor, which should never happen...


crates/blockifier/src/versioned_constants_test.rs line 44 at r1 (raw file):

Previously, elintul (Elin) wrote…

Do we have a test of loading the 0.13.1 versioned constants?

This line loads it during compilation, so if there's a bug it won't compile.

Is that what you meant?

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 all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (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.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/src/versioned_constants_test.rs line 44 at r1 (raw file):

Previously, giladchase wrote…

This line loads it during compilation, so if there's a bug it won't compile.

Is that what you meant?

Link is broken.

@giladchase giladchase force-pushed the gilad/os-resources-calldata-factor branch from d98ee70 to 551a75f Compare February 18, 2024 10:20
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 r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/src/versioned_constants.rs line 521 at r2 (raw file):

            (None, _) => {
                // If "constant" is not found, use the entire map for "constant" and default
                // "calldata_factor"

Suggestion:

                // If `constant` is not found, use the entire map for `constant` and default
                // `calldata_factor`.

crates/blockifier/src/versioned_constants.rs line 522 at r2 (raw file):

                // If "constant" is not found, use the entire map for "constant" and default
                // "calldata_factor"
                let entire_map = std::mem::take(&mut json_data.raw_resource_params_as_dict);

Suggestion:

entire_value

crates/blockifier/src/versioned_constants.rs line 528 at r2 (raw file):

        let constant = serde_json::from_value(constant)?;
        let calldata_factor = serde_json::from_value(calldata_factor)?;

I think you can pass them directly, without intermediate variables; short enough.

Code quote:

        let constant = serde_json::from_value(constant)?;
        let calldata_factor = serde_json::from_value(calldata_factor)?;

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


crates/blockifier/src/versioned_constants_test.rs line 44 at r1 (raw file):

Previously, elintul (Elin) wrote…

Link is broken.

odd 🤔

serde_json::from_str(DEFAULT_CONSTANTS_JSON)

@giladchase giladchase force-pushed the gilad/os-resources-calldata-factor branch from 551a75f to 26ed90c Compare February 18, 2024 10:24
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 r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/src/versioned_constants_test.rs line 44 at r1 (raw file):

Previously, giladchase wrote…

odd 🤔

serde_json::from_str(DEFAULT_CONSTANTS_JSON)

Yup.

@giladchase giladchase force-pushed the gilad/os-resources-calldata-factor branch from 26ed90c to 2ef72a9 Compare February 18, 2024 10:35
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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)


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

Previously, elintul (Elin) wrote…

Throughout the file.

Done


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

Previously, elintul (Elin) wrote…

Alphabetize?
Throughout this file.

Done, also removed Serialize, it isn't used and won't help anyone anyway.

elintul
elintul previously approved these changes Feb 18, 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.

:lgtm:

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

If os resources' inner tx is missing `constant` and `calldata_factor`
keys, then:
- assume it is a flat `ExecutionResources` instance, and put its
  contents inside a `constant` key.
- initialize calldata_factor as default (all zeros).
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 r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@giladchase giladchase merged commit c197155 into main-v0.13.1 Feb 18, 2024
8 checks passed
@giladchase giladchase deleted the gilad/os-resources-calldata-factor branch February 18, 2024 11:15
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
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