Add mnemonic validation #9648
Add mnemonic validation #9648
Conversation
Pull Request Checklist
|
|
@yenda @flexsurfer This is just a start on a PR to use the new status-go |
Jenkins BuildsClick to see older builds (30)
|
|
Status-go BIP39 PR is now merged. So...how long until that gets folded into the version of |
|
@acolytec3 Hi nice to see you on another issue! So there is two ways status-react communicates with status-go:
When you are using an rpc method, you don't need to write any new native code. But these only work when a node is started, which is not the case here. So the mnemonic validation is implemented as a direct call, which means you need to write some native code. This is not very hard though, because you can basically mimic what is done with other calls. The module is in https://github.com/status-im/status-react/tree/develop/modules/react-native-status For Android it's this file, in which I highlighted the kind of method you want to mimic: https://github.com/status-im/status-react/blob/develop/modules/react-native-status/android/src/main/java/im/status/ethereum/module/StatusModule.java#L1194-L1215 Same for iOS: |
|
@yenda Thanks for the guidance. I added the native code that seems to be necessary for Android and now Gradle is throwing the below error. This seems to indicate to me that Statusgo still isn't being compiled from the correct source since it can't find the To make sure I'm doing this right, here are my build steps:
Is there something else I'm missing in terms of where I need to add references to |
|
hey @acolytec3 are you sure your go version is used ? try this |
|
or do you know if you made changes for ValidateMnemonic method in status-go so its available in status-react? im not familiar with status-go , might be @cammellos could help |
|
@flexsurfer I think our responses crossed in the ether :-) See my response above. I outlined the steps I'm currently taking which include the one you suggested. And yes, I did add ValidateMnemonic to the @cammellos Any thoughts on my process above? Is there something else that you see I'm missing? |
|
@acolytec3 i mean you need to export variable for |
|
@flexsurfer Oh, so I have to do it explicitly in |
|
@acolytec3 since the method you want is already merged in status-go develop you should just rebase your branch and it should be included because I updated status-go version yesterday after merging my account PR |
|
@yenda Thanks for calling that out. I've got it working locally now and should have the basics of the PR done tomorrow morning if life allows. |
|
@yenda @flexsurfer @errorists @rasom Is below the error message we want to use when a mnemonic doesn't pass |
|
@acolytec3 the goal of this task to prevent importing custom phrases, so we need to change this popup, @andmironov do we have a design? @acolytec3, for now, i would just remove "continue" button and last sentence "If so, you'll...." |
|
Only this, not sure about the copy.
Figma: https://www.figma.com/file/dEIljL7UPbXgsZUA0Q4qlE5E/Onboarding?node-id=4540%3A11552 |
acolytec3
force-pushed
the
add-mnemonic-validation
branch
from
1058408 to
e7dee19
Compare
|
@yenda @flexsurfer @andmironov Here's what it looks like with the language removed from above. This look good? |
|
@yenda @flexsurfer @rasom This is now working locally and fixes both #9050 and #9670. Can someone tell me what the jenkins errors are keeping it from being built? |
|
@acolytec3 to fix builds you need to rebase onto the latest develop |
|
@hesterbruikman @rachelhamlin could pls take a look #9648 (comment) |
acolytec3
force-pushed
the
add-mnemonic-validation
branch
2 times, most recently
from
44962ba to
6efdaa6
Compare
|
Everything is rebased on current develop and Jenkins buiilds are completing so should be ready for testing. Thanks all! |
|
I'm assuming that second failed e2e test needs to be rewritten since we're no longer allowing custom seed phrases, right? |
Correct, i'll take care about that separately, @acolytec3 Tested with Android and iOS latest builds (Android one is https://status-im-prs.ams3.digitaloceanspaces.com/StatusIm-191231-105400-6efdaa-pr-universal.apk), all good with word count/entropy validation against BEP39, but we still don't trim seed phrase it seems. I.e. can reproduce #9670 bug: if whitespace or newline or whispace-in-between seed phrase is present - it leads to recovery of different account
2a) Enter 2b) Enter 2c) Enter 2d) Enter
|
|
@Serhy Thanks for validating. I'll take a look. I only tried variation 2b, that you mentioned above and my edits seemed to work when I restored a known account. That said, should be pretty straight forward to resolve and sanitize extra white space/newlines so will work on the method that sanitizes excess whitespace and push a new commit tonight or tomorrow. |
acolytec3
force-pushed
the
add-mnemonic-validation
branch
from
6efdaa6 to
1e171f7
Compare
|
@Serhy Please review my latest commit. Looks like the change that I had made previously to sanitize the seed phrases of newlines/spaces got reverted. It's been added back and appears to work as expected. |
|
Looks good to me now! Passphrase validated for checksum and whether any of words is not from BIP39, whitespace/new-line between words or at the end/beginning of seed phrase have no impact and appropriate account is recovered |
acolytec3
force-pushed
the
add-mnemonic-validation
branch
from
1e171f7 to
f309d6e
Compare
|
@yenda Done. Also rebased on current develop. |
Signed-off-by: yenda <eric@status.im>
yenda
force-pushed
the
add-mnemonic-validation
branch
from
f309d6e to
3bcf7ec
Compare
@rachelhamlin PR has been merged. Any chance you could just add a bounty on gitcoin and I'll submit this PR for it? |
Fixes #9050 and #9670
Summary
In account recovery,
status-reactdoesn't currently validate that a mnemonic seed phrase only contains words from the BIP39 dictionary and also doesn't validate the checksum. This PR will ultimately replace the existing mnemonic validation code in the account-recovery flow with a single call tovalidate-mnemonicfrom status-go that validates the checksum appropriately.Testing notes
Needs to be tested on iOS as I do not have a Mac or iPhone so can't test the iOS native code.
Platforms
Areas that maybe impacted
Account onboarding
Functional
Steps to test
For #9050
For #9670
status: ready