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

mmilad75 · 2024-02-19T13:09:01Z

Screen.Recording.2024-02-19.at.16.28.53.mov

status-im-auto · 2024-02-19T13:17:02Z

Jenkins Builds

Click to see older builds (20)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`7fffa73`	#1	2024-02-19 13:17:01	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`7fffa73`	#1	2024-02-19 13:17:30	~8 min	`ios`	📱`ipa` 📲
✔️	`7fffa73`	#1	2024-02-19 13:18:33	~9 min	`tests`	📄`log`
✔️	`7fffa73`	#1	2024-02-19 13:19:25	~10 min	`android`	🤖`apk` 📲

✔️	`b8a59b0`	#2	2024-02-20 15:18:07	~6 min	`tests`	📄`log`
✔️	`b8a59b0`	#2	2024-02-20 15:20:38	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`b8a59b0`	#2	2024-02-20 15:20:49	~8 min	`ios`	📱`ipa` 📲
✔️	`b8a59b0`	#2	2024-02-20 15:20:59	~8 min	`android`	🤖`apk` 📲

❌	`f0a0adc`	#3	2024-02-21 12:00:12	~1 min	`tests`	📄`log`
✔️	`f0a0adc`	#3	2024-02-21 12:05:56	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`f0a0adc`	#3	2024-02-21 12:06:05	~7 min	`ios`	📱`ipa` 📲
✔️	`f0a0adc`	#3	2024-02-21 12:07:11	~8 min	`android`	🤖`apk` 📲

❌	`3bec9d5`	#4	2024-02-21 13:24:09	~1 min	`tests`	📄`log`
✔️	`3bec9d5`	#4	2024-02-21 13:30:03	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`3bec9d5`	#4	2024-02-21 13:30:15	~8 min	`android`	🤖`apk` 📲
✔️	`3bec9d5`	#4	2024-02-21 13:31:01	~8 min	`ios`	📱`ipa` 📲

✔️	`6a88729`	#5	2024-02-21 13:38:43	~5 min	`tests`	📄`log`
✔️	`6a88729`	#5	2024-02-21 13:40:43	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`6a88729`	#5	2024-02-21 13:40:51	~7 min	`android`	🤖`apk` 📲
✔️	`6a88729`	#5	2024-02-21 13:41:55	~8 min	`ios`	📱`ipa` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`466ed2d`	#6	2024-02-21 14:50:15	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`466ed2d`	#6	2024-02-21 14:50:19	~8 min	`android`	🤖`apk` 📲
✔️	`466ed2d`	#6	2024-02-21 14:50:26	~8 min	`ios`	📱`ipa` 📲

✔️	`82826fa`	#7	2024-02-22 10:11:05	~6 min	`tests`	📄`log`
✔️	`82826fa`	#7	2024-02-22 10:12:15	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`82826fa`	#7	2024-02-22 10:14:11	~9 min	`android`	🤖`apk` 📲
✔️	`82826fa`	#7	2024-02-22 10:18:52	~13 min	`ios`	📱`ipa` 📲

clauxx · 2024-02-19T13:57:19Z

src/status_im/contexts/wallet/create_account/view.cljs

@@ -127,6 +127,7 @@
        {:size                :size-48
         :track-text          (i18n/label :t/slide-to-create-account)
         :customization-color @account-color
+         :on-close            #(rf/dispatch [:standard-auth/reset-login-password])


This should be handled by authorize, as I don't see scenarios where we wouldn't want to reset the password after the bottom-sheet is closed. @J-Son89 ?

... but I'd avoid adding it in this PR as authorize is fully refactored already over here and the conflicts might be hard to fix if that ns would be touched 😨.

@mmilad75 FYI added the password/error reset inside the authorization logic in this commit 41ca992

@clauxx Thanks man. I used your code

clauxx · 2024-02-19T14:02:27Z

src/status_im/common/standard_authentication/enter_password/view.cljs

+            on-change-password                  (fn [value]
+                                                  (reset! password-value value)
+                                                  (when (not= value (security/safe-unmask-data password))
+                                                    (rf/dispatch [:set-in [:profile/login :password]
+                                                                  (security/mask-data value)])
+                                                    (rf/dispatch [:set-in [:profile/login :error] ""])))]


might make sense to make a util for this inside standard-auth, as we're repeating it everywhere?

I think it's just better that we make a proper re-frame event, no?

I think we are supposed to move away from the :set-in method it just sort of lingered around because no one took the time to update it.
No better time than now to do so :)

hmm, then maybe it might be better to make an event inside standard-auth and call it from a single place - password-input? This way the consumers of standard-auth/password-input will only have to handle the ratom inside on-change.

orrr, what about not exposing on-change at all and handling the ratom inside password-input as well?

it's a bit unclear of what you mean here @clauxx - can you expand this a bit further?

My point is rather straightforward, we just define an event that updates the db as normal.

so instead of this:

(rf/dispatch [:set-in [:profile/login :password] (security/mask-data value)])

we have

(rf/reg-event-fx :profile/update-login-password (fn [{:keys [db]} [value]] {:db (assoc-in db [:profile/login :password (security/mask-data value)] ))

and then used as

(rf/dispatch [:profile/update-login-password value])

and similar events for updating the error and the key-uid

Oh, I agree with your suggestion of handling the db updates inside an event. Maybe even a single event to update the password and reset the error together and put them inside standard-auth like:

(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 ""))]))

My suggestion though is more related to simplifying the password-input use. Before this PR, to use the password-input, you didn't have to care about the on-change-text behaviour, it was handled implicitly. Now, you have to pass the on-change and dispatch the event(s) to update the password/error and update a ratom. It was fine for a POC of the fix, but now it seems like a bit of a DX regression if the consumers of password-input have to pass an extra prop with boilerplate. What do you think @J-Son89?

Yeah sure, sounds good to me

Regarding @J-Son89's comment about not using set-in. Here it's also being used twice in a row, as if the app-db is a synchronous state that can be mutated. This is usually a smell in my experience because it leads to the view layer knowing too much about the app state.

vkjr · 2024-02-19T14:13:40Z

When I've read the description "Password field contains saved data", I thought it is a feature and as a result, haven't understood the attached video) I'd suggest rephrasing the description to make it more clear. Like:

"Fixed bug: password field contains saved data" or
"Password field doesn't contain saved data anymore"

J-Son89 · 2024-02-19T14:30:47Z

src/status_im/common/standard_authentication/enter_password/view.cljs

-                                (rf/dispatch [:profile.login/verify-database-password password
-                                              #(on-enter-password password)]))}
-        button-label]]]]))
+  []


should be [_]?
cc @ulisesmac wdyt

Yeah, as a standard practice I think it'd be the best, although it's not needed for this component

ilmotta · 2024-02-19T19:33:23Z

src/status_im/common/standard_authentication/standard_auth/slide_button/view.cljs

                 container-style]
-          :or   {container-style {:flex 1}}}]
+          :or   {container-style {:flex 1}
+                 on-close        #()}}]


Just a reminder that if a caller of view-internal pass on-close as nil for any reason (easy to happen with Clojure conditionals), then the destructring code to set a default on-close won't be used and the app will break when :on-complete runs because on-close is called unchecked.

The pattern (when f (f)) is more robust here, and the default destructuring can be removed.

It's safer to do: (when (fn? f) (f)), no?

I like @J-Son89 's approach! too expressive and cheap to implement

It's safer to do: (when (fn? f) (f)), no?

Definitely safer, but I don't remember a case where we passed a string or other primitive instead of a function. As far as I've seen, it's either the value exists and it's a valid function instance or it's falsey.

ilmotta · 2024-02-19T19:37:00Z

src/status_im/contexts/profile/profiles/view.cljs

+                                                         (when (not= value
+                                                                     (security/safe-unmask-data
+                                                                      password))
+                                                           (rf/dispatch [:set-in


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

status-mobile/src/legacy/status_im/events.cljs

Lines 220 to 222 in bfed36d

;;TODO :replace by named events

(rf/defn set-in-event

{:events [:set-in]}

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

ilmotta · 2024-02-19T19:39:37Z

src/status_im/common/standard_authentication/enter_password/view.cljs

+            {:keys [error processing password]} (rf/sub [:profile/login])
+            sign-in-enabled?                    (rf/sub [:sign-in-enabled?])
+            on-change-password                  (fn [value]
+                                                  (reset! password-value value)


I think we could improve the naming of this atom. password-value for an atom is kind of confusing, because usually when we refer to the "value" of an atom is the deref'ed result, not the atom itself. The name could be just password or password-state for added clarity (for some).

clauxx · 2024-02-20T15:15:24Z

@mmilad75 I think you added by accident the changes from #18756

J-Son89 · 2024-02-20T16:49:39Z

src/status_im/common/standard_authentication/events.cljs

+             :on-success     #(rf/dispatch [:standard-auth/on-biometric-success on-auth-success])
+             :on-fail        #(rf/dispatch [:standard-auth/on-biometric-fail on-auth-fail %])}]]]}))
+
+(schema/=> authorize-with-biometric events-schema/?authorize-with-biometric)


I think we don't need/want schema added here just yet 👍
let's keep it focused until quo is covered completely, you can remove from this pr

It was added accidentally. I removed the codes

J-Son89 · 2024-02-20T16:52:31Z

src/status_im/contexts/wallet/events.cljs

@@ -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]]


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

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

Right, but the original order did that no?

J-Son89 · 2024-02-20T16:54:18Z

it now works for syncing and sending?

ilmotta · 2024-02-20T20:14:48Z

src/status_im/common/standard_authentication/enter_password/view.cljs

+                                                    (reset! password-state value)
+                                                    (rf/dispatch [:standard-auth/update-password
+                                                                  value])))]
+        [:<>


A fragment is wrapping a single view. Is this correct?

ilmotta · 2024-02-20T20:41:45Z

src/status_im/common/standard_authentication/enter_password/view.cljs

+                                                                  value])))]
+        [:<>
+         [rn/view {:style style/enter-password-container}
+          [rn/view


This view seems unnecessary.

ilmotta · 2024-02-20T21:37:26Z

src/status_im/common/standard_authentication/enter_password/view.cljs

+            on-change-password                  (fn [value]
+                                                  (when (not= value @password-state)
+                                                    (reset! password-state value)
+                                                    (rf/dispatch [:standard-auth/update-password


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:

There's an unnecessary overhead to involve re-frame on every key press. Why are we committing to the app-db all the time?

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.

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).

ilmotta · 2024-02-20T21:44:24Z

src/status_im/common/standard_authentication/events.cljs

+(defn on-biometric-fail
+  [_ [on-auth-fail error]]
+  (when on-auth-fail
+    (on-auth-fail error))


This is odd in the event handler. There's no return value, so it's assumed to be a side-effect, but event handlers should only describe side-effects, not perform them. In re-frame we aspire to separate planning from execution.

This file was added accidentally. I removed the entire file

Okay, thanks @mmilad75

mmilad75 · 2024-02-21T10:52:35Z

@mmilad75 I think you added by accident the changes from #18756

Removed

flexsurfer · 2024-03-18T09:46:59Z

hey @mmilad75 what's the state of this PR?

mmilad75 · 2024-03-19T12:02:11Z

hey @mmilad75 what's the state of this PR?

There was an issue with creating new account before, and now, the issue that this PR is fixing is no longer reproducible. I will close this PR

mmilad75 requested review from siddarthkay, smohamedjavid, clauxx, J-Son89, ilmotta, vkjr and ulisesmac and removed request for siddarthkay February 19, 2024 13:09

mmilad75 self-assigned this Feb 19, 2024

clauxx reviewed Feb 19, 2024

View reviewed changes

J-Son89 reviewed Feb 19, 2024

View reviewed changes

ilmotta reviewed Feb 19, 2024

View reviewed changes

mmilad75 requested review from J-Son89, clauxx and ilmotta February 20, 2024 15:12

J-Son89 reviewed Feb 20, 2024

View reviewed changes

J-Son89 approved these changes Feb 20, 2024

View reviewed changes

ilmotta reviewed Feb 20, 2024

View reviewed changes

mmilad75 force-pushed the milad/18447-fix-password-field-in-modal branch from b8a59b0 to f0a0adc Compare February 21, 2024 11:58

mmilad75 and others added 4 commits February 21, 2024 16:43

clear password on wallet/show-account-created-toast

5f32df6

hide bottom sheet earlier

1129f31

draft

6e00cda

not depending on the db password in the input

6e1087b

mmilad75 added 7 commits February 21, 2024 16:51

fix lint issues

f9342df

update enter_password usage on profile

07e0064

add default value for on-close in slide_button component

f8c2f65

fix lint issues

a243732

remove unused code

e2b919d

add event

cf12db0

fix changes

3bec9d5

mmilad75 force-pushed the milad/18447-fix-password-field-in-modal branch from f0a0adc to 3bec9d5 Compare February 21, 2024 13:21

fix lint issues

6a88729

mmilad75 requested a review from ilmotta February 21, 2024 13:36

mmilad75 added 2 commits February 21, 2024 14:41

Merge branch 'develop' into milad/18447-fix-password-field-in-modal

466ed2d

Merge branch 'develop' into milad/18447-fix-password-field-in-modal

82826fa

mmilad75 closed this Mar 19, 2024

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

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

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

Conversation

mmilad75 commented Feb 19, 2024

status-im-auto commented Feb 19, 2024 • edited Loading

Jenkins Builds

clauxx Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilad75 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkjr commented Feb 19, 2024

Choose a reason for hiding this comment

ulisesmac Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmotta Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauxx commented Feb 20, 2024

J-Son89 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Son89 commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilad75 commented Feb 21, 2024

flexsurfer commented Mar 18, 2024

mmilad75 commented Mar 19, 2024

status-im-auto commented Feb 19, 2024 •

edited

Loading

clauxx Feb 19, 2024 •

edited

Loading

mmilad75 Feb 20, 2024 •

edited

Loading

ulisesmac Feb 19, 2024 •

edited

Loading

ilmotta Feb 20, 2024 •

edited

Loading

J-Son89 Feb 20, 2024 •

edited

Loading