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

Wallet: Password field contains saved data once user put it before while transaction confirmation #18447 #18899

Closed
wants to merge 14 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -2,51 +2,59 @@
(:require
[quo.core :as quo]
[react-native.core :as rn]
[reagent.core :as reagent]
[status-im.common.standard-authentication.enter-password.style :as style]
[status-im.common.standard-authentication.password-input.view :as password-input]
[status-im.contexts.profile.utils :as profile.utils]
[utils.i18n :as i18n]
[utils.re-frame :as rf]))

(defn view
[{:keys [on-enter-password on-press-biometrics button-label button-icon-left]}]
(let [{:keys [key-uid customization-color] :as profile} (rf/sub [:profile/profile-with-image])
{:keys [error processing password]} (rf/sub [:profile/login])
sign-in-enabled? (rf/sub [:sign-in-enabled?])]
[:<>
[rn/view {:style style/enter-password-container}
[rn/view
[quo/text
{:accessibility-label :sync-code-generated
:weight :bold
:size :heading-1
:style {:margin-bottom 4}}
(i18n/label :t/enter-password)]
[rn/view
{:style style/context-tag}
[quo/context-tag
{:type :default
:blur? true
:profile-picture (profile.utils/photo profile)
:full-name (profile.utils/displayed-name profile)
:customization-color customization-color
:size 24}]]
[password-input/view
{:on-press-biometrics on-press-biometrics
:processing processing
:error error
:default-password password
:sign-in-enabled? sign-in-enabled?}]
[quo/button
{:size 40
:container-style style/enter-password-button
:type :primary
:customization-color (or customization-color :primary)
:accessibility-label :login-button
:icon-left button-icon-left
:disabled? (or (not sign-in-enabled?) processing)
:on-press (fn []
(rf/dispatch [:set-in [:profile/login :key-uid] key-uid])
(rf/dispatch [:profile.login/verify-database-password password
#(on-enter-password password)]))}
button-label]]]]))
[_]
(let [password-state (reagent/atom nil)]
(fn [{:keys [on-enter-password on-press-biometrics button-label button-icon-left]}]
(let [{:keys [key-uid customization-color]
:as profile} (rf/sub [:profile/profile-with-image])
{:keys [error processing password]} (rf/sub [:profile/login])
sign-in-enabled? (rf/sub [:sign-in-enabled?])
on-change-password (fn [value]
(when (not= value @password-state)
(reset! password-state value)
(rf/dispatch [:standard-auth/update-password
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are talking more about performance these days, I'd like to mention a few things for this enter-password component I deem significant and that appear in many of our PRs. In no order of importance:

  1. There's an unnecessary overhead to involve re-frame on every key press. Why are we committing to the app-db all the time?
  2. Because we dispatch :standard-auth/update-password on every key press, this forces the parent view component to be rendered twice, for every key press. This problem cascades to its children password-input/view and quo/button. input/view itself is quite heavy. We can save a bunch of CPU cycles if we process the parent view component and its children only once on every key press.
  3. The props passed to the button component are different on every Reagent pass because the on-press function is recreated over and over. This forces the button component to be re-processed by Reagent on every pass of the parent component.

Things that would allow us to render all components in enter-password once on every key press:

  • Use local state and don't update the app-db at paths [:profile/login :password] and [:profile/login :error] on every key press. Use local state and then the whole enter-password component and its children would render once. The global state should be left for things that change less often.
  • The profile key-uid doesn't change when the user is typing, so it can be moved outside the renderer, and then we can make the on-press of the button be created only once. This would allow Reagent to skip processing the button component entirely on every key press. The button component is not the cheapest too if we look at its implementation.

cc @flexsurfer A slightly different example than what we were discussing today in the perf channel. Still, a good example where multiple Reagent passes cascade down the component tree (and I haven't checked the other subcomponents, but on the surface they also have low-hanging fruits to optimize).

value])))]
[rn/view {:style style/enter-password-container}
[quo/text
{:accessibility-label :sync-code-generated
:weight :bold
:size :heading-1
:style {:margin-bottom 4}}
(i18n/label :t/enter-password)]
[rn/view
{:style style/context-tag}
[quo/context-tag
{:type :default
:blur? true
:profile-picture (profile.utils/photo profile)
:full-name (profile.utils/displayed-name profile)
:customization-color customization-color
:size 24}]]
[password-input/view
{:on-press-biometrics on-press-biometrics
:processing processing
:error error
:password @password-state
:on-change-password on-change-password
:sign-in-enabled? sign-in-enabled?}]
[quo/button
{:size 40
:container-style style/enter-password-button
:type :primary
:customization-color (or customization-color :primary)
:accessibility-label :login-button
:icon-left button-icon-left
:disabled? (or (not sign-in-enabled?) processing)
:on-press (fn []
(rf/dispatch [:set-in [:profile/login :key-uid] key-uid])
(rf/dispatch [:profile.login/verify-database-password password
#(on-enter-password password)]))}
button-label]]))))
10 changes: 9 additions & 1 deletion src/status_im/common/standard_authentication/events.cljs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
(ns status-im.common.standard-authentication.events
(:require
[utils.re-frame :as rf]))
[utils.re-frame :as rf]
[utils.security.core :as security]))

(rf/reg-event-fx :standard-auth/on-biometric-success
(fn [{:keys [db]} [callback]]
Expand All @@ -11,3 +12,10 @@
:standard-auth/reset-login-password
(fn [{:keys [db]}]
{:db (update db :profile/login dissoc :password :error)}))

(rf/reg-event-fx
:standard-auth/update-password
(fn [{:keys [db]} [value]]
{:db (-> db
(assoc-in [:profile/login :password] (security/mask-data value))
(assoc-in [:profile/login :error] ""))}))
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
[react-native.core :as rn]
[status-im.common.standard-authentication.forgot-password-doc.view :as forgot-password-doc]
[status-im.common.standard-authentication.password-input.style :as style]
[utils.debounce :as debounce]
[utils.i18n :as i18n]
[utils.re-frame :as rf]
[utils.security.core :as security]))
[utils.re-frame :as rf]))

(defn get-error-message
[error]
Expand All @@ -21,15 +19,8 @@
(i18n/label :t/oops-wrong-password)
error))

(defn- on-change-password
[entered-password]
(debounce/debounce-and-dispatch [:profile/on-password-input-changed
{:password (security/mask-data entered-password)
:error ""}]
100))

(defn- view-internal
[{:keys [default-password theme shell? on-press-biometrics blur?]}]
[{:keys [password on-change-password theme shell? on-press-biometrics blur?]}]
(let [{:keys [error processing]} (rf/sub [:profile/login])
error-message (get-error-message error)
error? (boolean (seq error-message))]
Expand All @@ -45,7 +36,7 @@
:error? error?
:label (i18n/label :t/profile-password)
:on-change-text on-change-password
:default-value (security/safe-unmask-data default-password)}]
:value password}]
(when on-press-biometrics
[quo/button
{:container-style style/auth-button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
(defn- view-internal
[_]
(let [reset-slider? (reagent/atom false)
on-close (fn []
reset-slider (fn []
(js/setTimeout
#(reset! reset-slider? true)
200))
Expand All @@ -26,14 +26,18 @@
size
theme
blur?
on-close
container-style]
:or {container-style {:flex 1}}}]
[rn/view {:style container-style}
[quo/slide-button
{:size size
:customization-color customization-color
:on-reset (when @reset-slider? #(reset! reset-slider? false))
:on-complete #(authorize/authorize {:on-close on-close
:on-complete #(authorize/authorize {:on-close (fn []
(when (fn? on-close)
(on-close))
(reset-slider))
:auth-button-icon-left auth-button-icon-left
:theme theme
:blur? blur?
Expand Down
126 changes: 76 additions & 50 deletions src/status_im/contexts/profile/profiles/view.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
[utils.debounce :as debounce]
[utils.i18n :as i18n]
[utils.re-frame :as rf]
[utils.security.core :as security]
[utils.transforms :as transforms]))

(defonce push-animation-fn-atom (atom nil))
Expand Down Expand Up @@ -190,56 +191,81 @@
:default-password password}]))

(defn login-section
[{:keys [set-show-profiles]}]
(let [processing (rf/sub [:profile/login-processing])
{:keys [key-uid name customization-color]} (rf/sub [:profile/login-profile])
sign-in-enabled? (rf/sub [:sign-in-enabled?])
profile-picture (rf/sub [:profile/login-profiles-picture key-uid])
login-multiaccount #(rf/dispatch [:profile.login/login])]
[rn/keyboard-avoiding-view
{:style style/login-container
:keyboardVerticalOffset (- (safe-area/get-bottom))}
[rn/view
{:style style/multi-profile-button-container}
(when config/quo-preview-enabled?
[quo/button
{:size 32
:type :grey
:background :blur
:icon-only? true
:on-press #(rf/dispatch [:navigate-to :quo-preview])
:disabled? processing
:accessibility-label :quo-preview
:container-style {:margin-right 12}}
:i/reveal-whitelist])
[quo/button
{:size 32
:type :grey
:background :blur
:icon-only? true
:on-press set-show-profiles
:disabled? processing
:accessibility-label :show-profiles}
:i/multi-profile]]
[rn/scroll-view
{:keyboard-should-persist-taps :always
:style {:flex 1}}
[quo/profile-card
{:name name
:customization-color (or customization-color :primary)
:profile-picture profile-picture
:card-style style/login-profile-card}]
[password-input]]
[quo/button
{:size 40
:type :primary
:customization-color (or customization-color :primary)
:accessibility-label :login-button
:icon-left :i/unlocked
:disabled? (or (not sign-in-enabled?) processing)
:on-press login-multiaccount
:container-style {:margin-bottom (+ (safe-area/get-bottom) 12)}}
(i18n/label :t/log-in)]]))
[]
(let [password-value (reagent/atom nil)]
(fn [{:keys [set-show-profiles]}]
(let [{:keys [processing password]} (rf/sub [:profile/login])
{:keys [key-uid name customization-color]} (rf/sub [:profile/login-profile])
sign-in-enabled? (rf/sub [:sign-in-enabled?])
profile-picture (rf/sub [:profile/login-profiles-picture key-uid])
auth-method (rf/sub [:auth-method])
login-multiaccount #(rf/dispatch [:profile.login/login])
on-change-password (fn [value]
(reset! password-value value)
(when (not= value
(security/safe-unmask-data
password))
(rf/dispatch [:set-in
Copy link
Contributor

Choose a reason for hiding this comment

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

These type of events :set-in, :set-once, etc are deprecated. Although not easy to realize that 🤷🏼

;;TODO :replace by named events
(rf/defn set-in-event
{:events [:set-in]}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

They are still being used here @mmilad75. Maybe you forgot to push?

[:profile/login :password]
(security/mask-data value)])
(rf/dispatch [:set-in [:profile/login :error]
""])))]
[rn/keyboard-avoiding-view
{:style style/login-container
:keyboardVerticalOffset (- (safe-area/get-bottom))}
[rn/view
{:style style/multi-profile-button-container}
(when config/quo-preview-enabled?
[quo/button
{:size 32
:type :grey
:background :blur
:icon-only? true
:on-press #(rf/dispatch [:navigate-to :quo-preview])
:disabled? processing
:accessibility-label :quo-preview
:container-style {:margin-right 12}}
:i/reveal-whitelist])
[quo/button
{:size 32
:type :grey
:background :blur
:icon-only? true
:on-press set-show-profiles
:disabled? processing
:accessibility-label :show-profiles}
:i/multi-profile]]
[rn/scroll-view
{:keyboard-should-persist-taps :always
:style {:flex 1}}
[quo/profile-card
{:name name
:customization-color (or customization-color :primary)
:profile-picture profile-picture
:card-style style/login-profile-card}]
[standard-authentication/password-input
{:shell? true
:blur? true
:on-press-biometrics (when (= auth-method constants/auth-method-biometric)
(fn []
(rf/dispatch [:biometric/authenticate
{:on-success #(rf/dispatch
[:profile.login/biometric-success])
:on-fail #(rf/dispatch
[:profile.login/biometric-auth-fail
%])}])))
:password @password-value
:on-change-password on-change-password}]]
[quo/button
{:size 40
:type :primary
:customization-color (or customization-color :primary)
:accessibility-label :login-button
:icon-left :i/unlocked
:disabled? (or (not sign-in-enabled?) processing)
:on-press login-multiaccount
:container-style {:margin-bottom (+ (safe-area/get-bottom) 12)}}
(i18n/label :t/log-in)]]))))

;; we had to register it here, because of hotreload, overwise on hotreload it will be reseted
(defonce show-profiles? (reagent/atom false))
Expand Down
1 change: 1 addition & 0 deletions src/status_im/contexts/wallet/create_account/view.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
[standard-auth/slide-button
{:size :size-48
:track-text (i18n/label :t/slide-to-create-account)
:on-close #(rf/dispatch [:profile.login/reset-password])
:customization-color @account-color
:on-auth-success (fn [entered-password]
(if new-keypair
Expand Down
6 changes: 3 additions & 3 deletions src/status_im/contexts/wallet/events.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@
(rf/reg-event-fx :wallet/navigate-to-new-account
(fn [{:keys [db]} [address]]
{:db (assoc-in db [:wallet :current-viewing-account-address] address)
:fx [[:dispatch [:hide-bottom-sheet]]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we definitely need to move this to the other event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we agreed to close the bottom sheet before the navigation happens

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but the original order did that no?

[:dispatch [:navigate-to :wallet-accounts address]]
:fx [[:dispatch [:navigate-to :wallet-accounts address]]
[:dispatch [:wallet/show-account-created-toast address]]]}))

(rf/reg-event-fx :wallet/switch-current-viewing-account
Expand Down Expand Up @@ -199,7 +198,8 @@
(fn [_ [account-details]]
(let [on-success (fn [derived-address-details]
(rf/dispatch [:wallet/add-account account-details
(first derived-address-details)]))]
(first derived-address-details)])
(rf/dispatch [:hide-bottom-sheet]))]
{:fx [[:dispatch [:wallet/create-derived-addresses account-details on-success]]]})))

(rf/reg-event-fx :wallet/bridge-select-token
Expand Down