Try/catch for external calls

[nventuro]

Abstract

Solidity wraps the call opcode in a higher-level concept, that of external function calls. An external function call does more than a simple call, including:

• checking that the destination address has code
• reverting on call failure
• decoding return data

This is a proposal to break down these steps so that reverting is optional, while keeping all other current semantics and behaviour.

Motivation

There are multiple scenarios in which it is necessary to not revert the whole transaction if one of the inner transactions failed, including batch transfers of tokens and most meta-tx solutions. In these cases, the calling contract should handle the failed call and continue its own execution.

The only way to do this currently is by using call directly, and performing a test on the success boolean value returned by it. However, keeping this consistent with other external function calls requires a deep understanding of how these are performed, and is not trivial to get right.

try/catch is a well-known pattern that should immediately convey the intent behind the construct to a developer familiar with other languages that support it (including JavaScript, Python, Java, C++, C#).

Since we're not attempting to create a full-fledged exception handling mechanism, a syntax that clearly indicates a single external call will be covered by the try block is desirable.

Specification

@chriseth and I discussed a couple alternatives on the solidity-dev gitter channel. The final proposal is as follows:

• ext_call is an external call expression (i.e. a call to an external function on a contract type), that (optionally) returns a value of type T

try (ext_call)
then (T ret_value) {
   // block to be executed if ext_call succeeds, i.e. regular Solidity behaviour
} 
catch (bytes error_value) {
   // block to be executed if ext_call fails, as opposed to reverting the current transaction
} 

Expected behaviour is equivalent to a Solidity function call, except that all conditions that may cause a revert (including no destination code, failed transaction, failure at decoding the return value) cause the execution flow to jump to the catch block. A missing then or catch block simply means said block will not be executed.

A couple notes about the proposed syntax:

• both ret_value and error_value are scoped to the blocks where they are valid and initialized
• I'm not completely sold on the keyword then, but it does have the benefit of not being a common variable or function name, and is highly related to execution flow
• if no error-handling is done, and the external function returns no value, both then and catch can be removed, leaving simply try (ext_call), an expression with no trailing semicolon. We may want to make catch mandatory

Backwards Compatibility

Both try and catch are reserved keywords as of v0.5.9. then is not, nor could I identify any other reserved keyword (e.g. success) that we could use in its place.

Since dropping the then keyword would lead to very weird syntax, we should add a new one in the next breaking change release (see #6040).

[nventuro]

#1505 is related to this, but I wanted to create a new issue altogether to discuss the proposal without interfering with the discussion in the previous one, which is not restricted to a single external call.

Note however that @SilentCicero's comment there illustrates the scenario I described above.

[maraoz]

Thoughts on removing the then? (I'm sorry if already discussed on gitter or elsewhere)

try {
  // block to be executed fully if all external calls succeed, i.e. regular Solidity behavior
  T ret_value = ext_call
  T ret_value2 = ext_call 
} catch (bytes error_value) {
   // block to be executed if any ext_call fails, as opposed to reverting the current transaction
} 

This would be more in line with other traditional programming languages.

[nventuro]

@maraoz going for full-featured exception support (e.g. including require statements or internal function calls inside a try block) is a much more complex task requiring runtime support, and is out of scope of this feature request.

If we indeed restrict try to external calls, a syntax such as the one you proposed may (will?) lead people into thinking they any revert inside the try block will revert all changes (including local ones!) and jump to the catch block.

This is the main reason why @chriseth suggested to have a single expression covered by try, and the parens nicely convey that behaviour (as they do in if, while, etc.).

[alcuadrado]

If we indeed restrict try to external calls, a syntax such as the one you proposed may (will?) lead people into thinking they any revert inside the try block will revert all changes (including local ones!) and jump to the catch block.

I really dislike the then keyword, but I agree with this.

This is the main reason why @chriseth suggested to have a single expression covered by try, and the parens nicely convey that behaviour (as they do in if, while, etc.).

Java has a similar syntax for try-with-resources:

try (T ret = f()) {

} catch (...) {

} 

Its semantics are completely different from the ones proposed here, so I think it would also be confusing.

Isn't the root of this confusion reusing try, which has approximately the same semantics in most mainstream languages? If this is going to be released in a breaking version, it may be worth exploring alternative keywords.

[nventuro]

I really dislike the then keyword

I would prefer something like onSuccess, but dislike double-worded keywords, and wouldn't make success a keyword by itself, since it is a very common variable name.

Isn't the root of this confusion reusing try, which has approximately the same semantics in most mainstream languages? If this is going to be released in a breaking version, it may be worth exploring alternative keywords.

I've also considered this, and am not against it, but have no good suggestions.

Since try/catch is so similar to what we want to do, I think a bit of quirkiness in how it looks/behaves (we have a fair share of this already in the ecosystem) is preferable to a completely new construct.

[abcoathup]

My concern is using try/catch for this use case could be confusing for developers coming from other languages.

Is it possible to wrap an external call and append a success bool and error data to the return types so that an if/else pattern could be used?

(T ret_value, bool success, bytes error_value) = ext_call(doStuff())

if (success) {
    // external call successful do something based on ret_value
} else {
    // external call unsuccessful do something based on error_value
}

[nventuro]

Is it possible to wrap an external call and append a success bool and error data to the return types so that an if/else pattern could be used?

Yes, but then you lose the niceties of having ret_value and error_value scoped only to the blocks where they make sense. Would you e.g. leave ret_value uninitialized, or with some default value when success is false?

[abcoathup]

Yes, but then you lose the niceties

Which explains why appended success/error used with if/else is not a runner unfortunately. 😄

Synonyms for then don't show any great alternatives.
https://www.thesaurus.com/browse/then

Same goes for try. Though dab could be fun.
https://www.thesaurus.com/browse/try
Could reuse external instead of try, though I think try captures the concept better.

external (ext_call)
then (T ret_value) {
   // block to be executed if ext_call succeeds, i.e. regular Solidity behaviour
} 
catch (bytes error_value) {
   // block to be executed if ext_call fails, as opposed to reverting the current transaction
} 

I am more reassured seeing the Java try-with-resources.

The key will be education on its use.

[maxsam4]

I'd love to see this implemented. I talked about try catch in my eth new york talk and wrote a post with an example on how to do it with current syntax just a couple of days back https://blog.polymath.network/try-catch-in-solidity-handling-the-revert-exception-f53718f76047

Although what this proposal proposes can already be done fairly easily, I still really want this to be implemented because this will bring in the static checks done by the compiler with it and make it slightly easier to do.

Now back to the proposal, do you think the syntax of try(ext_call) should also accept try(self_public_call) and make an external call to that public/external function on its own? In other words, should a token be able to do try(transfer(alice, 100)) ?

[nventuro]

posted a post with an example on how to do it with current syntax just a couple of days back

Your blogpost reminded me of this issue and prompted me opening this feature request 😂

Now back to the proposal, do you think the syntax of try(ext_call) should also accept try(self_public_call) and make an external call to that public/external function on its own? In other words, should a token be able to do try(transfer(alice, 100)) ?

public functions can be externally called by going trough this, so your example could be written as

try (this.transfer(alice, 100))

I guess we could have the compiler transform an internal call to an external one automatically when inside try, but that may be too automagical. I can live with this.foo, especially for an MVP.

[maxsam4]

I guess we could have the compiler transform an internal call to an external one automatically when inside try, but that may be too automagical. I can live with this.foo, especially for an MVP.

Yeah, that's what I was thinking. It makes things a bit cleaner but brings in a bit of ambiguity. I'd be happy to see this implemented in either way.

[frangio]

Alternatively we can use as or in instead of then. Also, the whitespace looks better like this, IMO:

try (external_call) as (T ret_value) {
   // statements in case of success
}  catch (bytes error_value) {
   // statements in case of revert
} 

The as keyword is currently used for renaming in import statements. Reusing it for this purpose might not be a good idea, I haven't made up my mind on this yet.

[nventuro]

Alternatively we can use as or in instead of then. Also, the whitespace looks better like this, IMO:

Ohhh, I really like this! That usage of as remind me of how it's used in Python, where it serves the same purpose of assigning an expression to a variable (with open('file.jpg') as f).

[maxsam4]

I like as but not in.
promise is also cool imo.

try (external_call) promise (T ret_value) {
   // statements in case of success
}  catch (bytes error_value) {
   // statements in case of revert
} 

or even use let instead of try

let (external_call) promise (T ret_value) {
   // statements in case of success
}  catch (bytes error_value) {
   // statements in case of revert
} 

[maraoz]

I love @frangio's proposal!!

[chriseth]

Just fiddling a little:

try T ret_value = external_call(...) {
   // statements in case of success
}  catch (bytes error_value) {
   // statements in case of revert
} 

[chriseth]

try external_call(...) result (T ret_value) {
   // statements in case of success
}  catch (bytes error_value) {
   // statements in case of revert
}

[chriseth]

try external_call(...) returns (T ret_value) {
   // statements in case of success
}  error (bytes error_value) {
   // statements in case of revert
}

(sorry for the spam)

[nventuro]

@chriseth I like the second and third options you proposed, the first one might trick people into thinking ret_value is scoped to the outer block. It'd be simply a matter of choosing between as, result and returns.

Would implementing this be feasible for an external contributor? If so, could you give some pointers regarding what would need to be done? I'm thinking the whole parsing of the new syntax might be a bit too much, but perhaps people could help on the implementation, tests, etc. once that's done.

[chriseth]

I definitely like this to be in rather sooner than later. We would of course be very happy to have this done externally!

The following components would need changes:

• liblangutil/Token.h to add new keywords (even better if we can reuse some existing reserved keywords)
• libsolidity/ast/AST.h to add a new AST node type, also modify ASTVisitor.h and AST_accept.h accordingly
• libsolidity/parsing/Parser.cpp, in Parser::parseStatement to start reacting on try plus a parsing function
• libsolidity/analysis/NameAndTypeResolver.h (and maybe related files) to properly register the variables in their scopes
• libsolidity/analysis/TypeChecker.cpp - check that the return variables match the function return type
• libsolidity/codegen/ContractCompiler.cpp for the actual code generation

[chriseth]

Since error is not a keyword and might be a rather popular variable name, I'm starting to implement the following now (suggestions still welcome):

try x.f(...) returns (T ret_value) {
   // statements in case of success
}  catch Error(string memory error_value) {
   // statements in case of revert with message
}  catch (bytes memory data) {
   // statements in case of revert without message or non-matching revert data type
}

[chriseth]

We stumbled across some problematic edge cases that require some discussion:

What happens if the return values of x.f() cannot be properly decoded? Is it fine to revert in that case? Or should one of the catch clauses be invoked? If yes, what would be the error data and how can the two cases be distinguished? @ekpyron proposed to introduce something like DecodingError in that case.

Related, but maybe a little different: What happens if error message decoding fails?

[maxsam4]

We stumbled across some problematic edge cases that require some discussion:

What happens if the return values of x.f() cannot be properly decoded? Is it fine to revert in that case? Or should one of the catch clauses be invoked? If yes, what would be the error data and how can the two cases be distinguished? @ekpyron proposed to introduce something like DecodingError in that case.

Related, but maybe a little different: What happens if error message decoding fails?

Let's assume contract alpha calls contract bravo using try-catch.
I think that it should just revert if the returned data can not be decoded.

If I am not mistaken, this problem can only arise when contract bravo completes its execution properly but the contract alpha is unable to decode the data returned by bravo. I think this try-catch is meant to catch reverts that happen in the contract bravo. Since this error is in contract alpha, a normal revert should happen.

[chriseth]

@maxsam4 thanks for your comment! Yes, this is a striking argument against catching decoding errors for success return data: If control-flow ends up in any of the catch clauses, then you assume that the external call reverted and especially that any state-changes it did reverted. So it looks like we have no choice there.

The only question left for debate thus seems to be how to handle decoding errors in the failure data. Should we continue in the catch-all-catch-clause or should we just revert?

[nventuro]

Reverting sounds like the way to go, I would only go to the catch blocks if the underlying call was not successfully completed (and didn't alter state).

What happens if only one of the two catch variants is supplied? Do uncaught reverts with string go to the catch-all, and uncaught reverts with no string cause a revert in the caller?

[chriseth]

My current plan is as follows:

• revert if the call itself finished successfully but there was a decoding error
• if there is a structured (error string) catch clause and the signature matches, try to decode and execute. If decoding fails or the structured catch clause does not exist or the signature does not match, go to the low-level one.
• if the low-level catch clause does not exist, but the structured does not match, revert.
• if there is no structured catch clause, always go to the low-level one.

[leonardoalt]

@chriseth Can this be closed?

[chriseth]

Implemented in #7328

[forshtat]

The only question left for debate thus seems to be how to handle decoding errors in the failure data. Should we continue in the catch-all-catch-clause or should we just revert?

Hey @chriseth @nventuro
I have missed on the conversation but recently attempted to use try/catch in the GSN RelayHub and found it not useful because of the decision made here about decoding errors.

In our case, target contracts are a user input, and also it is critical for the code to be confident this external call can not revert.

Is there a way to receive a return value from the target and have a confidence this "line" will not revert? I would say, something like 'try to decode'?
Otherwise, this behaviour really breaks expectations. Especially taking into account calls to random addresses do not revert, but neither do they return any value?

Code that would do the job for us currently would look somewhat like this:

(bool success, bytes memory ret) = relayRequest.request.to.staticcall(
    abi.encodeWithSelector(
        IRelayRecipient.isTrustedForwarder.selector, relayRequest.relayData.forwarder
    )
);
require(success, "isTrustedForwarder reverted");
require(ret.length == 32, "isTrustedForwarder returned invalid response");
require(abi.decode(ret, (bool)), "invalid forwarder for recipient");

[chriseth]

Thanks for your feedback, @forshtat !

Can you provide some code that uses try/catch and explain how you would like it to behave?

"Code that would do the job for us" - I don't see how this is different from the behaviour of try/catch. abi.decode also reverts for invalidly encoded data in the same way as a try f() returns (bool success) { ... } would.

[nventuro]

@chriseth is it possible to use try without the returns part to avoid any decoding of the returned data, but still somehow access it inside the 'success' block (and e.g. decode conditionally based on its size).

[chriseth]

function getReturndata() internal retruns (bytes memory rd) {
  uint length;
  assembly { length := returndatasize() }
  rd = new bytes(length);
  assembly { returndatacopy(add(rd, 0x20), 0, returndatasize()) }
}
...
try x.() {
  bytes memory data = getReturndata();
}