[identity] Add RPC for finding user ID
ClosedPublic
Actions

Authored by bartek on Nov 12 2023, 10:50 PM.

Details

Reviewers

varun
michal
will
ashoat

Commits

rCOMM2d1b959295e8: [identity] Add RPC for finding user ID

Summary

ENG-4132. Added RPC to find user ID by either username or wallet address.
The API is the same as for GetOutboundKeys RPC, which will then be refactored to instead accept user ID.

Test Plan

Created a test user, used BloomRPC to call the RPC with the username. It returned the user ID - confirmed with DDB. A unit test is added in the next diff.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Nov 12 2023, 10:50 PM

bartek held this revision as a draft.

Herald added subscribers: will, tomek. · View Herald TranscriptNov 12 2023, 10:50 PM

bartek added a child revision: D9835: [commtest][identity] Add test for FindUserID RPC.Nov 12 2023, 10:51 PM

bartek added a reviewer: will.Nov 12 2023, 10:53 PM

Harbormaster completed remote builds in B24024: Diff 33107.Nov 12 2023, 11:09 PM

bartek published this revision for review.Nov 12 2023, 11:10 PM

API looks good to me! Deferring to others on Rust

varun requested changes to this revision.Nov 13 2023, 11:07 AM

varun added inline comments.

shared/protos/identity_authenticated.proto
28 ↗	(On Diff #33107)	is there a reason why we're using the verb `Find` instead of `Get` here? I am fine with it, but want to understand the distinction
29 ↗	(On Diff #33107)	do we need a line break here? seems like it will be under 80 characters on one line
73–76 ↗	(On Diff #33107)	do you think we should factor this out to its own message type? we use it in a bunch of places and it's a little inconvenient to have multiple `Identifier` types in our Rust code when they're all identical. Also, if we want to introduce a new identifier in the future, it'll be easier to do so if we only have one `Identifier` message type

This revision now requires changes to proceed.Nov 13 2023, 11:07 AM

bartek added inline comments.Nov 14 2023, 11:02 PM

shared/protos/identity_authenticated.proto
28 ↗	(On Diff #33107)	Initially, this RPC was supposed to check if a username exists, so no user found is an expected result. I'm not a native speaker, in my understanding "get" is stronger and we either receive a user ID or error, while when we try to "find" something, we expect it might not exist. However, now I think that `Find` might be confused with @wyilio's APIs for user search. I'm okay with either `Find` or `Get`
29 ↗	(On Diff #33107)	yeah right
73–76 ↗	(On Diff #33107)	No. We want `userID` as an identifier for all other places, and this RPC should be the only one which accepts usernames, just to retrieve user ID: username -> FindUserID RPC -> userID -> all other RPCs This is what we've decided during one of the multi-keyserver scoping meetings and this is what ENG-5659 is about.