[#21318] Keycard - Allow user to initiate Profile key pair migration … #21359

flexsurfer · 2024-10-01T14:23:29Z

status-im-auto · 2024-10-01T14:28:47Z

Jenkins Builds

Click to see older builds (4)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`55e5b44`	#1	2024-10-01 14:28:47	~4 min	`tests`	📄`log`
✔️	`55e5b44`	#1	2024-10-01 14:30:44	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`55e5b44`	#1	2024-10-01 14:31:58	~8 min	`android`	🤖`apk` 📲
✔️	`55e5b44`	#1	2024-10-01 14:34:06	~10 min	`ios`	📱`ipa` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`d757f02`	#2	2024-10-04 14:02:49	~4 min	`tests`	📄`log`
✔️	`d757f02`	#2	2024-10-04 14:05:31	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`d757f02`	#2	2024-10-04 14:06:55	~8 min	`android`	🤖`apk` 📲
✔️	`d757f02`	#2	2024-10-04 14:08:38	~10 min	`ios`	📱`ipa` 📲

✔️	`2e25235`	#3	2024-10-07 09:32:22	~4 min	`tests`	📄`log`
✔️	`2e25235`	#3	2024-10-07 09:35:30	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`2e25235`	#3	2024-10-07 09:36:18	~8 min	`android`	🤖`apk` 📲
✔️	`2e25235`	#3	2024-10-07 09:38:31	~10 min	`ios`	📱`ipa` 📲

…on an empty Keycard

flexsurfer · 2024-10-07T09:30:36Z

src/status_im/contexts/keycard/error/view.cljs

@@ -0,0 +1,42 @@
+(ns status-im.contexts.keycard.error.view


do not review this screen, is a temporary implementation will be finished in the next PRs

seanstrom

Looking good! Thanks for the PR 🙌
Left a few comments and questions, let me know what you think 🙏

src/status_im/contexts/keycard/events.cljs

src/status_im/feature_flags.cljs

src/status_im/contexts/keycard/sheets/migrate/view.cljs

seanstrom · 2024-10-07T10:15:28Z

src/status_im/contexts/keycard/events.cljs

+         event-vector [:keycard/get-application-info
+                       {:on-success #(rf/dispatch [:keycard/on-get-application-info-success % args])}]]


I think we might need to be careful with storing functions in the database. Maybe it's not a problem in this case, but if we try to serialise the db-state then we may not be able to deserialise the function later.

i wouldn't think about this much, i mean serializing db ) we should do it how it's better for now, but I agree, I'm also not a fun of such an approach, we could think how we could improve it later

later on if we update the keycard effects to use call-continuation, then we may be able to do something like this:

[:keycard/get-application-info {:on-success [:keycard/on-get-application-info-success args]}]

But we would need to refactor :keycard/on-get-application-info-success to the it's arguments in a different order (I think).

src/status_im/contexts/keycard/events.cljs

flexsurfer · 2024-10-07T10:42:47Z

thank you for the review @seanstrom

ulisesmac

Hi @flexsurfer !

Thanks for this PR! I left a couple of comments but it looks great at code level in general.

I'd like to ask for a PR description summarizing the changes made and, if possible, a demo video or some screenshots of the implementation running, some steps to test would be also be helpful for reviewers.

I know we aren't asking for QA now, is it because the feature isn't complete yet? If it's testable I'd suggest to ask for QA so that we don't accumulate possible bugs and the future feature flag removal is easier for everyone.

ulisesmac · 2024-10-07T16:31:58Z

src/status_im/contexts/keycard/check/view.cljs

+  []
+  [:<>
+   [quo/page-nav
+    {:key        :header


Just wondering if we really need to provide key

ulisesmac · 2024-10-07T16:33:33Z

src/quo/components/markdown/list/style.cljs

-          :flex           1
          :align-items    :flex-start}))

 (def index
  {:margin-left 5})

 (def text-container
-  {:margin-left 8
-   :flex        1})
+  {:margin-left 8})


Since we modified the markdown-list component, are we checking the UI isn't broken in other screens where it is used?

ulisesmac · 2024-10-07T16:39:20Z

src/status_im/contexts/keycard/effects.cljs

@@ -10,10 +10,15 @@

 (defonce ^:private active-listeners (atom []))


Just wondering why do we use a global atom to track the state. I know this was already here, but I think this could live in re-frame 🤔

ulisesmac · 2024-10-07T16:41:22Z

src/status_im/contexts/keycard/empty/view.cljs

+      :on-press            #(rf/dispatch [:show-bottom-sheet
+                                          {:theme   :dark
+                                           :content (fn [] [sheets.migrate/view])}])}]]


I think the. value for :content is already a function, so we don't need to wrap it into another one, i.e. we can directly pass sheets.migrate/view

ulisesmac · 2024-10-07T20:05:54Z

src/status_im/contexts/keycard/events.cljs

 (rf/reg-event-fx :keycard/on-action-with-pin-error
 (fn [{:keys [db]} [error]]
-   (log/debug "[keycard] get keys error: " error)
+   (log/debug "[keycard] on-action-with-pin-error: " error)


AFAIK we have an event to log errors, so we can use it

ulisesmac · 2024-10-07T20:10:31Z

src/status_im/contexts/keycard/nfc_sheet/view.cljs

-       "Ready to Scan"]
+       (i18n/label :t/ready-to-scan)]
      [rn/image
       {:source (resources/get-image :nfc-prompt)}]
      [rn/text {:style {:font-size 16 :color :white :margin-vertical 36}}
       (if connected?
-         "Connected. Don’t move your card."
-         "Hold your phone near a Status Keycard")]
+         (i18n/label :t/connected-dont-move)
+         (i18n/label :t/hold-phone-near-keycard))]


ulisesmac · 2024-10-07T20:12:26Z

src/status_im/contexts/keycard/sheets/migrate/view.cljs

+      [quo/text {}
+       (i18n/label :t/migrate-key-pair-keycard-default-key {:name profile-name})]


I think we can avoid this empty map

ulisesmac · 2024-10-07T20:14:19Z

src/status_im/contexts/keycard/sign/events.cljs

+                                :on-success
+                                #(do
+                                   (rf/dispatch [:keycard/disconnect])
+                                   (rf/dispatch


Since we are modifying this code, we could change #(do ...) -> (fn [] ...)

flexsurfer · 2024-10-08T09:15:07Z

thanks @ulisesmac, all the information is in the task itself, there is no need to duplicate it in the PR, it's not worth it to waste time on each PR, it is better to test the final result with full functionality

flexsurfer self-assigned this Oct 1, 2024

flexsurfer force-pushed the feature/Keycard_-_Allow_user_to_initiate_Profile_key_pair_migration_on_an_empty_Keycard_#21318 branch from 55e5b44 to d757f02 Compare October 4, 2024 13:58

flexsurfer added the keycard label Oct 4, 2024

[#21318] Keycard - Allow user to initiate Profile key pair migration …

2e25235

…on an empty Keycard

flexsurfer force-pushed the feature/Keycard_-_Allow_user_to_initiate_Profile_key_pair_migration_on_an_empty_Keycard_#21318 branch from d757f02 to 2e25235 Compare October 7, 2024 09:27

flexsurfer commented Oct 7, 2024

View reviewed changes

flexsurfer changed the title ~~[WIP] [#21318] Keycard - Allow user to initiate Profile key pair migration …~~ [#21318] Keycard - Allow user to initiate Profile key pair migration … Oct 7, 2024

flexsurfer added the skip-manual-qa label Oct 7, 2024

flexsurfer requested review from Parveshdhull, seanstrom and ulisesmac October 7, 2024 09:31

seanstrom reviewed Oct 7, 2024

View reviewed changes

seanstrom approved these changes Oct 7, 2024

View reviewed changes

flexsurfer merged commit 80ad2b8 into develop Oct 7, 2024
6 checks passed

flexsurfer deleted the feature/Keycard_-_Allow_user_to_initiate_Profile_key_pair_migration_on_an_empty_Keycard_#21318 branch October 7, 2024 18:15

ulisesmac reviewed Oct 7, 2024

View reviewed changes

flexsurfer mentioned this pull request Oct 28, 2024

Keycard - refactor keycard core namespace #21511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#21318] Keycard - Allow user to initiate Profile key pair migration … #21359

[#21318] Keycard - Allow user to initiate Profile key pair migration … #21359

flexsurfer commented Oct 1, 2024 •

edited

Loading

status-im-auto commented Oct 1, 2024 •

edited

Loading

flexsurfer Oct 7, 2024

seanstrom left a comment

seanstrom Oct 7, 2024 •

edited

Loading

flexsurfer Oct 7, 2024 •

edited

Loading

seanstrom Oct 7, 2024

flexsurfer commented Oct 7, 2024

ulisesmac left a comment

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

ulisesmac Oct 7, 2024

flexsurfer commented Oct 8, 2024

		event-vector [:keycard/get-application-info
		{:on-success #(rf/dispatch [:keycard/on-get-application-info-success % args])}]]

		@@ -10,10 +10,15 @@

		(defonce ^:private active-listeners (atom []))

		[quo/text {}
		(i18n/label :t/migrate-key-pair-keycard-default-key {:name profile-name})]

[#21318] Keycard - Allow user to initiate Profile key pair migration … #21359

[#21318] Keycard - Allow user to initiate Profile key pair migration … #21359

Conversation

flexsurfer commented Oct 1, 2024 • edited Loading

status-im-auto commented Oct 1, 2024 • edited Loading

Jenkins Builds

Choose a reason for hiding this comment

seanstrom left a comment

Choose a reason for hiding this comment

seanstrom Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

flexsurfer Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flexsurfer commented Oct 7, 2024

ulisesmac left a 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

flexsurfer commented Oct 8, 2024

flexsurfer commented Oct 1, 2024 •

edited

Loading

status-im-auto commented Oct 1, 2024 •

edited

Loading

seanstrom Oct 7, 2024 •

edited

Loading

flexsurfer Oct 7, 2024 •

edited

Loading