feat: recalculate network max value #19953
Merged
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
BalogunofAfrica
requested review from
vkjr,
briansztamfater,
J-Son89 and
ulisesmac
Jenkins BuildsClick to see older builds (57)
|
J-Son89
reviewed
May 9, 2024
| {token-symbol :symbol | ||
| token-networks :networks} (rf/sub [:wallet/wallet-send-token]) | ||
| enabled-from-networks (->> token-networks |
There was a problem hiding this comment.
enabled from networks seems like it should be a sub with this data instead? 🤔
J-Son89
reviewed
May 9, 2024
| (filter #(not (contains? (set disabled-from-chain-ids) | ||
| (:chain-id %)))) | ||
| set) | ||
| enabled-from-chain-ids (map :chain-id enabled-from-networks) |
There was a problem hiding this comment.
in fact if enabled-from-chain-ids is all you need than all of this computation should be done in the sub:
There was a problem hiding this comment.
No, its not just the enabled-from-chain-ids, I am also passing the enabled-from-networks to filter the chain icons that is displayed
J-Son89
reviewed
May 9, 2024
| @@ -249,11 +249,13 @@ | |||
| :wallet/current-viewing-account-tokens-filtered | |||
| :<- [:wallet/current-viewing-account] | |||
| :<- [:wallet/network-details] | |||
| (fn [[account networks] [_ query]] | |||
| (fn [[account networks] [_ query chain-ids]] | |||
There was a problem hiding this comment.
hmm is there not tests on this sub already?
If so we should probably add some coverage of this approach to test this behaviour too 👍
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
from
ca7ce32
to
01eaf38
Compare
|
Gonna test your pr shortly @BalogunofAfrica 🔍 |
|
So I don't think the max balances is showing right 🤔 
|
|
also not directly related to your pr but just the general UX on the send screen is very unclear to me. Not sure what others think on this but it is a bit confusing of what the expected behaviour is |
You have both Optimism and Arbitrum selected in the from routes. Both their balances adds up to 0.02127ETH |
Not sure about the exact steps to reproduce this (taps are not shown on the video and it is confusing), but if a chain is disabled, the network card should always be visible with value 0 and opacity, so if that is not happening it is a bug and I can look at it. If that is the case, feel free to file an issue with some steps to reproduce it and assign it to me. |
Closed
5 tasks
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
2 times, most recently
from
e2578af
to
9ef9abb
Compare
You have both Optimism and Arbitrum selected in the from routes. Both their balances adds up to 0.02127ETH |
J-Son89
approved these changes
May 10, 2024
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
6 times, most recently
from
d304283
to
04ecc49
Compare
|
@BalogunofAfrica I see you updated the PR to also cover #19910. Can you please update the description and steps to test for manual QA? Also a quick video with the updated changes would be useful 🙏 |
65% of end-end tests have passedFailed tests (16)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (34)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
from
04ecc49
to
c907f48
Compare
Done |
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
2 times, most recently
from
047eec5
to
4f0cad8
Compare
e2e failures are not PR related |
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
from
4f0cad8
to
7a53303
Compare
VolodLytvynenko
force-pushed
the
feat/recalculate-network-max-value
branch
from
7a53303
to
4949aa9
Compare
|
Hi @BalogunofAfrica thank you for your work. No issues from my side. PR is ready to be merged |
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
2 times, most recently
from
30d452c
to
ac19c87
Compare
BalogunofAfrica
force-pushed
the
feat/recalculate-network-max-value
branch
from
ac19c87
to
d1968f6
Compare
|
@VolodLytvynenko seems this PR introduced other bugs and I think it doesn't fully fix #19910 See this, we always display "No routes found" when user enters an invalid amount: Can you please verify? Maybe we should revert this PR, or introduce proper fixes in another PR. |
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
#19791
#19910
Summary
Recalculates the max value based on the chain the user select in the from column
Platforms
Functional
Steps to test
status: ready