-
Notifications
You must be signed in to change notification settings - Fork 869
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
A useSignIn()
hook for accessing the Sign In With Solana feature
#2928
A useSignIn()
hook for accessing the Sign In With Solana feature
#2928
Conversation
🦋 Changeset detectedLatest commit: 686539f The changes in this PR will be included in the next version bump. This PR includes changesets to release 37 packages
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @steveluscher and the rest of your teammates on Graphite |
46ad8e6
to
7aaf056
Compare
return useCallback( | ||
async (...inputs) => { | ||
const inputsWithAddressAndChainId = inputs.map(input => ({ | ||
...input, |
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 default the domain
to window.location.host
if not supplied?
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 think it makes sense yeah. I'm trying to think about security reasons not to do this but can't see any.
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.
We can, but the wallet must set this anyway if it's not provided.
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'm going to leave it unsupplied.
); | ||
} | ||
|
||
function useSignIns( |
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 think we can do away with this plural API, seems very unlikely to be used and certainly doesn't work today.
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.
Oh I see, this isn't exported anyway, this is just internally how it's implemented because the Wallet Standard is this way. Carry on!
const resultsWithoutSignatureType = results.map( | ||
({ | ||
account, | ||
signatureType: _, // Solana signatures are always of type `ed25519` so drop this property. |
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.
Yeah, this was a misguided design. The idea was that we could support secp256r1 signatures potentially later, but that's better handled through a new feature/version.
.changeset/fuzzy-lizards-end.md
Outdated
try { | ||
const { account, signedMessage, signature } = await signIn({ | ||
domain: window.location.host, | ||
nonce: csrfToken, |
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.
Aside: It's extremely dumb that SIWE has request-id
and nonce
which are duplicative in almost all cases, and JWT-esque issued-at
and expires-at
timestamps when these are not intended to be and should not be used as JWTs directly. Apps should generate a random session ID from the server and use it as request-id
, then verify and issue a proper JWT if needed from the server the correct way.
Ideally we can reflect the best way to do this in docs and examples and users should not touch most of the 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.
nit: I do prefer requestId
to nonce
because it makes it more clear that it should come from a server, it's not using a cryptography term improperly, and it's not confused by Brits for... other meanings of the term.
.changeset/fuzzy-lizards-end.md
Outdated
onClick={async () => { | ||
try { | ||
const { account, signedMessage, signature } = await signIn({ | ||
domain: window.location.host, |
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.
domain
probably not needed in the example since apps shouldn't provide it.
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're right about that! I noticed some wallets fail awkwardly when you don't supply it, but that's a bug with their implementation.
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.
export function useSignIn(uiWallet: UiWallet): (input?: Input) => Promise<Output>; | ||
export function useSignIn(uiWalletHandle: UiWalletHandle): (input?: Input) => Promise<Output> { |
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.
Nice 👍
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.
LGTM, had some non-critical comments that apply mainly to examples/defaults
7aaf056
to
d7db531
Compare
Merge activity
|
908235e
to
bcf481a
Compare
bcf481a
to
686539f
Compare
🎉 This PR is included in version 1.95.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
Summary
This hook returns essentially the raw sign in function from the
solana:signIn
feature. It upcasts the returnedWalletAccount
to aUiWalletAccount
usable with the rest of the@solana/react
API.Callers are responsible for verifying the signed message and its signature.
Test plan