- Expose a new endpoint that allows linking the DCs
- Add an option to provide the token during registration
https://linear.app/comm/issue/ENG-10893/receive-and-store-a-token-from-farcaster
Details
Details
- Reviewers
kamil bartek - Commits
- rCOMMdec2db8187d2: [identity] Allow linking Farcaster DCs
Tested in combination with the rest of the stack. Checked if linking during registration and from the profile screen works.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- farcaster2
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Generally looks okay, given this diff contains multiple changes:
- [proto] Adds new LinkFarcasterDCsAccount RPC
- [identity] Implements the RPC
- grpc-web changes for this seem to be applied in D14927
- [proto] Adds new param to Registration RPCs - it's okay, this is also backwards-compatible
- [native] Exposes it via JSI to JS
- [identity] Adds the DCs token to the DBIdentity type, it is used in D14931
This diff should be landed together with the two diffs following it in the stack.
Deployment order (deploying client vs releasing Identity) shouldn't matter, since the value is defaulted to an empty string when absent/unknown by either side.
| services/identity/src/grpc_services/authenticated.rs | ||
|---|---|---|
| 1140–1153 ↗ | (On Diff #48722) | Technically, it's either missing or multiple users with the same FID. Separately, I'm wondering if we should really trigger Identity alerts (by using error!) when a client doesn't have FID linked and calls this RPC. I guess this would indicate error in our client logic which shouldn't allow calling it for non-farcaster users |
| shared/protos/identity_auth.proto | ||
| 102 ↗ | (On Diff #48722) | It's unfortunate that this name casing causes Rust function name to be ...facrcaseter_d_cs_. I guess DC or Dcs would fix Rust names but on the other hand it looks weird here. |
Comment Actions
Improve error handling
| services/identity/src/grpc_services/authenticated.rs | ||
|---|---|---|
| 1140–1153 ↗ | (On Diff #48722) | Makes sense! |
| shared/protos/identity_auth.proto | ||
| 102 ↗ | (On Diff #48722) | Yeah, agree. Either way we have to compromise, so maybe keeping the current name is more pragmatic. |
Comment Actions
This diff should be landed together with the two diffs following it in the stack.
Deployment order (deploying client vs releasing Identity) shouldn't matter, since the value is defaulted to an empty string when absent/unknown by either side.
Tested running a new client with the prod services: registering, connecting Farcaster, getting user identities - and it works fine, so we're safe to land both the client and service changes at the same time. Even if we decide to deploy prod services after publishing the client changes, nothing will break.
In the future, though, I should separate the client and services changes so that they can be landed in isolation.