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.

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

VIP: Try Catch exception handling for External Calls #1517

Open
haydenadams opened this issue Jul 8, 2019 · 16 comments
Open

VIP: Try Catch exception handling for External Calls #1517

haydenadams opened this issue Jul 8, 2019 · 16 comments
Labels
VIP: Approved VIP Approved

Comments

@haydenadams
Copy link

haydenadams commented Jul 8, 2019

Summary

External calls initiated from smart contracts should be allowed to fail without reverting the entire transaction.

Motivation

Catching reversions will increase the flexibility of Vyper and allow experimenting with new smart contract patterns. This is potentially a requirement for Uniswap V2.

The main draw here is you can have a non-upgradable contract that makes external calls to an upgradable contract without the ability for the upgradeable contract to break the non-upgradable contract. This uses a combination of max_gas, mutex, and catching reverts.

Specification

Credit to @jacqueswww for helping out here. It could look something like:

try:
     this.someToken.transfer(a, b, allow_fail=True)
except TransactionFailed:
      <block of code>

Potentially it could handle multiple calls, not sure:

try:
     this.someToken.transfer(a, b, allow_fail=True)
     this.call2()
     this.call3(allow_fail=True)
except TransactionFailed:
      <block of code>

Backwards Compatibility

Should be fine?

Dependencies

None

Copyright

Copyright and related rights waived via CC0

@jacqueswww
Copy link
Contributor

jacqueswww commented Jul 8, 2019

I approve this message. I will add this to our next meeting for discussion as well.
I much prefer just going with the try catch approach (with forced try catch if allow_fail=True) is set, over making a second version of raw_call and loosing the clarity of emitting failure on call, and enforcing a clear Call / Transaction failed.

@jakerockland
Copy link
Contributor

jakerockland commented Jul 8, 2019

@haydenadams @jacqueswww is TransactionFailed the only type of exception that would be supported with this feature?

Generally, I think it would be good for the purposes of Vyper that only explicit exceptions be "catchable", disallowing something like, with any uncaught exception resulting in a reversion:

try:
     this.someToken.transfer(a, b, allow_fail=True)
     this.call2()
     this.call3(allow_fail=True)
except AnyError:
      <block of code>

@jacqueswww jacqueswww added the bug Bug that shouldn't change language semantics when fixed. label Jul 8, 2019
@jacqueswww
Copy link
Contributor

jacqueswww commented Jul 8, 2019

@jakerockland yes I fully agree, this would be a single built-in Exception for CALL exceptions. I am not in favour of having a generic / large groups of exceptions.

@haydenadams
Copy link
Author

Hmm good question. What are some other Vyper exceptions that might be worth catching?

@jacqueswww jacqueswww changed the title VIP: Try Catch exception handling VIP: Try Catch exception handling for External Calls Jul 8, 2019
@jacqueswww
Copy link
Contributor

We don't have other runtime exceptions, except reverting.

@jakerockland
Copy link
Contributor

Yeah, I don't see a problem with the only current catchable exception to being TransactionFailed, I just wanted to point out that I don't think we want to open the gateway for generic error catching.

@jakerockland
Copy link
Contributor

jakerockland commented Jul 8, 2019

Though perhaps we do want to allow custom (programmer defined) error types down the road...?, which would make my point more relevant/less abstract 😅

@brockelmore
Copy link
Contributor

My two cents on the matter: if someone is interacting with my contract, it may be nice to be able to pass the error up to the dev utilizing my sc. Chained errors may become more important as protocols start compounding on each other and enable better UX.

Agree that could just be a potential down-the-road thing

@fubuloubu
Copy link
Member

I mean personally, I would discourage using this type of behavior over read-only function APIs in my contracts, but I can see utility for this in order to make decisions when executing trades

@charles-cooper
Copy link
Member

charles-cooper commented Jul 21, 2019

This is neat, we should also make the revert reason accessible to the programmer. For clarity, there could be a single exception class named TransactionReverted and the reason accessible as e.reason, e.g.

try:
  external_contract.foo()
except TransactionReverted as e:
  return e.reason

(A single exception class is used because there is no way (to my knowledge) to differentiate between a message call failing due to REVERT and failing due to an exceptional halting condition.)

Personally I find finally clauses to be hard to reason about, but we should consider them because they could be useful in certain cases.

As an alternative to exception-handling style, we could use error code style, so if allow_fail is set, the external call returns a tuple as in Go, (retval, err). The reason string could be accessed via a global variable ERR_REASON if err is not nil (opposite the EVM semantics where CALL returns 0 on failure). Although both ways are obviously equal in expressive power, the exception handling style is more pythonic; but I personally prefer the error code style because there is no messing with control flow.

@haydenadams
Copy link
Author

@fubuloubu the most important usage here is external calls to upgradable addresses such that the upgradable contract can never halt the calling contract through reversion, provided there is a max gas limit on the call. You can't replicate this will read-only.

@fubuloubu
Copy link
Member

Upgradeable contract gotchas strikes again!

@haydenadams
Copy link
Author

The external call could also be to a non-upgradable contract that has some chance of reverting.

In general I think catching errors is super useful considering the async / delayed nature of Ethereum transactions.

@jacqueswww jacqueswww added VIP: Approved VIP Approved and removed bug Bug that shouldn't change language semantics when fixed. labels Jul 22, 2019
@haydenadams
Copy link
Author

btw would it be possible to catch reversion for an internal call?

@charles-cooper
Copy link
Member

Not really as revert halts the current message call

@iamdefinitelyahuman
Copy link
Contributor

Looks like solidity plans to implement this for 0.6 - https://github.com/ethereum/solidity/blob/develop_060/docs/control-structures.rst#trycatch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved
Projects
None yet
Development

No branches or pull requests

7 participants