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 padding support & make it optional #21

Merged
merged 14 commits into from
Oct 28, 2024

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Oct 25, 2024

Description

This PR makes it possible to handle encoding/decoding of correctly padding base64 (which previously wasn't supported) or to optionally encode/decode base64 for which the padding has already been stripped.

Additionally, this PR again reformats to the most updated version.

Problem*

The standard encoding for base64 specifies padding, which isn't handled in this library and may be needed in some cases by default. However, since it will be more efficient without handling padding in cases where that's available, it's nice to make this optional

Summary*

  • splits encoder/decoder into modules
  • adds optional padding to encoder & decoder
  • adds tests for each
  • handles new formatting

Additional Context

PR Checklist*

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

@grjte grjte force-pushed the grjte/optional-padding branch from 88cbfda to a5d4afd Compare October 25, 2024 13:34
@grjte
Copy link
Contributor Author

grjte commented Oct 25, 2024

  1. It turns out that the way I've architected (with globals for the different encoder/decoder options) this is not possible before v0.35.0. I bumped the compiler version, but "Test on Nargo 0.34.0" is listed as a required workflow step, so maybe another approach is needed. Lmk
  2. I had formatted this with the nightly compiler to avoid the issue that came up last time, but it's not formatted correctly for v0.35.0. Please let me know what you prefer here (I'm guessing I should just format on v0.35.0, but I'd prefer to get feedback before running the workflow again)

@saleel
Copy link
Contributor

saleel commented Oct 25, 2024

Test on Nargo 0.34.0 Expected — Waiting for status to be reported

This seems to be a github bug that I have seen happen before as well - closing the PR and opening again fixes it sometimes. Let me try

Update: No, didnt work. I will check this, but for the other issue, maybe we can try upgrading to 0.36 now?

@saleel saleel closed this Oct 25, 2024
@saleel saleel reopened this Oct 25, 2024
@grjte grjte force-pushed the grjte/optional-padding branch from 72c090d to 10100a1 Compare October 25, 2024 14:14
@saleel saleel closed this Oct 25, 2024
@saleel saleel reopened this Oct 25, 2024
@TomAFrench TomAFrench changed the title feat: add padding support & make it optional feat!: add padding support & make it optional Oct 25, 2024
@TomAFrench TomAFrench requested a review from saleel October 25, 2024 14:31
@TomAFrench
Copy link
Member

I've updated the branch protection rules so this PR can go through. @saleel can you review?

src/encoder.nr Outdated
// handle padding
let rem = InputBytes % 3;
if (rem == 1) {
result[base64_offset + num_elements_in_final_chunk - 1] = BASE64_PADDING_CHAR;
Copy link
Contributor

@saleel saleel Oct 25, 2024

Choose a reason for hiding this comment

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

I think this could be result[OutputElements - 1] = BASE64_PADDING_CHAR;

along with something like assert((InputBytes + 2) / 3 * 4 == OutputElements); at the top.

@saleel saleel merged commit d9bd959 into noir-lang:main Oct 28, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Oct 28, 2024
@grjte grjte deleted the grjte/optional-padding branch October 28, 2024 14:48
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