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

Implement Wallet-Based Authentication with Stellar Wallet Kit #20

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

josephchimebuka
Copy link

@josephchimebuka josephchimebuka commented Sep 28, 2024

Fixes #17
i) Integrated Stellar Wallet Kit for wallet connection
ii) Updated walletStore with wallet-based registration and login methods
iii) Simplified signup component with immediate dashboard redirect after wallet connection

Screenshot from 2024-09-28 17-15-16

@josephchimebuka
Copy link
Author

Hello @ElliotFriend GM ser pls can you review my PR

Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

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

Hey, there @josephchimebuka!! Thanks so much for taking on this task!! 🎉

I've left some feedback and comments throughout. Can you also do me a favor and run yarn lint to make sure the formatting is applied? Thanks!

src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/routes/dashboard/components/WalletKitProvider.svelte Outdated Show resolved Hide resolved
src/routes/login/+page.svelte Outdated Show resolved Hide resolved
src/routes/login/+page.svelte Outdated Show resolved Hide resolved
src/routes/signup/+page.svelte Outdated Show resolved Hide resolved
src/routes/dashboard/components/WalletKitProvider.svelte Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@josephchimebuka
Copy link
Author

Hello @ElliotFriend I made the changes according to changes requested during review and I added some other comments for easier understanding

Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

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

Hi, there @josephchimebuka!!

This is lookin' pretty good! The registration and login flow seem to work really well. Couple more things I'd like to see.

  1. The walletStore's sign function needs to be modified to check if the user is needing to sign with a wallet, or with their pincode.
  • You could do this as simply as making a check for if keyId and publicKey are equal. Then, you'll know that they signed up with the registerWithWallet method, and you'll need to use the wallet kit to prompt for the signature.
  • You could also modify the WalletStore typedef on lines 9-15, and store an (options) walletId or something. The presence of which, in the sign function would tip off the same signal to use a wallet to sign transactions.
  1. Looks like there are some errant formatting issues. Please run yarn format in the repository and commit any necessary changes. Edit: Scratch that, there's a bunch of other problems with the formatting, so i'll take care of that some other time. Don't worry about formatting.

src/routes/dashboard/components/WalletKitProvider.svelte Outdated Show resolved Hide resolved
src/routes/login/+page.svelte Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
@josephchimebuka
Copy link
Author

@ElliotFriend I have added the new changes according to the previous review

Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

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

Looking really close! couple of somewhat nit-picky things, but I think you're almost there!

src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Outdated Show resolved Hide resolved
src/lib/stores/walletStore.js Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
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.

Integrate the stellar-wallets-kit package
2 participants