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

Add unit tests to make sure we are recovering correct account in keycard flows #22209

Open
Parveshdhull opened this issue Feb 28, 2025 · 2 comments

Comments

@Parveshdhull
Copy link
Member

Parveshdhull commented Feb 28, 2025

Problem

Prerequisite:

Image

Result:

Same seed phrase different account.

signal-2025-02-28-152219.mp4

Consequences

With this small change/bug, the user may recover the wrong account using the same seed phrase. As a result, they might request funds or take other actions on an account whose seed phrase is unknown to them.

We need to add tests (possibly also in the onboarding flow) to ensure that the correct account is recovered.

If provided value is not vector, after above change, seed phrase becomes this

Old: start course intact drastic coffee heavy orphan all barrel quarter allow pupil
new: s t a r t   c o u r s e   i n t a c t   d r a s t i c   c o f f e e   h e a v y   o r p h a n   a l l   b a r r e l   q u a r t e r   a l l o w   p u p i l

Anyone knows why we are still able to create account after this change, and why it is different? cc @flexsurfer @ilmotta

@Parveshdhull
Copy link
Member Author

I think we should not ignore this by considering, why we are making above change, if its not vector.

While developing, we might not always be fully aware of the nature of the data. For example, in the same file, we have many instances of masked-seed-phrase and masked-phrase-vector. It's unclear why some events return a vector and others return a string. A developer might accidentally copy-paste vector-related code and use it for a string. I personally did this when developing the recovery phrase flow for Keycard. However, since this was a sensitive area, I double-checked to ensure that the recovery phrase was recovering the same account in both flows (onboarding and Keycard), which helped me avoid this bug.

@flexsurfer
Copy link
Member

Thank you very much for raising this issue, these are very important points

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

2 participants