Page MenuHomePhabricator
Differential D12685

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

Authored by ginsu on Jul 8 2024, 1:26 AM.
Tags
None
Referenced Files
F3137928: D12685.id42182.diff
Sat, Nov 2, 11:40 PM
Unknown Object (File)
Fri, Oct 25, 2:44 AM
Unknown Object (File)
Fri, Oct 18, 8:36 AM
Unknown Object (File)
Wed, Oct 16, 12:26 AM
Unknown Object (File)
Tue, Oct 15, 8:11 AM
Unknown Object (File)
Tue, Oct 15, 5:18 AM
Unknown Object (File)
Tue, Oct 15, 2:07 AM
Unknown Object (File)
Fri, Oct 11, 3:31 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
ashoat
inka
Commits
rCOMMd5263d00f887: [lib] check if the user fid is still valid
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 created this revision.Jul 8 2024, 1:26 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 8 2024, 1:26 AM
ginsu edited the summary of this revision. (Show Details)Jul 8 2024, 1:30 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, inka.
ginsu added a parent revision: D12569: [lib/web] introduce handleCurrentUserFID to FarcasterDataHandler.
ginsu edited the summary of this revision. (Show Details)
ginsu updated this revision to Diff 42100.Jul 8 2024, 1:32 AM

update

ginsu edited the summary of this revision. (Show Details)Jul 8 2024, 1:32 AM
Harbormaster completed remote builds in B30187: Diff 42099.Jul 8 2024, 1:46 AM
Harbormaster completed remote builds in B30188: Diff 42100.
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 ↗(On Diff #42100)

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 ↗(On Diff #42100)

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 ↗(On Diff #42100)

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
ginsu updated this revision to Diff 42133.Jul 8 2024, 3:55 PM

address comments

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

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

lib/utils/neynar-client.js
313 ↗(On Diff #42100)

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)

Harbormaster failed remote builds in B30213: Diff 42133!Jul 8 2024, 3:59 PM
ginsu attached a referenced file: F2214147: Screenshot 2024-07-08 at 6.47.51 PM.png. (Show Details)Jul 8 2024, 3:59 PM
ginsu updated this revision to Diff 42134.Jul 8 2024, 4:00 PM

forgot to save/commit changes

ginsu added inline comments.Jul 8 2024, 4:12 PM
lib/components/farcaster-data-handler.react.js
178–189

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

Harbormaster completed remote builds in B30214: Diff 42134.Jul 8 2024, 4:16 PM
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

Feels like this line could be moved outside the callback

156–157

Should these be combined into !canQuery?

161

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

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
ashoat mentioned this in D12569: [lib/web] introduce handleCurrentUserFID to FarcasterDataHandler.Jul 9 2024, 10:30 AM
ginsu updated this revision to Diff 42173.Jul 9 2024, 1:40 PM

address comments

ginsu added inline comments.Jul 9 2024, 1:42 PM
lib/components/farcaster-data-handler.react.js
148

Addressed this in D12569

156–157

Addressed this in D12569

161

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
ginsu added a comment.Jul 9 2024, 1:48 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

Harbormaster completed remote builds in B30240: Diff 42173.Jul 9 2024, 1:58 PM
ashoat accepted this revision.Jul 9 2024, 2:27 PM

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
ginsu updated this revision to Diff 42182.Jul 9 2024, 3:31 PM

rebase before landing

Harbormaster completed remote builds in B30247: Diff 42182.Jul 9 2024, 3:48 PM
Closed by commit rCOMMd5263d00f887: [lib] check if the user fid is still valid (authored by ginsu). · Explain WhyJul 9 2024, 3:57 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMd5263d00f887: [lib] check if the user fid is still valid.

Revision Contents
Changeset List

PathSize
lib/
components/
36 lines
utils/
22 lines

Diff 42134

View Options

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

Loading...
View Options

lib/utils/neynar-client.js

Loading...