near / fast-auth-signer

https://fast-auth-signer.vercel.app
MIT License
30 stars 9 forks source link

Firebase Account Indexer #167

Closed Pessina closed 8 months ago

Pessina commented 8 months ago
vercel[bot] commented 8 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fast-auth-signer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 2, 2024 4:57pm
hcho112 commented 8 months ago

I manage to get the full end to end manual testing test working using https://github.com/near/near-discovery/pull/929

Chrome: :heavy_check_mark: Create account :heavy_check_mark: Sign in on above account

Firefox (Non-webauthN): :heavy_check_mark: Check account :x: Sign in with an account created on Chrome :x: Sign in with an account created on Firefox (itself) (I think non-webauthN browser signin is broken, it hangs on Signing transaction text view and stay there)

Safari: :heavy_check_mark: Sign in with an account created on Chrome :heavy_check_mark: Redirected to device page and go through full flow (Also confirmed that it does delete public key records well) :x: Sign out and sign back in with same account (failed at kitwallet failure)

Result: After conducting the full manual test, I have two things that need to point out for next step forward.

  1. In this PR, we need to get non-webauthn flow fixed. Also ideally we need to replace every place in this repo that reference kit wallet for public key -> account id call.
  2. (Main problem) unfortunately making this PR/this repo fully replacing kitwallet won't fix the all problem. Because, the package @near-js/biometric-ed25519``getKeys method also uses kit wallet. But then, the point is that this package is design to be used with community (not tied to Pagoda internal usage) who doesn't support/use firebase. TDLR: if we really want to use firebase, we need to implement logic that try firebase if exist, or fall back to queryAPI(or kitwallet for now). CC @esaminu @DavidM-D
Pessina commented 8 months ago

@hcho112 I understand your concerns. But as I discussed with @esaminu, this PR isn't replacing KitWallet. We are introducing Firebase as a fallback in case the KitWallet can't return the accountId (due to recent hight instability).

As you can see on this file, we first check if the account is available on KitWallet, if not, we check for Firebase.

Firebase will only index accounts created after this PR, adding redundancy. Old accounts should be available by the KitWallet.

The idea isn't index accounts created before this PR on Firebase.


Regarding the usage of KitWallet on @near-js/biometric-ed25519``getKeys, can you please share with me where we call it?

I checked the code for getKeys but I think I'm missing something. Maybe I'm checking the wrong method 😅

hcho112 commented 8 months ago

@Pessina , file <- this implementation is exactly that I was talking about. But it looks like reference change isn't part of this PR or exist in main branch. I would suggest to above change to be merged to main soon or include in this PR together? (If so, this PR would be perfect!)

Maybe we shall discuss on the call together first

Pessina commented 8 months ago

@hcho112 I didn't manage to reproduce the bugs you metioned, can you please re-test on your side and share a video or step-by-step how to reproduce the bugs?

Setup:

near-discovery: main branch fast-auth-signer: 122-firebase-account-indexer branch

There are my testing videos

1 - Safari (Sign out and sign back in with same account (failed at kitwallet failure)) https://drive.google.com/file/d/1gdIcAZxz1mh6Py6QEa-XoYrStgd-YRkc/view?usp=sharing

2 - Firefox(Sign in with an account created on Firefox (itself)) https://drive.google.com/file/d/1BwWY9NVei22fvyQWkMj3eCwL9QlUU9bB/view?usp=sharing

3 - Chrome + Firefox (Sign in with an account created on Chrome) https://drive.google.com/file/d/1mc64K23vQgFhZ2VKPCVl4ft9DcnWcr2F/view?usp=sharing

Please let me know if you don't have access to the recordings

hcho112 commented 8 months ago

Thanks for the update and making records on each steps @Pessina First of all, I don't if it is from your latest change, but when I test again on FF, it worked finally. So I'm not against this PR going forward.

Just feedback to your changes:

  1. Interesting thing is that, when I tested it, I didn't even had to type the url manually, it actually redirected me straight back to where it was.
  2. It works as expected
  3. I can also see that it works. (Also unlike your video, it redirects within same browser tab so I didn't had to manually type anything)

So at this point, I would say this is LGTM on my side. Lets follow up with @esaminu and ship it. Good work @Pessina