[#18817] Import private key: UI for key pair name #18817 #19747
Conversation
Jenkins BuildsClick to see older builds (64)
|
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
4 times, most recently
from
e88bc35
to
bf387c3
Compare
Rende11
changed the title
Rende11
changed the title
src/status_im/contexts/wallet/add_account/create_account/events.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/events.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...tatus_im/contexts/wallet/add_account/create_account/import_private_key/private_key/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...tatus_im/contexts/wallet/add_account/create_account/import_private_key/private_key/view.cljs
Outdated
Show resolved
Hide resolved
...tatus_im/contexts/wallet/add_account/create_account/import_private_key/private_key/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/select_keypair/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/events.cljs
Outdated
Show resolved
Hide resolved
...im/contexts/wallet/add_account/create_account/import_private_key/add_key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
bf387c3
to
cfef154
Compare
| (rf/reg-event-fx | ||
| :wallet/set-private-key | ||
| (fn [{:keys [db]} [value]] | ||
| {:db (assoc-in db [:wallet :ui :import-private-key :private-key] value)})) |
There was a problem hiding this comment.
probably we need to use security/mask-data for the private-key
...tatus_im/contexts/wallet/add_account/create_account/import_private_key/private_key/view.cljs
Outdated
Show resolved
Hide resolved
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
77a5fa0
to
e7fa186
Compare
|
Thanks @Rende11 for addressing the comments. There is currently a conflict in the |
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
2 times, most recently
from
c09c632
to
0d22806
Compare
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
2 times, most recently
from
bd5e16a
to
5de289d
Compare
src/status_im/contexts/wallet/add_account/create_account/import_private_key/view.cljs
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/import_private_key/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/wallet/add_account/create_account/key_pair_name/view.cljs
Outdated
Show resolved
Hide resolved
| [{:keys [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) | ||
| ;; 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])))))) | ||
| 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))))))] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As I understand, set-flow-state provided from use-state hook never changes
There was a problem hiding this comment.
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 🦀
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
5de289d
to
399a44e
Compare
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
399a44e
to
76e3ffa
Compare
|
@status-im/mobile-qa - this pr will skip manual QA as it is all behind a feature flag 👍 |
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
76e3ffa
to
0d332e6
Compare
Move files Fix label Enable feature and fix auto-focus Add events and subs Some fixes Merge screens Fixes Minor fixes Some tweaks Fix feature flag
Rende11
force-pushed
the
18817-ui-key-pair-name
branch
from
0d332e6
to
a82a2b3
Compare
85% of end-end tests have passedFailed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
| 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) |
There was a problem hiding this comment.
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?
| [rn/view {:style (style/public-address flow-state theme)} | ||
| [quo/text "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045"]]]) | ||
| (when (seq input-value) | ||
| (when (seq public-address) |
There was a problem hiding this comment.
(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.
| :new-key-pair | ||
| (rf/dispatch [:wallet/new-keypair-continue | ||
| {:keypair-name key-pair-name}]) | ||
| (js/alert "Unknown workflow"))) |
There was a problem hiding this comment.
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
| (def ^:private key-pair-name-max-length 15) | ||
| (def ^:private key-pair-name-min-length 5) | ||
|
|
||
| (def error-messages |
| on-continue (rn/use-callback | ||
| (fn [_] | ||
| (case workflow | ||
| ;; TODO issue #19759. Implement creation account from |
There was a problem hiding this comment.
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.
| (when-not (empty? clipboard) | ||
| (on-change clipboard))))))] | ||
| [{:keys [input-value set-flow-state error?]}] | ||
| (let [check-address |
There was a problem hiding this comment.
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.
| (fn [v] | ||
| (if (empty? v) | ||
| (set-flow-state nil) | ||
| ;; TODO check for validation |
There was a problem hiding this comment.
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.
| {:keypair-name key-pair-name}]) | ||
| (js/alert "Unknown workflow"))) | ||
| [workflow key-pair-name]) | ||
| disabled? (or (some? error) (string/blank? key-pair-name))] |
There was a problem hiding this comment.
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.
| (set-key-pair-name value) | ||
| (cond | ||
| (> (count value) key-pair-name-max-length) | ||
| (set-error :too-long) |
There was a problem hiding this comment.
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.
|
Hey @Rende11 🙌! Thanks for your PR! |
|
hey @J-Son89 should we merge this? |
|
@flexsurfer - yes let's merge and I can address @ilmotta's comments in a follow up pr 👍 |
Fixes #18817
Summary
UI for adding key pair name, re-frame subs and events
Testing notes
Example of private key -
0xcaed41dd92c1548cf7536c290e6a1871757fb5fea5721dea3a08c6d4abcd16cf,result of account activity (no activity/has activity) is random now.
Public address of private key is hardcoded now.
!!! Important
It reuses
status-im.contexts.wallet.add-account.create-account.key-pair-name.viewscreen in thisimport private keyworkflow and innew key pair(check video)Before and after screenshots comparison
Design
!!! After updates
Simulator.Screen.Recording.-.iPhone.13.-.2024-05-03.at.03.04.03.mp4
|
|
|
|
|
|
|
|
|
status: ready