feat: add onTx status callbacks #65
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // completed trade's impact is reflected in future fetched trades. | ||
| setOldestValidBlock(receipt.blockNumber) | ||
| }) | ||
| .catch((e) => onTxFail?.(e, transaction.hash)) |
There was a problem hiding this comment.
If a transaction is cancelled by the user, it gets caught here. Does that count as a tx failing?
|
one thing I'm not sure about is all the different cases in which txs might fail -- too low gas, cancelled, contract execution error, etc -- and how to test them? |
| @@ -129,12 +132,13 @@ export default function useSendSwapTransaction( | |||
| throw new Error(t`Transaction rejected.`) | |||
| } else { | |||
| // otherwise, the error was unexpected and we need to convey that | |||
| console.error(`Swap failed`, error, address, calldata, value) | |||
| console.error(`Swap failed`, error, calldata, value) | |||
| onTxFail?.(error, error?.transactionHash) | |||
There was a problem hiding this comment.
this isn't necessarily an ethereum tx fail issue -- it would throw here if we haven't formed a valid tx and therefore can't submit it
should we still call onTxFail here?
| // completed trade's impact is reflected in future fetched trades. | ||
| setOldestValidBlock(receipt.blockNumber) | ||
| }) | ||
| .catch((e) => onTxFail?.(e, transaction.hash)) // Do we run onTxFail on tx cancelled? |
There was a problem hiding this comment.
this isn't necessarily an ethereum tx fail — the ethers library throws an error on cancelled tx, which is what we're catching here
There was a problem hiding this comment.
I don't think we should call onTxFail in this case. @akarys92 thoughts?
(aside-in the cancellation case we wouldn't have a transaction.hash here)
src/hooks/transactions/index.tsx
Outdated
| }) | ||
| oldTxs.current = pendingTransactions | ||
| } | ||
| }, [pendingTransactions, onTxSubmit]) |
There was a problem hiding this comment.
pendingTransactions is just txs[our current active chainId]
using pendingTransactions as opposed to txs from const txs = useAtomValue(transactionsAtom) to mirror how onReceipt is used https://github.com/Uniswap/widgets/blob/main/src/hooks/transactions/updater.tsx#L75
There was a problem hiding this comment.
nit: can you update the variable names new... and old... for clarity? currentPendingTransactions, oldPendingTransactions, etc.
|
|
||
| throw new Error(t`Swap failed: ${swapErrorToUserReadableMessage(error)}`) | ||
| console.error(`Swap failed`, error, calldata, value) | ||
| throw new Error(t`Swap failed: ${swapErrorToUserReadableMessage(error)}`) // FIXME: this prints to console as [object Object] |
There was a problem hiding this comment.
separate small PR here to fix this
#66
src/hooks/transactions/index.tsx
Outdated
| }) | ||
| oldTxs.current = pendingTransactions | ||
| } | ||
| }, [pendingTransactions, onTxSubmit]) |
There was a problem hiding this comment.
nit: can you update the variable names new... and old... for clarity? currentPendingTransactions, oldPendingTransactions, etc.
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adding onTxSubmit/Success/Fail callbacks