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

Prepare for migration to next release of Miden crypto #1287

Merged
merged 8 commits into from
Mar 25, 2024
Merged

Conversation

Al-Kindi-0
Copy link
Collaborator

Describe your changes

Mostly no-std issues and Falcon DSA.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@Al-Kindi-0 Al-Kindi-0 marked this pull request as draft March 23, 2024 15:10
@bobbinth
Copy link
Contributor

miden-crypto v0.9.0 is now on crates.io.

@Al-Kindi-0
Copy link
Collaborator Author

miden-crypto v0.9.0 is now on crates.io.

Updated the PR accordingly. There is a clippy warning which I am not not sure about the best way to handle it.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

LGTM!

Rebasing my PR on this shouldn't be too bad, probably not worth trying to introduce any additional formatting changes here though, or it will make conflicts harder to untangle. I did a quick review of the commits in my PR, and I don't have a specific commit where I introduced formatting changes explicitly, so I think any such changes fell out of modifying a file for other reasons, and the formatter changing the file in the process. Of course, with the number of files touched, that means there is a fair amount of formatting noise in that PR.

Once this is merged, I'll rebase my PR on it and that should make future rebases less problematic.

@bitwalker
Copy link
Contributor

There is a clippy warning which I am not not sure about the best way to handle it.

I would add a #[allow(dead_code)] annotation on that field for now, but if it is truly never read, then it can probably just be removed entirely, along with any code that writes to it.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I pushed a small commit which fixes the clippy warning. In the longer run, we should remove this field as I don't think we need it any more (I believe it was used for auxiliary trace hints, but these are now done differently).

One thing I wonder about: maybe we should handle signature generation code via events and have the decorator only for reading the signature from the advice provider. I'll try to write up an issue about this.

@bobbinth bobbinth marked this pull request as ready for review March 25, 2024 20:05
@bobbinth bobbinth merged commit d5de231 into next Mar 25, 2024
15 checks passed
@bobbinth bobbinth deleted the al-migrate-crypto branch March 25, 2024 20:16
bobbinth pushed a commit that referenced this pull request Apr 3, 2024
* chore: fix no-std errors
* fix: Falcon DSA decorators and tests
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