feat: speedup pending transactions #3425
Conversation
Branch preview✅ Deploy successful! |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
997.54 KB (🟡 +433 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Eight Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/apps/open |
78.52 KB (🟡 +11.72 KB) |
1.05 MB |
/balances |
29.91 KB (🟡 +2 B) |
1 MB |
/home |
57.38 KB (🟡 +1 B) |
1.03 MB |
/transactions |
103.71 KB (🟡 +11.72 KB) |
1.08 MB |
/transactions/history |
103.67 KB (🟡 +11.72 KB) |
1.08 MB |
/transactions/messages |
63.38 KB (🟡 +11.72 KB) |
1.04 MB |
/transactions/queue |
58.98 KB (🟡 +11.72 KB) |
1.03 MB |
/transactions/tx |
48.33 KB (🔴 +11.72 KB) |
1.02 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1408 tests passing in 196 suites. Report generated by 🧪jest coverage report action from e10be15 |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
compojoom
force-pushed
the
feat/pending-txs
branch
from
69f078d
to
9d0ae84
Compare
ESLint Summary View Full Report
[warning] react-hooks/exhaustive-deps
Report generated by eslint-plus-action |
There was a problem hiding this comment.
Works really nice already! As discussed with @compojoom we need to find a way to clean up pending transactions if they were sped-up from outside the UI. Our idea is to check the txHistory whenever it updates and compare the newest nonce with the pending tx nonce. If its the same we can remove the pending tx.
I also noticed that the notifications in Figma have a linear progress bar. Did we decide not to implement that?
| <Alert | ||
| severity="warning" | ||
| icon={<SvgIcon component={Rocket} />} | ||
| action={<Button onClick={() => setOpenSpeedUpModal(true)}>{`Speed up >`}</Button>} | ||
| > |
There was a problem hiding this comment.
The Alert box in Figma looks sligthly different i.e. no border, no title and the action is centered vertically
We decided to not go with sticky notifications yet as we have too many popups / overlays already. Focusing on the new logic by just adding speed up buttons to all components currently showing pending txs. |
| switch (status) { | ||
| case PendingStatus.PROCESSING: | ||
| case PendingStatus.RELAYING: | ||
| StatusComponent = <ProcessingStatus txId={txId} pendingTx={pendingTx} /> | ||
| break | ||
| case PendingStatus.INDEXING: | ||
| StatusComponent = <IndexingStatus /> | ||
| break | ||
| default: | ||
| StatusComponent = <DefaultStatus error={error} /> |
There was a problem hiding this comment.
The StatusMessage component has the same switch cases. Is there a reason we are not using it anymore? All three of these components seem to have the same DOM structure.
There was a problem hiding this comment.
the statusMessage component was packing everything in a blue container (the message: "this tx was confirmed and is now ..."), which according to @TanyaEfremova was not correct. I couldn't figure out a way how to refactor it to accomodate this, so I went for writing as is now in the successScreen.
The StatusMessage component is actually no longer used anywhere. If you have ideas how to make it work with the new requirements let me know, otherwise I would actually remove it.
There was a problem hiding this comment.
I see that you use a similar StatusMessage component in safe creation. The main difference is in the steps and a bit in the UI. The problem here is that if we are in the processing state we need the pendingTx to display the speed up. So it is no longer possible to just pass a status and be done with it. If you have some ideas how to abstract this let me know.
There was a problem hiding this comment.
We could render the pendingTx part as a child i.e.
<StatusMessage status={status} error={error}>
{pendingTx && <SpeedUpMonitor txId={txId} pendingTx={pendingTx} modalTrigger={'alertBox'} />}
</StatusMessage>
That way we could reuse all the other elements and their styles.
There was a problem hiding this comment.
I thought about this, but then the speedUp is only shown if you we are processing. With this change we are passing it as if it was a generic child, so someone can pass something else and would think that it's a child component that renders below the StatusMessage. The speedup is tightly coupled to the processing state and generalizing the component like this feels awkward. At least to me. I understand the need for a general MessageStatus component, but can't think of a pattern that I like.
There was a problem hiding this comment.
How about having it inside StatusMessage and only rendering it if status == PROCESSING then?
ESLint Summary View Full Report
Report generated by eslint-plus-action |
compojoom
force-pushed
the
feat/pending-txs
branch
from
33b5daf
to
538e5fe
Compare
| if (results.filter((item) => isTransactionListItem(item)).length === 0) { | ||
| results.splice(0, 1) | ||
| } | ||
| } | ||
|
|
||
| if (results[1] && isConflictHeaderListItem(results[1])) { | ||
| // Check if we both conflicting txs are still pending | ||
| if (results.filter((item) => isTransactionListItem(item)).length <= 1) { | ||
| results.splice(1, 1) | ||
| } |
There was a problem hiding this comment.
Not sure I understand these two conditions. If the first element is a label and there is nothing else we remove the label. Why not check for results.length === 1 in that case?
For the second condition I don't see how it is connected to pending transactions. If we filter all of them out anyway does it matter if one or two are pending?
There was a problem hiding this comment.
You could execute the same tx twice. Once with i.e. metamask and once with e.g. ledger. So there is in theory the possibility to have conflicting pending txs. But if only one of the untrusted queue txs is pending we have to remove the conflicting tx label.
compojoom
force-pushed
the
feat/pending-txs
branch
from
f433c18
to
e94bdb5
Compare
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
The execute button looks broken in this PR. It doesn't look like that in current dev: EDIT: |
|
Question. If the tx is sped up in the wallet itself (like MM extension or MM mobile), should the safe app catch that and show the updated tx hash? |
Unfortunately this is not possible. If the user speeds up in MM we don't know this. We only find out about it later when suddenly a tx is included on the chain with the same nonce. |
|
An issue I have with MM mobile. Speeding up the tx in the Safe App creates a 2nd transaction in the MM phone app that is meant to replace the first one. This makes the safe app to get stuck waiting for that sped up transaction Steps to make it clearly:
Speeding it up in the Safe app
|
|
Question: In polygon the speed up option doesn't show up. Waited for like 5 mins. Is the feature not present there? (I've checked the admin and I didn't see a feature toggle for it) |
|
Execution of a batched tx works fine, but looks confusing. The speed up feature show in all of them and if you use one it just dissapears from one of the tx Idk if there is anythign here to fix without design helping out Note: This is an edge case since we don't allow gas fee edition in tx in bulk, you have to edit it manually in your wallet. So this is low prio
There are less tx in this snapshot because it was a 2nd attempt, but it happened the same for the 3 tx above |
This is a bug in MM Mobile. I've reported it here MetaMask/metamask-mobile#9118 |
compojoom
force-pushed
the
feat/pending-txs
branch
from
7ccb9b0
to
881a21b
Compare
Co-authored-by: Usame Algan <5880855+usame-algan@users.noreply.github.com> Co-authored-by: Daniel Dimitrov <daniel.d@safe.global>
compojoom
force-pushed
the
feat/pending-txs
branch
from
881a21b
to
4a98635
Compare
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
@francovenica I just rebased, please try again once a new version is deployed. With the latest changes that @schmanu did you should be able to speed up txs on polygon as well. The problem there was that we were polling the rpc node for the transaction status, but if the transaction is submitted with a too low gas then no RPC is indexing the TX. Now, we are storing the tx data locally and are able to offer speed up even if the RPC doesn't know the TX. |
|
@compojoom @schmanu Can we fix the visual issues like this one? so we can create the RC |
|
Now that Poly works I decided to try a few phone apps 1inch: Rainbow: Trust: Also this trust app has issues. The tx doesn't go through in our app and a ton of errors show up in the console, but in the blockexplorer you can see the tx actually executed just fine Conclussion: |
|
Btw, just tested with Rabby and it has the same issue as MM Mobile. It ignores the nonce we provide and instead sets one higher. This leads to a stuck tx. The user would have to speed up the first tx and then the second will execute. |
| @@ -0,0 +1,19 @@ | |||
| import { useEffect, useState } from 'react' | |||
|
|
|||
| export const useCounter = (startTimestamp: number | undefined) => { | |||
There was a problem hiding this comment.
Can't we use the existing useIntervalCounter?
| ? { | ||
| chainId, | ||
| safeAddress, | ||
| txId, | ||
| status: PendingStatus.PROCESSING, | ||
| txHash: detail.txHash, | ||
| signerAddress: detail.signerAddress, | ||
| signerNonce: detail.signerNonce, | ||
| submittedAt: Date.now(), | ||
| txType: PendingTxType.CUSTOM_TX, | ||
| data: detail.data, | ||
| to: detail.to, | ||
| } | ||
| : { | ||
| chainId, | ||
| safeAddress, | ||
| txId, | ||
| status: PendingStatus.PROCESSING, | ||
| txHash: detail.txHash, | ||
| signerAddress: detail.signerAddress, | ||
| signerNonce: detail.signerNonce, | ||
| submittedAt: Date.now(), | ||
| gasLimit: detail.gasLimit, | ||
| txType: PendingTxType.SAFE_TX, | ||
| } |
There was a problem hiding this comment.
Is it possible to reduce the code duplication here?
| export const dispatchSafeTxSpeedUp = async ( | ||
| txOptions: Omit<TransactionOptions, 'nonce'> & { nonce: number }, | ||
| txId: string, | ||
| onboard: OnboardAPI, | ||
| chainId: SafeInfo['chainId'], | ||
| safeAddress: string, | ||
| ) => { | ||
| const sdkUnchecked = await getUncheckedSafeSDK(onboard, chainId) | ||
| const eventParams = { txId } | ||
| const wallet = await assertWalletChain(onboard, chainId) | ||
| const signerNonce = txOptions.nonce | ||
|
|
||
| // Execute the tx | ||
| let result: TransactionResult | undefined | ||
| try { | ||
| const safeTx = await createExistingTx(chainId, safeAddress, txId) | ||
| result = await sdkUnchecked.executeTransaction(safeTx, txOptions) | ||
| txDispatch(TxEvent.EXECUTING, eventParams) | ||
| } catch (error) { | ||
| txDispatch(TxEvent.SPEEDUP_FAILED, { ...eventParams, error: asError(error) }) | ||
| throw error | ||
| } | ||
|
|
||
| txDispatch(TxEvent.PROCESSING, { | ||
| ...eventParams, | ||
| txHash: result.hash, | ||
| signerAddress: wallet.address, | ||
| signerNonce, | ||
| gasLimit: txOptions.gasLimit, | ||
| txType: 'SafeTx', | ||
| }) | ||
|
|
||
| const provider = getWeb3ReadOnly() | ||
|
|
||
| if (provider) { | ||
| // don't await as we don't want to block | ||
| waitForTx(provider, [txId], result.hash, safeAddress, wallet.address, signerNonce) | ||
| } | ||
|
|
||
| return result.hash | ||
| } | ||
|
|
||
| export const dispatchCustomTxSpeedUp = async ( | ||
| txOptions: Omit<TransactionOptions, 'nonce'> & { nonce: number }, | ||
| txId: string, | ||
| to: string, | ||
| data: string, | ||
| onboard: OnboardAPI, | ||
| chainId: SafeInfo['chainId'], | ||
| safeAddress: string, | ||
| ) => { | ||
| const eventParams = { txId } | ||
| const wallet = await assertWalletChain(onboard, chainId) | ||
| const signerNonce = txOptions.nonce | ||
| const web3Provider = createWeb3(wallet.provider) | ||
| const signer = await web3Provider.getSigner() | ||
|
|
||
| // Execute the tx | ||
| let result: TransactionResponse | undefined | ||
| try { | ||
| result = await signer.sendTransaction({ to, data, ...txOptions }) | ||
| txDispatch(TxEvent.EXECUTING, eventParams) | ||
| } catch (error) { | ||
| txDispatch(TxEvent.SPEEDUP_FAILED, { ...eventParams, error: asError(error) }) | ||
| throw error | ||
| } | ||
|
|
||
| txDispatch(TxEvent.PROCESSING, { | ||
| txHash: result.hash, | ||
| signerAddress: wallet.address, | ||
| signerNonce, | ||
| data, | ||
| to, | ||
| groupKey: result?.hash, | ||
| txType: 'Custom', | ||
| }) | ||
|
|
||
| const provider = getWeb3ReadOnly() | ||
|
|
||
| if (provider) { | ||
| // don't await as we don't want to block | ||
| waitForTx(provider, [txId], result.hash, safeAddress, wallet.address, signerNonce) | ||
| } | ||
|
|
||
| return result.hash | ||
| } |
There was a problem hiding this comment.
Again, these are extremely similar, possible to exract the common part?
There are a lot of wallets out there that ignore the specified nonce. If we offer the option to speed up when the tx is not signed we can end up with multiple transactions in the wallet. The moment the user manages to speed the one tx that is stuck all subsequent txs will be executed and this could lead to potential loss of funds(funds sent multiple times). Because of this we no longer display the speed up option, but rather ask the user to manually speed up through the wallet.
|
The ticket is not in QA, but still I've checked the fix regarding the buttons positions and looks good. I don't agree with the last commit. I think a big point of this feature is that wallets that do not have the speed up option like physical devices (trezor/ledger) have now the chance of speeding it up. |
|
@francovenica speeding up is not disabled for hw wallets. It's only disabled for immediately executed tx (unsigned txs) with all wallets. |
|
@katspaugh I'm aware of that. but most people in safes 1/1 dont' sign, queue the tx and then execute, they execute right away, making these particular trezor/ledger users prone to see a message saying "go and speed it up in your wallet" whne they cannot do that. I still think the risk we are trying to avoid are not worth the disconfort of these users. |
|
I see, thanks for clarifying. That's a fair point. However, consider the following:
|
What it solves
Stuck transactions due to moving gas prices disappear after a 1 minute timeout.
Resolves #3107
How this PR fixes it
maxPriorityFeePerGasby twomaxFeePerGasaccordinglyHow to test it
Screenshots
Checklist