fix(wallet)_: token supported networks #21451
Conversation
Jenkins BuildsClick to see older builds (36)
|
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
015a233 to
f59d0c0
Compare
smohamedjavid
requested review from
alwx,
shivekkhurana,
vkjr,
briansztamfater,
clauxx and
mohsen-ghafouri
There was a problem hiding this comment.
Hi @smohamedjavid thanks for you PR!
I added some minor comments related to the code.
There was a problem hiding this comment.
hey @smohamedjavid, I noticed this PR adds changes in similar places as this one, just as an FYI.
Also, wondering if it's possible to isolate the changes in one place only, for instance: when we get the token data from status-go, can we update balances-per-chain to include the missing chains? I may be missing context here though, just wondering if the other changes are necessary.
@clauxx - thanks for the heads up 👍 I guess I missed the context. I was under the impression that we need to display the supported networks below each token as we used to display (depending on the balance-per-chain key). I guess I will change it. Please merge your PR and I will make changes on top of it.
That was my first thought too. I was under the impression that the status-go returns the balance even if it's zero in supported chains for all tokens, but it's different for non-default tokens ( I will quickly add the missing chain while we save it to the app-db. I need to ensure the token list (as we depend on it for supported chains for each token) is fetched before we fetch the tokens for each account so that we can add any missing chains in balance-per-chain. I will make a quick update. |
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
f59d0c0 to
f7dc535
Compare
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
f7dc535 to
7e5df76
Compare
smohamedjavid
requested review from
ulisesmac,
clauxx and
vkjr
and removed request for
ulisesmac
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
86c4705 to
e42042e
Compare
| {:sources (:sources tokens) | ||
| :supported-chains-by-symbol supported-chains-by-symbol | ||
| :by-address (-> tokens :by-address vals) | ||
| :by-symbol by-symbol-vals}) |
There was a problem hiding this comment.
The only thing that confuses me a little is that we're saving:supported-chains-by-symbol inside a structure called :tokens
Maybe it'd be better for it all to be saved in [:wallet :chains-by-symbol] instead?
There was a problem hiding this comment.
Since we group the chains IDs (by symbol) depending on the tokens list, I felt placing that data under the :wallet :tokens seems reasonable.
| :by-address (-> tokens :by-address vals) | ||
| :by-symbol by-symbol-vals}) |
There was a problem hiding this comment.
Not a change you introduced, but what's the point of grouping the tokens by symbol and by address (as a map of symbols/tokens and token data), if we end up dismissing that and use the vector of token data? I assume it was originally done to make extraction of token data easier/quicker, without having to go through the entire list of tokens. Here though, it seems like we are storing the tokens as a vector in the db three times, and it's not clear to me why. The token list is pretty large, so storing it 3 times seems excessive tbh.
There's also quite a bit of data here that we're fetching/storing, which I guess was added for swaps, but I'm not sure if this data ends up being used anywhere. Specifically, :details-per-token, :prices-per-token, :market-values-per-token.
cc: @status-im/wallet-mobile-devs (in case anyone knows the reason)
There was a problem hiding this comment.
I guess @alwx is the best person to answer.
There was a problem hiding this comment.
The original idea was to copy this logic from desktop :) It was part of a bigger issue and I assumed that it's better to do all of it at once.
88% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
|
Hi @smohamedjavid thank you for PR. take a look at found issue: ISSUE 1: Network drawer shows networks with 0 balancesSteps:
Actual result:The network drawer displays networks where the asset isn't present. Example: I have Chainlink on Optimism only, but I see Mainnet and Arbitrum in the network selection drawer as well networks_pr.mp4Expected result:The drawer should only show networks where the selected asset exists. In case if user has assets only on one network, then current user should be navigated directly to the swap page. See on nightly networks_night.mp4Devices:Pixel 7a, Android 13 |
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
a910aef to
c920da6
Compare
|
@VolodLytvynenko - Thanks for testing the PR 🙏 . The issue 1 should be fixed now. Please retest. |
ISSUE 2: 'Not available' shown in TO section if token isn't available on selected networkSteps:
Actual result:User can't build a bridge route on the send page for a network where the asset isn't available. Example: In my case the user has DAI on the L1 network, but in the "send to" page, Optimism and Arbitrum are shown as 'Not available' in the TO section.
Expected result:The networks should be shown as available on send to page in case if user even does not have assets on current networks OS:IOS, Android Devices:
|
|
@VolodLytvynenko - The issue 2 should be fixed now. Please retest. 🙏 |
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
6e59d72 to
435fb40
Compare
62% of end-end tests have passedFailed tests (3)Click to expandClass TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Passed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
@smohamedjavid Thank you for fixes. Unfortunatly one more place related to this issue is introduced after last commit, however can be considered as follow up PR_ISSUE 3: Token shown as 'Not available' in "edit preferences" drawer despite being supportedSteps::
notsup.mp4Actual result:The token is shown as 'Not available' in the "edit preferences" drawer, even though the token is supported on the selected networks.
Expected result:The token should be shown as supported in the "edit preferences" drawer if it is supported on those networks Devices:
|
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
435fb40 to
6090a53
Compare
|
@VolodLytvynenko - Issue 3 should be fixed now. Please retest. 🙏 |
|
Unfortunately can't understand the root cause of this issue. I will send you the user and the assets where this happens PR_ISSUE 4: 'Not available' still shown in TO sectionActual result:
Devices:
|
Signed-off-by: Mohamed Javid <[email protected]>
|
@VolodLytvynenko - Issue 4 should be fixed now. Please retest 🙏 |
75% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
0% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
|
hi @smohamedjavid thank you for PR. No issues from my side. PR can be merged |
This commit - fixes the networks/chains supported by the token based on the token list fetched from status-go instead of relying on the balance-per-chain map as status-go returns balance for chains only if there is a positive balance - adds supported-networks key to token data map for network details Signed-off-by: Mohamed Javid <[email protected]>
This commit - fixes the networks/chains supported by the token based on the token list fetched from status-go instead of relying on the balance-per-chain map as status-go returns balance for chains only if there is a positive balance - adds supported-networks key to token data map for network details Signed-off-by: Mohamed Javid <[email protected]>
fixes #20988
Summary
This PR fixes the networks/chains supported by the token based on the token list fetched from status-go instead of relying on the
balance-per-chainmap as status-go returns balance for chains only if there is a positive balance.Platforms
Areas that may be impacted
Functional
Steps to test
Prerequisites: A wallet account with a token balance on one network
Wallet > Account > BridgeWallet > Account > SendNot availabletext is not shown on the receiver sidestatus: ready