[protos] Add PlatformDetails to PeersDeviceListsResponse
ClosedPublic
Actions

Authored by bartek on Jun 3 2024, 4:19 AM.

Details

Reviewers

marcin
ashoat

Commits

rCOMMc1a7a6082c38: [protos] Add PlatformDetails to PeersDeviceListsResponse

Summary

Extended response for device list multi-fetch RPC to include PlatformDetails for each device.

Unfortunately, proto doesn't allow for map<K_outer, map<K_inner, V>> directly, so needed to introduce intermediate message type.

This message could be nested, but grpc-web codegen generated Flow-unfriendly types.

Depends on D12271

Test Plan

proto compiles, identity compiles, flow for web

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Jun 3 2024, 4:19 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptJun 3 2024, 4:19 AM

bartek added a child revision: D12273: [identity] Fetch platform details along with device lists.Jun 3 2024, 4:22 AM

Harbormaster completed remote builds in B29351: Diff 40836.Jun 3 2024, 4:35 AM

CI failure unrelated

Proto and Flow changes look reasonable. I have two questions inline. I think stateVersion should probably be required, but we could alternately consider just having a single PlatformDetails JSON blob depending on how it's used. This would avoid having to convert between the two formats on the client

shared/protos/identity_auth.proto
237–240 ↗	(On Diff #40836)	We can do this, but why not just use a JSON blob that matches `PlatformDetails` exactly? It seems like it might be easier to avoid having multiple formats for the same data. Curious for other perspectives
239 ↗	(On Diff #40836)	Is this really optional? `codeVersion` is listed as optional in `PlatformDetails`, but I don't think it is anymore (that was for old web clients). I think `stateVersion` is in a similar bucket, but not sure – would be good to have more context on why you made it optional here

bartek added a subscriber: varun.Jun 3 2024, 10:56 PM

bartek added inline comments.

shared/protos/identity_auth.proto
237–240 ↗	(On Diff #40836)	I don't have any strong opinion on this. It somehow makes sense and might make it easier to parse for @marcin on the other hand IIRC having JSON is an anti-pattern in protos, cc @varun.
239 ↗	(On Diff #40836)	We can have keyserver as a device. I don't think we have any state version there For existing devices in DDB, we don't have this value stored. If this shouldn't be optional, we need to default this value e.g. to 0 or sth

bartek mentioned this in D12308: Enable the client to fetch users platform details from identity RPC.Jun 4 2024, 1:51 AM

marcin added inline comments.Jun 4 2024, 8:59 AM

shared/protos/identity_auth.proto
237–240 ↗	(On Diff #40836)	Parsing in Rust is not an issue for me. Additionally how does using JSON blob prevent the client from sending invalid JSON (JSON missing state_version)?

marcin added a subscriber: ashoat.Jun 4 2024, 9:35 AM

marcin added inline comments.

shared/protos/identity_auth.proto
237–240 ↗	(On Diff #40836)	My preference is to use some hardcoded constant for keyserver. I agree with @ashoat that we should require state version.