Page MenuHomePhabricator

[lib] check if the user fid is still valid
ClosedPublic

Authored by ginsu on Jul 8 2024, 1:26 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 12:29 AM
Unknown Object (File)
Sun, Nov 17, 12:48 PM
Unknown Object (File)
Thu, Nov 14, 10:53 PM
Unknown Object (File)
Sun, Nov 10, 9:23 AM
Unknown Object (File)
Sun, Nov 10, 9:17 AM
Unknown Object (File)
Sun, Nov 10, 8:19 AM
Unknown Object (File)
Sun, Nov 10, 7:25 AM
Unknown Object (File)
Sun, Nov 10, 4:31 AM
Subscribers

Details

Summary

This diff checks if the fid stored in the synced metadata table store is still valid. We could run into this situation, if a user connects their facaster account to their comm account, then later deletes their farcaster account, but their comm account still has the old deleted fid. If this is the case we should unlink this fid from the user's comm account

Linear task: https://linear.app/comm/issue/ENG-7277/check-if-the-user-fid-stored-in-the-synced-metadata-table-is-still

Depends on D12569

Test Plan
  1. Linked a new comm account with my fid
  2. Intentionally passed in a hardcoded invalid fid into checkIfCurrentUserFIDIsValid to trigger this unlink mechanism
  3. Confirmed in the identity logs that the fid was removed and the account is now unlinked from my farcaster account

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, inka.
ginsu edited the summary of this revision. (Show Details)
ginsu requested review of this revision.Jul 8 2024, 1:48 AM
ashoat requested changes to this revision.Jul 8 2024, 12:25 PM

Worried that once !shouldCheckCurrentFIDRef.current, we'll be executing lines 171 and on, even after our FID is already set locally

lib/components/farcaster-data-handler.react.js
171–182

Previously, this logic would not be executed if fid was set

Now, it can be executed if fid is set to NO_FID_METADATA (which seems correct) or if shouldCheckCurrentFIDRef.current is false (which seems incorrect)

172

I know this code is not changed here, but wondering – is this line really safe? Is it possible that identity responds with an object that doesn't have a currentUserID key? If that happens, the .farcasterID dereference will crash the app

Should we say ?.farcasterID here?

lib/utils/neynar-client.js
313

Does it really work to pass a single fid here as fids? Are you sure we don't need to wrap it in an array or something?

This revision now requires changes to proceed.Jul 8 2024, 12:25 PM

address comments

lib/components/farcaster-data-handler.react.js
172

I introduced this line in D12569 so updated that diff to address this concern

lib/utils/neynar-client.js
313

Logged out the url and confirmed that the value being logged out matched perfectly with the url provided in the neynar docs

Log:
https://api.neynar.com/v2/farcaster/user/bulk?fids=388441

Neynar docs:

Screenshot 2024-07-08 at 6.47.51 PM.png (692×2 px, 215 KB)

forgot to save/commit changes

lib/components/farcaster-data-handler.react.js
178–189 ↗(On Diff #42134)

Confirmed that this logic is only called if the fid is not set and it's only called once per app start / foreground event

ashoat requested changes to this revision.Jul 9 2024, 10:23 AM

Sorry for another round, but I'm concerned that we'll never query identity for an FID if the user dismisses the Farcaster prompt.

Broadly, I think treating fid === NO_FID_METADATA the same as !fid here makes sense. I'd suggest the following changes:

  1. Remove the check on line 161
  2. Update the check on line 166 to be fid && fid !== NO_FID_METADATA

This is what you had before, and in this comment I suggested it was fine ("seems correct")... I was just concerned about the shouldCheckCurrentFIDRef.current check.

lib/components/farcaster-data-handler.react.js
148 ↗(On Diff #42134)

Feels like this line could be moved outside the callback

156–157 ↗(On Diff #42134)

Should these be combined into !canQuery?

161 ↗(On Diff #42134)

What happens if the user has two devices, and dismisses the Farcaster prompt on one device, but then associates a Farcaster account on the second one. Will the first device never get the FID because of this condition?

170–175 ↗(On Diff #42134)

Nit: in this case I'd reverse the condition to simplify the logic

This revision now requires changes to proceed.Jul 9 2024, 10:23 AM
lib/components/farcaster-data-handler.react.js
148 ↗(On Diff #42134)

Addressed this in D12569

156–157 ↗(On Diff #42134)

Addressed this in D12569

161 ↗(On Diff #42134)

Updated this diff to address this case

ashoat requested changes to this revision.Jul 9 2024, 1:46 PM
ashoat added inline comments.
lib/components/farcaster-data-handler.react.js
162 ↗(On Diff #42173)

Did you intentionally skip this part of my feedback?

Update the check on line 166 to be fid && fid !== NO_FID_METADATA

This revision now requires changes to proceed.Jul 9 2024, 1:46 PM

Was writing inline comments explaining why fid && fid !== NO_FID_METADATA can be just fid

lib/components/farcaster-data-handler.react.js
162 ↗(On Diff #42173)

In D12683, we updated the logic for fid to return null if the user has decided NOT to set a Farcaster association and undefined if the user does not have an fid and has not been prompted yet. Due to this change, we should be able to simplify this check to just check if the fid is set here with a defined value.

174–185 ↗(On Diff #42173)

Confirmed that this logic is only called if the fid is set as null or undefined and it's only called once per app start / foreground event

Thanks for explaining

lib/components/farcaster-data-handler.react.js
162 ↗(On Diff #42173)

Got it!! Thanks, and sorry I missed that

This revision is now accepted and ready to land.Jul 9 2024, 2:27 PM
This revision was automatically updated to reflect the committed changes.