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

feat: introducing magic.link provider (email based authentication) #124

Merged
merged 13 commits into from
Jan 30, 2024

Conversation

aorumbayev
Copy link
Contributor

@aorumbayev aorumbayev commented Dec 11, 2023

Description

Please explain the changes you made here

  • Adding integration with https://magic.link/ algorand extension allowing seamless auth options using email

Checklist

Please, make sure to comply with the checklist below before expecting review

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@aorumbayev aorumbayev marked this pull request as ready for review December 12, 2023 17:24
@aorumbayev
Copy link
Contributor Author

@drichar hey, let me know if there are any tweaks to address, ill merge latest from main and if you got some further comments can patch those up

Copy link
Collaborator

@drichar drichar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for adding Magic Link support @aorumbayev! And I'm so sorry about the wait to get it reviewed.

I spun up the example in Storybook and it works flawlessly. 🚀

There are a few things to address in the code:

  1. Since the API key is a required option, the ProviderDef union type should be updated to enforce this.
  2. The provider.connect function exposed in the public API by the useWallet hook shouldn't accept an onDisconnect argument; it's used internally by the wallet clients' connect method. See my comment for a detailed explanation.

Besides that, there are a few other very minor change requests.

Overall it works great and I love the implementation. If you're busy (and don't mind), I'd be happy to make these changes myself to get this merged and released ASAP. Let me know what you'd prefer.

Thanks again for this, and for all of your contributions to the library! ❤️

src/clients/magic/client.ts Show resolved Hide resolved
src/clients/magic/index.ts Outdated Show resolved Hide resolved
src/components/Example/Example.tsx Outdated Show resolved Hide resolved
src/components/Example/Example.tsx Outdated Show resolved Hide resolved
src/types/providers.ts Outdated Show resolved Hide resolved
src/types/providers.ts Outdated Show resolved Hide resolved
src/hooks/useWallet.ts Outdated Show resolved Hide resolved
src/hooks/useWallet.ts Outdated Show resolved Hide resolved
src/types/wallet.ts Outdated Show resolved Hide resolved
src/components/Example/Connect.tsx Outdated Show resolved Hide resolved
@aorumbayev
Copy link
Contributor Author

@drichar hey thank you for the review! Agree with all comments and I believe all should be addressed with last commits, also lmk if you are ok with tweaks in babel are ok, for some reason with latest magic sdks storybook started failing for me. Had to add extra plugin.

@drichar
Copy link
Collaborator

drichar commented Jan 30, 2024

@aorumbayev Thank you for making these changes!

Yeah I'm fine with updating the Babel config for Storybook... I did update and remove some dependencies in https://github.com/TxnLab/use-wallet/releases/tag/v2.5.0 that might be the reason Storybook worked for you before and then didn't.

I've been thinking about the addition of the email argument to BaseClient's connect method, and wondering if it could potentially cause problems down the road. Perhaps it makes more sense to change it from from email?: string to arg?: any and doing a manual type check inside the individual provider(s)?

// something like:
if (typeof email !== 'string') {
  throw new Error('Magic Link provider requires an email (string) to connect')
}

Then if some future wallet provider needs to accept an argument to connect – say, an object – we won't be forced to either add a new optional argument or introduce a breaking change?

Let me know what you think. Besides that, everything else looks great!

@drichar drichar merged commit 85f8bef into TxnLab:main Jan 30, 2024
1 check passed
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.

2 participants