-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The select assets screen is shown when token is going to be sent which available on both watch-only and regular account #19745 #20050
Conversation
Jenkins BuildsClick to see older builds (24)
|
Hello @mmilad75 Could you please write a description about what is being solved and what was the strategy applied to solve it? I can see the UI shows 0 Uniswap but it still displays one account in the cc: @J-Son89 |
@@ -1,10 +1,10 @@ | |||
(ns status-im.contexts.wallet.wallet-connect.utils | |||
;; NOTE: Not sorting namespaces since @walletconnect/react-native-compat should be the first | |||
#_{:clj-kondo/ignore [:unsorted-required-namespaces]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linting is auto updating this btw 🦀
yep I agree, although the issue often captures the description well. imo it's best to capture in a description what the pr does become some details can change and it makes it easier for the reviewer and the QA to get a quick context of what is happening/ what to check etc 👍 |
Not related to this PR in particular but in the video it is noticeable that we are showing the X button at the top-left in screens when action is navigate back. We should show an X only when the action is dismissing the modal (in this case, only the |
If we want to skip the assets with 0 balance, it should be handled in every situation. I don't think it's a good idea to skip it in one place and leave it as the same in other places. WDYT? @ulisesmac @J-Son89 |
I added the description |
Thanks for noticing it. I will fix it in this PR |
:<- [:wallet/accounts-without-watched-accounts] | ||
:<- [:wallet/wallet-send-token-symbol] | ||
:<- [:wallet/wallet-send-token] | ||
(fn [[accounts token-symbol token]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey guys, historically, I find myself having to comment too frequently on PRs about missing unit tests for layer-3 subs. This recommendation is in our guidelines and I believe we should be more strict about following it (reasons below).
Test layer-3 subscriptions by
actually subscribing to them, so reframe's signal graph gets validated too.
These tests are powerful because we test using the subscription machinery on purpose instead of just testing the sub handler. Our macro deftest-sub
helps with that. This means that if any of the input subscriptions, say :wallet/wallet-send-token
change, the output of its calculation will be picked up by the tests of this sub :wallet/accounts-with-current-asset
.
This means that by covering layer-3 subs with unit-tests, we are covering the subscription graph of the app, and they can all affect each other, giving us a useful layer of protection.
cc @J-Son89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the tests @ilmotta
Makes sense @mmilad75, let's keep it and if it's undesired, then we'll remove it from everywhere 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mmilad75
The fix looks good, actually it was a pending taks after I merged the wizard PR, so thank you for taking care of it 👍
@@ -12,6 +12,14 @@ | |||
(use-fixtures :each | |||
{:before #(reset! rf-db/app-db {})}) | |||
|
|||
(def ^:private accounts-with-tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick side note @mmilad75. In theory, we shouldn't worry about encapsulation (private/public vars) in namespaces that won't ever be required elsewhere (like test namespaces) because there's no API to be exposed. The same argument works for preview namespaces. In these namespaces, everything can be public. Consider that nobody needs to refactor a component preview namespace or a test namespace and take in consideration vars exposed from them.
cc'ing @ulisesmac @briansztamfater just to give some visibility :)
(assoc-in [:wallet :accounts] accounts-with-tokens) | ||
(assoc-in [:wallet :ui :send :token] {:symbol "ETH"}))) | ||
(let [result (rf/sub [sub-name])] | ||
(is (match? result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this repo, the majority of the assertions are (= expected actual)
or (match? expected actual)
not the other way around as it's used here. It's just a consistency thing, not that one is way is wrong. Although in the newest version of clj-kondo, it even has a new linter that will check the expected argument appears first by default (https://github.com/clj-kondo/clj-kondo/blob/master/doc/linters.md#equals-expected-position). We can't use this linter version yet.
|
||
(testing "returns the accounts list when there is no current asset" | ||
(swap! rf-db/app-db | ||
#(-> % |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't need the anonymous function because there's only one assoc-in.
87% of end-end tests have passed
Failed tests (4)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Thank you @mmilad75, PR can be merged! |
fixes #19745
The issue was after selecting an asset to send, all of the accounts was shown, even if an account didn't have the selected asset. When selecting an account without having the selected asset, when checking the assets of the selected account in the
Select Asset
screen in order to skip it, the result of it was null, so the skipping was ignored and theSelect Asset
was rendered and shown.Now by removing the accounts without the selected asset, the issue is fixed.
Screen.Recording.2024-05-15.at.21.36.23.mov