fix: upgrades @walletconnect/ethereum-provider and @walletconnect/modal
#868
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import type { Actions, ProviderRpcError } from '@web3-react/types' | |
| import { Connector } from '@web3-react/types' | ||
| import EventEmitter3 from 'eventemitter3' | ||
|
|
||
| import { ArrayOneOrMore, getBestUrlMap, getChainsWithDefault, isArrayOneOrMore } from './utils' | ||
|
|
||
| export const URI_AVAILABLE = 'URI_AVAILABLE' | ||
| const DEFAULT_TIMEOUT = 5000 | ||
|
|
@@ -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 | ||
|
There was a problem hiding this comment. You should make an upstream issue/PR to have that exported. There was a problem hiding this comment. 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. |
||
| */ | ||
| type ChainsProps = | ||
| | { | ||
| chains: ArrayOneOrMore<number> | ||
| optionalChains?: number[] | ||
| } | ||
| | { | ||
| chains?: number[] | ||
| optionalChains: ArrayOneOrMore<number> | ||
| } | ||
|
|
||
| /** | ||
| * Options to configure the WalletConnect connector. | ||
| */ | ||
|
|
@@ -51,22 +64,26 @@ export class WalletConnect extends Connector { | |
| private readonly options: Omit<WalletConnectOptions, 'rpcMap' | 'chains'> | ||
|
|
||
| private readonly rpcMap?: Record<number, string | string[]> | ||
| private readonly chains: number[] | ArrayOneOrMore<number> | undefined | ||
| private readonly optionalChains: number[] | ArrayOneOrMore<number> | undefined | ||
| private readonly defaultChainId?: number | ||
| private readonly timeout: number | ||
|
|
||
| private eagerConnection?: Promise<WalletConnectProvider> | ||
|
|
||
| constructor({ actions, defaultChainId, options, timeout = DEFAULT_TIMEOUT, onError }: WalletConnectConstructorArgs) { | ||
| super(actions, onError) | ||
|
|
||
| const { rpcMap, rpc, ...rest } = options | ||
|
|
||
| this.options = rest | ||
|
There was a problem hiding this comment. IMO this should not be removed. The principle was that all the props that are custom modified / not passed straight to WalletConnect should be kept outside to avoid confusion. There was a problem hiding this comment. I was taking the opposite approach. If this is intended, we should at least include a comment explaining that we're intentionally not trying to be a thin pass-through layer. There was a problem hiding this comment. I think this is exactly what we're trying to do - to be a thin pass-through layer. It's just that internally we have made this decision to differentiate between properties that we transform vs the ones we pass-through to make it easier to maintain. That way, you can e.g. check for arrays during initialisation time and put them on class as a property for further reference. There was a problem hiding this comment. Also, I believe that, once this line of code is back, you should put the if statement here to check for number of chains (and optional chains) to ensure initialisation is valid and throw an error during the runtime. Note that this can't be changed once created, so doing if condition here makes definitely more sense instead of at the lazy init time. There was a problem hiding this comment. I would also recommend putting There was a problem hiding this comment. this makes sense given the approach you outlined above |
||
| this.defaultChainId = defaultChainId | ||
| this.rpcMap = rpcMap || rpc | ||
| this.timeout = timeout | ||
|
|
||
| const { chains, optionalChains } = this.getChainProps(rest.chains, rest.optionalChains, defaultChainId) | ||
| this.chains = chains | ||
| this.optionalChains = optionalChains | ||
| } | ||
|
|
||
| private disconnectListener = (error: ProviderRpcError) => { | ||
|
|
@@ -86,27 +103,51 @@ export class WalletConnect extends Connector { | |
| this.events.emit(URI_AVAILABLE, uri) | ||
| } | ||
|
|
||
| private async initializeProvider( | ||
|
There was a problem hiding this comment. I am wondering if this PR could be broken down into a smaller diff - the one that actually fixes the issues in PR title. I see a lot of changes like this one - which seem to be more of "refactoring" that are not related to the described issues, unless I am mistaken? Would definitely understand the reason behind creating this utility function or reordering private class properties. There was a problem hiding this comment. I am also not aware of recent changes in other files, but one thing to keep in mind is that, in principle, we always tried to keep providers similar as far as top-level methods and private APIs they have. If you would look at other existing providers, you would notice they all have There was a problem hiding this comment. Upgrading the package should (according to the wc team) get us the improvements mentioned in the title/description. All changes here are made to accommodate the new requirements from There was a problem hiding this comment. @grabbou Are you saying here that you'd rather a class import a utility function from another file rather than define its own private utility method? There was a problem hiding this comment.
What are the utility functions that are relevant to the PR apart from the array check? I was referring to the private methods defined on the class, that, to me, look like just extracted from their original places and still being referred only in those places. I would like to get a full picture - why those changes were necessary and how they relate to the PR |
||
| desiredChainId: number | undefined = this.defaultChainId | ||
| ): Promise<WalletConnectProvider> { | ||
| const rpcMap = this.rpcMap ? getBestUrlMap(this.rpcMap, this.timeout) : undefined | ||
| const chainProps = this.getChainProps(this.chains, this.optionalChains, desiredChainId) | ||
|
|
||
| const ethProviderModule = await import('@walletconnect/ethereum-provider') | ||
| this.provider = await ethProviderModule.default.init({ | ||
| ...this.options, | ||
| ...chainProps, | ||
| rpcMap: await rpcMap, | ||
| }) | ||
|
|
||
| return this.provider | ||
| .on('disconnect', this.disconnectListener) | ||
| .on('chainChanged', this.chainChangedListener) | ||
| .on('accountsChanged', this.accountsChangedListener) | ||
| .on('display_uri', this.URIListener) | ||
| } | ||
|
|
||
| private getChainProps( | ||
| chains: number[] | ArrayOneOrMore<number> | undefined, | ||
| optionalChains: number[] | ArrayOneOrMore<number> | undefined, | ||
| desiredChainId: number | undefined = this.defaultChainId | ||
| ): ChainsProps { | ||
| // Reorder chains and optionalChains if necessary | ||
| const orderedChains = getChainsWithDefault(chains, desiredChainId) | ||
| const orderedOptionalChains = getChainsWithDefault(optionalChains, desiredChainId) | ||
|
|
||
| // Validate and return the result. | ||
| // Type discrimination requires that we use these typeguard checks to guarantee a valid return type. | ||
| if (isArrayOneOrMore(orderedChains)) { | ||
|
There was a problem hiding this comment. Why is this function necessary? Can't we just do There was a problem hiding this comment. unfortunately no. This is a type guard that explicitly returns the named type that WC requires. If we did |
||
| return { chains: orderedChains, optionalChains: orderedOptionalChains } | ||
| } else if (isArrayOneOrMore(orderedOptionalChains)) { | ||
|
There was a problem hiding this comment. What is the difference between this branch of code and previous branch of code? Looks like you're literally executing same code. Could this be (if necessary) inlined into a single conditional, such as There was a problem hiding this comment. unfortunately this is not compatible with typescript's type discrimination system. gives the pictured error 
|
||
| return { chains: orderedChains, optionalChains: orderedOptionalChains } | ||
| } | ||
|
|
||
| throw new Error('Either chains or optionalChains must have at least one item.') | ||
| } | ||
|
|
||
| private isomorphicInitialize( | ||
| desiredChainId: number | undefined = this.defaultChainId | ||
| ): Promise<WalletConnectProvider> { | ||
| if (this.eagerConnection) return this.eagerConnection | ||
| return (this.eagerConnection = this.initializeProvider(desiredChainId)) | ||
| } | ||
|
|
||
| /** {@inheritdoc Connector.connectEagerly} */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React was complaining about this invalid syntax in the example, but it's not related to the core diff here.