Skip to content

Conversation

@ernestognw
Copy link
Member

Fixes #????

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2025

⚠️ No Changeset found

Latest commit: 45704d4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ernestognw ernestognw mentioned this pull request Jun 4, 2025
3 tasks
@ernestognw ernestognw marked this pull request as ready for review June 4, 2025 05:15
@ernestognw ernestognw requested a review from Amxx June 4, 2025 05:15
@Amxx
Copy link
Collaborator

Amxx commented Jun 4, 2025

My understanding is:

  • SignatureChecker doesn't need 0.8.24 now
  • SignatureChecker will need 0.8.24 when the ERC-7913 validation gets added
  • This PR is done ahead of the ERC-7913 addition to avoid the ERC-7913 PR touching too many files?

If this is all correct, then I approve.

Amxx
Amxx previously approved these changes Jun 4, 2025
@Amxx Amxx dismissed their stale review June 4, 2025 07:36

more discussion

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

I'm really wondering how much of a breaking change it is. I know @ernestognw wants to move forward with small things like that. I also don't think its a major issue at all ... but it might technically break projects that use Governor (or just SignatureChecker) and have pinned a version of the compiler >=0.8.20, <0.8.24.

Maybe we want to add a "breaking change" entry to the Changelog saying that some contract compiler requirement have evolved, and that users may need to upgrade the compiler version they are using.

@ernestognw
Copy link
Member Author

My understanding is:

  • SignatureChecker doesn't need 0.8.24 now
  • SignatureChecker will need 0.8.24 when the ERC-7913 validation gets added
  • This PR is done ahead of the ERC-7913 addition to avoid the ERC-7913 PR touching too many files?

If this is all correct, then I approve.

Yes, this is correct. Basically moving 7913Utils to SIgnatureChecker forced this pragma increase.

Maybe we want to add a "breaking change" entry to the Changelog saying that some contract compiler requirement have evolved, and that users may need to upgrade the compiler version they are using.

Also true, I added a small note I think should be good enough

@ernestognw ernestognw requested a review from Amxx June 4, 2025 15:32
@ernestognw ernestognw merged commit 37f873d into OpenZeppelin:master Jun 4, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants