Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved PB Timestamp index to 10 #861

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Moved PB Timestamp index to 10 #861

merged 2 commits into from
Feb 23, 2022

Conversation

s1fr0
Copy link
Contributor

@s1fr0 s1fr0 commented Feb 22, 2022

This is a fix to maintain retro-compatibility with timestamp protobuffer fields.

Timestamps now have Timestamp type (currently an alias for int64) and nanoseconds resolution. Their new protobuffer index is 10.

Previous versions had timestamps as float with subseconds contained in the fractional part and protobuffer index 4.

This PR is related to vacp2p/rfc#491, #842 and vacp2p/rfc#483.

@jm-clius
Copy link
Contributor

Thanks! This will buy some time for other clients to implement the new WakuMessage proto without breaking.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Thanks!

@status-im-auto
Copy link
Collaborator

status-im-auto commented Feb 22, 2022

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
09bfbd4 #1 2022-02-22 11:27:42 ~19 min linux 📄log
09bfbd4 #2 2022-02-22 11:31:13 ~2 min linux 📄log
✔️ 09bfbd4 #1 2022-02-22 11:37:03 ~28 min macos 📦bin
✔️ 09bfbd4 #1 2022-02-22 11:45:57 ~37 min windows 📦exe
✔️ 09bfbd4 #3 2022-02-22 13:40:33 ~15 min linux 📦bin
✔️ cd11a74 #4 2022-02-22 22:58:02 ~15 min linux 📦bin
✔️ cd11a74 #2 2022-02-22 23:05:24 ~22 min macos 📦bin
✔️ cd11a74 #2 2022-02-22 23:12:39 ~29 min windows 📦exe

Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Looks good to me, what triggered this change? what was the issue with index 4?

Update: found the answer in

Thanks! This will buy some time for other clients to implement the new WakuMessage proto without breaking.

Are we going to bring it back to index 4 once the timestamp type and resolution are updated by other clients?

@s1fr0 s1fr0 merged commit c27086e into master Feb 23, 2022
@s1fr0 s1fr0 deleted the pb-timestamp branch February 23, 2022 07:19
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.

6 participants