-
Notifications
You must be signed in to change notification settings - Fork 830
Add ERC: Wallet Connection API #779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ERC: Wallet Connection API #779
Conversation
|
✅ All reviewers have approved. |
| type WalletConnectResult = { | ||
| account: { | ||
| address: `0x${string}`; // connected account address | ||
| supportedChainsAndCapabilities: Record<`0x${string}`,any>; // chain-specific capabilities, mirrors ERC-5792 wallet_getCapabilities |
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.
| supportedChainsAndCapabilities: Record<`0x${string}`,any>; // chain-specific capabilities, mirrors ERC-5792 wallet_getCapabilities | |
| chains: `0x${string}`[]; // supported chains |
Should this just be an array of supported chains? Consumers can use wallet_getCapabilities to extract capabilities from a Wallet.
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.
The intent was to remove the need to make a separate wallet_getCapabilities request. That may not be necessary at API layer though because SDKs could do this automatically for apps.
ERCS/erc-7846.md
Outdated
| } | ||
|
|
||
| type SignInWithEthereumCapabilityResult = { | ||
| message: string, // formatted SIWE message |
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.
Might also be nice to add a payload property so consumers don't have to parse the message string to figure out the inferred parameters (e.g. address, domain, etc)?
| message: string, // formatted SIWE message | |
| message: string, // formatted SIWE message | |
| payload: { | |
| address: `0x{string}`, | |
| nonce: string, | |
| ... | |
| }, |
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.
devil's advocate, consumers not using the ABNF to parse it are risking all kinds of security problems, payload and message could fall out of sync (which is itself just an additional surface to secure), etc etc
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.
Wdym? If they are out of sync, then signature verification will fail.
ghost
left a comment
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.
Need changes
feat: `wallet_disconnect` + structure improvements
|
While I understand the motivation for creating EVM-specific standards for Wallets... there is nothing new or extra that ERC-7846 provides that isn't already covered by CAIP-222 https://chainagnostic.org/CAIPs/caip-222 Why restrict such an important pattern like "Connect + Sign" to only EVM wallets? |
one huge problem still: For passkey wallets, e.g. Signing Up with Coinbase Smart Wallet, you would still need 2 interactions with this ERC. First TouchID to get the Ethereum Address, second one to sign the message. Sign In With Ethereum is not a good solution. The address should not be part of the signed message. It's implicit from the signature. I'd suggest changing this ERC to allow signing an arbitrary message, makes it simpler and leaves the kind of signature to the dApp. |
I would tend to agree that having a multi-VM/multi-L1 syntax for every context except EVM and an EVM-specific context is needlessly inharmonious, if using the CAIP-222 RPC message would be functionally identical to this bespoke one. That said, we have an ERC here that seems to work, and are comparing it to the hypothetical ERC which achieves the same properties with a CAIP-222-conformant message. Would it help to write a competing ERC that has all the same properties and additional can be parsed by a multi-chain/CAIP-222 system like WalletConnect's? I'd rather not be the one to write it, but if no one else is offering AND evaluators of this ERC would appreciate an apples-to-apples ERC, I can write one... who knows, maybe the exercise of writing out CAIP-222 as an ERC for EVM-specific contexts will tease out subtle differences? |
ERCS/erc-7846.md
Outdated
| @@ -0,0 +1,233 @@ | |||
| --- | |||
| eip: 7846 | |||
| title: Wallet Connection API | |||
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.
I know there isn't a ton of room in the title (44 characters IIRC), but it would be nice if you could cram a bit more detail in here to help disambiguate.
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.
Would prefer to keep it as-is to be consistent with 5792 (Wallet Call API)
But if you feel strongly that we should disambiguate further then I'd suggest "Wallet Connection API with Capabilities"
ERCS/erc-7846.md
Outdated
|
|
||
| ## Abstract | ||
|
|
||
| This ERC introduces a new wallet connection JSON-RPC method focused on extensibility. It leverages the modular capabilities approach defined in [ERC-5792](https://eips.ethereum.org/EIPS/eip-5792#wallet_getcapabilities) to streamline connections and authentication into a single interaction. |
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.
You should mention the specific endpoint in the abstract, and have maybe a bit more technical detail here. An abstract is not an introduction.
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.
agree!
ERCS/erc-7846.md
Outdated
|
|
||
| #### `signInWithEthereum` | ||
|
|
||
| Adds support for offchain authentication using [ERC-4361: Sign-In with Ethereum](https://eips.ethereum.org/EIPS/eip-4361). |
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.
You'll need to use a relative link. Something like:
| Adds support for offchain authentication using [ERC-4361: Sign-In with Ethereum](https://eips.ethereum.org/EIPS/eip-4361). | |
| Adds support for offchain authentication using [ERC-4361](./eip-4361.md). |
ERCS/erc-7846.md
Outdated
|
|
||
| Same as ERC-4361 specification with minor modifications: | ||
| * The casing of multi-word fields has been adjusted to camelCase instead of kebab-case. Resources are an array field. | ||
| * The account address returned by `wallet_connect` MUST match the address inferred in the SIWE message. |
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.
You should expand the abbreviation the first time it is used:
| * The account address returned by `wallet_connect` MUST match the address inferred in the SIWE message. | |
| * The account address returned by `wallet_connect` MUST match the address inferred in the Sign-In with Ethereum (SIWE) message. |
ERCS/erc-7846.md
Outdated
|
|
||
| ## Copyright | ||
|
|
||
| Copyright and related rights waived via CC0. |
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.
Please use the exact waiver from the template.
Genebond
left a comment
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.
Review
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.
I really like this EIP, it removes friction from the users that need to auth as well as adding a disconnect method which has been always needed in the current standard (1193).
I like that it mentions that future EIP's might extend capabilities. Which makes the EIP flexible without introducing breaking changes.
The DX looks nice and simple!
In the future I would personally like to be added an option to submit transaction requests right away together with the "wallet_connect" request. Even batching, which I think the wallet could internally handle in its own way (in case it supports it or just running them "synchronously"). I think this would be particularly useful for "one action" (and not one transaction) user needs, like Swapping or payments.
|
|
||
| ```ts | ||
| type Request = { | ||
| method: 'wallet_disconnect' |
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.
should we add optional params in case the user wants to disconnect only specific accounts? or would it be too much.
|
|
||
| ## Rationale | ||
|
|
||
| ### Multiple Accounts |
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.
Should we add a method to request an accounts switch? To change the targeted account.
In such case should the wallet emit accountsChanged with the order of the accounts array altered?
Also, what if the user is connected with account A but now also wants the connect with B such that A and B are both connected? This has been something pretty much requested from dapp devs and we are using a workaround that only works with MetaMask AFAIK.
Maybe we should add specifications for wallets around this behavior, "wallet_connect" could be called by the dapp more than once to update the state of the connected accounts.
ERCS/erc-7846.md
Outdated
|
|
||
| Disconnects connected account(s). | ||
|
|
||
| - The wallet SHOULD revoke any capabilities associated with the account(s) that were granted upon connection via `wallet_connect`. |
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.
| - The wallet SHOULD revoke any capabilities associated with the account(s) that were granted upon connection via `wallet_connect`. | |
| - The wallet SHOULD revoke access to the user account(s) information, as well as to any capabilities associated with them that were granted upon connection via `wallet_connect`. |
|
The commit d91b259 (as a parent of 5fa6bd2) contains errors. |
fix: lint issues
|
Hi @SamWilsn and @glitch-txs thanks for the feedback. I've worked with @ilikesymmetry to update the PR |
| --- | ||
| eip: 7846 | ||
| title: Wallet Connection API | ||
| description: Adds JSON-RPC method for requesting wallet connection with modular capabilities. |
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.
Minor nit, but you repeat "wallet connection" from the title in your description. It might be better to reword it so the reader knows what a "wallet connection" is.
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
This proposal defines a new RPC for wallet connection with an emphasis on extensibility. Builds on the notion of optional “capabilities” defined in ERC-5792 to add new functionality modularly. This proposal defines one capability to reduce separate interactions for connection and authentication, but otherwise seeks to leave capability definitions open-ended.