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

Update MSRV; disallow long IDs #28434

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Update MSRV; disallow long IDs #28434

merged 2 commits into from
Nov 7, 2024

Conversation

mikebenfield
Copy link
Collaborator

See the commit messages.

Updating the minimum supported Rust version in other crates in the workspace was missed when updating it in the main Cargo.toml.

Also, SnarkVM will give a confusing parse error on identifiers that are too long, so we should disallow them here.

@mikebenfield mikebenfield requested a review from d0cd November 4, 2024 21:27
@mikebenfield
Copy link
Collaborator Author

Here I'm assuming a field can hold 250 bits, but in principle that could vary.

@mikebenfield mikebenfield force-pushed the long-ids branch 2 times, most recently from 31a8e4a to 154bc10 Compare November 5, 2024 16:41
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

Overall the PR looks good, left a minor nit and question on organization.

SnarkVM requires that identifiers fit in a field
element.
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

LGTM!

@d0cd d0cd merged commit 333d3eb into mainnet Nov 7, 2024
10 checks passed
@d0cd d0cd deleted the long-ids branch November 7, 2024 02:43
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.

2 participants