Remove NonceWrapper

[kwsantiago]

Closes #53 since this was fixed upstream.

Summary by CodeRabbit

• Refactor
  • Simplified internal nonce data handling by removing intermediate wrapper types and storing nonce information directly.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR removes the NonceWrapper type and updates both Coordinator and SigningSession to store SigningNonces directly, eliminating indirection. Field types are changed from Option<(Identifier, NonceWrapper)> to Option<(Identifier, SigningNonces)>, and all initialization and usage sites are adjusted accordingly.

Changes

Cohort / File(s)	Summary
Nonce wrapper removal keep-core/src/frost/coordinator.rs, keep-core/src/frost/signing.rs	Removed NonceWrapper type and its Zeroize implementation. Updated our_nonces field declarations to store SigningNonces directly instead of wrapped. Adjusted nonce initialization, storage, and all usage sites by removing .0 accessors and passing raw nonces to frost::round2::sign.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Zeroize FROST nonces on drop and use HMAC for rate limit file #38: Directly related—describes the inverse operation where a PR introduced/uses a NonceWrapper around SigningNonces; this PR reverses that pattern by removing the wrapper and using SigningNonces directly throughout nonce handling.

Poem

🐰 A wrapper shed, now nonces run free,
No more indirection, just clarity!
Direct storage, clean and neat,
This refactor's bittersweet. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Remove NonceWrapper' directly and clearly describes the main change in the pull request.
Linked Issues check	✅ Passed	The PR successfully removes NonceWrapper as required by issue #53, now that upstream frost-core implements Zeroize for SigningNonces.
Out of Scope Changes check	✅ Passed	All changes are directly related to removing NonceWrapper and updating nonce handling in coordinator.rs and signing.rs, with no extraneous modifications.

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI Agents

In @keep-core/src/frost/coordinator.rs:
- Around line 16-23: The Coordinator struct stores sensitive SigningNonces in
our_nonces but does not derive ZeroizeOnDrop; update the Coordinator definition
to derive Zeroize and ZeroizeOnDrop and mark all non-sensitive fields with
#[zeroize(skip)] while leaving our_nonces to be zeroized (i.e., do not skip it).
Specifically, add the Zeroize/ZeroizeOnDrop derives to the Coordinator struct
and annotate session_id, message, threshold, commitments, and signature_shares
with #[zeroize(skip)] so only our_nonces (Option<(Identifier, SigningNonces)>)
is cleared on drop, matching the SigningSession pattern.
- Around line 16-23: The comment claiming "SigningNonces doesn't implement
Zeroize" is outdated; remove the NonceWrapper wrapper in
keep-frost-net/src/session.rs and use SigningNonces directly (consistent with
keep-core::Coordinator's our_nonces field), or if the wrapper is still needed
for another reason, update the comment to clearly state that reason; locate the
NonceWrapper struct and its impl and either delete them and replace usages with
SigningNonces or revise the comment to explain the actual current rationale for
keeping the wrapper.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9eb4a3 and 2c93fed.

📒 Files selected for processing (2)

• keep-core/src/frost/coordinator.rs
• keep-core/src/frost/signing.rs

🧰 Additional context used

🧬 Code graph analysis (1)

keep-core/src/frost/signing.rs (1)

keep-frost-net/src/session.rs (1)

• zeroize (24-26)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build

🔇 Additional comments (5)

keep-core/src/frost/signing.rs (3)

100-100: LGTM!

Direct storage of SigningNonces is correct, assuming the upstream crate now implements Zeroize.

---

143-143: LGTM!

Passing &nonces directly is correct after removing the NonceWrapper.

---

43-43: Code change is correct—SigningNonces is properly zeroized.

The field our_nonces: Option<SigningNonces> will be automatically zeroized when the SigningSession struct is dropped because it has the #[derive(ZeroizeOnDrop)] attribute and lacks the #[zeroize(skip)] directive. Since the code compiles successfully with this configuration, SigningNonces from frost-secp256k1-tr 2.2.0 must implement the Zeroize trait.

keep-core/src/frost/coordinator.rs (2)

47-47: LGTM - but depends on adding ZeroizeOnDrop to Coordinator.

Direct storage of SigningNonces is correct. However, ensure the Coordinator struct derives ZeroizeOnDrop (flagged separately) for proper zeroization.

---

102-102: LGTM!

Passing &nonces directly is correct after removing the NonceWrapper.