Skip to content

Conversation

@agentpietrucha
Copy link
Contributor

@agentpietrucha agentpietrucha commented Nov 5, 2025

Ticket

JIRA-VPN-4401

Description

Cache skew with remote time with TTL = 4 hours

Checklist:

  • Changelog

Screenshots (optional, if UI related)


This change is Reviewable

Copy link
Collaborator

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mmsinclair)


a discussion (no related file):
Please cargo fmt the source.

Copy link
Collaborator

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

@pronebird reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @agentpietrucha and @mmsinclair)


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 50 at r1 (raw file):

#[derive(Default, Debug)]
struct SkewState {

Nit: you may really benefit from holding Option<SkewState> which would eliminate the need for members to be Optional as both skew and expires_at are set together.

@agentpietrucha agentpietrucha marked this pull request as ready for review November 5, 2025 17:18
@agentpietrucha agentpietrucha changed the title Cache JWT skew VPN-4401: Cache JWT skew Nov 5, 2025
Copy link
Collaborator

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

@pronebird reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @agentpietrucha and @mmsinclair)

@pronebird pronebird requested a review from trojanfoe November 6, 2025 13:07
Copy link
Collaborator

@neacsu neacsu left a comment

Choose a reason for hiding this comment

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

@neacsu reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @agentpietrucha, @mmsinclair, and @pronebird)


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 57 at r3 (raw file):

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum SkewStatus {
    Expired(),

nit: No need for it to be function, it can be just Expired


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 228 at r3 (raw file):

                tracing::debug!("Valid VPN API time skew");
                let local_time = OffsetDateTime::now_utc();
                let estimated_remote_time = local_time - skew;

This subtraction is dependent on the fact that the skew is computed in a certain way inside local_time_ahead_skew from what I can tell. Although it seems to be correct at this moment, it's somewhat code duplicating and I think it should be refactored and implementation details be kept in one place (maybe inside VpnApiTime methods)

@agentpietrucha
Copy link
Contributor Author

@neacsu I'll reference your latest comments in #3921

@agentpietrucha agentpietrucha merged commit a3abbbf into develop Nov 13, 2025
16 of 17 checks passed
@agentpietrucha agentpietrucha deleted the vpn-4401-jwt-skew-cache branch November 13, 2025 13:18
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.

5 participants