Update protos to follow style guide
ClosedPublic
Actions

Authored by bartek on Dec 21 2023, 9:44 AM.

Details

Reviewers

varun
ashoat

Commits

rCOMMbc5c70e7e902: Update protos to follow style guide

Summary

Resolves ENG-5651.
Made our protos follow the Style Guide.

WARNING: This is a breaking change! Fortunately, we don't use protos in prod yet, except keyserver cron job to update reserved usernames. Hopefully, format of that message hasn't been changed here, so at least in theory it should be fine.

NOTE: grpc-web codegen will be updated in the next diff, once we agree on casing here (this is very cumbersome to update each time)

Test Plan

Built identity, tunnelbroker, commtest, grpc_clients, rust-node-addon and native-rust-library. All CI jobs should pass.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Dec 21 2023, 9:44 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptDec 21 2023, 9:44 AM

bartek edited the summary of this revision. (Show Details)Dec 21 2023, 10:03 AM

Harbormaster failed remote builds in B25279: Diff 34916!Dec 21 2023, 10:04 AM

bartek edited the summary of this revision. (Show Details)Dec 21 2023, 10:06 AM

Fix commtest

Harbormaster completed remote builds in B25280: Diff 34917.Dec 21 2023, 10:26 AM

bartek added a child revision: D10430: Rename identity_client.proto to identity_unauthenticated.proto.Dec 21 2023, 10:49 AM

I have some questions inline

shared/protos/identity_authenticated.proto
63–64 ↗	(On Diff #34916)	IIRC we agreed on consistently using one of `prekeys` or `pre_keys`, but I don't remember which one it was. I'll update this diff to make this consistent across all places, now they're both mixed
shared/protos/identity_client.proto
21–26 ↗	(On Diff #34916)	@varun, you asked in the Linear comment to rename `login` to `log_in`. I renamed only RPC calls, left request/response messages unchanged. My reasoning is that "log in" is a verb (RPC call), and "login" is a noun (a message type). I'm not a native though so correct me if I'm wrong, I'm happy to fix these as well
87–93 ↗	(On Diff #34916)	https://protobuf.dev/programming-guides/style/#enums
222 ↗	(On Diff #34916)	I also recall that we decided to use "device ID" in our codebase and "signing public key" only in Olm-related places so I renamed it here

Oops, my inline comments are in the previous revision so adding again here

shared/protos/identity_authenticated.proto
63–64	IIRC we agreed on consistently using one of prekeys or pre_keys, but I don't remember which one it was. I'll update this diff to make this consistent across all places, now they're both mixed
shared/protos/identity_client.proto
21–26	@varun, you asked in the Linear comment to rename `login` to `log_in`. I renamed only RPC calls, left request/response messages unchanged. My reasoning is that "log in" is a verb (RPC call), and "login" is a noun (a message type). I'm not a native though so correct me if I'm wrong, I'm happy to fix these as well
87–93	https://protobuf.dev/programming-guides/style/#enums
222	I also recall that we decided to use "device ID" in our codebase and "signing public key" only in Olm-related places so I renamed it here

varun accepted this revision.Dec 22 2023, 3:02 AM

varun added inline comments.

shared/protos/identity_authenticated.proto
63–64	the whitepaper uses `prekey` so we should use that i think
shared/protos/identity_client.proto
21–26	your reasoning is correct!
222	good call

Fortunately, we don't use protos in prod yet, except keyserver cron job to update reserved usernames. Hopefully, format of that message hasn't been changed here, so at least in theory it should be fine.

Will we need to "sync" deploys of keyserver and Comm services after landing this diff?

shared/protos/identity_authenticated.proto
63–64	I'd agree with this. We got this from Signal, which uses "prekey" (eg. here)

This revision is now accepted and ready to land.Dec 22 2023, 7:22 AM

In D10429#302126, @ashoat wrote:

Fortunately, we don't use protos in prod yet, except keyserver cron job to update reserved usernames. Hopefully, format of that message hasn't been changed here, so at least in theory it should be fine.

Will we need to "sync" deploys of keyserver and Comm services after landing this diff?

In theory, we shouldn't have to, as this RPC hasn't been changed, but it's a good idea to do so anyway. I'll ping a heads-up on "Releases and beyond" when landing this

Renamed pre_key -> prekey and PreKey -> Prekey

bartek mentioned this in D10573: Update gRPC-web generated files.Jan 8 2024, 2:35 AM

Harbormaster failed remote builds in B25582: Diff 35349!Jan 8 2024, 2:42 AM

This revision was landed with ongoing or failed builds.Jan 9 2024, 1:23 AM

Closed by commit rCOMMbc5c70e7e902: Update protos to follow style guide (authored by bartek). · Explain Why

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMMbc5c70e7e902: Update protos to follow style guide.

bartek mentioned this in rCOMM2028825c7ee2: Update gRPC-web generated files.

bartek mentioned this in D10582: [terraform] Deploy updated Identity and Tunnelbroker.Jan 9 2024, 6:13 AM

bartek mentioned this in rCOMM44740d23f987: [terraform] Deploy updated Identity and Tunnelbroker.Jan 9 2024, 6:47 AM