create wallet account #17496
Merged
create wallet account #17496
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 (62)
|
J-Son89
force-pushed
the
jc/create-wallet-account
branch
2 times, most recently
from
7330e6c
to
54763b1
Compare
J-Son89
commented
Oct 11, 2023
| @@ -40,11 +40,13 @@ | |||
| (rf/dispatch [:show-bottom-sheet | |||
| {:on-close on-close | |||
| :theme theme | |||
| :shell? blur? | |||
| :shell? false | |||
There was a problem hiding this comment.
I need to swap this back 😓
J-Son89
commented
Oct 11, 2023
| @@ -75,6 +75,7 @@ | |||
| {:db (assoc db | |||
| :chats/loading? true | |||
| :networks/current-network current-network | |||
| :wallet-2/tokens-loading? true | |||
There was a problem hiding this comment.
I need to revert this, related to @mmilad75's work
J-Son89
commented
Oct 11, 2023
J-Son89
commented
Oct 11, 2023
| :button-props {:title (i18n/label :t/edit)} | ||
| :left-icon :i/placeholder | ||
| :description :text | ||
| :description-props {:text "On device"}} |
There was a problem hiding this comment.
will make it an i18n label
J-Son89
commented
Oct 11, 2023
| [taoensso.timbre :as log] | ||
| [utils.security.core :as security] | ||
| [native-module.core :as native-module])) | ||
|
|
There was a problem hiding this comment.
@ilmotta would appreciate a review on these events 👍
There was a problem hiding this comment.
also do we have a way to test these rpc requests? 🤔 - I can add them here if there's a good example?
cc @cammellos
There was a problem hiding this comment.
@ilmotta would appreciate a review on these events
Will do :)
also do we have a way to test these rpc requests?
The integration tests can make actual JSON RPC calls. If you would like to be sure more complex flows are correct I believe that's the place. If you want to venture in those tests, it would be good to write them in a separate file than status-im.integration-test, that one is crowded.
There was a problem hiding this comment.
that filename sounds like it could be improved upon to me anyway. Will put them in wallet specific test files.
sounds great. thanks 👍
J-Son89
commented
Oct 11, 2023
| @@ -78,3 +78,5 @@ | |||
| (def sub (comp deref re-frame/subscribe)) | |||
|
|
|||
| (def dispatch re-frame/dispatch) | |||
|
|
|||
| (def reg-event-fx re-frame/reg-event-fx) | |||
J-Son89
requested review from
OmarBasem,
vkjr,
tumanov-alex,
briansztamfater,
smohamedjavid,
mmilad75 and
ulisesmac
ilmotta
approved these changes
Oct 11, 2023
| (fn [_ [password account-details]] | ||
| (let [on-success (fn [derived-adress-details] | ||
| (rf/dispatch [:wallet-2/add-account password account-details (first derived-adress-details)]))] | ||
| (rf/dispatch [:wallet-2/create-derived-addresses password account-details on-success])))) |
There was a problem hiding this comment.
@J-Son89, in re-frame, event handlers should never directly call re-frame.core/dispatch, only effects should do that (:json-rpc/call is one example that does this behind the scenes). The event handler should be pure and always either return nil or a map of effects.
I updated the guidelines recently:
Register events with
re-frame.core/reg-event-fxand follow re-frame's best
practice so use only:dband:fxeffects.
So here the code would be:
(rf/reg-event-fx :wallet-2/derive-address-and-add-account
(fn [_ [password account-details]]
(let [on-success (fn [derived-adress-details]
(rf/dispatch [:wallet-2/add-account password account-details
(first derived-adress-details)]))]
{:fx [[:dispatch [:wallet-2/create-derived-addresses password account-details on-success]]]})))| [utils.security.core :as security] | ||
| [native-module.core :as native-module])) | ||
|
|
||
| (rf/reg-event-fx |
There was a problem hiding this comment.
The more common way events are formatted in re-frame is to keep the event ID in the same line as reg-event-fx. I recently updated zprint to take this into account so that the args after the event ID are aligned like re-frame.core/reg-sub (see
Line 34 in 6e897f0
| :colorID color})] | ||
| {:json-rpc/call [{:method "accounts_addAccount" | ||
| :params [sha3-pwd account-config] | ||
| :on-success #(rf/dispatch [:navigate-to :wallet-accounts]) |
There was a problem hiding this comment.
FYI: :on-success can be an event vector too, leaving the dispatch responsibility to the :json-rpc/call effect and being more friendly for unit testing (because of equality checks).
:on-success [:navigate-to :wallet-accounts]| (fn [{:keys [db]} [password {:keys [emoji account-name color]} {:keys [public-key address path]}]] | ||
| (let [key-uid (get-in db [:profile/profile :key-uid]) | ||
| sha3-pwd (native-module/sha3 (security/safe-unmask-data password)) | ||
| account-config (clj->js {:key-uid key-uid |
There was a problem hiding this comment.
clj->js is unnecessary because the effect takes care of calling utils.transforms/clj->json on the entire payload before making the RPC call. Same for the other event :wallet-2/create-derived-addresses.
ulisesmac
reviewed
Oct 11, 2023
Comment on lines
2
to
11
| (:require | ||
| [quo2.core :as quo] | ||
| [quo2.theme :as quo.theme] | ||
| [reagent.core :as reagent] | ||
| [utils.re-frame :as rf] | ||
| [react-native.touch-id :as biometric] | ||
| [utils.i18n :as i18n] | ||
| [taoensso.timbre :as log] | ||
| [status-im2.common.standard-authentication.enter-password.view :as enter-password] | ||
| [react-native.core :as rn])) | ||
| [quo2.core :as quo] | ||
| [quo2.theme :as quo.theme] | ||
| [reagent.core :as reagent] | ||
| [utils.re-frame :as rf] | ||
| [react-native.touch-id :as biometric] | ||
| [utils.i18n :as i18n] | ||
| [taoensso.timbre :as log] | ||
| [status-im2.common.standard-authentication.enter-password.view :as enter-password] | ||
| [react-native.core :as rn])) |
There was a problem hiding this comment.
@ulisesmac, this issue seems to be related to how clojure-lsp cleans up namespaces. It aligns the opening square bracket [ with the colon in :require. The way we configured zprint causes it to align the namespaces with the letter r.
When I run make lint-fix zprint aligns the namespaces to r in :require, which seems to be correct.
There was a problem hiding this comment.
When I run make lint-fix zprint aligns the namespaces to r in :require, which seems to be correct.
I think this behavior is not correct. it is a list, not a function call:
;; list:
(:require
:b
:c
[])
;; fn call
(require
:b
:c
[]) There was a problem hiding this comment.
apolgies, it's probably my local formatter but not the linter. I'll check
There was a problem hiding this comment.
I think this behavior is not correct. it is a list, not a function call:
I meant correct from zprint's point of view. But I agree how clojure-lsp formats is the correct way. @yqrashawn has the greatest knowledge about zprint.
@yqrashawn, do you know if we can tell zprint to format namespaces identically to clojure-lsp? At the moment, clojure-lsp clean-ns command has one less whitespace than zprint.
There was a problem hiding this comment.
Will look into this maybe later next week #17641
OmarBasem
approved these changes
Oct 12, 2023
J-Son89
force-pushed
the
jc/create-wallet-account
branch
2 times, most recently
from
1298703
to
9f6f78f
Compare
79% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (34)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@pavloburykh, @Francesca-G - we are still somewhat not requiring design reviews on wallet screens. If the developer wants them that is fine but imo I do not need one here as I intend to clean this UI at a later point anyway. |
J-Son89
force-pushed
the
jc/create-wallet-account
branch
from
729cad3
to
7310334
Compare
|
@pavloburykh this pr is working ok? cc @cammellos can I merge that pr on Status go? 🤔 |
J-Son89
force-pushed
the
jc/create-wallet-account
branch
from
7310334
to
d62b18e
Compare
Hey @J-Son89! It was working okay when I moved it to merge column last week. But now go and mobile branches are outdated. That's why we need to rebase both branches and perform another round of testing (including re-run e2e) before merge to make sure no issues introduced by updated go. |
J-Son89
force-pushed
the
jc/create-wallet-account
branch
2 times, most recently
from
e857496
to
8f51e23
Compare
|
Thanks @pavloburykh. The changes needed on Status Go were merged and I have updated this pr to point at the right version of Status Go. |
Thank you @J-Son89! Let's have another e2e run before merging. |
|
perfect! 👌 |
73% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (33)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
71% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (32)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 hold on with merging please. E2E results are suspicious, I will check tomorrow. |
87% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
|
@J-Son89 PR is ready for merge. |
J-Son89
force-pushed
the
jc/create-wallet-account
branch
from
8f51e23
to
b84425d
Compare
Hey @J-Son89! Could you please clarify if the above ISSUE 1 and 2 were logged as followups as I couldn't find them. If not please let me know and I will log them. |
|
Hi @pavloburykh - apologies I assumed - ISSUE 1 Laggy emoji picker screen -had a corresponding issue as it is something Javid and I had discussed. I believe he is on holiday this week and I can not find the issue. I will create one anyway for tracking and send it to Javid, he can close the issue if it's a duplicate. 👍 with 2 - it's a more general problem and once we have the code in Ulises & my pr - #17737 |
|
@pavloburykh - #17889 |
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
|
Here is a followup of ISSUE 2 |
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.
fixes:
#17061
This pr implements the ability to create an account in the new wallet ui using the default keypair and wallet-root-address.
To test this branch I worked with @mmilad75's pr: #17447 - see the video attached
Screen.Recording.2023-10-11.at.15.17.41.mov
There are some follow ups to this work ->
improve mechanism to derive path -> should be based upon the derivation path of last derived path sharing the same key-uid.
Navigation currently goes to the wrong place after creating an account. there is some screens that need to be added for animation while it generates the account.
Testing ->
To test this feature it generates a new wallet account in the new UI. To see that the new account is generated in the UI you can create an account and then log out and in again and check the old wallet UI to see that there is a new account in that list. Also if the create account request is successful the page will navigate to the default account page.
It's also important to create a new account to test this feature. From a QA perspective it might also be okay to ignore harshly testing this feature until some other work is in place so we can test these features in full, e.g the real list of users accounts displayed so we can see accounts are generated and they have the correct name, emoji etc 👍
this touches off the basic auth mechanism which is also used on the syncing page 👍