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

Support Cairo 2.5.0 #898

Merged
merged 10 commits into from Feb 6, 2024
Merged

Support Cairo 2.5.0 #898

merged 10 commits into from Feb 6, 2024

Conversation

andrew-fleming
Copy link
Collaborator

Fixes #877.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

LGTM!


assert_eq!(x, expected_x);
}

#[test]
fn test_secp256k1_serialization() {
let (big_point_1, big_point_2) = get_points();
let curve_size = Secp256k1Impl::get_curve_size();
Copy link
Contributor

Choose a reason for hiding this comment

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

huh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2.5 issues warnings for unused variables

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i was a bit more surprised of the unused line, which i believe made it through the audit

Comment on lines -15 to -20
trait UpgradesV1Trait<TState> {
fn set_value(ref self: TState, val: felt252);
fn get_value(self: @TState) -> felt252;
fn remove_selector(self: @TState);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing the traits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We were only using the upgrades mock traits to define the v1 and v2 impls, but it seems cleaner to just define impls of the v1 and v2 interfaces

src/tests/token/test_erc20.cairo Show resolved Hide resolved
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looking great Andrew! Left a small comment.

:eth-account-upgradeable-class-hash: 0x03dda9bcfa854795d91d586b1a4275a68ab1ab185b33a5c00ce647c75875b0ff
:erc20-class-hash: 0x03af5816946625d3d2c94ea451225715784762050eba736f0b0ad9186685bced
:erc721-class-hash: 0x045c96d1b24c3dc060680e4bfd4bdc32161aefe8f8939cd4be3954c5d8688d75
:account-class-hash: 0x07bb7a849957e721ee9e87e9844cba8ba139e87e671d4c34b17e09829b65134c
Copy link
Member

Choose a reason for hiding this comment

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

After merging main the Account hashes changes from using unwrap_syscall instead of unwrap.

I wonder if we shouldn't directly use 2.5.3 in this bump. That would also affect the class hashes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm yeah, might as well use 2.5.3. Will update

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

LGTM!

@martriay martriay merged commit a34025c into OpenZeppelin:main Feb 6, 2024
5 checks passed
@andrew-fleming andrew-fleming deleted the bump-to-2.5 branch February 12, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with Cairo and Scarb 2.5.0
3 participants