Open hrkrshnn opened 3 years ago
The first item to implement this is to add a flag to all abi decoding functions so that they do not revert on error but instead return a failure flag. This is useful for users, too (we could add abi.tryDecode
), but it is needed for catching errors since we want every failure in the external contract to end up in the "catch-all" catch case, even if the return data is malformed.
maybe duplicate of https://github.com/ethereum/solidity/issues/10933
I believe Error handling in Solidity, as of now, is quite strict in Nature because of Simply REVERTing an entire transaction. While this is an imperative feature in a smart contract, I believe having TRY CATCH for CUSTOM ERRORS is quite significant as well.
I was looking for a native way to try-catch custom errors and ended up here. While this is not implemented, here is a workaround:
error MyCustomError();
contract C {
function throwMyCustomError() external {
revert MyCustomError();
}
function foo() external {
try throwMyCustomError() {
// ...
} catch (bytes memory err) {
// Note: you probably want to pre calc this hash
if (keccak256(abi.encodeWithSignature("MyCustomError()")) == keccak256(err)) {
// handle MyCustomError
}
}
}
}
Note: using this to assert errors on solidity-based tests
This is awesome, have not seen before!!
Combine this with a custom chai matcher like this and you have a nice way to test for specific custom reverts:
// Chai matcher for custom revert errors
export default function supportRevertCustomError(Assertion: Chai.AssertionStatic) {
Assertion.addMethod('revertWithCustomError', async function (this: any, errorName: string, params?: any[]) {
const promise = this._obj
const onSuccess = (value: any) => {
this.assert(
false,
'Expected transaction to be reverted',
'Expected transaction NOT to be reverted',
'Transaction reverted.',
'Transaction NOT reverted.'
)
return value
}
const onError = (error: any) => {
const message = error instanceof Object && 'message' in error ? (error.message as string) : JSON.stringify(error)
console.log('GOT MSG', message)
const delimiter = 'revert'
const [, revertMsg] = message?.split(delimiter)
const [actualErrorName, actualParamsRaw] = revertMsg.split('(')
const actualParams = actualParamsRaw
.substring(0, actualParamsRaw.length - 1)
.replace(/ /g, '')
.split(',')
const expectedError = errorName.split('(')[0]
this.assert(
actualErrorName.trim() === expectedError.trim(),
`Expected ${actualErrorName} to be ${expectedError}`,
`Expected ${actualErrorName} NOT to be ${expectedError}`,
expectedError,
actualErrorName
)
if (params && params.length > 0) {
for (let i = 0; i < actualParams.length; i += 1) {
if (typeof actualParams[i] === 'undefined') continue
const actual = actualParams[i].trim()
const expected = params[i].trim()
this.assert(
actual === expected,
`Expected ${actual} to be ${expected}`,
`Expected ${actual} NOT to be ${expected}`,
expected,
actual
)
}
}
}
const derivedPromise = promise.then(onSuccess, onError)
this.then = derivedPromise.then.bind(derivedPromise)
this.catch = derivedPromise.catch.bind(derivedPromise)
this.promise = derivedPromise
return this
})
}
A suggestion from Nick.
Instead of trying to decode the error data, if try-catch for custom errors (and for the rest) reverts if the abi.decode
fails, then the implementation would be far simpler!
However, this behaviour would be inconsistent with the current try-catch statement.
I think reverting on decoding failure if there is no fallback catch clause would actually be consistent with the current implementation.
Maybe we could change that behaviour for 0.9.0? The property "if execution ends up inside a catch block, then the call must have failed" is something I would not want to change, but reverting on error decoding failure is actually not too bad. It would even be consistent with reverting on return data decoding failure.
What about use cases where you're using try
/catch
because what you really want is to silence errors? Any errors. E.g you have a contract that's batching several similar but completely independent operations and you don't want to revert if some of them fail. Instead you want to execute as many as you can and just tell the caller which ones failed.
In this case, you can use inline assembly and abi.encodeCall
or just the fallback catch clause. I think try/catch is really only safe to use for trusted external calls and allow you to react to known errors. If the called contract is not trusted, it can fool you in so many ways.
We can start implementing for the case that there is no default
catch clause, because then a decoding error can just forward-revert.
In the meantime, there's a couple of language features that would make it at least possible to have a workaround:
bytes memory
, so we can check function signature and decode revert data manually without resorting to assemblyabi.tryDecodeWithSelector()
that takes a selector and a set of arguments, and decodes the arguments only if the selector matches.Implementing something like abi.tryDecode
(that returns a bool on decoding failure instead of reverting) is unfortunately the bulk of the work for this feature. What we could do "rather quickly" is catching custom errors for try/catch without a default case, because then, we can just forward-revert on decoding errors.
This might also be related to https://github.com/ethereum/solidity/issues/10381
I've been using this to bubble up custom errors:
try contract.externalCall(data) {
// on success
} catch (bytes memory reason) {
// on revert
assembly {
revert(add(32, reason), mload(reason))
}
}
Here's a new syntax proposal for this that would, among other things, address handling errors in decoding the error itself: https://github.com/ethereum/solidity/issues/13869#issuecomment-1422879881. I.e. we could skip to catch internal (bytes memory)
{}.
Solidity currently doesn't have a convenient way to catch custom errors.
An issue to track progress on this.
Part of https://github.com/ethereum/solidity/issues/7877