[identity] modify get user identity rpc to take user ids and return identity map with farcaster id
ClosedPublic
Actions

Authored by will on Wed, May 8, 10:41 PM.

Details

Reviewers

bartek
kamil
ashoat

Commits

rCOMMca5fa9314dd8: [identity] modify get user identity rpc to take user ids and return identity…

Summary

This diff modifies our protos for the get user identity rpc to take in a list of user ids and return their corresponding identities.

Additionally, for our farcaster work, the Identity message will now also include a farcaster id field

Depends on D11951

Test Plan

Deployed on staging and called the rpcs with a list of user ids, including a default username user, an ethereum user, a default user with farcaster id, and a non-existent user.

The rpc returned all the proper Identity info and didn't include the non-existent userID

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

will created this revision.Wed, May 8, 10:41 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptWed, May 8, 10:41 PM

will edited the summary of this revision. (Show Details)Wed, May 8, 10:43 PM

will added a parent revision: D11951: Refactor get user identifier to return identity with farcaster id.

will added a child revision: D11953: [web] Make find user identities available to client.Wed, May 8, 10:50 PM

Harbormaster completed remote builds in B28773: Diff 39960.Wed, May 8, 10:56 PM

will requested review of this revision.Wed, May 8, 10:56 PM

Rust part is ok

Adding @ashoat due to proto changes

services/commtest/tests/identity_integration_tests.rs
42–43 ↗	(On Diff #39960)	Not related to this diff but I realized we have no Farcaster integration tests. Idea for a simple integration test: Copy-paste this test. Call `client.link_farcaster_account()` before querying Call `find_user_identities()` but instead of username, check for fid being present
services/identity/src/database.rs
744 ↗	(On Diff #39960)	We talked about adding more logs in the last retro
754 ↗	(On Diff #39960)
shared/protos/identity_auth.proto
98 ↗	(On Diff #39960)	I was wondering if it's more correct to have it inside `EthereumIdentity`, but the approach here is more consistent with what you agreed on in this discussion. So it probably should stay this way

will added inline comments.Thu, May 9, 10:53 AM

shared/protos/identity_auth.proto
98 ↗	(On Diff #39960)	To clarify for anyone else reading this. You can have a `farcaster_id` without an `EthereumIdentity` so the two should remain separate

Proto changes look good!

web/protobufs/identity-auth-client.cjs.flow
225 ↗	(On Diff #39960)	Don't need this newline

This revision is now accepted and ready to land.Thu, May 9, 4:11 PM

will added a child revision: D11998: [native] implement 'FindUserIdentities' RPC in `native_rust_library`.Fri, May 10, 9:48 PM

will removed a child revision: D11953: [web] Make find user identities available to client.Fri, May 10, 9:53 PM

will marked 3 inline comments as done.Wed, May 15, 8:37 AM

will added inline comments.

services/commtest/tests/identity_integration_tests.rs
42–43 ↗	(On Diff #39960)	@bartek created a linear issue addressing this here: https://linear.app/comm/issue/ENG-8065/add-commtest-tests-for-farcaster-accounts