Skip to content

Optimize DecodePubCurveKey and fix off-by-one#23

Merged
mtmk merged 4 commits intomainfrom
optimize-DecodePubCurveKey
Mar 17, 2026
Merged

Optimize DecodePubCurveKey and fix off-by-one#23
mtmk merged 4 commits intomainfrom
optimize-DecodePubCurveKey

Conversation

@mtmk
Copy link
Copy Markdown
Member

@mtmk mtmk commented Mar 17, 2026

Optimized allocations of DecodePubCurveKey also it returned 33 bytes instead of 32, including a stray CRC byte in the Curve25519 public key material. Worked since TweetNaCl only reads the first 32 bytes, but diverged from the Go reference implementation.

  • Fix slice to use CurveKeyLen (32) matching Go's raw[1:end]
  • Optimize: reduce allocations
  • More tests covering with extensive coverage of DecodePubCurveKey

@mtmk mtmk requested a review from Copilot March 17, 2026 12:00
@mtmk mtmk changed the title Optimize DecodePubCurveKey Fix DecodePubCurveKey off-by-one and add comprehensive tests Mar 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves Curve public key decoding in KeyPair to reduce allocations and make the decoded output consistently 32 bytes, and updates the test setup to validate the behavior and rely on internals access rather than compiling library sources into the test project.

Changes:

  • Refactors DecodePubCurveKey to use Span/stackalloc and adds an overload that decodes into a caller-provided buffer.
  • Adds extensive unit tests covering decoding correctness, CRC validation, and seal/open round-trips.
  • Simplifies the test project by removing direct compilation of library internal sources and enabling access via InternalsVisibleTo.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
NATS.NKeys/NATS.NKeys.csproj Grants internals access to test assemblies via InternalsVisibleTo.
NATS.NKeys/KeyPair.cs Reworks curve public key decoding to be allocation-light and returns exactly 32 bytes.
NATS.NKeys.Tests/NKeysTest.cs Adds targeted tests for curve public key decoding and integration scenarios.
NATS.NKeys.Tests/NATS.NKeys.Tests.csproj Removes compiled-in library sources now that internals are visible from the main assembly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread NATS.NKeys/KeyPair.cs
Comment thread NATS.NKeys.Tests/NKeysTest.cs Outdated
@mtmk mtmk changed the title Fix DecodePubCurveKey off-by-one and add comprehensive tests Optimize DecodePubCurveKey and fix off-by-one Mar 17, 2026
Copy link
Copy Markdown

@scottf scottf left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit 73d147d into main Mar 17, 2026
8 checks passed
@mtmk mtmk deleted the optimize-DecodePubCurveKey branch March 17, 2026 12:28
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.

4 participants