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

[#18817] Import private key: UI for key pair name #18817 #19747

Merged
merged 3 commits into from
May 13, 2024
Merged

[#18817] Import private key: UI for key pair name #18817 #19747

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a82a2b3
[#18817] Import private key: UI for key pair name
Rende11 Apr 20, 2024
067ae6f
Merge branch 'develop' into 18817-ui-key-pair-name
J-Son89 May 8, 2024
54f1b50
Merge branch 'develop' into 18817-ui-key-pair-name
J-Son89 May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/quo/components/text_combinations/standard_title/style.cljs
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@

(def container
{:flex-direction :row
:flex 1
:justify-content :space-between})

(def right-counter
Expand Down
15 changes: 15 additions & 0 deletions src/status_im/contexts/wallet/add_account/create_account/events.cljs
View file
Open in desktop
Expand Up @@ -90,3 +90,18 @@
(cske/transform-keys transforms/->kebab-case-keyword derived-address)))}))

(rf/reg-event-fx :wallet/get-derived-addresses-success get-derived-addresses-success)

(rf/reg-event-fx
:wallet/set-private-key
(fn [{:keys [db]} [value]]
{:db (assoc-in db [:wallet :ui :create-account :private-key] (security/mask-data value))}))

(rf/reg-event-fx
:wallet/set-public-address
(fn [{:keys [db]} [value]]
{:db (assoc-in db [:wallet :ui :create-account :public-address] value)}))

(rf/reg-event-fx
:wallet/clear-private-key-data
(fn [{:keys [db]} _]
{:db (update-in db [:wallet :ui :create-account] dissoc :private-key :public-address)}))
109 changes: 65 additions & 44 deletions src/status_im/contexts/wallet/add_account/create_account/import_private_key/view.cljs
View file
Open in desktop
@@ -1,5 +1,6 @@
(ns status-im.contexts.wallet.add-account.create-account.import-private-key.view
(:require
[clojure.string :as string]
[quo.core :as quo]
[quo.theme :as theme]
[react-native.clipboard :as clipboard]
Expand All @@ -12,45 +13,50 @@
[utils.re-frame :as rf]))

(defn- address-input
[{:keys [input-value set-input-value set-flow-state error?]}]
(let [check-address (rn/use-callback
(debounce/debounce (fn [v]
(if (empty? v)
(set-flow-state nil)
;; TODO check for validation
(if-not (v/private-key? v)
(set-flow-state :invalid-private-key)
;; TODO add real requests
(do
(set-flow-state :scanning)
(js/setTimeout
set-flow-state
400
(rand-nth [:active-address :inactive-address]))))))
500))
on-change (rn/use-callback
(fn [v]
(set-input-value v)
(check-address v)))
on-paste (rn/use-callback
(fn []
(clipboard/get-string
(fn [clipboard]
(when-not (empty? clipboard)
(on-change clipboard))))))]
[{:keys [input-value set-flow-state error?]}]
(let [check-address
Copy link
Contributor

Choose a reason for hiding this comment

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

This check-address function is quite confusing to me with the TODO comments and hardcoded values. It's something we would not usually approve for merge in the past.

We've done this before, I mean, when we need to implement client features before the backend supports them, we can write re-frame subs/events/etc as if the backend is ready even though it isn't. Then, in the near future, it'll be cheaper to plug in the real data. The re-frame architecture is perfect for that. IMO it's also a better practice because it leads to higher quality client code.

I guess this strategy should've been aligned before any implementation was done. Anyway, just a light suggestion for refactor because I know you inherited the implementation like this before working on this PR, so you're just following the flow.

(rn/use-callback
(debounce/debounce
(fn [v]
(if (empty? v)
(set-flow-state nil)
;; TODO check for validation
Copy link
Contributor

Choose a reason for hiding this comment

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

Following our guidelines for TODOs, I think these TODOs should be deleted and instead, whatever is left to do after this PR should be explained in a new or existing issue.

(if-not (v/private-key? v)
(set-flow-state :invalid-private-key)
;; TODO add real requests
(do
(set-flow-state :scanning)
;; TODO get real address
;; Should be fixed in #18819
(rf/dispatch [:wallet/set-public-address
"0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045"])
(js/setTimeout set-flow-state 400 (rand-nth [:active-address :inactive-address]))))))
seanstrom marked this conversation as resolved.
Show resolved Hide resolved
500))

on-change (rn/use-callback
(fn [v]
(rf/dispatch [:wallet/set-private-key v])
(check-address v)))
on-paste (rn/use-callback
(fn []
(clipboard/get-string
(fn [clipboard]
(when-not (empty? clipboard)
(on-change clipboard))))))]
Comment on lines +16 to +45
Copy link
Member

Choose a reason for hiding this comment

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

Small tweak 🔧

We may want to update these use-callback usages and provide them with their dependency arrays (?). For example: check-address depends on set-flow-state, on-change depends on check-address, and on-paste depends on on-change.

Copy link
Collaborator Author

@Rende11 Rende11 May 7, 2024 •
edited

Choose a reason for hiding this comment

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

As I understand, set-flow-state provided from use-state hook never changes

Copy link
Member

Choose a reason for hiding this comment

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

adding use-callback in this way is questionable enough already, I think there is even further questions to adding state setters in the dependency array as I can only assume React has already considered this to some extent and as @Rende11 pointed out this method never changes so there is most likely no value in doing so 👍
If anything we are just making the code really hard to read 🦀

[quo/input
{:accessibility-label :add-address-to-watch
{:accessibility-label :import-private-key
:placeholder (i18n/label :t/enter-private-key-placeholder)
:container-style style/input
:label (i18n/label :t/private-key)
:type :password
:error? error?
:return-key-type :done
:auto-focus true
:on-change-text on-change
:button (when (empty? input-value)
{:on-press on-paste
:text (i18n/label :t/paste)})
:value input-value}]))
:default-value input-value}]))

(defn- activity-indicator
[state]
Expand Down Expand Up @@ -84,21 +90,37 @@
:style style/indicator)
(i18n/label message)])))

(defn on-unmount
[]
(rf/dispatch [:wallet/clear-private-key-data]))

(defn navigate-back
[]
(rf/dispatch [:navigate-back]))

(defn navigate-to-keypair-import
[]
(rf/dispatch [:navigate-to
:screen/wallet.keypair-name
{:workflow :import-private-key}]))

(defn view
[]
(let [theme (theme/use-theme)
customization-color (rf/sub [:profile/customization-color])
[input-value set-input-value] (rn/use-state "")
[flow-state set-flow-state] (rn/use-state)
error? (= :invalid-private-key flow-state)]
(let [theme (theme/use-theme)
customization-color (rf/sub [:profile/customization-color])
private-key (rf/sub [:wallet/import-private-key])
public-address (rf/sub [:wallet/public-address])
[flow-state set-flow-state] (rn/use-state nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what a nil flow-state means. Nil is generally overloaded in meaning. What do you think about using an explicit value that's self-explanatory?

error? (= :invalid-private-key flow-state)]
(rn/use-unmount on-unmount)
[rn/view {:flex 1}
[floating-button-page/view
{:customization-color customization-color
:header [quo/page-nav
{:background :white
:type :no-title
:icon-name :i/close
:on-press #(rf/dispatch [:navigate-back])}]
:on-press navigate-back}]
:footer [:<>
(when-not (#{:active-address :inactive-address} flow-state)
[quo/information-box
Expand All @@ -108,26 +130,25 @@
(i18n/label :t/import-private-key-info)])
[quo/button
{:customization-color customization-color
:disabled? (or (empty? input-value) error?)
:on-press #()}
:disabled? (or (string/blank? private-key) error?)
:on-press navigate-to-keypair-import}
(i18n/label :t/continue)]]}
[quo/page-top
{:container-style style/page-top
:title (i18n/label :t/import-private-key)
:description :text
:description-text (i18n/label :t/enter-private-key)}]
[address-input
{:input-value input-value
:set-input-value set-input-value
:set-flow-state set-flow-state
:error? error?}]
{:input-value private-key
:set-flow-state set-flow-state
:error? error?}]
(when (#{:scanning :active-address :inactive-address} flow-state)
[rn/view {:style style/key-section}
[quo/section-label
{:section (i18n/label :t/private-key-public-address)
:container-style style/section-label}]
;; TODO Get real address
[rn/view {:style (style/public-address flow-state theme)}
[quo/text "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045"]]])
(when (seq input-value)
(when (seq public-address)
Copy link
Contributor

Choose a reason for hiding this comment

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

(seq some-string) is slower than using (not string/blank? some-string). We can always say this is unimportant, but it's a better default in our slow mobile universe. In Clojure on the JVM the drop in performance is even more pronounced.

I'd say string/blank? is also generally clearer in intention. There's no guessing about the type of argument passed to it. Here I guess it's fine because we can guess public-address is a string.

Side note: historically in PRs, I've also seen the result of (seq some-string) be passed over to components as if it's a boolean, but this is also costly and will cause Reagent to spend more time figuring out if component args are different.

[rn/view {:style (style/public-address flow-state theme)}
[quo/text public-address]])])
(when (seq private-key)
[activity-indicator flow-state])]]))
12 changes: 12 additions & 0 deletions src/status_im/contexts/wallet/add_account/create_account/key_pair_name/style.cljs
View file
Open in desktop
@@ -0,0 +1,12 @@
(ns status-im.contexts.wallet.add-account.create-account.key-pair-name.style)

(def page-top
{:margin-top 2})

(def input
{:padding-top 12
:padding-horizontal 20})

(def error-container
{:padding-horizontal 20
:padding-vertical 8})
94 changes: 94 additions & 0 deletions src/status_im/contexts/wallet/add_account/create_account/key_pair_name/view.cljs
View file
Open in desktop
@@ -0,0 +1,94 @@
(ns status-im.contexts.wallet.add-account.create-account.key-pair-name.view
(:require
[clojure.string :as string]
[quo.core :as quo]
[react-native.core :as rn]
[status-im.common.floating-button-page.view :as floating-button-page]
[status-im.common.not-implemented :as not-implemented]
[status-im.common.validation.general :as validators]
[status-im.contexts.wallet.add-account.create-account.key-pair-name.style :as style]
[utils.i18n :as i18n]
[utils.re-frame :as rf]))

(def ^:private key-pair-name-max-length 15)
(def ^:private key-pair-name-min-length 5)

(def error-messages
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be private.

{:too-long (i18n/label :t/key-name-error-length)
:too-short (i18n/label :t/key-name-error-too-short {:count key-pair-name-min-length})
:emoji (i18n/label :t/key-name-error-emoji)
:special-char (i18n/label :t/key-name-error-special-char)})

(defn navigate-back
[]
(rf/dispatch [:navigate-back]))

(defn view
[]
(let [{:keys [workflow]} (rf/sub [:get-screen-params])
customization-color (rf/sub [:profile/customization-color])
[key-pair-name set-key-pair-name] (rn/use-state "")
[error set-error] (rn/use-state nil)
on-change-text (rn/use-callback
(fn [value]
(set-key-pair-name value)
(cond
(> (count value) key-pair-name-max-length)
(set-error :too-long)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, I think we can separate side-effectful code from pure code.

My suggestion here is to define a pure function validate-key-pair-name as a separate var that returns the type of error or nil. Then, your on-change-text will be just two nice lines:

(rn/use-callback
 (fn [value]
   (set-key-pair-name value)
   (set-error (validate-key-pair-name value))))

After this new function is defined, I would unit test it in a file named view_test.cljs.

Another interesting option is to completely extract the validation part from the view namespace and move them to a validation.cljs file inside key_pair_name/. That way the design sort of forces developers to keep pure code more separated from side-effects and also creates a stronger incentive to unit test the behavior because in view namespaces there's a strong tendency to not unit test anything there.


(< 0 (count value) key-pair-name-min-length)
(set-error :too-short)

(validators/has-emojis? value)
(set-error :emoji)

(validators/has-special-characters? value)
(set-error :special-char)

:else (set-error nil))))
on-continue (rn/use-callback
(fn [_]
(case workflow
;; TODO issue #19759. Implement creation account from
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not in favor of adding TODO comments in the code according to our guidelines https://github.com/status-im/status-mobile/blob/497c95fd2682653472722f7fcc13ec74e244318e/doc/new-guidelines.md#using-todos-comments.

;; private key
:import-private-key
(not-implemented/alert)

:new-key-pair
(rf/dispatch [:wallet/new-keypair-continue
{:keypair-name key-pair-name}])
(js/alert "Unknown workflow")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we alert the user of an unknown workflow? What will they do with the popup? In theory, I imagine in production an unhandled workflow would be considered a bug? @J-Son89

[workflow key-pair-name])
disabled? (or (some? error) (string/blank? key-pair-name))]
Copy link
Contributor

Choose a reason for hiding this comment

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

A light suggestion: disabled? is a name coupled with what the footer expects, but this is not necessarily obvious because it requires the dev to jump down a few lines. If we name the binding invalid-key-name? then I think we decouple them more and the binding has meaning on its own, which helps read the code line by line.

[rn/view {:style {:flex 1}}
[floating-button-page/view
{:customization-color customization-color
:header [quo/page-nav
{:icon-name :i/arrow-left
:on-press navigate-back
:accessibility-label :top-bar}]
:footer [quo/button
{:customization-color customization-color
:disabled? disabled?
:on-press on-continue}
(i18n/label :t/continue)]}
[quo/page-top
{:title (i18n/label :t/keypair-name)
:description-text (i18n/label :t/keypair-name-description)
:description :text
:container-style style/page-top}]
[quo/input
{:container-style style/input
:placeholder (i18n/label :t/keypair-name-input-placeholder)
:label (i18n/label :t/keypair-name)
:char-limit key-pair-name-max-length
:auto-focus true
:on-change-text on-change-text
:error error}]
(when error
[quo/info-message
{:type :error
:size :default
:icon :i/info
:container-style style/error-container}
(get error-messages error)])]]))
3 changes: 2 additions & 1 deletion ...tus_im/contexts/wallet/add_account/create_account/new_keypair/check_your_backup/view.cljs
View file
Open in desktop
Expand Up @@ -81,7 +81,8 @@
(reset! show-error? false)
(when (= @quiz-index questions-count)
(rf/dispatch [:navigate-to
:screen/wallet.keypair-name])))
:screen/wallet.keypair-name
{:workflow :new-key-pair}])))
(do
(when (> @incorrect-count 0)
(rf/dispatch [:show-bottom-sheet
Expand Down
13 changes: 0 additions & 13 deletions src/status_im/contexts/wallet/add_account/create_account/new_keypair/keypair_name/style.cljs
View file
Open in desktop

This file was deleted.