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: add support for base64url alphabet #24

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Oct 29, 2024

Description

This completes the process of making the library configurable for padded/unpadded inputs and the commonly required base64 alphabets (standard and base64url).

Problem*

Resolves #15 and also addresses asserts for input/output lengths during decoding as mentioned in #19.

Summary*

  • fix issue with loop bound for decoding padded inputs and add length asserts
  • add configurable support for base64url
  • add new tests for base64url input
  • update docs with new configuration and performance numbers

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@grjte
Copy link
Contributor Author

grjte commented Oct 29, 2024

@saleel I have questions about 2 points:

  1. I defined the configurations with the prefix BASE64_, e.g. BASE64_ENCODER_STANDARD or BASE64_ENCODER_STANDARD_NO_PAD. I did it this way because the previous function names were prefixed with base64_, but these seem really long to me, and maybe it would be better to omit the prefix and just keep base64 in the lib, e.g. noir_base64::ENCODER_STANDARD. Wdyt?
  2. Let me know if you want the performance benchmarks done differently. I'm not sure if you guys have a particular system/pattern you follow. If so, I'm happy to update

@grjte grjte force-pushed the feat/multi-alphabet branch from 145825b to 36e5a54 Compare October 29, 2024 11:40
@saleel
Copy link
Contributor

saleel commented Oct 29, 2024

Looks good 🚀

  1. Yes, that make sense. Users can alias when importing if needed.
  2. I don't think there are any specific patterns now, but we have been discussing about it. What you have now is perfect - one thing we can mention is that the benchmarks are for barretenberg backend (as this is a Noir library and Noir is backend agnostic). Its arguable if backend specific things should even be there in this repo, but we can keep it here for now and move it elsewhere once we have a different pattern / multiple backends.

- note: based on profiling, it seems that the previous costs were wrong and that the current costs have been the same since the reversed encoding/decoding was fixed in commit cc5b18a.
@grjte grjte force-pushed the feat/multi-alphabet branch from 36e5a54 to 3fc82c6 Compare October 30, 2024 09:42
@grjte
Copy link
Contributor Author

grjte commented Oct 30, 2024

Thanks @saleel! I've updated with those 2 changes

@grjte grjte force-pushed the feat/multi-alphabet branch from 3fc82c6 to 291426b Compare October 30, 2024 17:58
@grjte grjte force-pushed the feat/multi-alphabet branch from 291426b to 69cb9e9 Compare October 30, 2024 18:01
@saleel saleel merged commit dfed9dd into noir-lang:main Oct 30, 2024
5 checks passed
@github-actions github-actions bot mentioned this pull request Oct 30, 2024
@grjte grjte deleted the feat/multi-alphabet branch November 7, 2024 09:45
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.

Add base64url support
2 participants