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

Linked a new comm account with my fid
Intentionally passed in a hardcoded invalid fid into checkIfCurrentUserFIDIsValid to trigger this unlink mechanism
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)

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

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

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:

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

forgot to save/commit changes

ginsu added inline comments.Jul 8 2024, 4:12 PM

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

Harbormaster completed remote builds in B30214: Diff 42134.Jul 8 2024, 4:16 PM

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:

Remove the check on line 161
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

ashoat mentioned this in D12569: [lib/web] introduce handleCurrentUserFID to FarcasterDataHandler.Jul 9 2024, 10:30 AM

address comments

ginsu added inline comments.Jul 9 2024, 1:42 PM

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

Harbormaster completed remote builds in B30240: Diff 42173.Jul 9 2024, 1:58 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

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.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42099

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

lib/utils/neynar-client.js

[lib] check if the user fid is still validClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42099

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

lib/utils/neynar-client.js

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

Revision Contents
Changeset List