Seed Vault React Native Lib #140

Funkatronics · 2023-06-13T21:23:32Z

Initial react native library for seed vault (#141)

Similar to how we handled the react native wallet lib, lets review this PR @Michaelsulistio, make any small changes here, and create project tasks for larger refactors/features.

Funkatronics · 2023-06-13T21:26:18Z

Note @Michaelsulistio, this PR does not include an example app. I do have a react app that I used to exercise the lib and test it, but its pretty hacked together/scripty. We can iterate on that to create a proper example app.

Michaelsulistio · 2023-06-14T17:26:21Z

js/packages/seed-vault/src/useSeedVault.ts

@@ -0,0 +1,103 @@
+import { useEffect } from 'react';


nitpick: The file name useSeedVault implies there is some custom hook function called useSeedVault. Maybe we can rename the exported function to useSeedVault? Or just rename the file itself?

The export must be renamed use* for sure, since it contains a useEffect itself.

...droid/src/main/java/com/solanamobile/seedvault/reactnative/SolanaMobileSeedVaultLibModule.kt

steveluscher · 2023-06-20T17:39:58Z

...droid/src/main/java/com/solanamobile/seedvault/reactnative/SolanaMobileSeedVaultLibModule.kt

+                try {
+                    val authToken = Wallet.onAuthorizeSeedResult(resultCode, data)
+                    Log.d(TAG, "Seed authorized, AuthToken=$authToken")
+                    sendSeedVaultEvent(SeedVaultEvent.SeedAuthorized(authToken))


Is there anything that prevents this from engendering a race condition – ie. these events being fired before the app starts to listen for them?

Some things to consider:

fataling if there are no listeners

accumulating events and playing them all into the next listener to attach

Yes good call out. Mike and I have been discussing this a lot.

With this current design, there is nothing stopping the consumer from calling a method like authorizeNewSeed before they add a listener. In this case the caller would get nothing back, not even an error. We should at the very least throw some errors if there are no listeners.

I think we can do even better tho. I think exposing an async-await/promise based API would be much more straightforward for js consumers, and hide the listener under the hood.

Given that this library is not yet released on npm, I think we can handle this feedback in a separate PR. I'd like to experiment with wrapping up the native API into an async-await on the js side.

steveluscher · 2023-06-20T17:41:26Z

js/packages/seed-vault/src/useSeedVault.ts

@@ -0,0 +1,103 @@
+import { useEffect } from 'react';


The export must be renamed use* for sure, since it contains a useEffect itself.

steveluscher · 2023-06-20T17:42:58Z

js/packages/seed-vault/src/useSeedVault.ts

+    useEffect(() => {
+        const seedVaultEventEmitter = new NativeEventEmitter();
+        const listener = seedVaultEventEmitter.addListener(SEED_VAULT_EVENT_BRIDGE_NAME, (nativeEvent) => {
+            // console.log('Received seed vault event: ' + JSON.stringify(nativeEvent, null, 4));


js/packages/seed-vault/src/useSeedVault.ts

steveluscher · 2023-06-20T17:45:15Z

js/.eslintrc

+    "extends": ["eslint:recommended", "plugin:@typescript-eslint/recommended", "plugin:require-extensions/recommended"],
+    "rules": {
+        "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }],
+        "simple-import-sort/imports": "error"


Must add rules-of-hooks here, which would have caught that dependency related bug in useSeedVault.ts.

https://www.npmjs.com/package/eslint-plugin-react-hooks

steveluscher · 2023-06-20T17:45:57Z

js/rollup.config.ts

+}: {
+    bundleName: string;
+    format: 'cjs' | 'esm';
+    runtime: 'browser' | 'node' | 'react-native';


Kill the node and browser builds, right?

steveluscher · 2023-06-20T17:46:40Z

js/packages/seed-vault/tsconfig.json

+    "include": ["src"],
+    "compilerOptions": {
+        "declarationDir": "./lib/types",
+        "outDir": "lib/esm"


I don't think you need CommonJS and ESM builds since this is exclusively for React Native. Just use ESM.

steveluscher · 2023-06-20T17:47:34Z

js/rollup.config.ts

+                            const { id: resolvedId } = resolved;
+                            const directory = path.dirname(resolvedId);
+                            const moduleFilename = path.basename(resolvedId);
+                            const forkPath = path.join(directory, '__forks__', runtime, moduleFilename);


None of this is relevant. You won't have forks; this library will only ever be for React Native.

tbh, I have a very limited understanding of how any of this rollup/ts config stuff works. I copied fiels from the MWA repo until things started building successfully lol. I'll backtrack through and clean all this up, but might do that in a separate PR.

@Michaelsulistio should I refactor this lib in the same way you did the walletlib here?

Yup you can pretty much copy what I did there, although I used commonJS as that was the default in the bob builder config I copied over.

Looking at it now, we should probably refactor walletlib to use ESM.

steveluscher · 2023-06-20T17:48:19Z

js/packages/seed-vault/package.json

+    "browser": {
+        "./lib/cjs/index.js": "./lib/cjs/index.browser.js",
+        "./lib/esm/index.js": "./lib/esm/index.browser.js"
+    },


Kill all the browser and node exports. Make this package.json only work with React Native.

steveluscher · 2023-07-06T15:56:40Z

js/packages/seed-vault/src/useSeedVault.ts

@@ -78,7 +76,7 @@ export function subscribeToNativeEvents(
        return () => {
            listener.remove();
        };
-    }, []);
+    }, [handleContentChange, handleContentChange]);


Suggested change

}, [handleContentChange, handleContentChange]);

}, [handleContentChange, handleSeedVaultEvent]);

steveluscher · 2023-07-06T16:04:05Z

js/packages/seed-vault/src/useSeedVault.ts

    handleSeedVaultEvent: (event: SeedVaultEvent) => void,
    handleContentChange: (event: SeedVaultContentChange) => void,


So, there's a way of designing this hook so that the useEffect doesn't constantly fire. You can bet your bottom dollar that people are going to do this:

useSeedVault( e => // handle seed vault event, e => // handle content change, );

Every single time that code runs, the non-memoized functions will be different which means that the useEffect's dependencies will be busted which means that the remove() will fire and the NativeEventEmitter will be set up all over again – on every render.

So here's the thing. Since these functions will only ever get called between renders (ie. when the native event emitter fires) you can:

Store the functions in a ref – refs do not have to be part of the useEffect's dependencies

Update that ref on every render

Reach into the ref when you need the function to call

That way, the useEffect will only ever run once to set up the listener, and once when the whole thing unmounts.

https://legacy.reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

Keyword: ‘Keep latest props in a ref’

outdated, reviving this pr after months

Seed Vault react native lib, pull to new clean branch

fabd508

Funkatronics requested review from steveluscher and Michaelsulistio June 13, 2023 21:23

Michaelsulistio approved these changes Jun 14, 2023

View reviewed changes

steveluscher previously requested changes Jun 20, 2023

View reviewed changes

Funkatronics added 2 commits June 28, 2023 15:16

pr feeback pass 1: bob build and cleanup

dced1d8

remove rollup config from root, not used

f6a69a1

steveluscher reviewed Jul 6, 2023

View reviewed changes

pr feedback

52d34cc

Funkatronics added 2 commits February 27, 2024 11:20

align versions

9dad65e

adapt ci conditions

3bc2b9b

Funkatronics merged commit 068a89a into main Feb 27, 2024
3 checks passed

Funkatronics deleted the seed-vault-react-native branch February 27, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seed Vault React Native Lib #140

Seed Vault React Native Lib #140

Funkatronics commented Jun 13, 2023 •

edited

Loading

Funkatronics commented Jun 13, 2023

Michaelsulistio Jun 14, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

Funkatronics Jun 29, 2023

Funkatronics Jun 29, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

steveluscher Jun 20, 2023

Funkatronics Jun 28, 2023

Funkatronics Jun 28, 2023

Michaelsulistio Jun 28, 2023 •

edited

Loading

steveluscher Jun 20, 2023

steveluscher Jul 6, 2023

steveluscher Jul 6, 2023 •

edited

Loading

	}, [handleContentChange, handleContentChange]);
	}, [handleContentChange, handleSeedVaultEvent]);

		handleSeedVaultEvent: (event: SeedVaultEvent) => void,
		handleContentChange: (event: SeedVaultContentChange) => void,

Seed Vault React Native Lib #140

Seed Vault React Native Lib #140

Conversation

Funkatronics commented Jun 13, 2023 • edited Loading

Funkatronics commented Jun 13, 2023

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

Michaelsulistio Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveluscher Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Funkatronics commented Jun 13, 2023 •

edited

Loading

Michaelsulistio Jun 28, 2023 •

edited

Loading

steveluscher Jul 6, 2023 •

edited

Loading