fix: Multichain: UX: Check for transactions on all networks and QueuedRequestCount #25614
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [ca54b8e]
Page Load Metrics (227 ± 217 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25614 +/- ##
===========================================
+ Coverage 69.60% 69.60% +0.01%
===========================================
Files 1364 1364
Lines 48172 48183 +11
Branches 13291 13293 +2
===========================================
+ Hits 33526 33537 +11
Misses 14646 14646 ☔ View full report in Codecov by Sentry. |
| @@ -1778,7 +1779,9 @@ export function getShowRecoveryPhraseReminder(state) { | |||
| * @returns Number of unapproved transactions | |||
| */ | |||
| export function getNumberOfAllUnapprovedTransactionsAndMessages(state) { | |||
There was a problem hiding this comment.
can we also rename this variable to numberOfAllUnapprovedTransactionsAndMessages?
|
LGTM after the one comment |
Builds ready [008fee2]
Page Load Metrics (77 ± 12 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -1778,7 +1779,9 @@ export function getShowRecoveryPhraseReminder(state) { | |||
| * @returns Number of unapproved transactions | |||
| */ | |||
| export function getNumberOfAllUnapprovedTransactionsAndMessages(state) { | |||
There was a problem hiding this comment.
I see that this selector is used for the routes page prop unapprovedTransactions. This prop name was already confusing before now for a few reasons: it's just the count, not the transactions themselves, and it's not just transactions (it includes non-transaction confirmations).
But it's slightly more confusing now that we're including unapproved confirmations across all chains. Perhaps we should take this opportunity to make the prop name more clear? e.g. something like totalUnapprovedConfirmationCount
darkwing
force-pushed
the
multichain-all-transactions-check
branch
from
008fee2
to
9d1458a
Compare
|
Builds ready [9d1458a]
Page Load Metrics (144 ± 167 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
release-12.2.0
|
Missing release label release-12.0.0 on PR. Adding release label release-12.0.0 on PR and removing other release labels(release-12.2.0), as PR was cherry-picked in branch 12.0.0. |
Description
There's a case where there could be a race condition between the
QueuedRequestController's network switching and the Routes' network switching. The root issue is that the use ofgetUnapprovedTransactionsonly gets transactions on thecurrentchain, thus triggering Routes to switch networks once the last transaction of the current chain is done, but there could be more transactions on other chains which we should allow QRC to handle network switching for.Thus, this PR lets QRC control network switching until there are absolutely no transactions or queued requests left.
This PR includes an **E2E** for the scenario where there are multiple queued transactions but since this is a race issue, it's difficult to reproduce in a provable E2E
Related issues
Possibly related: #25596
Manual testing steps
Tab 1, connect toUniswaponEthereum MainnetTab 2, connect toPancakeSwaponBNB ChainTab 3, connect toTest DapponSepoliaTab 1andTab 2BUT DO NOT CONFIRM IT, JUST MOVE ON TO THE NEXT TABTab 3BUT DO NOT CONFIRM ITTab 1(Uniswap) onEthereum Mainnet; confirm or reject it. See the confirmation window closePancakeSwap/Tab 2confirmation onBNBchain; confirm or reject it. See the confirmation window closeTab 3/Test Dappsend onSepolia. Confirm or reject it.Create transactions on different networks, reject and/or confirm them, click around between popup and notification window, ensure nothing unexpected occurs
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist