-
Notifications
You must be signed in to change notification settings - Fork 32
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
refactor: drop siwe and ethers #224
refactor: drop siwe and ethers #224
Conversation
@horsefacts I'm not sure wether it was listed on the bounty or not (i cannot open it due to their cloudflare rules), but are breaking changes allowed? |
@dalechyn if we have to that's fine - could you add a couple of lines explaining why its necessary? also, all the CI seems to be breaking |
yep, still in the works, my 1h quickscope attempt failed, this definitely needs another session, that's why PR is in Draft. i will list out the required breaking changes but will try to introduce as little of them as possible. |
if (fid !== BigInt(response.fid)) { | ||
response.success = false; | ||
response.error = new SiweError( | ||
`Invalid resource: signer ${signer} does not own fid ${response.fid}.`, | ||
response.fid.toString(), | ||
fid.toString(), |
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.
this did not make much sense to me, so now it will throw an error as below.
previously it both threw an error during .unwrap
and additionally returned .error
and .success
for that specific error.
I believe it's better to stick to one error handling flow. If this change is not appreciated I can make it up as before.
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.
Good change, it's cleaner. IIRC this was a hack around how we were catching and formatting SiweError
. Now that we're not using it, not necessary.
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.
Breaking changes:
- AppClient.verifySignInMessage now doesn't return
success
and instead throws an error if the verification didn't succeed. Reasoning: It actually threw an error in the past since the result of the operation was unwrapped with.unwrap()
, but it also return asuccess
bool in case if issuedAt
inSiweMessage
type that comes fromviem
now is of typeDate
, notstring
as beforeauth-client
:viem
peer dependency bumped to2.x
publicClient
forauth-client
is now required to be provided (instead ofprovider: JSONRpcProvicer
previously), as theverifyHash
makes an rpc request and requires an actual client to do it.
CI pipelines seem to fail because of some docker authorization issue, not code related. |
something to think about – viem's public client might be an optional parameter if for hash verification only ecrecover is used (without eip6942 support) |
A little surprised this doesn't try ecrecover first, then 6942, but this seems fine. |
Closed in favour of #225 to let the CI run. |
Motivation
Bounty.
Change Summary
Dropped siwe and ethers in favour of viem.
TODO: pitfalls and breaking changes.
Merge Checklist
Choose all relevant options below by adding an
x
now or at any time before submitting for reviewAdditional Context
If this is a relatively large or complex change, provide more details here that will help reviewers