anza-xyz / wallet-standard

Solana extensions to the Wallet Standard.
Apache License 2.0
85 stars 44 forks source link

"Sign In With Solana" feature #12

Closed jordaaash closed 1 year ago

jordaaash commented 1 year ago

This PR adds a "Sign In With Solana" (solana:signIn) feature to the Solana Wallet Standard. This feature is expected to be approximately compatible with "Sign In With Ethereum" (EIP-4361).

Some notable aspects of the design:

See also:

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: dd13db5693a1e149c3ba84d204026a54441a7cde

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

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

vovacodes commented 1 year ago

Would be sick if we could make this work for Smart Contract Wallets too. EIP-4361 actually takes this concern into account here:

Screenshot 2023-02-19 at 12 33 50

The problem is that it relies upon ERC-1271: Standard Signature Validation Method for Contracts which doesn't have an analog on Solana yet. So implementing one would be a prerequisite for the Smart Contract Wallets support.

That said, it looks like we could add it later, maybe by introducing a new field to SolanaSignInInput. I was thinking about something like:

/** The `WalletAccount` that signs the message on behalf of the `account`. */
readonly signer?: WalletAccount;

But again, we could probably add it later.

obstropolos commented 1 year ago

Would be sick if we could make this work for Smart Contract Wallets too. EIP-4361 actually takes this concern into account here: Screenshot 2023-02-19 at 12 33 50

The problem is that it relies upon ERC-1271: Standard Signature Validation Method for Contracts which doesn't have an analog on Solana yet. So implementing one would be a prerequisite for the Smart Contract Wallets support.

That said, it looks like we could add it later, maybe by introducing a new field to SolanaSignInInput. I was thinking about something like:

/** The `WalletAccount` that signs the message on behalf of the `account`. */
readonly signer?: WalletAccount;

But again, we could probably add it later.

Super great to see this on the Solana side and getting pushed forward! We're also down to share any experiences we've had on the wallet integration side in the Ethereum ecosystem as well (considerations like domain binding to protect users).

jordaaash commented 1 year ago

@vovacodes @obstropolos do you have some ideas about how ERC-1271 would port over to Solana wallets? I'm not sure I understand the trust model here -- what is generating the signature, and how is the wallet guaranteeing that the smart contract verifies it?

josip-volarevic commented 1 year ago

A feature that I've been wanting for months! Awesome proposal and something that will surely put a smile to faces of dApp developers.

My question is (and this might be the wrong place to ask it), how does this work with the mobile wallet adapter?

jordaaash commented 1 year ago

That said, it looks like we could add it later, maybe by introducing a new field to SolanaSignInInput. I was thinking about something like:

/** The `WalletAccount` that signs the message on behalf of the `account`. */
readonly signer?: WalletAccount;

But again, we could probably add it later.

@vovacodes this seems like it would require the calling app to be aware that the wallet is a program wallet, and have a way of obtaining the signer WalletAccount first, right? The way I expect most apps will use this is to call signIn without connecting first, so they have no way of getting a WalletAccount.

Also, any solution that requires dapps to know they are calling a program wallet and do something different for it will be difficult to drive adoption for, and sort of violates the contract the Wallet Standard presents to an app (it's not actually an optional param for a program wallet, even though the API describes it as optional).

jordaaash commented 1 year ago

how does this work with the mobile wallet adapter?

@josip-volarevic it's a good question, and I reached out to someone on the Solana Mobile team for comment. It's not an immediate concern for the Wallet Standard, but we are aiming for maximum compatibility between this and the MWA protocol, so I don't want to design something that won't work there.

vovacodes commented 1 year ago

@vovacodes this seems like it would require the calling app to be aware that the wallet is a program wallet, and have a way of obtaining the signer WalletAccount first, right? The way I expect most apps will use this is to call signIn without connecting first, so they have no way of getting a WalletAccount.

Not exactly, how I see it is that signer is resolved by the wallet in exactly the same way as account. The wallet (e.g. Fuse) already knows that its "active" account is a smart wallet, it also knows which keypair (owner wallet) controls the smart wallet, so it can fill in both account: activeWallet and signer: ownerWallet

vovacodes commented 1 year ago

@vovacodes @obstropolos do you have some ideas about how ERC-1271 would port over to Solana wallets? I'm not sure I understand the trust model here -- what is generating the signature, and how is the wallet guaranteeing that the smart contract verifies it?

Frankly, I'm not totally sure how isValidSignature is supposed to work, the example of an implementation in the EIP doc just recovers the signer out of the signature and verifies that that signer is the owner of the contract. I'm not sure if we want to do the same stuff on Solana.

Below is my initial thoughts of how that can be implemented. Note: this is probably skewed towards our use-case - a multisig program. I would expose a standard instruction (probably using anchor's standard 8-bytes discriminator so both anchor and non-anchor programs can implement it) called authenticate or sign_in. That instruction would take the following accounts:

and the following instruction data:

The program can then verify if signer indeed represents an authority of account and can sign in on its behalf. It will throw an error if the signer is invalid. The absence of an error would mean the sign-in is permitted. Dapp devs can verify it on-chain.

Other notes

Anyways, this a literally a brain-dump, just to start a discussion, so please share your ideas.

bfriel commented 1 year ago

Thanks for kicking this off! Looks great. This is pretty similar to how Phantom currently supports "Sign In With". Bundling connect + signMessage into a single step is a welcome UX enhancement.

If the optional account is provided by the dApp, is the expectation here that the wallet must look for that account and switch to it if found? IMO it would be cleaner for the wallet to have the final decision on which account is used during signIn.

This may be a question for a separate thread, but will SolanaSignInInput also be added to Wallet Standard's solana:signMessage feature? We prefer to promote solana:signIn, but we can think of a few cases where dApps may want to "Sign In" after they are already connected.

cc @jozanza

jordaaash commented 1 year ago

If the optional account is provided by the dApp, is the expectation here that the wallet must look for that account and switch to it if found? IMO it would be cleaner for the wallet to have the final decision on which account is used during signIn.

The WalletAccount object provided as account must be provided by the wallet itself. In a wallet like Phantom which is singly modal with respect to the connected account, there's only one WalletAccount object in the accounts array exposed after connecting. Basically, only a wallet that already supports JIT account switching needs to worry about it. Any other wallet can just throw an error because the current account isn't valid for the app's request. But 99% of the time, I expect this will be called with no argument and without connecting first. The wallet should then prompt the user to sign and connect with one prompt, ideally.

jordaaash commented 1 year ago

This may be a question for a separate thread, but will SolanaSignInInput also be added to Wallet Standard's solana:signMessage feature? We prefer to promote solana:signIn, but we can think of a few cases where dApps may want to "Sign In" after they are already connected.

We won't add those params to that method, since some of the fields are required, which would make it a breaking change and an incompatible API with regular message signing. But the signIn method can be called after connecting, either with or without an account. This is up to the wallet of course, but signIn lets the wallet express that it supports this complete API. And the wallet can change the features it exposes and fire the event for this at any time, so if for some reason it only wanted to expose it after connecting, it could.

vpontis commented 1 year ago

I think it would be nice to do something like how we do this in Glow and just have a simple constructed string the wallet signs: https://github.com/glow-xyz/glow-js/blob/master/packages/glow-client/src/utils.ts#L9

We've been supporting the window.glow.signIn for over a year now.

jordaaash commented 1 year ago

This PR has been targeted against a new alpha branch and uses a prerelease changeset so that we can release an npm alpha version. I'm merging it now for this reason, but we can continue to accept comments and make changes.