NYM-1408: Use SkewManager with registration clients. - #6973
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Thank you for making this first PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRegistration configuration now accepts an optional bandwidth spend-time skew. The skew is propagated through mixnet, LP, nested-session, and authenticator registration paths, with the current UTC time remaining the fallback when no skew is supplied. ChangesBandwidth spend-time injection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegistrationBuilder
participant RegistrationClient
participant AuthenticatorOrGateway
participant BandwidthTicketProvider
RegistrationBuilder->>RegistrationClient: construct with spend_time_skew
RegistrationClient->>AuthenticatorOrGateway: register with spend_time_skew
AuthenticatorOrGateway->>BandwidthTicketProvider: request ticket at current UTC time minus skew
BandwidthTicketProvider-->>AuthenticatorOrGateway: bandwidth ticket
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nym-registration-client/src/builder/config.rs (1)
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the public spend-time skew contract documentation.
The implementation consistently subtracts the skew, but callers are not told the sign convention, and the new parameter is missing from several LP method argument lists.
nym-registration-client/src/builder/config.rs#L42-L50: document what positive and negative skew values mean.nym-registration-client/src/lp_client/client.rs#L546-L547: documentspend_time_skewforregister_dvpn,finalise_dvpn_registration(Line 437), andhandshake_and_register_with_retry(Line 656).nym-registration-client/src/lp_client/nested_session/mod.rs#L346-L347: document it forregister_dvpn,finalise_dvpn_registration(Line 230),handshake_and_register_dvpn(Line 459), and the retry variant (Line 517).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nym-registration-client/src/builder/config.rs` around lines 42 - 50, Complete the public spend-time skew documentation: in nym-registration-client/src/builder/config.rs lines 42-50, state that positive values subtract time and negative values add time; document spend_time_skew in register_dvpn, finalise_dvpn_registration, and handshake_and_register_with_retry in nym-registration-client/src/lp_client/client.rs lines 546-547; and document it in register_dvpn, finalise_dvpn_registration, handshake_and_register_dvpn, and the retry variant in nym-registration-client/src/lp_client/nested_session/mod.rs lines 346-347.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nym-registration-client/src/builder/config.rs`:
- Around line 42-50: Complete the public spend-time skew documentation: in
nym-registration-client/src/builder/config.rs lines 42-50, state that positive
values subtract time and negative values add time; document spend_time_skew in
register_dvpn, finalise_dvpn_registration, and handshake_and_register_with_retry
in nym-registration-client/src/lp_client/client.rs lines 546-547; and document
it in register_dvpn, finalise_dvpn_registration, handshake_and_register_dvpn,
and the retry variant in
nym-registration-client/src/lp_client/nested_session/mod.rs lines 346-347.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e5e1406-024e-41c6-aa32-227fbd18abbb
📒 Files selected for processing (7)
nym-authenticator-client/src/lib.rsnym-registration-client/src/builder/config.rsnym-registration-client/src/builder/mod.rsnym-registration-client/src/clients/lp.rsnym-registration-client/src/clients/mixnet.rsnym-registration-client/src/lp_client/client.rsnym-registration-client/src/lp_client/nested_session/mod.rs
…me skew Clippy's too_many_arguments lint (deny-by-default via -D warnings in CI) started failing after the spend_time_skew parameter pushed these methods to 8 arguments, matching the existing allow already present on the sibling handshake_and_register_dvpn_with_retry.
integration-tests and nym-gateway-probe call the LP registration client's register_dvpn directly and were missing the new spend_time_skew argument, which broke the build on all platforms.
…s doc Per review: the skew is plain config data shared with entry/exit/mode, so it belongs alongside them in RegistrationClientConfig rather than as a separate field next to bandwidth_provider on each client struct. Also trimmed the field doc to the essentials and folded in the sign convention clarification that came up in review.
* NYM-1408: Use `SkewManager` with registration clients. * Simplify approach. * Pass skew off to registration clients. * fix: allow too_many_arguments on registration methods taking spend-time skew Clippy's too_many_arguments lint (deny-by-default via -D warnings in CI) started failing after the spend_time_skew parameter pushed these methods to 8 arguments, matching the existing allow already present on the sibling handshake_and_register_dvpn_with_retry. * fix: update remaining register_dvpn call sites for spend_time_skew param integration-tests and nym-gateway-probe call the LP registration client's register_dvpn directly and were missing the new spend_time_skew argument, which broke the build on all platforms. * refactor: move spend_time_skew into RegistrationClientConfig, trim its doc Per review: the skew is plain config data shared with entry/exit/mode, so it belongs alongside them in RegistrationClientConfig rather than as a separate field next to bandwidth_provider on each client struct. Also trimmed the field doc to the essentials and folded in the sign convention clarification that came up in review.
The NYM-1408 SkewManager registration-client changes have landed on the release branch (nymtech/nym@ab72e0362), so drop the temporary pin to the feature branch used to unblock CI while nymtech/nym#6973 was in review.
NYM-1408: Use
SkewManagerwith registration clients.nym-vpn-clientPR: nymtech/nym-vpn-client#5912This change is
Summary by CodeRabbit