feat: walletprovider adapters type narrowing #5468

gomesalexandre · 2023-10-16T10:55:44Z

Description

Ensures the types of adapters and related structures/methods across the <WalletProvider /> domain are properly narrowed down, vs. any currently.

Consequently adds additional type safety to accommodate for the added typings.

Pull Request Type

🐛 Bug fix (Non-breaking Change: Fixes an issue)
🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

tackles Identify and implement lazy imports #5445

Risk

The same as the PRs below - while this mostly tackles things at type-level, there are slight runtime changes, mostly moving pieces arounds, which does present a regression risk

Testing

Same as feat: actually lazy load hdwallet adapter modules #5466

Engineering

☝🏽

Operations

☝🏽

Screenshots (if applicable)

gomesalexandre · 2023-10-16T10:55:59Z

Current dependencies on/for this PR:

develop
- PR fix: don't initialize wallet in state.keyring effect #5462
  - PR feat: lazy load hdwallet adapter modules part 1 #5463
    - PR feat: actually lazy load hdwallet adapter modules #5466
      - PR feat: walletprovider adapters type narrowing #5468 👈
        
        PR perf: defi only render current step if active #5476

This comment was auto-generated by Graphite.

src/context/WalletProvider/WalletProvider.tsx

gomesalexandre · 2023-10-16T10:59:19Z

src/context/WalletProvider/config.ts

+import type { KkRestAdapter } from '@keepkey/hdwallet-keepkey-rest'
+import type { CoinbaseAdapter } from '@shapeshiftoss/hdwallet-coinbase'
+import type { WebUSBKeepKeyAdapter } from '@shapeshiftoss/hdwallet-keepkey-webusb'
+import type { KeplrAdapter } from '@shapeshiftoss/hdwallet-keplr'
+import type { WebUSBLedgerAdapter as LedgerAdapter } from '@shapeshiftoss/hdwallet-ledger-webusb'
+import type { MetaMaskAdapter } from '@shapeshiftoss/hdwallet-metamask'
+import type { NativeAdapter } from '@shapeshiftoss/hdwallet-native'
+import type { MetaMaskAdapter as MetaMaskMultiChainAdapter } from '@shapeshiftoss/hdwallet-shapeshift-multichain'
+import type { WalletConnectV2Adapter } from '@shapeshiftoss/hdwallet-walletconnectv2'
+import type { XDEFIAdapter } from '@shapeshiftoss/hdwallet-xdefi'


Note how all of these are imported as type, which shouldn't bring any import at runtime, ensuring we don't void our lazy guarantees

gomesalexandre · 2023-10-16T10:59:39Z

src/context/WalletProvider/config.ts

+  [KeyManager.Native]: SupportedWalletInfo<typeof NativeAdapter>
+  [KeyManager.Mobile]: SupportedWalletInfo<typeof NativeAdapter>
+  [KeyManager.Demo]: SupportedWalletInfo<typeof NativeAdapter>
+  // TODO(gomes): export WebUSBKeepKeyAdapter as a type in hdwallet, not a declare const


Most likely the root cause of all the ts-ignore across this PR

Keeping it as-is for now, the types refactor required to make this work would be a lot more complex, rage quitted after ~2 hours

gomesalexandre · 2023-10-16T13:50:01Z

src/context/WalletProvider/KeepKey/components/Connect.tsx

@@ -65,6 +67,7 @@ export const KeepKeyConnect = () => {
          return wallet
        } catch (e) {
          const secondAdapter = await getAdapter(KeyManager.KeepKey, 1)
+          // @ts-ignore TODO(gomes): FIXME, most likely borked because of WebUSBKeepKeyAdapter


WebUSBKeepKeyAdapter types are too borked, fiddled for two hours in hdwallet with no success here, leaving this as-is for now - there is no effective runtime change here

Keeping it as-is for now, the types refactor required to make this work would be a lot more complex, rage quitted after ~2 hours

gomesalexandre · 2023-10-16T13:50:20Z

src/context/WalletProvider/WalletProvider.tsx

          try {
-            currentAdapters.set(localWalletType, [adapter])
+            // @ts-ignore TODO(gomes): FIXME, most likely borked because of WebUSBKeepKeyAdapter


WebUSBKeepKeyAdapter types are too borked, fiddled for two hours in hdwallet with no success here, leaving this as-is for now - there is no effective runtime change here

Actually a different issue - things are not narrowed down enough at this stage and need to declare the adapter in each switch case before assigning it - 875f795

gomesalexandre · 2023-10-16T13:50:33Z

src/context/WalletProvider/WalletProvider.tsx

                  const sdk = await setupKeepKeySDK()
-                  return await keepKeyAdapters[0]?.pairDevice(sdk)
+                  // @ts-ignore TODO(gomes): FIXME, most likely borked because of WebUSBKeepKeyAdapter


WebUSBKeepKeyAdapter types are too borked, fiddled for two hours in hdwallet with no success here, leaving this as-is for now - there is no effective runtime change here

gomesalexandre · 2023-10-16T14:00:55Z

src/context/WalletProvider/MetaMask/config.ts

  adapters: [
    {
      loadAdapter: () =>
+        // @ts-ignore TODO(gomes): FIXME


Not happy with this, but probably our best bet a0622d8

gomesalexandre · 2023-10-16T17:20:58Z

src/context/WalletProvider/WalletProvider.tsx

@@ -900,7 +900,7 @@ export const WalletProvider = ({ children }: { children: React.ReactNode }): JSX
    })
  }, [])

-  useEffect(() => load(), [load, state.adapters, state.keyring])
+  useEffect(() => load(), [load, state.keyring])


We don't want to run this reactively on state.adapters anymore after this stack/PR refactor:

This stack's refactor makes it so that we getAdapter() so we don't need to be reactive on state.adapters anymore

This PR's refactor moved some initialization logic in each switch case instead of the previous outer scope, meaning we would have infinite dispatch and reactivity after the dispatch of WalletActions.SET_ADAPTERS in load()

src/context/WalletProvider/types.ts

woodenfurniture · 2023-10-16T19:37:40Z

src/context/WalletProvider/MetaMask/config.ts

  adapters: [
    {
      loadAdapter: () =>
        getConfig().REACT_APP_EXPERIMENTAL_MM_SNAPPY_FINGERS
-          ? import('@shapeshiftoss/hdwallet-shapeshift-multichain').then(m => m.MetaMaskAdapter)
+          ? import('@shapeshiftoss/hdwallet-shapeshift-multichain').then(
+              m => m.MetaMaskAdapter as typeof MetaMaskAdapter,


is this cast still required?

Still is unfortunately:

Type 'Promise<MetaMaskShapeShiftMultiChainHDWallet | undefined>' is not assignable to type 'Promise<MetaMaskHDWallet | undefined>'. Type 'MetaMaskShapeShiftMultiChainHDWallet | undefined' is not assignable to type 'MetaMaskHDWallet | undefined'. Type 'MetaMaskShapeShiftMultiChainHDWallet' is missing the following properties from type 'MetaMaskHDWallet': _supportsSecretInfo, _supportsSecret, _supportsKava, _supportsKavaInfo, and 2 more. [2322]

This is the type error we're seeing, which is why we're lying to TS that this is always a MetaMaskAdapter

src/context/WalletProvider/WalletProvider.tsx

src/context/WalletProvider/config.ts

src/context/WalletProvider/types.ts

0xApotheosis

Tested wallet connections to Native, MetaMask and WalletConnect as a sanity check, all seemed gucci.

Nice one.

src/context/WalletProvider/WalletProvider.tsx

This was referenced Oct 16, 2023

fix: don't initialize wallet in state.keyring effect #5462

Merged

feat: lazy load hdwallet adapter modules part 1 #5463

Merged

feat: actually lazy load hdwallet adapter modules #5466

Merged

gomesalexandre changed the title ~~wip: walletprovider type narrowing~~ wip: walletprovider adapters type narrowing Oct 16, 2023

gomesalexandre commented Oct 16, 2023

View reviewed changes

src/context/WalletProvider/WalletProvider.tsx Outdated Show resolved Hide resolved

gomesalexandre commented Oct 16, 2023

View reviewed changes

gomesalexandre changed the title ~~wip: walletprovider adapters type narrowing~~ feat: walletprovider adapters type narrowing Oct 16, 2023

gomesalexandre commented Oct 16, 2023

View reviewed changes

gomesalexandre marked this pull request as ready for review October 16, 2023 17:17

gomesalexandre requested a review from a team as a code owner October 16, 2023 17:17

gomesalexandre commented Oct 16, 2023

View reviewed changes

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/types.ts Outdated Show resolved Hide resolved

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/types.ts Outdated Show resolved Hide resolved

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/types.ts Outdated Show resolved Hide resolved

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/WalletProvider.tsx Show resolved Hide resolved

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/WalletProvider.tsx Outdated Show resolved Hide resolved

gomesalexandre commented Oct 16, 2023

View reviewed changes

src/context/WalletProvider/config.ts Outdated Show resolved Hide resolved

woodenfurniture reviewed Oct 16, 2023

View reviewed changes

src/context/WalletProvider/types.ts Outdated Show resolved Hide resolved

woodenfurniture force-pushed the feat_perf_lazy_3 branch from 2a6e615 to 7094513 Compare October 17, 2023 00:25

Base automatically changed from feat_perf_lazy_3 to develop October 17, 2023 00:40

gomesalexandre added 4 commits October 17, 2023 10:23

wip: walletprovider type narrowing

ce276fd

feat: cast KKRestAdapter type

c654e32

feat: cast multichain adapter as MetaMaskAdapter

37d33a5

feat: rm ts-ignore

628fc0e

gomesalexandre and others added 4 commits October 17, 2023 10:23

fix: infinite dispatch

79707ca

feat: rm useless ts-ignore

a6f6b91

wip

e57f316

feat: gm, she compiles again

764ea6b

gomesalexandre force-pushed the feat_perf_lazy_4 branch from bfdb922 to 764ea6b Compare October 17, 2023 08:25

gomesalexandre mentioned this pull request Oct 17, 2023

perf: defi only render current step if active #5476

Merged

3 tasks

0xApotheosis self-assigned this Oct 17, 2023

0xApotheosis approved these changes Oct 17, 2023

View reviewed changes

src/context/WalletProvider/WalletProvider.tsx Outdated Show resolved Hide resolved

gomesalexandre added 3 commits October 18, 2023 09:24

Merge remote-tracking branch 'origin/develop' into feat_perf_lazy_4

65eba4c

feat: cleanup

5e3b768

Merge branch 'develop' into feat_perf_lazy_4

4c1bc93

gomesalexandre enabled auto-merge (squash) October 18, 2023 19:01

Merge branch 'develop' into feat_perf_lazy_4

cb0b180

0xApotheosis disabled auto-merge October 18, 2023 22:43

0xApotheosis merged commit c790afb into develop Oct 18, 2023
3 checks passed

0xApotheosis deleted the feat_perf_lazy_4 branch October 18, 2023 22:43

0xApotheosis mentioned this pull request Oct 18, 2023

chore: release v1.460.0 #5495

Merged

gomesalexandre restored the feat_perf_lazy_4 branch October 19, 2023 11:53

0xApotheosis mentioned this pull request Oct 19, 2023

chore: release v1.461.0 #5502

Merged

0xean deleted the feat_perf_lazy_4 branch December 6, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: walletprovider adapters type narrowing #5468

feat: walletprovider adapters type narrowing #5468

gomesalexandre commented Oct 16, 2023 •

edited

Loading

gomesalexandre commented Oct 16, 2023 •

edited

Loading

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

gomesalexandre Oct 16, 2023

woodenfurniture Oct 16, 2023

gomesalexandre Oct 16, 2023

0xApotheosis left a comment

feat: walletprovider adapters type narrowing #5468

feat: walletprovider adapters type narrowing #5468

Conversation

gomesalexandre commented Oct 16, 2023 • edited Loading

Description

Pull Request Type

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

gomesalexandre commented Oct 16, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xApotheosis left a comment

Choose a reason for hiding this comment

gomesalexandre commented Oct 16, 2023 •

edited

Loading

gomesalexandre commented Oct 16, 2023 •

edited

Loading