Page MenuHomePhabricator

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

Authored by will on May 8 2024, 10:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 10:18 PM
Unknown Object (File)
Fri, Oct 18, 2:20 AM
Unknown Object (File)
Fri, Oct 18, 2:18 AM
Unknown Object (File)
Thu, Oct 17, 12:43 PM
Unknown Object (File)
Thu, Oct 17, 12:43 PM
Unknown Object (File)
Thu, Oct 17, 12:42 PM
Unknown Object (File)
Sep 26 2024, 10:36 AM
Unknown Object (File)
Sep 26 2024, 3:00 AM
Subscribers

Details

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 requested review of this revision.May 8 2024, 10:56 PM
bartek added a reviewer: ashoat.

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

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.May 9 2024, 4:11 PM
will marked 3 inline comments as done.May 15 2024, 8:37 AM
will added inline comments.
services/commtest/tests/identity_integration_tests.rs
42–43 ↗(On Diff #39960)