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

refactor: drop siwe and ethers #224

Closed

Conversation

dalechyn
Copy link
Collaborator

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 review

  • PR title adheres to the conventional commits standard
  • PR has a changeset
  • PR has been tagged with a change label(s) (i.e. documentation, feature, bugfix, or chore)
  • PR includes documentation if necessary
  • All commits have been signed

Additional Context

If this is a relatively large or complex change, provide more details here that will help reviewers

@dalechyn
Copy link
Collaborator Author

@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?
if so, where? and how major?

@varunsrin
Copy link
Member

varunsrin commented Sep 24, 2024

@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

@dalechyn
Copy link
Collaborator Author

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

Comment on lines -88 to -93
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(),
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

@dalechyn dalechyn left a 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 a success bool in case if
  • issuedAt in SiweMessage type that comes from viem now is of type Date, not string as before
  • auth-client: viem peer dependency bumped to 2.x
  • publicClient for auth-client is now required to be provided (instead of provider: JSONRpcProvicer previously), as the verifyHash makes an rpc request and requires an actual client to do it.

@dalechyn
Copy link
Collaborator Author

@varunsrin,

also, all the CI seems to be breaking

CI pipelines seem to fail because of some docker authorization issue, not code related.

@dalechyn dalechyn marked this pull request as ready for review September 25, 2024 23:45
@dalechyn
Copy link
Collaborator Author

something to think about – viem's public client might be an optional parameter if for hash verification only ecrecover is used (without eip6942 support)

@horsefacts
Copy link
Collaborator

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.

@dalechyn
Copy link
Collaborator Author

Closed in favour of #225 to let the CI run.

@dalechyn dalechyn closed this Sep 30, 2024
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.

3 participants