Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: wrap cairo lang version id for code dedup #83

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Jul 25, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

There is code duplication between Cairo-Lang's version ID and the gateway's version ID.

What is the new behavior?

  • Refactor the Gateway's version ID as a wrapper.

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Jul 25, 2024

@ArniStarkware ArniStarkware marked this pull request as ready for review July 25, 2024 10:10
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch 2 times, most recently from f4e1e45 to 6513dc1 Compare July 25, 2024 10:44
@yair-starkware
Copy link
Contributor

Why don't we use the type directly from Cairo?

Copy link
Contributor Author

@ArniStarkware ArniStarkware 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 5 files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

a discussion (no related file):

Previously, yair-starkware (Yair) wrote…

Why don't we use the type directly from Cairo?

The issue is that Cairo-lang does not implement Serialize and Deserialize.
We define a wrapper that will have the required functionality.



crates/gateway/src/compiler_version.rs line 22 at r1 (raw file):

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct VersionId(pub CairoLangVersionId);

@yair-starkware Your question is about this line.

The issue is that Cairo-lang does not implement Serialize and Deserialize.
We define a wrapper that will have the required functionality.

Code quote:

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct VersionId(pub CairoLangVersionId);

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 6513dc1 to 37bed1e Compare July 31, 2024 08:01
Copy link
Contributor

@yair-starkware yair-starkware 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 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 37bed1e to 82f73f8 Compare July 31, 2024 10:05
@ArniStarkware ArniStarkware requested a review from elintul July 31, 2024 10:47
@ArniStarkware
Copy link
Contributor Author

crates/gateway/src/compiler_version.rs line 22 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

@yair-starkware Your question is about this line.

The issue is that Cairo-lang does not implement Serialize and Deserialize.
We define a wrapper that will have the required functionality.

+reviewer:@elintul

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch 9 times, most recently from ae82e5b to 4a9a9a1 Compare August 7, 2024 07:22
@ArniStarkware ArniStarkware changed the base branch from main to arni/chore/code_dedup/version_id_serialization August 7, 2024 07:22
@ArniStarkware
Copy link
Contributor Author

I reordered the PRs.
This PR deals with wrapping the CairoLangVersionId with our VersionId.

This struct offers additional functionality for serialization as this object is used for configuration.

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_serialization branch from 23f9e9a to f89b7dc Compare August 7, 2024 07:28
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 4a9a9a1 to b38f02f Compare August 7, 2024 07:28
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_serialization branch from f89b7dc to 8189e02 Compare August 7, 2024 07:32
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from b38f02f to 050b7b4 Compare August 7, 2024 07:33
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_serialization branch from 8189e02 to a5f2ac3 Compare August 7, 2024 13:11
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_serialization branch from 4f163f1 to c5367e3 Compare August 12, 2024 08:30
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 5e941e5 to 2e651a8 Compare August 12, 2024 08:30
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_serialization branch from c5367e3 to cee6c24 Compare August 12, 2024 09:33
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 2e651a8 to 5b652d5 Compare August 12, 2024 09:33
@ArniStarkware ArniStarkware changed the base branch from arni/chore/code_dedup/version_id_serialization to graphite-base/83 August 12, 2024 10:37
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 5b652d5 to c72b5d4 Compare August 12, 2024 10:37
@ArniStarkware ArniStarkware changed the base branch from graphite-base/83 to main August 12, 2024 10:37
@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from c72b5d4 to e5a33dd Compare August 12, 2024 10:40
@ArniStarkware
Copy link
Contributor Author

Open a PR in compilers for them to implement serialization/deserialization on Version Id.

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from e5a33dd to 6b9b5a5 Compare August 13, 2024 07:37
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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 5 files reviewed, all discussions resolved (waiting on @elintul, @Yael-Starkware, and @yair-starkware)

a discussion (no related file):

Previously, ArniStarkware (Arnon Hod) wrote…

Open a PR in compilers for them to implement serialization/deserialization on Version Id.

Done.


@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 6b9b5a5 to 71fd727 Compare August 13, 2024 09:24
@ArniStarkware
Copy link
Contributor Author

crates/gateway/src/compiler_version.rs line 44 at r6 (raw file):

        let sierra_program_for_compiler = sierra_program_as_felts_to_big_uint_as_hex(
            safe_slice(&sierra_program, 0..6).map_err(|_| VersionIdError::InvalidVersion {
                message: format!("Sierra program is too short."),

@yair-starkware - You already said - we should remove the message from InvalidVersion.

Code quote:

            safe_slice(&sierra_program, 0..6).map_err(|_| VersionIdError::InvalidVersion {
                message: format!("Sierra program is too short."),

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch 2 times, most recently from 2ba5a8a to 97f3e51 Compare August 13, 2024 09:55
@yair-starkware
Copy link
Contributor

crates/gateway/src/compiler_version.rs line 44 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

@yair-starkware - You already said - we should remove the message from InvalidVersion.

Not sure what you mean, but maybe the error should be: "Couldn't get version from the program - program too short (expecting at least 6 felts, got {}."

Copy link
Contributor

@yair-starkware yair-starkware 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 5 files at r4, 1 of 2 files at r5, 1 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @elintul, and @Yael-Starkware)


crates/gateway/src/stateless_transaction_validator_test.rs line 34 at r7 (raw file):

    static MIN_SIERRA_VERSION: OnceLock<VersionId> = OnceLock::new();
    MIN_SIERRA_VERSION.get_or_init(|| VersionId::new(1, 1, 0))
}

Can you use LazyLock instead of OnceLock?
https://doc.rust-lang.org/stable/std/sync/struct.LazyLock.html

Code quote:

fn min_sierra_version() -> &'static VersionId {
    static MIN_SIERRA_VERSION: OnceLock<VersionId> = OnceLock::new();
    MIN_SIERRA_VERSION.get_or_init(|| VersionId::new(1, 1, 0))
}

crates/gateway/src/utils.rs line 168 at r7 (raw file):

// TODO(Arni): Move this to a more general location.
pub fn safe_slice<T, R>(slice: &[T], range: R) -> Result<&[T], ()>

Why not to use get?
https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 97f3e51 to 22f89d8 Compare August 15, 2024 10:20
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @elintul, @Yael-Starkware, and @yair-starkware)


crates/gateway/src/compiler_version.rs line 44 at r6 (raw file):

Previously, yair-starkware (Yair) wrote…

Not sure what you mean, but maybe the error should be: "Couldn't get version from the program - program too short (expecting at least 6 felts, got {}."

I used ChatGPT for phrasing.
Done.


crates/gateway/src/stateless_transaction_validator_test.rs line 34 at r7 (raw file):

Previously, yair-starkware (Yair) wrote…

Can you use LazyLock instead of OnceLock?
https://doc.rust-lang.org/stable/std/sync/struct.LazyLock.html

The compiler tells me: error[E0658]: use of unstable library feature 'lazy_cell'.


crates/gateway/src/utils.rs line 168 at r7 (raw file):

Previously, yair-starkware (Yair) wrote…

Why not to use get?
https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get

Done.

@ArniStarkware ArniStarkware force-pushed the arni/chore/code_dedup/version_id_wrapper branch from 22f89d8 to 8574bb6 Compare August 15, 2024 10:56
@elintul elintul removed their request for review August 18, 2024 06:37
Copy link
Contributor

@yair-starkware yair-starkware 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 5 of 5 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

@ArniStarkware ArniStarkware merged commit 7e067de into main Aug 18, 2024
10 checks passed
@ArniStarkware ArniStarkware deleted the arni/chore/code_dedup/version_id_wrapper branch August 18, 2024 09:02
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 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.

2 participants