:feat add passport react context and hooks #2068

shineli1984 · 2024-08-05T08:18:16Z

Summary

Add passport react context and hooks to reduce the code user need to write in a react app.

Detail and impact of the change

Added

Passport context and hooks for react app

todo:

nx-cloud · 2024-08-05T08:25:02Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c984dfb. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

keithbro-imx

Some initial comments, but i'll try this in a real app and provide feedback

keithbro-imx · 2024-08-08T05:41:25Z

packages/passport/sdk/src/react/PassportContext.tsx

+  const [isLoggedIn, setLoggedIn] = useState(false);
+  const [isLoading, setIsLoading] = useState(false);
+  const [accounts, setAccounts] = useState<string[]>([]);
+  const [error, setError] = useState<Error | null>(null);


Hmmm no... I don't think you want to say this is Error or null when you're putting any in to it.

casted to Error in catch statements

keithbro-imx · 2024-08-08T05:51:11Z

packages/passport/sdk/src/react/PassportContext.tsx

+      setLoading(true);
+      passportInstance
+        .getIdToken()
+        .then((t) => setIdToken(t || null))


I think we'd want to handle t being undefined here in some way. What would cause this? And what would we expect to get back from useIdToken in this scenario?

this is from calling oidcClient on this line:

ts-immutable-sdk/packages/passport/sdk/src/authManager.ts

Line 525 in f4f6eee

const oidcUser = await this.userManager.getUser();

So we don't really have control over.

Hmm but in what scenario would this happen? If isLoggedIn is true and getIdToken returns undefined, should that set an error for example? Consider the result of useIdToken - it would be { idToken: null, error: null, loading: false } - not helpful

This should be equivalent to an internal error / bug scenario. I can set the error in this case. however, it complex things a bit more because now we need to consider if we need to clear up all the other fields.

keithbro-imx · 2024-08-08T05:53:41Z

packages/passport/sdk/src/react/PassportContext.tsx

+  login: () => Promise<string[]>;
+  logout: () => Promise<void>;
+  loginWithoutWallet: () => Promise<UserProfile | null>;
+  loginWithEthersjs: () => Promise<{ accounts: string[], provider: Web3Provider | null }>;


When would provider be null here?

I'll move provider onto the context but it's null when provider.send('eth_requestAccounts', []) rejects.

keithbro-imx · 2024-08-08T07:11:19Z

packages/passport/sdk/src/react/PassportContext.tsx

+  accounts: string[];
+};
+
+const PassportContext = createContext<PassportContextType>({


Alternatively, its less finicky if you do

const PassportContext = createContext<PassportContextType|null>(null);

Works just as well as far as I know.

I'll do this.

Jon-Alonso · 2024-08-08T07:10:17Z

packages/passport/sdk/src/react/PassportContext.tsx

+    isLoading,
+    accounts,
+    error,
+    login: async () => {


Why couple the login method to EVM? Any reason why we can't follow the existing interface?

I'm going with our documentation to separate

login: https://docs.immutable.com/products/zkEVM/passport/identity/login#2-initialise-the-provider-and-login-users

and login without wallet: https://docs.immutable.com/products/zkEVM/passport/identity/login#2-initialise-the-provider-and-login-users

I assume we want user to use the first one? what are you suggesting? name this function as connectEVM and the other one as login?

Jon-Alonso · 2024-08-08T07:11:17Z

packages/passport/sdk/src/react/PassportContext.tsx

+        setIsLoading(false);
+      }
+    },
+    loginWithEthersjs: async () => {


Shouldn't ethers be treated as a peer dependency? Isn't it safe to assume that partners intending to use ethers will already have it installed?

Passport sdk package already has @ethersproject/providers as an dependency tho. Should I change it to peer?

As far as I know that library is used internally only whereas you're exposing it here

We use some of the modules from Ethers internally, but we do not expose Ethers to consumers of Passport. By exposing Ethers from Passport like this, we'll need to continue to support it indefinitely, or make a breaking change if we decide to remove it.

This could also open up the door for partners to request that we add methods for other wallet connection libraries (e.g. Viem), so I think it's best if we just expose the raw Passport provider and let consumers wrap it themselves.

I'll try remove this and see what kind of experience it is with 3rd party provider.

Jon-Alonso · 2024-08-08T07:17:37Z

packages/passport/sdk/src/react/PassportContext.tsx

+  accounts: string[];
+};
+
+const PassportContext = createContext<PassportContextType>({


Do we need to consider the loginCallback too?

yeah. I can add that

keithbro-imx · 2024-08-09T01:03:42Z

packages/passport/sdk/src/react/PassportContext.tsx

+  children: React.ReactNode;
+};
+
+export function PassportProvider({ children, config }: PassportProviderProps) {


I'd recommend calling this something like ReactProvider to avoid confusion with the provider on line 17

Considering the usage side, maybe PassportReactProvider?

Normally you're going to be doing

import { passport } from '@imtbl/sdk';

so the double "passport" in

passport.PassportReactProvider

would be unnecessary IMO

keithbro-imx · 2024-08-09T01:17:17Z

packages/passport/sdk/src/react/PassportContext.tsx

+  children: React.ReactNode;
+};
+
+export function PassportProvider({ children, config }: PassportProviderProps) {


It would be useful if we could pass in the passport instance, rather than the config.

Reason: when using checkout, you want to be able to pass the passport instance to checkout. Obviously you can still get it by calling usePassport but it makes it more tricky as you would want to initialise checkout and passport together

haydenfowler · 2024-08-13T01:18:04Z

packages/passport/sdk/src/react/PassportContext.tsx

+    login: async (withoutWallet: boolean = false) => {
+      try {
+        setError(null);
+        setIsLoading(true);
+        if (withoutWallet) {
+          const p = await passportInstance.login();
+          setProfile(p);
+          setLoggedIn(true);
+        } else {
+          const provider = getPassportProvider();
+          const acc = await provider.request({ method: 'eth_requestAccounts' });
+          setLoggedIn(true);
+          setAccounts(acc);
+        }
+      } catch (e) {
+        setError(e as Error);
+      } finally {
+        setIsLoading(false);
+      }
+    },


The purpose of the login method is to authenticate the user without initialising their wallet, and login is intended to be rollup agnostic so we shouldn't be doing anything zkEvm specific in here (calling getPassportProvider() or provider.request().

We'll probably want to expose the useCachedSession argument for the login method here as well, as consumers typically call login({ useCachedSession: true }); on page load to determine if the user has an existing session

I'm only supporting zkevm here. I'll change the component name to reflect on that.
In that case, I think it makes more sense to have a login for both auth + wallet, which is our preferred use case? Then treating auth without wallet as edge case.

imx-mikhala · 2024-08-13T01:28:03Z

packages/passport/sdk/src/react/PassportContext.tsx

+export function useProfile() {
+  const { profile, isLoading, error } = usePassport();
+  return { profile, isLoading, error };
+}


These three hooks seem redundant if they can just get the values from usePassport

This is mostly for convenience.

haydenfowler · 2024-08-13T01:28:46Z

packages/passport/sdk/src/react/PassportContext.tsx

+    loginCallback: async () => {
+      await passportInstance.loginCallback();
+    },
+    linkExternalWallet: passportInstance.linkExternalWallet.bind(passportInstance),


Is the bind call here necessary? It's not clear to me why we can't just call linkExternalWallet directly:

Suggested change

linkExternalWallet: passportInstance.linkExternalWallet.bind(passportInstance),

linkExternalWallet: passportInstance.linkExternalWallet,

linkExternalWallet uses this in its function body tho. Without the bind, it'll work?

This reverts commit 4383ba8.

zaidarain1 · 2024-08-20T00:52:28Z

packages/react/passport/package.json

Regarding the package.json:

Could the "d" command be updated to be a rollup config build without the NODE_ENV=production, and have the rollup.config.js to conditionally use typescript or swc when rebuilding to match the change that was merged in yesterday?

The unplugin-swc dependency will then need to be added to this package

The exports object needs to be set up for the conditional types for live types locally, matching the other package.jsons that were updated

I'll drop the tsconfig change needed here; the customConditions value also needs to be added to support the live types.

You can use the passport sdk package.json/rollup.config/tsconfig.json as a point of reference

haydenfowler · 2024-08-21T23:43:28Z

packages/react/passport/src/PassportContext.tsx

+  passportInstance: Passport;
+  isLoggedIn: boolean;
+  isLoading: boolean;
+  withWallet: boolean;


What's the use case for withWallet? If the consumer specifies whether they want the wallet initialised or not, then the consumer would already know if the wallet has been initialised

haydenfowler · 2024-08-21T23:47:18Z

packages/react/passport/src/PassportContext.tsx

+export function ZkEvmReactProvider({ children, config }: ZkEvmReactProviderProps) {
+  setPassportClientId(config.clientId);
+  track('passport', 'ZkEvmProviderInitialised');
+  const [isLoggedIn, setLoggedIn] = useState(false);


Can we update the setter to setIsLoggedIn please?

haydenfowler · 2024-08-21T23:54:26Z

packages/react/passport/src/PassportContext.tsx

+  const checkLogggedIn = useCallback(async () => {
+    const p = await passportInstance.login({
+      useCachedSession: true,
+    });
+    setLoggedIn(!!p);
+  }, [passportInstance]);


This has the potential to cause re-rendering issues for consumers. My assumption is that we expect marketplaces to consume the provider like this:

function MyComponent() { const { isLoading, isLoggedIn } = reactPassport.usePassport(); if (isLoading) { return <LoadingAnimation />; } if (isLoggedIn) { return <SomeComponentForAuthenticatedUsers />; } return <PromptUserToLogin />; }

In the above scenario, if login triggers a token refresh, PromptUserToLogin would render momentarily. Once the token refresh finishes and login returns true, SomeComponentForAuthenticatedUsers would then render.

haydenfowler · 2024-08-21T23:55:44Z

packages/react/passport/src/PassportContext.tsx

+    getAccounts: async () => {
+      setIsLoading(true);
+      try {
+        const provider = passportInstance.connectEvm();
+        const accounts = await provider.request({ method: 'eth_requestAccounts' });
+        return accounts;
+      } catch (e: any) {
+        setError(e as Error);
+      } finally {
+        setIsLoading(false);
+      }
+      return [];
+    },


eth_requestAccounts initialises the wallet, so if we're going to keep the withWallet property, then this should also call setWithWallet

haydenfowler · 2024-08-21T23:57:13Z

packages/react/passport/src/PassportContext.tsx

+  withWallet: boolean;
+  error: Error | null;
+  getIdToken: Passport['getIdToken'];
+  getAccessToken: Passport['getAccessToken'];
+  getAccounts: () => Promise<string[]>;


Can I suggest that we expose the connectEvm method instead of getAccounts like this? This interface would make it difficult to support something like app-chains in the future

add passport react context and hooks

bc72f13

shineli1984 added 17 commits August 5, 2024 19:29

fix typing

736aabf

Merge branch 'main' into add-passport-react-context-and-hooks

d72ecd3

exclude react in types

8245bf6

change hooks

0b7fc6d

use context

df56bbf

change hook interfaces

a9b60d0

add code snippets in example

a418e26

add typeAssertions back

2d97c60

lint example

ea500f3

lint

84cfbc1

Merge branch 'main' into add-passport-react-context-and-hooks

6442f79

remove doc mk for now

7239164

add error to context

81010a6

example lint

57e3c50

return provider when login with web3 provider

de02f09

add accounts context

5cc9dc6

export useAccount

2e8cf9b

keithbro-imx reviewed Aug 8, 2024

View reviewed changes

Jon-Alonso reviewed Aug 8, 2024

View reviewed changes

shineli1984 added 4 commits August 8, 2024 18:23

address comments

f1274ba

merge main

bcd17fb

add to the consolidated example

b5f63bc

yarn lock

c42f2dd

keithbro-imx reviewed Aug 9, 2024

View reviewed changes

shineli1984 added 2 commits August 9, 2024 11:50

rename PassportProvider

49b8f9a

change provider name

08634f2

fix example

aaefc91

haydenfowler reviewed Aug 13, 2024

View reviewed changes

imx-mikhala reviewed Aug 13, 2024

View reviewed changes

haydenfowler reviewed Aug 13, 2024

View reviewed changes

shineli1984 added 21 commits August 13, 2024 13:20

add tests

aa47ad0

yarn

8ae1029

add missing types

4bf86e1

improve interface

2fc813e

yarn

7a3f0bf

fix example

b7d419b

separate to different context

dbac851

yarn

efd65ed

yarn lock

cc0450f

change interface

0525454

yarn lock

973c147

fix snippet

c779ca9

fix snippets

b9b94e1

fix import

36f7c96

move tokens out of hooks

02cbcea

use events from passport

4383ba8

Revert "use events from passport"

d2e9e3e

This reverts commit 4383ba8.

revert the event emitter

74bb0f5

resolve conflicts

8a55532

add mapper

c708c61

revert back to using passport methods

c984dfb

shineli1984 marked this pull request as ready for review August 20, 2024 00:26

shineli1984 requested review from a team as code owners August 20, 2024 00:26

zaidarain1 requested changes Aug 20, 2024

View reviewed changes

haydenfowler reviewed Aug 21, 2024

View reviewed changes

	linkExternalWallet: passportInstance.linkExternalWallet.bind(passportInstance),
	linkExternalWallet: passportInstance.linkExternalWallet,

:feat add passport react context and hooks #2068

Are you sure you want to change the base?

:feat add passport react context and hooks #2068

Conversation

shineli1984 commented Aug 5, 2024 • edited Loading

Summary

Detail and impact of the change

Added

nx-cloud bot commented Aug 5, 2024 • edited Loading

☁️ Nx Cloud Report

keithbro-imx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imx-mikhala Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shineli1984 commented Aug 5, 2024 •

edited

Loading

nx-cloud bot commented Aug 5, 2024 •

edited

Loading

imx-mikhala Aug 13, 2024 •

edited

Loading