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
mmilad75
requested review from
shivekkhurana,
ulisesmac,
ilmotta and
J-Son89
and removed request for
shivekkhurana and
ilmotta
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.
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.
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.
I added the tests @ilmotta
Makes sense @mmilad75, let's keep it and if it's undesired, then we'll remove it from everywhere 👍 |
| @@ -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.
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.
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.
Here we don't need the anonymous function because there's only one assoc-in.
87% of end-end tests have passedFailed 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 Assetscreen in order to skip it, the result of it was null, so the skipping was ignored and theSelect Assetwas 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