feat: walletconnect #529
feat: walletconnect #529
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| data: msg.data, | ||
| chainId: msg.chainId, | ||
| nonce: msg.nonce, | ||
| // MetaMask, like other Web3 libraries, derives its transaction schema from Ethereum's official JSON-RPC API specification |
There was a problem hiding this comment.
If this comment is relevant to WalletConnect as well, update the text to match:
| // MetaMask, like other Web3 libraries, derives its transaction schema from Ethereum's official JSON-RPC API specification | |
| // WalletConnect, like other Web3 libraries, derives its transaction schema from Ethereum's official JSON-RPC API specification |
There was a problem hiding this comment.
Pretty sure my entire ethereum.ts can be removed
| connected = false; | ||
| chainId: number = -1; | ||
| accounts: string[] = []; | ||
| // TODO: confirm empty string doesn't break |
There was a problem hiding this comment.
Test this and remove comment
…feature-walletconnect
| } | ||
|
|
||
| public async ethGetAddress(): Promise<string | null> { | ||
| return this.ethAddress; |
There was a problem hiding this comment.
| return this.ethAddress; | |
| if (!this.ethAddress) { | |
| if (!this.connected) { | |
| console.error("WalletConnect provider not connected."); | |
| return null; | |
| } | |
| const { accounts } = this.provider.connector; | |
| const [address] = accounts; | |
| this.ethAddress = address; | |
| } | |
| return this.ethAddress; |
There was a problem hiding this comment.
Is the setting of ethAddress inside of it's "getter" an anti-pattern?
| provider: WalletConnectProvider; | ||
| connected = false; | ||
| chainId: number = -1; | ||
| accounts: string[] = []; | ||
| ethAddress: string = ""; |
There was a problem hiding this comment.
I'm thinking maybe it might be an improvement to make these fields private. Is there any reason not to?
There was a problem hiding this comment.
If you're only talking about the three (chainId, accounts, connected) that I added, then yes, I could see it as an improvement.
| * @see https://docs.walletconnect.com/client-api#sign-transaction-eth_signtransaction | ||
| */ | ||
| public async ethSignTx(msg: core.ETHSignTx & { from: string }): Promise<core.ETHSignedTx | null> { | ||
| msg.from = this.ethAddress; |
There was a problem hiding this comment.
This works but sort of breaks the pattern established in the other wallet adapters. Only for consistency, I think we should keep these functions as wrappers and move the logic down into ethereum.ts, passing whatever state is necessary into those functions. The same goes for ethSendTx(), ethSignMessage() and ethVerifyMessage().
…feature-walletconnect
… into feature-walletconnect
|
closing in favor of #544 |
Adds a package for integrating WalletConnect
Closes #499