fix: tempo disable confirm button until gas autoselect#28722
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. |
281323b to
1b844da
Compare
1b844da to
a48f2e0
Compare
a48f2e0 to
d5f98e2
Compare
d5f98e2 to
cc20675
Compare
cc20675 to
972e979
Compare
972e979 to
29c57f3
Compare
29c57f3 to
40efe92
Compare
1a72f8f to
d21da8d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28722 +/- ##
=========================================
Coverage 82.14% 82.15%
=========================================
Files 4949 5000 +51
Lines 130070 131267 +1197
Branches 29004 29289 +285
=========================================
+ Hits 106851 107840 +989
- Misses 15923 16087 +164
- Partials 7296 7340 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d21da8d to
5aad6bc
Compare
5aad6bc to
84f14f2
Compare
84f14f2 to
519d6d7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 519d6d7. Configure here.
519d6d7 to
5f8bb07
Compare
5f8bb07 to
164017b
Compare
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: The changes in this PR are focused on the confirmations system with gasless transaction support:
Tag Selection Rationale:
Not selected:
Performance Test Selection: |
|
✅ E2E Fixture Validation — Schema is up to date |
|
|
Tested locally, fixed, adding the "qa passed" label. |
| return isRelaySupported(chainId as Hex); | ||
| }, [chainId, shouldCheck7702Eligibility]); | ||
|
|
||
| const { data: relaySupportsChain, isFetching: relayPending } = useQuery({ |
There was a problem hiding this comment.
What's the benefit of switching to useQuery here? Is this required?
Any caching should be handled by the server and browser / os based on caching headers.
There was a problem hiding this comment.
The problem was that useAsync was not properly managing the { enabled } mode with the shouldCheck7702Eligibility && Boolean(chainId) condition. The hook go through cycles where chainId has some value, then hasn't, then has some value again.
I first tried to change useAsync so it would behave more that useQuery, then decided it was too risky cause useAsync is used in multiple places.
Then I tried to wrap the shouldCheck7702Eligibility && Boolean(chainId) condition manually in useIsGaslessSupported but I kept getting race conditions.
useQuery on the other hand worked immediately and exacted as one would expect when reading the code. So I went for it - after 1/2 hrs of die and retry.
There was a problem hiding this comment.
Then the deeper issue is why the chainId is ever not set for an active transaction?
It should be available on the transaction meta for its entire lifetime, why doesn't the same behaviour happen in extension?
There was a problem hiding this comment.
Then the deeper issue is why the chainId is ever not set for an active transaction?
I think this never happens, but that transactionMeta is not always set. Maybe I was wrong about that one re-becoming undefined but at least one variable was doing back-and-forth - I should have noted this one.
I'll start over and try to redo it but it might not be as bullet-proof as the useQuery solution. The current solutions works (as opposed as before). It is just not the original code.
| const isPending = | ||
| smartTransactionPending || (shouldCheck7702Eligibility && relayPending); | ||
| const is7702SupportedPending = shouldCheck7702Eligibility && relayPending; | ||
| const pending = smartTransactionPending || is7702SupportedPending; |
There was a problem hiding this comment.
Is this needed? If not, does this file need changing at all?
There was a problem hiding this comment.
No I can roll back those two lines if reducing the LoCs / amount of changes is import. I still want to fight for my useQuery though, unless you see a clear alternative: Only one I'd see otherwise is making the hook more complex by not using either useQuery or useAsync.
| isTransactionValueUpdating || | ||
| isPayLoading; | ||
| isPayLoading || | ||
| isGaslessLoading; |
There was a problem hiding this comment.
Does this have larger implications if mobile wasn't previously disabled while gas station was loading?
There was a problem hiding this comment.
Yes it might actually feel less flaky on slower devices - the "Confirm" button blinks much more on Mobile than on Extension, maybe due to isTransactionValueUpdating. If isGaslessLoading remains "true" longer than the time for isTransactionValueUpdating to go true then false for example.
But on Tempo this "blinking" is more than UX gimmick, it makes the tx fail if the user taps during one of those blinks.
There was a problem hiding this comment.
We wouldn't need any changes in Predict code if we didn't change to useQuery?
There was a problem hiding this comment.
No but I couldn't find any clean alternative.
I modified those 2 test files after making the changes in renderWithProvider.tsx
|
Dropped and replaced by #29188 |




Description
The goal of the change is to prevent a premature "enablement" for the Confirm button when a transaction is loading. There are pre-existing race conditions that allowed users to confirm a transaction before everything is fully loaded. Those conditions could make the tx skip gasless flows, falling back to native token for gas payment.
The impact is bigger on Tempo because using a native token is not an option (there is no native token on Tempo), which means that such race condition will always result in a failed tx (the tx won't be sent to the RPC or relayer).
The changes solve the race conditions cases, as well as adds some extra conditions for the loading when on a chain that has no native token.
Changelog
CHANGELOG entry: add new
isGaslessLoadingguard to Confirmation button - consistently with Extension.CHANGELOG entry:
useIsGaslessSupportedto useuseQueryinstead ofuseAsyncto avoid race conditions.Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/NEB-944?atlOrigin=eyJpIjoiYWFiY2U0NjU1ZjEzNGNmODlmOTZkNTQwNzM4N2MxMTUiLCJwIjoiaiJ9
Manual testing steps
Repeating instructions shared for Extension since Mobile flow will be very similar (just "tap" instead of click ;))
pathUSDand click on "Send".What should be observed is that despite spam-clicking, it should not be possible to "Confirm" the transaction "too soon" and it should always succeed no matter how "early" you click on "Confirm".
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Changes confirmation disablement logic around gasless support and switches gasless eligibility checks to React Query; could affect when users can confirm transactions and may introduce edge-case loading states on chains without native gas.
Overview
Prevents premature transaction confirmation by disabling the Confirm button while gasless support/fee-token auto-selection is still loading (
Footernow gates on newuseIsGaslessLoadingin addition to transaction pay loading).Adds
useIsGaslessLoadingto detect gasless-related loading (including Tempo-style no native token cases and mismatchedselectedGasFeeToken), and refactorsuseIsGaslessSupportedto use React Query (useQuery) for relay support checks to reduce race conditions.Test infrastructure is updated to wrap
renderWithProvider/renderHookWithProviderwith aQueryClientProvider, and relevant tests are adjusted/mocked accordingly (including new unit tests foruseIsGaslessLoading).Reviewed by Cursor Bugbot for commit 164017b. Bugbot is set up for automated code reviews on this repo. Configure here.