feat: SIWE plugin #2579
feat: SIWE plugin #2579
Conversation
|
@rokitgg is attempting to deploy a commit to the better-auth Team on Vercel. A member of the Team first needs to authorize it. |
commit: |
|
we need remove |
|
Given that viem directly implements siwe, using siwe's bloat is not so great imho. it's be also beneficial to make the Ps; 've also made the rpc url an option in mine, in case I need to use a custom rpc when validating via publicClient. Here's a small snippet of my local plugin I've been using: import { generateSiweNonce, parseSiweMessage } from "viem/siwe";
import { createPublicClient, http, type Address, type Hex } from "viem";
const publicClient = createPublicClient({
chain: mainnet,
transport: http()
})
...
const nonce = generateSiweNonce();
...
const parsedMessage = parseSiweMessage(message);
...
const isValid = await publicClient.verifyMessage({
message,
signature: signature as Hex,
address: parsedMessage.address,
});So pretty much almost the same what comes with siwe. |
Agree but packages like viem use siwe under the hood so isn't that quite the same problem after all? |
its no problem, i mean let the user do it : import { betterAuth } from "better-auth"
// import viem or siwe or other libs ...
export const auth = betterAuth({
// ... other config options
plugins: [
siwe({
async generateSiweNonce() {
// Implement
},
async parseSiweMessage() {
}
})
]
}) |
Ohh okay now I get what u mean by user functions, that def sounds like the best solution! |
|
So plugin options could look something the likes of interface SiweOptions {
domain: string;
generateSiweNonce: () => Promise<string>;
parseSiweMessage: (message: string, signature: string) => Promise<boolean>;
}Since the siwe message should be generated on the client, that's probably all it takes in terms of plugin user functions - lmk if missed something |
|
@rokitgg and need |
|
@rokitgg I was about to start writing this as a community plugin 'cause I need it to migrate a project from Auth.js 😅 On your question with external deps, I think adding We should however add it to peer deps so the user is expected to install it to keep bundle size down and add a note in the docs. Not sure what @8times4 means by bloat as it's 76.4 kB unpacked compared to If going with @reslear solution (user defines the functions) then add a custom RPC to the config as the |
ah, my bad then, I stand corrected on bloat there. unrelated; I just noticed that siwe is also implemented in |
| }); | ||
|
|
||
| if (!user) { | ||
| const tempEmail = `${ctx.body.publicKey}@${process.env.NEXT_PUBLIC_BASE_URL}`; |
There was a problem hiding this comment.
Use
const { emailDomainName = getOrigin(ctx.context.baseURL) } = options || {};`
const email = `${publicKey}@${emailDomainName}`;
| if (!user) { | ||
| const tempEmail = `${ctx.body.publicKey}@${process.env.NEXT_PUBLIC_BASE_URL}`; | ||
| user = await ctx.context.internalAdapter.createUser({ | ||
| name: ctx.body.publicKey, |
There was a problem hiding this comment.
Missed opportunity to retrieve ENS name, take it in as an optional param from the client and fallback to something else other than publicKey. Check the Anonymous plugin AnonymousOptions interface for some other ideas.
There was a problem hiding this comment.
Yep, I had ENSName and ENSAvatar in a previous implementation but dropped it before drafting PR because it depended on wagmi. But happy to add that!
| import type { User } from "../../types"; | ||
|
|
||
| export interface SiweUser extends User { | ||
| publicKey?: string; |
There was a problem hiding this comment.
walletAddress would be better as publicKey is too general IMO.
|
@8times4 Yeah You're actually not wrong on SIWE maintenance it has seen better days 😅 Maybe we use it for now and write our own utils phasing it out 1:1 at some point. This industry moves really fast though so we will be stuck on maintaining it ofc. Tradeoffs. |
Thank you so much for this comments! I really do agree with your vision. |
- Add ensName as an optional param from the client - Add emailDomainName to plugin options - fallback to getOrigin() - Move schema to a separate file
Bug ReportThe SIWE plugin's
Comments? Email us. |
Bug Report
Comments? Email us. |
|
Damn this jazzberry bot really is smart 😂 |
Bug Report
Comments? Email us. |
Bug Report
Comments? Email us. |
|
Thank @rokitgg you for your hard work @8times4 @b-bot Still disagree, this is not consistent with the whole library:
Imagine if we added only 1 provider to the plugin https://www.better-auth.com/docs/concepts/email |
|
I certainly think @reslear makes a really strong point here. If i had no other option but to add any deps (as peer) my personal choice would be |
|
Yeah let's go with typing the function and leaving implementation to the user to pass in the config 👌🏼 As a final note (and why I mentioned Anonymous plugin) we need to handle account linking. ie. user signs in with google while signed in with eth and it should replace the fake email. This would allow retrieval of the account through multiple methods. |
|
Next point - it will be necessary to check how it works when a user has many addresses, and maybe switching or linking |
… and message verification through plugin options - Removed direct dependency on 'siwe' package. - Introduced 'generateSiweNonce' and 'verifySiweMessage' as user functions - Updated tests to reflect changes in nonce handling and message verification logic (WIP)
Bug Report
Comments? Email us. |
|
Alright so just removed the Where I'm a bit lost is on how to write exhaustive tests for this plugin - If anyone is willing to share a bit of light there I would very much appreciate that! |
|
@rokitgg You're going to want to finalise everything before writing test cases but I would say that because it's more of an open implementation we can mock most things. I don't know the policy about adding packages for testing (since dev deps aren't shipped in bundle) but this exists: https://wagmi.sh/react/api/connectors/mock Use AI to gen some cases after finishing everything and we can look for gaps. There's also examples of how SIWE is testing to give you ideas: https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.test.ts PS. I thought of one more thing we might want to add to config. An |
|
Thank you so much for the suggestions @b-bot - already implemented the anonymous flag Regarding tests, the issue here when using the Will be taking a closer look at the siwe tests you mentioned there, that's probably quite helpful. |
… & other edge cases
|
@rokitgg looking good here, excited to use this! |
| export const schema = { | ||
| user: { | ||
| fields: { | ||
| walletAddress: { |
There was a problem hiding this comment.
regarding this, wouldn't it possibly make sense to call it evmAddress?
only reason i'm asking is because SIWS (Sign In With Solana) could not use publicKey as it being too generic, - but solAddress would not be so "streamlined" compared to walletAddress.
(This is to future-proof as I'm planning to add SIWS later)
There was a problem hiding this comment.
@8times4 wouldn't that be another plugin as this is tightly coupled to ENS etc?
(I don't know SIWS)
There was a problem hiding this comment.
correct @b-bot although SIWX exists, that would be overkill most likely.
SIWS has SNS instead of ENS, only proposing this evmAddress change due to: #2579 (comment)
so that it's somewhat similar/understandable in the schema which does what if someone uses both SIWE and SIWS
There was a problem hiding this comment.
Don't rlly have any strong views on this so absolutely open to rename the field into whatever feels the most convenient for everyone - naming things is truly hard and it shows
Sure! Go ahead whenever u wish to. |
|
Hey! Just chiming in to suggest that the plugin should probably store the checksummed version of the wallet address instead of the raw input. Not all wallets send the address properly checksummed, and this could lead to inconsistencies when trying to match the user given a wallet. |
|
@vimtor thanks for your suggestion, totally missed that one! Will absolutely look to implement the checksum parsing. The issue I've found here is that the hashing algorithm
If we want strict EIP-55 compliance, we need a Keccak256 implementation somehow - happy to discuss the go to way for this. For reference: https://ethereum.stackexchange.com/questions/550/which-cryptographic-hash-function-does-ethereum-use https://eips.ethereum.org/EIPS/eip-55 As defined in the EIP-55 reference (link above), this is the appropriate implementation: const createKeccakHash = require('keccak')
function toChecksumAddress (address) {
address = address.toLowerCase().replace('0x', '')
var hash = createKeccakHash('keccak256').update(address).digest('hex')
var ret = '0x'
for (var i = 0; i < address.length; i++) {
if (parseInt(hash[i], 16) >= 8) {
ret += address[i].toUpperCase()
} else {
ret += address[i]
}
}
return ret
} |
|
@rokitgg |
Aims to close #41
Tried my best to implement SIWE within the boundaries of better-auth, and seems like this draft may be a good starting point for it! Although I do feel like a bit of discussion is needed on some topics.
Things to consider:
Any feedback/contributions are highly welcomed!