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

16 implement create account route #24

Merged
merged 42 commits into from
Aug 21, 2023

Conversation

amirsaran3
Copy link
Collaborator

Implements /create-account and /verify-email routes. Copied logic and UI from NEAR Discovery repo.

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts here @amirsaran3 -- nice progress. I've added notes/comments throughout -- some stuff needs to be 🔪 since it was near-discovery specific, and we can clean up some of the logic in a few places as we migrate it over.

.eslintrc.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/App.tsx Outdated Show resolved Hide resolved
src/App.tsx Outdated Show resolved Hide resolved
email,
...(accountId ? { accountId } : {}),
...(isRecovery ? { isRecovery: 'true' } : {}),
...(success_url ? { success_url } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than separate URL for success and failure, let's use a single variablecallback_url to represent the URL to redirect to in either the success or failure case, but include the result in its own URL parameter result as 'success'|'fail'.

If result is set to fail, we will include reason as an additional parameter which will include some information about the error/failure countered.

Copy link
Collaborator

@esaminu esaminu Aug 7, 2023

Choose a reason for hiding this comment

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

@MaximusHaximus can this interface change be pushed forward? I'm currently using almost a duplicate of WalletConnection from NAJ to interface with the signer app from dapps and would prefer to keep the interface consistent with that to avoid having to deviate from that code too much and test it separately

src/utils/zendesk.ts Outdated Show resolved Hide resolved
src/utils/segment-analytics.ts Outdated Show resolved Hide resolved
src/utils/rudder-analytics.ts Outdated Show resolved Hide resolved
src/types/rudderstack-analytics.ts Outdated Show resolved Hide resolved
src/stores/current-component.ts Outdated Show resolved Hide resolved
@esaminu esaminu changed the base branch from main to add-device-route August 2, 2023 20:57
Copy link
Collaborator

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

I just want to add one thing. Upon creating account, it should utilise /new_account on , mpc recovery endpoint

If you struggle to implement, please reach out to me, I have implemented few endpoints successfully from latest mpc recovery so I might be able to help. (I don't think my work is ready for PR and not sure if merging it right now would help this PR or not.)

Lastly, use https://mpc-recovery-leader-dev-7tk2cmmtcq-ue.a.run.app/ as endpoint for communicating with mpc recovery. This is the URL that contains latest work from mpc-service.

@esaminu esaminu force-pushed the 16-implement-create-account-route branch from 858ec87 to f1017c6 Compare August 7, 2023 19:42
@amirsaran3 amirsaran3 self-assigned this Aug 9, 2023
@amirsaran3 amirsaran3 linked an issue Aug 14, 2023 that may be closed by this pull request
@DavidM-D
Copy link
Contributor

How's this coming along @esaminu

Comment on lines +133 to +145
const checkPassKey = async (): Promise<void> => {
const isPasskeyReady = await isPassKeyAvailable();
if (!isPasskeyReady) {
openToast({
title: '',
type: 'INFO',
description:
'Passkey support is required for account creation. Try using an updated version of Chrome or Safari to create an account.',
duration: 5000,
});
}
};
checkPassKey();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is no longer there in the latest version of fast auth - we are just creating accounts with LAKs in this scenario so we would redirect to successUrl with added LAK in this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand this one. Do I just remove this code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will address this as a separate issue, created #35

@esaminu
Copy link
Collaborator

esaminu commented Aug 17, 2023

@amirsaran3 can you use the inIframe util I wrote to make this route unusable in an iframe by rendering an overlay with a "Continue on fast auth" button that will open the same route with all query params in a new tab using window.open with '_parent' as the second arg. This is because users cant createKey from within the iframe currently and we don't have a way update parent url without user interaction

Copy link
Collaborator

@esaminu esaminu left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@esaminu esaminu merged commit f0a2437 into add-device-route Aug 21, 2023
@esaminu esaminu deleted the 16-implement-create-account-route branch August 21, 2023 21:07
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.

🔷 Implement /create-account route
5 participants