Page MenuHomePhabricator

[lib/native] introduce useLinkFID hook
ClosedPublic

Authored by ginsu on Thu, May 2, 11:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 23, 9:31 AM
Unknown Object (File)
Thu, May 23, 7:40 AM
Unknown Object (File)
Thu, May 23, 5:32 AM
Unknown Object (File)
Mon, May 13, 6:30 AM
Unknown Object (File)
Sun, May 12, 10:46 AM
Unknown Object (File)
Sun, May 12, 5:37 AM
Unknown Object (File)
Sun, May 12, 5:33 AM
Unknown Object (File)
Thu, May 9, 4:56 PM
Subscribers

Details

Summary

This diff introduces useLinkFID which is a hook that returns a callback that calls the linkFarcasterAccount identity rpc and then sets the fid into the synced metadata client store

Depends on D11873

Test Plan

Confirmed that the fid was being set into the synced metadata client store + confirmed in meeting with @varun that I was correctly calling and getting the expected behavior of linkFarcasterAccount RPC.

Confirmed that loading + error states worked as expected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu added reviewers: will, varun.
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, May 2, 11:48 PM
Harbormaster failed remote builds in B28638: Diff 39756!
ginsu requested review of this revision.Thu, May 2, 11:54 PM

will make sure ci passes before landing

ashoat requested changes to this revision.Fri, May 3, 12:01 PM

It's unclear to me why we need to call linkFID from registration. Doesn't the identity registration call already link the FID?

native/account/registration/registration-server-call.js
269 ↗(On Diff #39756)

EDIT: actually not sure why this is even here. Leaving the comment below in case you decide to keep it, but it's not clear to me why this change is necessary.

You're introducing a big risk here. If linkFID fails it will leave the registration is a partially-successful state, and won't delete the resultant identity account. As a result, if the user retries after this failure, they will get a second error that the username is already taken.

They'll also be using the same private keys to attempt to register two different identity accounts, which would be a problem if the user backs out and changes the username, and retries to register account. It will succeed and result in two different identity accounts with the same keys.

Can you wrap this in a try-catch that:

  1. Shows an error message if it fails. If it fails due to the Farcaster account already being linked, let's show the same error text that I introduced in D11862 (should probably be factored out into alert-messages.js)
  2. Runs this code to clear the identity account, both locally and on the identity service:
void dispatchActionPromise(
  deleteAccountActionTypes,
  discardIdentityAccountPromise,
);
await waitUntilDatabaseDeleted();

See how it works in the effect later in the file. Also you'll need to make sure that waitUntilDatabaseDeleted finishes before you reject the promise.

native/components/connect-farcaster-bottom-sheet.react.js
38 ↗(On Diff #39756)

Should probably be renamed since failure can now still occur

38–43 ↗(On Diff #39756)

What is the point of this onSuccess function now? Can it be deleted? (Probably not if you take my above advice)

40 ↗(On Diff #39756)

What happens on failure? If it fails because the FC account is already linked, let's show the same error text I introduced in D11862 (should probably be factored out into alert-messages.js). It it fails otherwise, we can use UnknownErrorAlertDetails

We probably also need to call setWebViewState('closed') on failure. By the way, why aren't we calling it on success?

native/profile/farcaster-account-settings.react.js
47–51 ↗(On Diff #39756)

Error-handling needed here too

This revision now requires changes to proceed.Fri, May 3, 12:01 PM
native/account/registration/registration-server-call.js
269 ↗(On Diff #39756)

Spoke to @varun about this, and concluded that this was a big misunderstanding on my part

ashoat requested changes to this revision.Sat, May 4, 11:31 AM
ashoat added inline comments.
native/components/connect-farcaster-bottom-sheet.react.js
42 ↗(On Diff #39781)

Are we making sure the in-progress state is reflected well in the UI? When does the bottom sheet get dismissed? When does the spinner in the button stop spinning?

native/utils/farcaster-utils.js
15 ↗(On Diff #39781)

Can we rename this to tryLinkFID or something? The naming is confusing and I think the functionality of this function gets more clear once setWebViewState is removed from here

22 ↗(On Diff #39781)

I’m not sure this needs to be factored out. I think it makes things more complicated than they need to be. Can we leave this part to be handled by the component that calls this hook?

28 ↗(On Diff #39781)

You need getMessageForException. I think this code can crash if somebody throws a non-object

This revision now requires changes to proceed.Sat, May 4, 11:31 AM
ashoat added inline comments.
native/components/connect-farcaster-bottom-sheet.react.js
50 ↗(On Diff #40140)

This try and the one in FarcasterAccountSettings aren't really needed because as you've written it, tryLinkFID can never throw. I guess it doesn't hurt to have them in case somebody changes tryLinkFID later though?

native/utils/farcaster-utils.js
14 ↗(On Diff #40140)

Nit: newFID

28 ↗(On Diff #40140)

Nit: I'd remove this newline

This revision is now accepted and ready to land.Tue, May 14, 9:28 AM
native/components/connect-farcaster-bottom-sheet.react.js
50 ↗(On Diff #40140)

Yea thought it would be good to have this to make sure setIsLoadingLinkFID will always run despite any future changes to tryLinkFID

address comments + rebase before landing