fix: upgrades `@walletconnect/ethereum-provider` and `@walletconnect/modal` #868

JFrankfurt · 2023-07-28T16:45:40Z

These packages updates should remove polyfills, fix crypto.decode errors, allow empty required chains, and improve deep link ux.

Unfortunately, @walletconnect/ethereum-provider has some issues with exported types here. I've added a couple types and a util to guard for one of them. ArrayOneOrMore<number> must be enforced on either chains or optionalChains prior to the init call. This leads to a couple slightly awkward looking lines of code.

export type ArrayOneOrMore<T> = {
  0: T
} & Array<T>

/**
 * This is a type guard for ArrayOneOrMore
 */
export function isArrayOneOrMore<T>(input: T[] = []): input is ArrayOneOrMore<T> {
  return input.length > 0
}

https://uniswapteam.slack.com/archives/C03R5G8T8BH/p1690896081926629?thread_ts=1690481148.038409&cid=C03R5G8T8BH

… empty required chains, improve deep link ux

vercel · 2023-07-28T16:45:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
web3-react	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 4, 2023 5:27pm

JFrankfurt · 2023-08-01T21:29:21Z

example/components/ConnectWithSelect.tsx

+      <option hidden disabled>
        Select chain
      </option>
-      <option value={-1} selected={activeChainId === -1}>
-        Default
-      </option>
+      <option value={-1}>Default</option>
      {chainIds.map((chainId) => (
-        <option key={chainId} value={chainId} selected={chainId === activeChainId}>
+        <option key={chainId} value={chainId}>
          {CHAINS[chainId]?.name ?? chainId}
        </option>


React was complaining about this invalid syntax in the example, but it's not related to the core diff here.

JFrankfurt · 2023-08-01T21:31:06Z

packages/walletconnect-v2/src/index.spec.ts

@@ -68,7 +72,7 @@ class MockWalletConnectProvider extends MockEIP1193Provider<number> {
   * We mock this method later in the test suite to test behavior when optional chains are not supported.
   */
  public getConnectedChains() {
-    return this.opts.chains.concat(this.opts.optionalChains || [])
+    return (this.opts.chains || []).concat(this.opts.optionalChains || [])


since chains can be undefined now we need a default value

grabbou · 2023-08-02T21:00:27Z

packages/walletconnect-v2/src/index.ts

-    const { rpcMap, rpc, chains, ...rest } = options
-
-    this.options = rest
-    this.chains = chains


I would also recommend putting optionalChains outside so that we can perform necessary reordering/check for presence.

grabbou · 2023-08-02T21:02:48Z

packages/walletconnect-v2/src/index.ts

+    if (!chains || !desiredChainId || !chains.includes(desiredChainId) || chains.length === 0) {
+      return chains
+    }
+    return getChainsWithDefault(chains, desiredChainId)


This seems unnecessary to me. Why not include that if additional logic from lines 126-128 inside getChainsWithDefault? We spent a lot of time recently with @cmcewen thinking about the ways to name this function properly and while we debated reoderChains, we figured getWithDefault makes more sense to emphasise it's literally putting the first one as default.

It's just a question of where we put the new type checks and changes. getChainsWithDefault does not support chains or optionalChains as arguments any longer with the new upstream options type changes from @walletconnect/ethereum-provider. We either need to diff the caller or diff the util. I chose the caller, but I don't have a preference between the two.

Yeah, I understand your mixed feelings there. I think - personally - since the only utility of that getChainsWithDefault was to cope with WC "abstraction" of choosing the preferred chain, I think putting it all there would make sense.

BTW, just one tiny consideration for the future work: we could consider (if we're putting chains and optional chains as a class property) to e.g. make their type "arrayoneormore | undefined" and do not set them if they're either not defined or empty in length. That way, I think typings would become much easier down the road.

grabbou · 2023-08-02T21:06:52Z

packages/walletconnect-v2/src/index.ts

+    chains: number[] | undefined,
+    desiredChainId: number | undefined
+  ): number[] | undefined {
+    if (!chains || !desiredChainId || !chains.includes(desiredChainId) || chains.length === 0) {


@JFrankfurt

This specifically

!chains.includes(desiredChainId)

is what is causing the tests to fail.

You should not handle this case here. If you take a look at getChainsWithDefault code, you will see it handles that case on its own:

web3-react/packages/walletconnect-v2/src/utils.ts

Lines 88 to 94 in 505e164

export function getChainsWithDefault(chains: number[], defaultChainId: number) {

const idx = chains.indexOf(defaultChainId)

if (idx === -1) {

throw new Error(

`Invalid chainId ${defaultChainId}. Make sure default chain is included in "chains" - chains specified in "optionalChains" may not be selected as the default, as they may not be supported by the wallet.`

)

}

letting others review this unless otherwise requested

JFrankfurt · 2023-08-02T23:06:58Z

example/pages/index.tsx

@@ -13,7 +12,6 @@ export default function Home() {
      <div style={{ display: 'flex', flexFlow: 'wrap', fontFamily: 'sans-serif' }}>
        <MetaMaskCard />
        <WalletConnectV2Card />
-        <WalletConnectCard />


removing broken example

packages/walletconnect-v2/src/index.ts

zzmp · 2023-08-04T00:35:24Z

packages/walletconnect-v2/src/index.ts

+    return provider
+      .on('disconnect', this.disconnectListener)
+      .on('chainChanged', this.chainChangedListener)
+      .on('accountsChanged', this.accountsChangedListener)
+      .on('display_uri', this.URIListener)


This is only done once. I'd rather it just be inlined into initializeProvider, but I won't make a stink about it.

Yup, this is somewhat related to my previous comment #868 (comment) and also +1 here. We should definitely keep things inline unless required by this PR.

Co-authored-by: Zach Pomerantz <zzmp@uniswap.org>

zzmp · 2023-08-08T16:30:19Z

packages/walletconnect-v2/src/index.ts

@@ -23,6 +23,19 @@ export type WalletConnectOptions = Omit<Parameters<typeof WalletConnectProvider.
  rpc?: { [chainId: number]: string | string[] }
 }

+/**
+ * Necessary type to interface with @walletconnect/ethereum-provider@2.9.2 which is currently unexported


You should make an upstream issue/PR to have that exported.

Talked to them about it when I first started working on this PR and they said they would take care of exporting it. May be worth checking in again.

addressed input

fix: update wcv2 to remove polyfills, fix crypto.decode errors, allow…

742b05d

… empty required chains, improve deep link ux

JFrankfurt requested review from vm, just-toby and a team July 28, 2023 16:45

JFrankfurt self-assigned this Jul 28, 2023

just-toby previously approved these changes Jul 28, 2023

View reviewed changes

vercel bot had a problem deploying to Preview July 28, 2023 16:46 Failure

update to newer package version

018df9c

vercel bot deployed to Preview August 1, 2023 15:46 View deployment

enforce new rules on optional/required chains parameters

8076f78

vercel bot deployed to Preview August 1, 2023 18:38 View deployment

fix test types

894dfc9

vercel bot deployed to Preview August 1, 2023 19:24 View deployment

fix defaultChainId

4e186f7

vercel bot deployed to Preview August 1, 2023 20:36 View deployment

JFrankfurt commented Aug 1, 2023

View reviewed changes

vercel bot deployed to Preview August 1, 2023 21:29 View deployment

JFrankfurt force-pushed the update-wcv2 branch from a4555ea to beebc6d Compare August 1, 2023 21:30

vercel bot deployed to Preview August 1, 2023 21:30 View deployment

remove unnecessary changes

75f0284

JFrankfurt force-pushed the update-wcv2 branch from beebc6d to 75f0284 Compare August 1, 2023 21:32

JFrankfurt commented Aug 1, 2023

View reviewed changes

vercel bot deployed to Preview August 1, 2023 21:33 View deployment

assign this.provider

e914415

vercel bot deployed to Preview August 2, 2023 15:54 View deployment

fiddle with tests

9f35871

vercel bot deployed to Preview August 2, 2023 17:57 View deployment

add test for optional chains config

2972b90

JFrankfurt requested a review from grabbou August 2, 2023 18:36

grabbou suggested changes Aug 2, 2023

View reviewed changes

grabbou previously requested changes Aug 2, 2023

View reviewed changes

pr review

4fad2fd

JFrankfurt requested a review from grabbou August 2, 2023 22:59

vercel bot deployed to Preview August 2, 2023 22:59 View deployment

just-toby removed their request for review August 2, 2023 23:02

JFrankfurt changed the title ~~fix: update wcv2 to remove polyfills, fix crypto.decode errors, allow…~~ fix: upgrades @walletconnect/ethereum-provider and @walletconnect/modal Aug 2, 2023

JFrankfurt commented Aug 2, 2023

View reviewed changes

vercel bot deployed to Preview August 2, 2023 23:28 View deployment

refactor to validate chainProps on init

9bd1a89

JFrankfurt force-pushed the update-wcv2 branch from 09538db to 9bd1a89 Compare August 2, 2023 23:36

vercel bot deployed to Preview August 2, 2023 23:37 View deployment

JFrankfurt requested a review from just-toby August 3, 2023 14:43

zzmp suggested changes Aug 4, 2023

View reviewed changes

Update packages/walletconnect-v2/src/index.ts

50ce46f

Co-authored-by: Zach Pomerantz <zzmp@uniswap.org>

vercel bot deployed to Preview August 4, 2023 17:16 View deployment

JFrankfurt and others added 3 commits August 4, 2023 12:17

Update packages/walletconnect-v2/src/index.ts

89a5307

Co-authored-by: Zach Pomerantz <zzmp@uniswap.org>

Update packages/walletconnect-v2/src/index.ts

f998eee

Co-authored-by: Zach Pomerantz <zzmp@uniswap.org>

Update packages/walletconnect-v2/src/index.ts

0977b59

Co-authored-by: Zach Pomerantz <zzmp@uniswap.org>

vercel bot had a problem deploying to Preview August 4, 2023 17:19 Failure

pr review

abbd9fc

JFrankfurt requested a review from zzmp August 4, 2023 17:25

Merge branch 'main' into update-wcv2

5bd0f6a

vercel bot deployed to Preview August 4, 2023 17:27 View deployment

zzmp approved these changes Aug 8, 2023

View reviewed changes

just-toby approved these changes Aug 10, 2023

View reviewed changes

JFrankfurt merged commit 4d4180d into main Aug 10, 2023
6 checks passed

JFrankfurt deleted the update-wcv2 branch August 10, 2023 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: upgrades `@walletconnect/ethereum-provider` and `@walletconnect/modal` #868

fix: upgrades `@walletconnect/ethereum-provider` and `@walletconnect/modal` #868

JFrankfurt commented Jul 28, 2023 •

edited

Loading

vercel bot commented Jul 28, 2023 •

edited

Loading

JFrankfurt Aug 1, 2023

JFrankfurt Aug 1, 2023

grabbou Aug 2, 2023

grabbou Aug 2, 2023

JFrankfurt Aug 2, 2023

grabbou Aug 4, 2023

grabbou Aug 4, 2023

grabbou Aug 2, 2023

JFrankfurt Aug 2, 2023

zzmp Aug 4, 2023

grabbou Aug 4, 2023

zzmp Aug 8, 2023

JFrankfurt Aug 8, 2023

	export function getChainsWithDefault(chains: number[], defaultChainId: number) {
	const idx = chains.indexOf(defaultChainId)
	if (idx === -1) {
	throw new Error(
	`Invalid chainId ${defaultChainId}. Make sure default chain is included in "chains" - chains specified in "optionalChains" may not be selected as the default, as they may not be supported by the wallet.`
	)
	}

fix: upgrades @walletconnect/ethereum-provider and @walletconnect/modal #868

fix: upgrades @walletconnect/ethereum-provider and @walletconnect/modal #868

Conversation

JFrankfurt commented Jul 28, 2023 • edited Loading

vercel bot commented Jul 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

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

fix: upgrades `@walletconnect/ethereum-provider` and `@walletconnect/modal` #868

fix: upgrades `@walletconnect/ethereum-provider` and `@walletconnect/modal` #868

JFrankfurt commented Jul 28, 2023 •

edited

Loading

vercel bot commented Jul 28, 2023 •

edited

Loading