[#20035] feat: import missing key pair by scanning QR code #20144
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 (64)
|
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
8 times, most recently
from
1ff4a34
to
eb855e7
Compare
mohsen-ghafouri
commented
May 23, 2024
ilmotta
reviewed
May 23, 2024
There was a problem hiding this comment.
Hey @mohsen-ghafouri. Thanks for adding me as a reviewer. I did a longer review this time.
Event or effect?
Events should be pure and return a map of effects or nil. Side-effects should not happen (our known exception is logging, which we think is acceptable). So for instance, the event :wallet/connection-string-for-key-pair-import doesn't return a map. Actually, the results happen inside the callback input-connection-string-for-importing-keypairs-keystores-callback. This tells us the entire event should be a re-frame effect instead and moved with it should go the function input-connection-string-for-importing-keypairs-keystores-callback.
Test coverage
We have a soft, but I'd say strong enough agreement across teams that non-trivial code, especially outside UI namespaces should be unit tested. This is the top recommendation from the re-frame author as well. That's one of the main goals of the re-frame architecture, to build a highly testable system composed mostly of pure functions (and pure functions are cheap to test). But not all events need to be tested, if we can at least test the ones with more conditionals, data processing, etc that's good. Events that just dispatch a JSON RPC call without doing much else may not need a test.
Please, don't take the current quality of the code as a standard. We started to test more events not long ago.
Just last week we had a thread about test coverage and the lack of tests in events resurfaces all the time. I think we could be more clear about quality in our docs. Something that I take as an action item, but of course anyone can do.
A few reasons why testing is important (generally speaking):
- When untested code is merged, we make a disservice to the next developer in a future PR that will need to retest things manually if they change something.
- We can be faster without tests in some cases (questionable), but the next developer might be slower, and the next one, and so on because the cost of not having tests is there "forever".
- Untested code creates a negative incentive for other devs to refactor code because they will need to have the motivation, plus deal with a higher risk of regressions. This is especially important in a repository like ours with thousands of lines of code being churned all the time.
- We rely too much on manual QA and manual dev-QA, but lots of branches and permutations of the code in this namespace (to give one example) can't be easily tested manually. It's also not a given that unit tests will cover them. For that we need to go beyond happy path tests and cover more or all of the permutations if we can (e.g. every
ifrequires two assertions at least and permutations can grow exponentially). The events in this PR have a few conditions to test, so having only a happy path test per event won't cover much. - Without tests, as a reviewer, I can't easily recommend refactors. Many times I have to checkout the branch, capture real data, then armed with that understanding I can suggest improvements. This is expensive and me and others won't do as often as we could. The lack of example data creates a negative incentive to do higher quality reviews.
- Without tests I have to approve the code by a leap of faith on the author and that QAs might catch potential problems, which, very often, is unrealistic to assume.
src/status_im/contexts/settings/wallet/keypairs_and_accounts/scan_qr/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/wallet/keypairs_and_accounts/scan_qr/view.cljs
Outdated
Show resolved
Hide resolved
|
Hey @ilmotta, Thank you for your invaluable feedbacks. I completely agree about the importance of tests. I’ll make sure to add sufficient integration and unit tests. |
seanstrom
reviewed
May 24, 2024
src/status_im/contexts/settings/wallet/keypairs_and_accounts/view.cljs
Outdated
Show resolved
Hide resolved
seanstrom
reviewed
May 24, 2024
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
6 times, most recently
from
efe5550
to
3192cfb
Compare
|
Hello @seanstrom @ilmotta Would you mind giving my PR another review? 🙌🏻 |
seanstrom
approved these changes
May 29, 2024
src/status_im/contexts/settings/wallet/keypairs_and_accounts/encrypted_qr/view.cljs
Show resolved
Hide resolved
seanstrom
reviewed
May 29, 2024
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
from
3192cfb
to
7b9cc07
Compare
80% of end-end tests have passedNot executed tests (1)Failed tests (6)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@status-im/mobile-qa could you please check results? skiped manual QA as it's behind the feature flag |
src/status_im/contexts/settings/wallet/keypairs_and_accounts/scan_qr/view.cljs
Outdated
Show resolved
Hide resolved
| [utils.security.core :as security] | ||
| [utils.transforms :as transforms])) | ||
|
|
||
| (rf/reg-fx :effects.connection-string/export-keypair |
There was a problem hiding this comment.
I'm not sure about having effects as a prefix.
Additionally, these functions are part of sync/local-pairing features (connection-string is an arg), I would suggest naming them as syncing.effects/export-keypairs-keystores and syncing.effects/import-keypairs-keystores. WDYT?
There was a problem hiding this comment.
@smohamedjavid, the prefix :effects is the convention nowadays. We can change in the future of course. Some effects are not yet following this convention, but most are.
There was a problem hiding this comment.
Right, my bad.
I guess in that case effects.syncing/export-keypairs-keystores and effects.syncing/import-keypairs-keystores would be a good choice
There was a problem hiding this comment.
I will rename it in my next PR related to import single missing keypair.
| connection-string | ||
| config-map) | ||
| (promesa/then (fn [res] | ||
| (if-let [error (when (syncing-events/extract-error res) |
There was a problem hiding this comment.
if I'm not mistaken, this error extraction is already part of promesa:
status-mobile/src/native_module/utils.cljs
Lines 7 to 23 in 841a614
We can skip this whole check
smohamedjavid
approved these changes
May 29, 2024
|
Hi @mohsen-ghafouri. Thank you for the PR. I noticed a new commit (f6c38d0) added after running the e2e tests. Is this a rebase or new fixes? Also, I'm unsure about test case 702894; it seems like a delivery issue. I've rerun this case one more time |
|
Hi @VolodLytvynenko, sry just some new feedbacks appears after I asked QA to check the result and have to resolve them. thanks |
100% of end-end tests have passedPassed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
|
86% of end-end tests have passedNot executed tests (1)Failed tests (3)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestWalletMultipleDevice:
Expected to fail tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
from
f6c38d0
to
ff4a73e
Compare
|
@VolodLytvynenko I just rebased, if all good please let me know. thanks |
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
from
ff4a73e
to
f76d0fd
Compare
VolodLytvynenko
force-pushed
the
feat/import-missing-keypair
branch
from
f76d0fd
to
5d72f58
Compare
|
@mohsen-ghafouri sorry for delay. The e2e results are reviewed, the failed tests are not related to the PR. |
mohsen-ghafouri
force-pushed
the
feat/import-missing-keypair
branch
from
5d72f58
to
d08fede
Compare
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 #20035
Summary
Implement the UI and UX flow inside the wallet settings for scanning a missing key pair's QR code.
Areas that maybe impacted
Test note
To have a missing key pair on Device B, after you pair devices you can add new keypair on Device A and the on device B logout/login again. you will see missing keypair on device B
Steps to test
Result
2024-05-23.18.38.10.mp4
status: ready