Details
- registered two users: one password user and one wallet user
- called the RPC for each, made sure that username is retrieved for the first, and wallet address + social proof pair is retrieved for the second
If the RPC format is approved, I'll add a commtest test too.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- barthap/protos1
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| shared/protos/identity_auth.proto | ||
|---|---|---|
| 71 ↗ | (On Diff #38286) | I called the RPC FindUserIdentity because of this type, but the word Identity was initially a bit confusing to me |
Would be good for @varun to review the Rust.
At a later point we might want to introduce a feature where somebody with an ETH account can still have a username. This would make sense for ETH users that don't have an ENS name.
I wonder if we should future-proof this API now. I guess the best thing to do would be to modify Identity as follows:
message Identity { string username = 1; optional EthereumIdentity eth_identity = 2; }
If the user is using their ETH wallet address as a username, then username will just be set to the wallet address.
I'm guessing making that change for all places that use Identity now might be tough, but it should be easier to do for this new RPC. And arguably, this is the time to change all of the RPCs since we're trying to finalizing RPC protos.
Accepting to avoid blocking this work, but curious for what you think of my proposed changes.
Actually I like this proposal, created ENG-7532 to discuss this
I'm guessing making that change for all places that use Identity now might be tough, but it should be easier to do for this new RPC. And arguably, this is the time to change all of the RPCs since we're trying to finalizing RPC protos.
Actually, looked at client code and we're using the Identity field only in one place on keyserver (inbound keys), so it might be easy to update it
what happens if the user doesn't exist? it seems like we'd want to return a "not found" tonic status in this case, but i'm not sure that's what we're doing here
- Made get_user_identifier() return Option
- Updated callsites to return Status::not_found if missing identifier
do we need to change any client code now that some of these RPCs are returning "not found" ?
No, I don't think so. Apparently, clients haven't had to deal with this error case. Previously it was "unexpected error" so we would know if clients were failing for any reason, including user not found.