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

Add seed phrase validation for keycard and status-go calls #22269

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+12 −7
Open

Add seed phrase validation for keycard and status-go calls #22269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/status_im/common/standard_authentication/events_test.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
(ns status-im.common.standard-authentication.events-test
(:require [cljs.test :refer [deftest is are testing]]
(:require [cljs.test :refer [are deftest is testing]]
matcher-combinators.test
[status-im.common.standard-authentication.events :as sut]))

Expand Down
8 changes: 6 additions & 2 deletions src/status_im/contexts/keycard/effects.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,12 @@
(keycard/init-card (keycard.utils/wrap-handlers args))))

(rf/reg-fx :effects.keycard/generate-and-load-key
(fn [args]
(keycard/generate-and-load-key (keycard.utils/wrap-handlers args))))
(fn [{:keys [mnemonic on-failure] :as args}]
(-> mnemonic
(native-module/validate-mnemonic)
(promesa/then #(keycard/generate-and-load-key
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to add the mnemonic validation directly in the function generate-and-load-key? Otherwise, all devs will need to remember to call it before generate-and-load-key. Or another way to frame the question @Parveshdhull, do you think there's any situation where we shouldn't validate the mnemonic before calling generate-and-load-key?

Same question for the function create-account-from-mnemonic, is there any scenario where we don't want to validate the mnemonic? If the answer is no, then it's better to call native-module/validate-mnemonic inside native-module/create-account-from-mnemonic.

Copy link
Member Author

Choose a reason for hiding this comment

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

hi @ilmotta, Thank you very much for reviewing the PR.

Wouldn't it be better to add the mnemonic validation directly in the function generate-and-load-key?

This is how I originally implemented validation, please check out #22269 (comment)

(keycard.utils/wrap-handlers args)))
(promesa/catch on-failure))))

(rf/reg-fx :effects.keycard/login-with-keycard
(fn [{:keys [key-uid password whisper-private-key]}]
Expand Down
9 changes: 5 additions & 4 deletions src/status_im/contexts/wallet/effects.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@
string? mnemonic-phrase
coll? (string/join " " mnemonic-phrase)
(log/error "Unexpected value " mnemonic-phrase))]
(native-module/create-account-from-mnemonic
{:MnemonicPhrase phrase
:paths paths}
on-success))))
(-> phrase
(native-module/validate-mnemonic)
(promesa/then #(native-module/create-account-from-mnemonic
{:MnemonicPhrase phrase :paths paths}
on-success))))))

(defn create-account-from-private-key
[private-key]
Expand Down