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

feat: support varlen hex strings in digest macro #988

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

Mirko-von-Leipzig
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig commented Nov 27, 2024

Somewhat mutilates/extends the digest! macro from #984 to add support for variable length hex strings.

Previously, the macro only supported hex strings of felt-length multiples. See this comment. Variable length is supported by manually unrolling the hex characters into u64/felts in reverse. This supports shorter lengths as the initial values will remain zero.

I also moved the Felt import inside the macro so that callers of digest! no longer have to import Felt.

I think this PR also fixes a LE/BE bug, where digest!("0x123")' maps to 0x1230000...0instead of0x123. Though I might just be misunderstanding the try_from` implementation it utilised. edit - indeed my misunderstanding - no bug


Possible future work

Currently this macro will give a compile time error IF we assign it to a constant, but won't if its not assigned to a constant.

// Compile time error (missing 0x prefix)
const EXAMPLE: Digest = digest("ABC");

// Runtime error.
let example = digest!("ABC");

We can also force the latter to a compile time error by moving the const allocation inside the macro itself i.e. by adding

const DIGEST: $crate::Digest = ...;
DIGEST

to the end of the macro. This has further implications though, namely:

  1. The macro can no longer accept non-const literals
  2. I think the current macro insides need to move outwards into a pub const fn. Which isn't so bad because this then supports the cases from (1)

I can add this in if we want it.

@Mirko-von-Leipzig Mirko-von-Leipzig added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Nov 27, 2024
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review November 27, 2024 16:25
@Mirko-von-Leipzig Mirko-von-Leipzig force-pushed the mirko/feat/varlen-digest branch 2 times, most recently from 5989171 to bd65790 Compare November 28, 2024 09:31
Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Looks awesome!
Very clever approach, I'll keep it in mind for the future.

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 left one nit inline.

We can also force the latter to a compile time error by moving the const allocation inside the macro itself i.e. by adding

I think it is fine as is for now. We can create an issue with this potential improvement (could be a "good first issue").

miden-lib/build.rs Show resolved Hide resolved
@Mirko-von-Leipzig Mirko-von-Leipzig merged commit e7b8030 into next Nov 29, 2024
9 checks passed
@Mirko-von-Leipzig Mirko-von-Leipzig deleted the mirko/feat/varlen-digest branch November 29, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants