fix!: rename timestamp fields to embed time unit in field name#44
Open
VasilevNStas wants to merge 1 commit into
Open
fix!: rename timestamp fields to embed time unit in field name#44VasilevNStas wants to merge 1 commit into
VasilevNStas wants to merge 1 commit into
Conversation
Three different time units were used across the protocol with no naming distinction: Auth.timestamp (minutes since project epoch), Ping/Pong.timestamp (ms since Unix epoch), and all lease/expiry fields (seconds since Unix epoch). A client computing e.g. Pong.cutoff - Pong.timestamp would get a result in mixed seconds-minus-milliseconds with no type-level guard. Rename: Auth.timestamp -> timestamp_min (minutes since 2024-01-01 UTC) Ping.timestamp -> timestamp_ms (ms since Unix epoch) Pong.timestamp -> timestamp_ms (echo of Ping.timestamp_ms) Field numbers are unchanged, so the wire format is compatible. Generated code symbols change (source break).
Contributor
Author
|
@davvd plz review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Renames three
timestampfields across auth.proto, ping.proto, and pong.proto to embed their time unit in the field name, preventing silent arithmetic errors when values in different units are inadvertently combined.Problem
The protocol uses three different time units with identical field names:
Auth.timestampPing.timestamp/Pong.timestampA client computing
Pong.cutoff - Pong.timestampwould get a result in mixed seconds-minus-milliseconds. Proto3 provides no type-level guard since all fields areuint64.Fix
Auth.timestamp = 2Auth.timestamp_min = 2Ping.timestamp = 1Ping.timestamp_ms = 1Pong.timestamp = 1Pong.timestamp_ms = 1Migration
Wire format is backward-compatible (field numbers unchanged). Source-level: all generated struct field names change from
timestamptotimestamp_min/timestamp_ms.Closes #12