Speed up safe creation #2473
Speed up safe creation #2473
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
There was a problem hiding this comment.
I think it's good enough as a bug fix at this point.
I've left some comments but I remember, @juampibermani, you mentioned you had ideas for a more significant refactoring of safeCreate. So perfecting this bug fix might be a dead end. I'd love to hear your refactoring ideas.
| @@ -137,6 +137,8 @@ export const SafeDeployment = ({ | |||
| (err: Error) => { | |||
| if (isTxPendingError(err)) { | |||
| dispatch(enqueueSnackbar({ ...NOTIFICATIONS.TX_PENDING_MSG })) | |||
| } else { | |||
| dispatch(enqueueSnackbar({ ...NOTIFICATIONS.CREATE_SAFE_FAILED_MSG })) | |||
There was a problem hiding this comment.
I think it wasn't being done before because the interface itself shows an error.
There was a problem hiding this comment.
I didn't see the notification
There was a problem hiding this comment.
There was no notification, I meant that the page itself shows an error graphic and suggests to retry.
Notification doesn't hurt though.
src/routes/open/container/Open.tsx
Outdated
| } | ||
| } | ||
| }, 500) | ||
| } |
There was a problem hiding this comment.
Looks good! It would be nice to extract this into a utility so that this setTimeout loop code doesn't clutter the safe creation function.
Signature suggestion: waitWithTimeout(comparator, onSuccess, onError, delay = 500, maxTimes = 1200).
There was a problem hiding this comment.
I solved it in a simpler way, you can see the change in the last commit
| // transaction found | ||
| return txMonitor({ sender, hash, data, nonce: transaction.nonce, gasPrice: transaction.gasPrice }, cb, options) | ||
| } else { | ||
| return txMonitor({ sender, hash, data }, cb, options) |
There was a problem hiding this comment.
It looks like txMonitor itself also implements a recursive timeout loop.
So we could use waitWithTimeout here as well, and use a promise instead of the callback:
export const txMonitor = (txMonitorProps) => {
return new Promise((resolve, reject) => {
waitWithTimeout(() => { ... }, resolve, reject)
})
}
There was a problem hiding this comment.
I solved it in a simpler way, you can see the change in the last commit
|
I tried this twice and in both times it failed. gif: |
| if (nonce === undefined || gasPrice === undefined) { | ||
| // this block is accessed only the first time, to lookup the tx nonce and gasPrice | ||
| // find the nonce for the current tx | ||
| const transaction = await web3ReadOnly.eth.getTransaction(hash) |
There was a problem hiding this comment.
There are several places like this in txMonitor, where we don't catch a potential async exception.
I think txMonitor should return a promise instead of taking a callback. Then we can try-catch each await call and reject the returned promise in the catch.
* Refactor * Add unit tests
|
I tried creating a safe with the lowest gas MM was suggesting (the slow setting) and then as soon as it was possible I sped it up with the "fast gas setting". Note: I won't report this as an issue since it is a really uncommon situation, still @dasanra and @juampibermani I leave it here you know the case exists. I will pass the ticket |
I think we're supposed to clean up some localStorage cache when the creation succeeds. |
|
Tried making the tx fail on purpose by setting the amount of gas really low. Tested by creating a safe normally (suggested gas price and amount) and cancelled the tx in MM right away, setting the gas price in "fast" so it would make sure the cancel gets mined first. Issue to solve: snapshot: every call is waiting for the original tx and not the cancellation. |
Just to make sure, this is the case in the current dev as well, isn't it? |
What it solves
Resolves #2362
How this PR fixes it
Waits for either the speed up tx or the original tx
How to test it