Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Handle timeout error in safe creation #1293

Merged
merged 3 commits into from
Dec 5, 2022
Merged

fix: Handle timeout error in safe creation #1293

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
924425d
fix: Handle timeout error in safe creation
usame-algan Nov 30, 2022
f389100
fix: Failing tests, adjust isContractError
usame-algan Dec 1, 2022
6ecb3eb
chore: Add TODO
usame-algan Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hooks/useIsValidExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import ContractErrorCodes from '@/services/contracts/ContractErrorCodes'
import { sameAddress } from '@/utils/addresses'

const isContractError = <T extends EthersError>(error: T): error is T & { reason: keyof typeof ContractErrorCodes } => {
return Object.keys(ContractErrorCodes).includes(error.reason)
return Object.keys(ContractErrorCodes).includes(error.reason!)
Copy link
Member Author

@usame-algan usame-algan Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't bother with this as it will be removed with #1285 Actually not so we should handle it better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest something along the lines of:

  const isEthersError = <T extends Error | EthersError>(error: T): error is EthersError

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the function need to handle Error and EthersError if it is only used in one place? I think we should rework EthersError in general. Ethers v6 will add proper error types but for the time being I would suggest opting for a more simple solution e.g.

const isContractError = (error: EthersError) => {
  if (!error.reason) return false

  return Object.keys(ContractErrorCodes).includes(error.reason)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning behind the union was because a standard error could be caught. Let's keep your solution and adjust the type as suggested in the future.

}

const useIsValidExecution = (
Expand Down
5 changes: 2 additions & 3 deletions src/services/tx/txEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { ContractReceipt } from 'ethers/lib/ethers'
import EventBus from '@/services/EventBus'
import type { RequestId } from '@gnosis.pm/safe-apps-sdk'

Expand Down Expand Up @@ -35,8 +34,8 @@ interface TxEvents {
[TxEvent.EXECUTING]: Id
[TxEvent.PROCESSING]: Id & { txHash: string }
[TxEvent.PROCESSING_MODULE]: Id & { txHash: string }
[TxEvent.PROCESSED]: Id & { receipt: ContractReceipt }
[TxEvent.REVERTED]: Id & { error: Error; receipt: ContractReceipt }
[TxEvent.PROCESSED]: Id
[TxEvent.REVERTED]: Id & { error: Error }
[TxEvent.FAILED]: Id & { error: Error }
[TxEvent.SUCCESS]: Id
[TxEvent.SAFE_APPS_REQUEST]: Id & { safeAppRequestId: RequestId }
Expand Down
1 change: 0 additions & 1 deletion src/services/tx/txMonitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const waitForTx = async (provider: JsonRpcProvider, txId: string, txHash:
txDispatch(TxEvent.REVERTED, {
txId,
error: new Error(`Transaction reverted by EVM.`),
receipt,
})
}

Expand Down
14 changes: 6 additions & 8 deletions src/services/tx/txSender.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,16 +311,16 @@ export const dispatchTxExecution = async (
?.wait()
.then((receipt) => {
if (didRevert(receipt)) {
txDispatch(TxEvent.REVERTED, { txId, receipt, error: new Error('Transaction reverted by EVM') })
txDispatch(TxEvent.REVERTED, { txId, error: new Error('Transaction reverted by EVM') })
} else {
txDispatch(TxEvent.PROCESSED, { txId, receipt })
txDispatch(TxEvent.PROCESSED, { txId })
}
})
.catch((err) => {
const error = err as EthersError

if (didReprice(error)) {
txDispatch(TxEvent.PROCESSED, { txId, receipt: error.receipt })
txDispatch(TxEvent.PROCESSED, { txId })
} else {
txDispatch(TxEvent.FAILED, { txId, error: error as Error })
}
Expand Down Expand Up @@ -363,7 +363,6 @@ export const dispatchBatchExecution = async (
txs.forEach(({ txId }) => {
txDispatch(TxEvent.REVERTED, {
txId,
receipt,
error: new Error('Transaction reverted by EVM'),
groupKey,
})
Expand All @@ -372,7 +371,6 @@ export const dispatchBatchExecution = async (
txs.forEach(({ txId }) => {
txDispatch(TxEvent.PROCESSED, {
txId,
receipt,
groupKey,
})
})
Expand All @@ -383,7 +381,7 @@ export const dispatchBatchExecution = async (

if (didReprice(error)) {
txs.forEach(({ txId }) => {
txDispatch(TxEvent.PROCESSED, { txId, receipt: error.receipt })
txDispatch(TxEvent.PROCESSED, { txId })
})
} else {
txs.forEach(({ txId }) => {
Expand Down Expand Up @@ -438,9 +436,9 @@ export const dispatchSpendingLimitTxExecution = async (
?.wait()
.then((receipt) => {
if (didRevert(receipt)) {
txDispatch(TxEvent.REVERTED, { groupKey: id, receipt, error: new Error('Transaction reverted by EVM') })
txDispatch(TxEvent.REVERTED, { groupKey: id, error: new Error('Transaction reverted by EVM') })
} else {
txDispatch(TxEvent.PROCESSED, { groupKey: id, receipt })
txDispatch(TxEvent.PROCESSED, { groupKey: id })
}
})
.catch((error) => {
Expand Down
6 changes: 3 additions & 3 deletions src/utils/ethers-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ export enum EthersTxReplacedReason {
replaced = 'replaced',
}

export type EthersError = Error & { code: ErrorCode; reason: EthersTxReplacedReason; receipt: TransactionReceipt }
export type EthersError = Error & { code: ErrorCode; reason?: EthersTxReplacedReason; receipt?: TransactionReceipt }

export const didRevert = (receipt: TransactionReceipt): boolean => {
return receipt.status === 0
export const didRevert = (receipt: EthersError['receipt']): boolean => {
return receipt?.status === 0
}

export const didReprice = (error: EthersError): boolean => {
Expand Down