allow wallet config #808

MananTank · 2023-03-27T15:45:57Z

New API allows configuring wallet config like this:

<ThirdwebProvider 
	supportedWallets={[
		metamaskWallet(),
		coinbaseWallet(),
		paperWallet({ clientId: 'some-id' })
	]}
/>

changeset-bot · 2023-03-27T15:46:01Z

🦋 Changeset detected

Latest commit: 2f4e1e5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
@thirdweb-dev/react-native	Patch
@thirdweb-dev/react-core	Patch
@thirdweb-dev/wallets	Patch
@thirdweb-dev/react	Patch
@thirdweb-dev/auth	Patch
@thirdweb-dev/unity-js-bridge	Patch
thirdweb	Patch
@thirdweb-dev/react-native-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

MananTank · 2023-03-27T15:48:01Z

/release-pr

codecov · 2023-03-27T15:59:12Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (e2ec70c) 69.60% compared to head (44b6282) 69.60%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #808   +/-   ##
=======================================
  Coverage   69.60%   69.60%           
=======================================
  Files         227      227           
  Lines       10602    10602           
  Branches     1298     1298           
=======================================
  Hits         7380     7380           
  Misses       2515     2515           
  Partials      707      707

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

packages/react-core/src/core/providers/thirdweb-wallet-provider.tsx

iketw · 2023-03-27T17:50:53Z

packages/react-core/src/core/providers/thirdweb-wallet-provider.tsx

-    }
+      dappMetadata: {
+        ...walletOptions.dappMetadata,
+        isDarkMode: theme === "dark",


Does this have to be part of the dappMetadata? I don't think this is used by any of the wallets/connectors we have right? We should be able to remove it also from WalletOptions/DAppMetaData

packages/react-core/src/core/providers/thirdweb-wallet-provider.tsx

packages/react-core/src/core/types/wallet.ts

packages/react-native/src/evm/providers/thirdweb-provider.tsx

packages/react-native/src/evm/wallets/wallets/all.ts

iketw · 2023-03-27T18:10:01Z

packages/wallets/src/evm/wallets/coinbase-wallet.ts

          appName: this.options.dappMetadata.name,
          reloadOnDisconnect: false,
-          darkMode: this.options.theme === "dark",
+          darkMode: this.options.dappMetadata.isDarkMode,


darkMode is specific to CoinbaseWallet in web, what do you think if we remove it from the dappMetadata and pass it as a special option for CoinbaseWallet instead?

I thought Wallet Connect has color schemes, too?

I don't think I've seen it in the connectors

v1 definitely had it I wonder if they got rid of it with v2? but I've seen dark and light mode wc v2 UI in the wild...

MananTank · 2023-03-27T19:28:14Z

/release-pr

MananTank · 2023-03-27T21:11:22Z

/release-pr

MananTank · 2023-03-27T21:18:46Z

/release-pr

… allow-wallet-config

iketw · 2023-03-27T22:12:30Z

./release-pr

iketw · 2023-03-28T04:47:42Z

./release-pr

jnsdls

I think there's more to clean up here to get it to a good point

jnsdls · 2023-03-28T07:31:54Z

packages/react-core/src/core/providers/thirdweb-provider.tsx

   * ```
   */
-  supportedWallets: SupportedWallet[];
+  supportedWallets: Wallet[];


should we rename this to wallets?

We also have a supportedChains prop here, I think it's good to have some consistency, should we rename supportedChains as well?

packages/react-core/src/core/providers/thirdweb-wallet-provider.tsx

jnsdls · 2023-03-28T07:35:25Z

packages/react-native/src/evm/wallets/wallets/coinbase-wallet.ts

 }
+
+export const coinbaseWallet = (config?: {callbackURL?: URL}) => {
+  const callbackURLNonNull = config?.callbackURL || new URL("https://thirdweb.com/wsegue");


what's this fallback URL?

@jnsdls that URL is required by the Coinbase Wallet SDK as a UniversalLink to talk back to iOS apps: https://coinbase.github.io/wallet-mobile-sdk/docs/client-sdk/rn-setup/#configuration.

I'm voting for removing the Coinbase wallet as a default wallet and figure out tree-shaking. Adding UniversalLinks is another friction point to build a new app + breaking Expo Go support. I opened a ticket with them as well: https://github.com/coinbase/wallet-mobile-sdk/issues/213.

jnsdls · 2023-03-28T07:37:30Z

packages/react/src/evm/hooks/wallets/usePaper.ts

+export function usePaperWalletUserEmail() {
+  const wallet = useWallet();
+  const [email, setEmail] = useState<string | undefined>(undefined);
+
+  useEffect(() => {
+    if (wallet?.walletId !== "PaperWallet") {
+      setEmail(undefined);
+      return;
+    }
+    (wallet as PaperWallet).getEmail().then((_email) => {
+      setEmail(_email);
+    });
+  }, [wallet]);
+
+  return email;
+}


I don't think we need to expose this explicitly, I'd also prefer to do this in a useQuery instead of a custom useEffect + state hook if we were to offer this

I've replaced the state+effect with react-query for now.
But let me know if it makes more sense to just remove the hook

I think it feels odd as a "wallet connector" to have an "email" hook?

jnsdls · 2023-03-28T07:38:07Z

packages/react/src/index.ts

-export {
-  MetamaskWallet,
-  CoinbaseWallet,
-  DeviceWallet,
-  WalletConnect,
-  WalletConnectV1,
-  PaperWallet,
-  SafeWallet,
-} from "./wallet/wallets";


jnsdls · 2023-03-28T07:45:42Z

packages/react/src/wallet/wallets/metamaskWallet.ts

+    id: MetaMask.id,
+    meta: MetaMask.meta,


let's invert this, the object should drive the ID + meta and the wallet can inherit it / be passed it during construction time (so we do not need to import the full wallet class just to have the metadata object)

jnsdls · 2023-03-28T07:46:18Z

packages/react/src/wallet/wallets/walletConnect.ts

@@ -0,0 +1,13 @@
+import { TW_WC_PROJECT_ID } from "@thirdweb-dev/react-core";


it feels weird that this is driven by react-core and not by wallets, why would this not live in the wallet package?

yeah, this shouldn't live in react-core. RN is using one in the react-native package. We could set a default one in the wallets package as well but passing one down from RN/React I think makes sense, it's another tracking indicator we could use.

Ok, moved into React, will also create it in the wallets package when we polish the wallets package (this week)

this should live in wallets not react or react-core

jnsdls · 2023-03-28T07:47:20Z

packages/react/src/wallet/wallets/walletConnect.ts

+export const walletConnect = (config?: { projectId?: string }) => {
+  const projectId = config?.projectId || TW_WC_PROJECT_ID;
+  return {
+    id: WalletConnect.id,
+    meta: WalletConnect.meta,
+    create: (options: WalletOptions) =>
+      new WalletConnect({ ...options, qrcode: true, projectId }),
+  } satisfies Wallet;
+};


I just realized that these functions live in react - why would they the not live in wallets?

Because it's only required for react,react-core - if anyone wants to use the wallets directly - they don't need this

so if it's required for react-core then at the very least it should live in react-core

packages/wallets/src/evm/connectors/paper/index.ts

jnsdls · 2023-03-28T07:49:33Z

packages/wallets/src/evm/wallets/coinbase-wallet.ts

          appName: this.options.dappMetadata.name,
          reloadOnDisconnect: false,
-          darkMode: this.options.theme === "dark",
+          darkMode: this.options.dappMetadata.isDarkMode,


v1 definitely had it I wonder if they got rid of it with v2? but I've seen dark and light mode wc v2 UI in the wild...

MananTank · 2023-03-28T14:28:11Z

Wallet connect v2 allows to set theme using qrModalOptions.themeMode
https://docs.walletconnect.com/2.0/javascript/providers/ethereum#supported-web3modal-options-qrmodaloptions

but looks like it's not available in the version we are using of @walletconnect/ethereum-provider ( ~2.4.6 ). latest version is 2.5.2 - but upgrading to that breaks a bunch of things.

I can't find any mention of configuring the theme in wallet connect v1 docs

Should we remove dAppMeta.isDarkMode from wallets and thirdweb-provider and only pass the theme instead to wallets?

@jnsdls @iketw

iketw · 2023-03-28T14:30:23Z

Wallet connect v2 allows to set theme using qrModalOptions.themeMode https://docs.walletconnect.com/2.0/javascript/providers/ethereum#supported-web3modal-options-qrmodaloptions

but looks like it's not available in the version we are using of @walletconnect/ethereum-provider ( ~2.4.6 ). latest version is 2.5.2 - but upgrading to that breaks a bunch of things.

I can't find any mention of configuring the theme in wallet connect v1 docs

Should we remove dAppMeta.isDarkMode from wallets and thirdweb-provider and only pass the theme instead to wallets?

@jnsdls @iketw

I think that makes sense, that way we'd pass it just to the wallets that support it.

MananTank · 2023-03-28T14:54:10Z

/release-pr

MananTank · 2023-03-28T18:21:43Z

/release-pr

- [RN] only exports wallet creator functions - [RN] remove coinbase wallet as a default wallet - [React] only export wallet creator functions - [React] Move WC projectId into react and out of react-core

iketw · 2023-03-29T00:16:35Z

@jnsdls we worked on some of the comments you made and we answered others. I created a ticket to track the ones we didn't cover here (SDK-870).

I'm merging this now since we need these changes to write the RN SDK guides + we can't keep letting this PR grow

iketw · 2023-03-29T00:16:42Z

./release-pr

allow wallet config, fix gnosis safe

24ebb79

MananTank requested a review from a team as a code owner March 27, 2023 15:45

MananTank requested a review from a team March 27, 2023 15:45

MananTank added 2 commits March 27, 2023 23:22

remove console log

d343958

Fix icon size issue with reset css

20a4bf9

iketw reviewed Mar 27, 2023

View reviewed changes

MananTank added 2 commits March 28, 2023 00:41

export wallet classes

89a0743

allow getting email from paper wallet

6d5138a

MananTank added 4 commits March 28, 2023 01:11

remove unnecessary try-catch

1f203c7

remove unnecessary comments

45a84be

Merge branch 'main' into allow-wallet-config

9fe0630

Merge branch 'main' into allow-wallet-config

555d23f

add changeset

10bd90b

MananTank changed the title ~~allow wallet config, fix gnosis safe~~ allow wallet config, Mar 27, 2023

ikethirdweb and others added 5 commits March 27, 2023 17:37

merge

8271dd3

merge

92d26a9

Merge branch 'main' into allow-wallet-config

c76fd4f

Merge branch 'main' into allow-wallet-config

4653b96

Merge branch 'allow-wallet-config' of github.com:thirdweb-dev/js into…

bd07a54

… allow-wallet-config

ikethirdweb added 3 commits March 27, 2023 19:18

react-native fixes

646dc4a

Merge branch 'main' into allow-wallet-config

2c05798

descriptive console.error

89629b4

ikethirdweb added 2 commits March 27, 2023 23:59

fix bug in wcv2, optionalChains are not optional

b245c50

[RN] coinbase-wallet allow passing callbackURL

b37c673

jnsdls reviewed Mar 28, 2023

View reviewed changes

MananTank added 2 commits March 28, 2023 16:13

Remove duplicate prop

74f8928

use react-query in usePaperWalletUserEmail()

063cd33

MananTank added 3 commits March 28, 2023 20:08

Merge branch 'main' into allow-wallet-config

337e678

Update Paper SDK version

2f9750e

wallets: fix build error after updating paper sdk version

66bd21c

auth: export PrivateKeyWallet

10366f5

MananTank changed the title ~~allow wallet config,~~ allow wallet config Mar 28, 2023

ikethirdweb added 3 commits March 28, 2023 20:11

- [RN] add listener for coinbase wallet

53d6247

- [RN] only exports wallet creator functions - [RN] remove coinbase wallet as a default wallet - [React] only export wallet creator functions - [React] Move WC projectId into react and out of react-core

changeset

2f4e1e5

Merge branch 'main' into allow-wallet-config

44b6282

iketw added this pull request to the merge queue Mar 29, 2023

Merged via the queue into main with commit e453e36 Mar 29, 2023

iketw deleted the allow-wallet-config branch March 29, 2023 00:32

		@@ -0,0 +1,13 @@
		import { TW_WC_PROJECT_ID } from "@thirdweb-dev/react-core";

allow wallet config #808

allow wallet config #808

Uh oh!

Conversation

MananTank commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

MananTank commented Mar 27, 2023

Uh oh!

codecov bot commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MananTank commented Mar 27, 2023

Uh oh!

MananTank commented Mar 27, 2023

Uh oh!

MananTank commented Mar 27, 2023

Uh oh!

iketw commented Mar 27, 2023

Uh oh!

iketw commented Mar 28, 2023

Uh oh!

jnsdls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iketw Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MananTank commented Mar 27, 2023 •

edited

Loading

changeset-bot bot commented Mar 27, 2023 •

edited

Loading

codecov bot commented Mar 27, 2023 •

edited

Loading

iketw Mar 28, 2023 •

edited

Loading