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

Verification key permissions: better-named wrappers #1639

Merged
merged 5 commits into from
May 9, 2024

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented May 8, 2024

This PR adds new wrappers for verification key permissions, which better convey their meaning.

This was suggested in the original VK permissions RFC: MinaProtocol/mina#13994 (comment)

It also makes setting VK permissions slightly nicer

auth: Permissions.proof(),
txnVersion: TransactionVersion.current(),
},
setVerificationKey: Permissions.VerificationKey.proofUntilHardfork(),

Choose a reason for hiding this comment

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

I appreciate the creativity to try get it in as a non-breaking change. It looks kind of awkward though to set that particularly permission differently to the rest.

WDYT about deprecating Impossible (so it still works, and the deprecation points to the new one. E.g.)

/**

  • @deprecated Impossible has been deprecated and will be removed in a future version. You should use ImpossibleUntilHardFork
    */
    Permissons.Impossible

Choose a reason for hiding this comment

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

And add Permissions.ImpossibleUntilHardFork

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was already looking differently than the rest though!

For any other permission, it's

Permission.impossible()

For verification keys, it used to be

{
  auth: Permission.impossible(),
  txnVersion: Transaction.current()
}

so I think it becomes less awkward with this change

Choose a reason for hiding this comment

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

Per slack, we resolved on ImpossibleDuringCurrentVersion as the most accurate and most descriptive (following the existing casing standards of course)

@mitschabaude mitschabaude marked this pull request as ready for review May 9, 2024 06:29
@mitschabaude mitschabaude merged commit 23cdfa3 into main May 9, 2024
14 checks passed
@mitschabaude mitschabaude deleted the feature/vk-permissions-wrapper branch May 9, 2024 09:33
@Trivo25 Trivo25 restored the feature/vk-permissions-wrapper branch June 8, 2024 14:27
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.

3 participants