multiaccounts refactoring S3, refactor keychain and touchid, move and refactor create/recover/login profile #16448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (23)
|
flexsurfer
changed the title
flexsurfer
force-pushed
the
feature/multiaccounts-refactoring-part3
branch
from
39131eb
to
c5174ef
Compare
0% of end-end tests have passedNot executed tests (1)Failed tests (33)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
flexsurfer
force-pushed
the
feature/multiaccounts-refactoring-part3
branch
from
c5174ef
to
fe80fbe
Compare
flexsurfer
changed the title
flexsurfer
commented
Jul 4, 2023
| @@ -0,0 +1,79 @@ | |||
| (ns react-native.keychain | |||
There was a problem hiding this comment.
just moved and cleaned, not many changes here
flexsurfer
commented
Jul 4, 2023
| @@ -0,0 +1,19 @@ | |||
| (ns react-native.touch-id | |||
There was a problem hiding this comment.
just moved and cleaned, not many changes here
flexsurfer
commented
Jul 4, 2023
There was a problem hiding this comment.
all test data is outdated and was removed with the outdated code
flexsurfer
commented
Jul 4, 2023
flexsurfer
commented
Jul 4, 2023
| @@ -0,0 +1,76 @@ | |||
| (ns status-im2.common.biometric.events | |||
There was a problem hiding this comment.
just moved and cleaned, not many changes here
flexsurfer
commented
Jul 4, 2023
| @@ -0,0 +1,110 @@ | |||
| (ns status-im2.common.keychain.events | |||
There was a problem hiding this comment.
just moved and cleaned, not many changes here
flexsurfer
commented
Jul 4, 2023
| @@ -0,0 +1,42 @@ | |||
| (ns status-im2.contexts.profile.config | |||
There was a problem hiding this comment.
just moved, not many changes here
J-Son89
reviewed
Jul 4, 2023
| (:require ["react-native-keychain" :as react-native-keychain] | ||
| [clojure.string :as string] | ||
| [taoensso.timbre :as log])) | ||
|
|
J-Son89
reviewed
Jul 4, 2023
| @@ -589,7 +588,8 @@ | |||
| (when (and (= card-state :profile/profile) | |||
| (= flow :import)) | |||
| (if (common/find-multiaccount-by-key-uid db key-uid) | |||
| (multiaccounts.recover/show-existing-multiaccount-alert key-uid) | |||
| ;; reimplement | |||
There was a problem hiding this comment.
please consider creating an issue and TODO etc 👍
There was a problem hiding this comment.
no need in issues and TODO, keycard will be implemented
J-Son89
reviewed
Jul 4, 2023
| [status-im2.db :as db])) | ||
|
|
||
| (re-frame/reg-fx | ||
| ::logout |
There was a problem hiding this comment.
hmm, do we not want to avoid fx's with :: syntax?
There was a problem hiding this comment.
idk, i also have this question, sometimes fx name is the same as event , which is confusing a little bit, we could user -fx suffix, but idk, i feel like :: is kinda ok for private usages, better to discuss with the team
There was a problem hiding this comment.
sure, I have no preference here as I'm still very fresh to cljs. 👍
J-Son89
reviewed
Jul 4, 2023
| :on-value-change #(re-frame/dispatch [:multiaccounts/save-password %])}] | ||
| {:checked? save-password? | ||
| :style {:margin-right 10}}] | ||
| ;; should be reimplemented |
There was a problem hiding this comment.
same here, can we have proper issues to track instead?
There was a problem hiding this comment.
/ just delete the commented code
There was a problem hiding this comment.
same here no issue, keycard functionality will be reimplemented with new designs
There was a problem hiding this comment.
i want to keep useful comments for developer who will be reimplementing keycard, just to save their time
J-Son89
reviewed
Jul 4, 2023
|
|
||
| ;; These helpers check if the device is okay to use for password storage | ||
| ;; They resolve callback with `true` if the check is passed, with `false` otherwise. | ||
| ;; Android only |
There was a problem hiding this comment.
nit, why not just change function name and remove comment
(defn- android-device-not-rooted?
There was a problem hiding this comment.
good question :) we should ask author of this code :)
J-Son89
approved these changes
Jul 4, 2023
There was a problem hiding this comment.
If possible that we can have these pr's split up, would make it much easier to review.
Nevertheless, great work @flexsurfer and very happy to see so much old code get nuked! 💥 💪
|
thank @VladimrLitvinenko fixed |
Hi @flexsurfer ISSUE 2 is still reproducedSteps:
###Actual result:
invalid_cyp_len.mp4
Expected result:The devices should be able to sync successfully without any errors if QR code is valid (not expired, have not been used before) Note:The behavior described in the actual result is expected only in cases if the user scans a n invalid sync QR code (QR has already been used. ) Logs:Devices:
|
|
hm |
@flexsurfer Could you please check the logs from Android? The previous logs I provided were from iOS. In the case of Android, the "try again" button is not visible during QR code scanning, which eliminates the possibility of accidentally scanning the QR code twice. |
|
hm also strange error |
|
oh i remember its new error for wrong password, so password is wrong for some reason, so login failed, but we don't show anything to user in that case |
| [{:keys [db]}] | ||
| (let [{:keys [key-uid name password]} (get-in db [:syncing :profile])] | ||
| {::login [key-uid | ||
| (transforms/clj->json {:name name |
There was a problem hiding this comment.
@flexsurfer - You should remove the (transforms/clj->json {:name name :key-uid key-uid}) as the ::login accepts only key-uid and hashed-password.
This is probably why the pairing is struck on the Syncing devices... screen as we are not passing the password to the login method correctly.
91% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Based on the logs, we are receiving the account data as expected. We need to update the This will resolve the issue. |
|
thanks @smohamedjavid , @VladimrLitvinenko should be fixed now |
85% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (29)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
|
Hi @flexsurfer. thank you for the fixes. PR is ready to be merged |
codemaster115
pushed a commit
that referenced
this pull request
Jul 7, 2023
… refactor create/recover/login profile (#16448) * multiaccounts refactoring S3 E1, refactor keychain and touchid,simplify app init flow, refactor biometric flow * S3 E2 move and refactor create/recover/login methods
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
new namespaces
react-native.touch-idreact-native.keychainstatus-im2.common.keychain.eventsstatus-im2.common.biometric.eventsmoved profiles views from onboarding to profile namespace
moved and refactored create/recover/login from multiaccounts to profile
removed a lot of outdated stale code