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.

Sign up for GitHub

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

Jump to bottom

Add verification flow #1827

Merged
merged 4 commits into from
Sep 5, 2024
Merged

Add verification flow #1827

merged 4 commits into from
Sep 5, 2024

Conversation

OKendigelyan
Copy link
Contributor

@OKendigelyan OKendigelyan commented Sep 2, 2024 •
edited
Loading

Proposed changes

Task link

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

Screenshots

Add the screenshots of how the app used to look like and how it looks now

Screen.Recording.2024-09-04.at.11.55.21.mov

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

@vercel Vercel
Copy link

vercel bot commented Sep 2, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
umami-v2-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 10:07am
umami-v2-web-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 10:07am

@OKendigelyan OKendigelyan marked this pull request as ready for review September 4, 2024 10:51
@OKendigelyan OKendigelyan force-pushed the add-verification-flow branch 2 times, most recently from 47259df to 589e716 Compare September 4, 2024 14:19
serjonya-trili
serjonya-trili requested changes Sep 4, 2024
View reviewed changes
apps/web/src/components/Onboarding/SetupPassword/SetupPassword.tsx Outdated
const restoreFromMnemonic = useRestoreFromMnemonic();
const restoreFromSecretKey = useRestoreFromSecretKey();
const checkPassword = useValidateMasterPassword();
const getNextAvailableAccountLabels = useGetNextAvailableAccountLabels();
const isPasswordSet = useIsPasswordSet();
const getDecryptedMnemonic = useGetDecryptedMnemonic();
const currentAccount = useCurrentAccount() as MnemonicAccount;
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a very brave type cast, please don't do that

apps/web/src/components/Onboarding/SetupPassword/SetupPassword.tsx Outdated
case "verification": {
const mnemonic = await getDecryptedMnemonic(currentAccount, password);

return openWith(<ImportantNoticeModal seedPhrase={mnemonic} />, { size: "xl" });
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency, maybe it's better to use just 1 thing - either seedphrase or mnemonic

apps/web/src/components/Onboarding/VerificationFlow/VerificationInfoModal.tsx
import { useHandleVerify } from "./useHandleVerify";
import { AlertIcon } from "../../../assets/icons";
import { useColor } from "../../../styles/useColor";
import { ModalCloseButton } from "../../CloseButton";

// TODO: Replace with actual copy paste
Copy link
Contributor

Choose a reason for hiding this comment

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

haven't you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't get the text content yet

apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx
} = form;

const seedphraseArray = seedPhrase.split(" ");
const [randomElements] = useState(selectRandomElements(seedphraseArray, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

useMemo?

Copy link
Contributor Author

@OKendigelyan OKendigelyan Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

basically they both do the same thing in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly. But we don't use scissors to cut bread even though, technically, it's possible

apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx
<form onSubmit={handleSubmit(onSubmit)}>
<ModalBody>
<Flex gap="12px">
{randomElements.map(({ index, value }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you either define the type for the form like
useForm<{word1: string, word2: string, word3: string}> or use useFieldArray. as you can clearly see, the crutches are hard to deal with

apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx Outdated

return (
<FormControl key={index} isInvalid={!!error}>
<Flex key={index} alignItems="center">
Copy link
Contributor

Choose a reason for hiding this comment

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

doubled key?

apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx Outdated
textAlign="right"
size={{ lg: "lg", base: "xs" }}
>
{String(index + 1).padStart(2, "0")}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw a very similar component "somewhere", please eliminate the duplication and create a new component

apps/web/src/components/Onboarding/VerificationFlow/useHandleVerify.test.tsx Outdated
});

it("should open ImportantNoticeModal modal if master password is set", async () => {
(useGetDecryptedMnemonic as jest.Mock).mockImplementation(() => () => mnemonic1);
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid using type casting, use jest.mocked instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is used here on purpose to avoid unnecessary type checking when mocking implementation

@OKendigelyan OKendigelyan force-pushed the add-verification-flow branch 2 times, most recently from f68dc55 to 6cdcba1 Compare September 4, 2024 18:31
serjonya-trili
serjonya-trili reviewed Sep 4, 2024
View reviewed changes
apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx
})
);

dispatch(accountsActions.setPassword(""));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to use null/undefined here? Empty string is still a string, so, technically, the password is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is default value for password. Technically empty string is falsy, assuming no password is set

serjonya-trili
serjonya-trili reviewed Sep 4, 2024
View reviewed changes
apps/web/src/components/Onboarding/VerificationFlow/VerifySeedphraseModal.tsx Outdated
index={index}
/>
{error?.message && (
<FormErrorMessage>{(error as any).message}</FormErrorMessage>
Copy link
Contributor

Choose a reason for hiding this comment

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

As any is not needed here

serjonya-trili
serjonya-trili reviewed Sep 4, 2024
View reviewed changes
apps/web/src/components/Onboarding/VerificationFlow/useHandleVerify.test.tsx Outdated
const { openWith } = dynamicModalContextMock;
const { result } = renderHook(useHandleVerify, { store });

await act(async () => await result.current());
Copy link
Contributor

Choose a reason for hiding this comment

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

Async await is redundant

@OKendigelyan OKendigelyan merged commit e72eb4e into main Sep 5, 2024
6 checks passed
@OKendigelyan OKendigelyan deleted the add-verification-flow branch September 5, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serjonya-trili serjonya-trili serjonya-trili approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OKendigelyan @serjonya-trili