Skip to content
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

"Sign In With Solana" feature #12

Merged
merged 13 commits into from
Jun 1, 2023
Merged

"Sign In With Solana" feature #12

merged 13 commits into from
Jun 1, 2023

Conversation

jordaaash
Copy link
Collaborator

@jordaaash jordaaash commented Feb 18, 2023

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:

  • The account is an optional Wallet Standard WalletAccount in the request, and required in the signed message and method output. This means an application can sign in and connect to the wallet in a single step. It doesn't need to asynchronously connect to get an account, prepare a message to sign in, then prompt the user to sign it. It can sign in without knowing what account to sign in with, and when the message is signed, the account will be connected as well.
  • The chain is a Wallet Standard chain IdentifierString. This would typically be solana:mainnet, but others are defined here, and this is arbitrary.
  • As with other Solana Wallet Standard features, the solana:signIn feature supports multiple arguments, allowing signing in with multiple accounts in a single prompt.
  • As with the solana:signMessage feature, the solana:signIn feature returns both the signature and the bytes that were signed. This is necessary because the final message bytes (including the account address) will be prepared by the wallet, not the application. These message bytes may also be prefixed or formatted (for example, for signing with the Solana Ledger app). The application must parse the signed message bytes and verify them against the signature.
  • The signature is always the raw Ed25519 signature bytes. There is no other signature type specification.
  • All other fields for the message are defined by EIP-4361.

See also:

@changeset-bot
Copy link

changeset-bot bot commented Feb 18, 2023

🦋 Changeset detected

Latest commit: dd13db5

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
Copy link

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
Copy link

obstropolos commented Feb 19, 2023

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
Copy link
Collaborator Author

@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
Copy link

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link

@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
Copy link

@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:

  • account - PDA controlled by the program, the account to sign into;
  • signer [signer] - The authority that claims it can sign in on behalf of the account;
  • remaining_accounts - additional accounts that a specific program might need for the verification process. For the Squads program this would be the multisig account that controls the account (vault in this case) and stores data about the multisig members. This is the part that I dislike a lot because it will be custom for each smart wallet program, hence voiding the whole idea of a standard implementation working for everyone, but at the same time, I expect that the actual calls to authenticate will be done by the wallet software that is already aware of what program it works with, so it can adjust the call as necessary.

and the following instruction data:

  • account_seeds: Vec<Vec<u8>> - the seeds of the account. This can be helpful in "identifying" the account.

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

  • Signing in will cost 5000 lamports, which is peanuts but still sucks a little. I believe, in ERC-1271 it also costs some gas.
  • Maybe, in order to truly check the signer's authority over account, the sign_in instruction should contain a CPI call to the Memo Program "signed" by account. Not sure if this would provide extra security though.

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

@bfriel
Copy link

bfriel commented Feb 22, 2023

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
Copy link
Collaborator Author

jordaaash commented Feb 22, 2023

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
Copy link
Collaborator Author

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
Copy link

vpontis commented Feb 23, 2023

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 jordaaash changed the base branch from master to alpha June 1, 2023 21:11
@jordaaash
Copy link
Collaborator Author

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.

@jordaaash jordaaash merged commit 9e862ba into alpha Jun 1, 2023
1 check passed
@jordaaash jordaaash deleted the sign-in branch June 1, 2023 21:17
@jordaaash jordaaash mentioned this pull request Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants