Unhandled ValueError when calling contract functions

[ml31415]

Version: master

What was wrong?

Whenever a contract function call fails, for whatever reason, it throws an unspecified ValueError, which pops up through all the call stack. For libraries on top it's not possible to properly reason and deal with such an unspecific error. This problem shows up in a lot of issue reports, e.g. #1725 #1717 #651 and nosofa/uniswap-v2-py#10 .

How can it be fixed?

To my humble understanding, this should best be handled somewhere in call_contract_function()

web3.py/web3/contract.py

Line 1470 in d8996bd

def call_contract_function(

. E.g. in

web3.py/web3/contract.py

Line 1528 in d8996bd

raise BadFunctionCallOutput(msg) from e

the function nicely throws a perfectly understandable BadFunctionCallOutput. Imho the two function calls in

web3.py/web3/contract.py

Line 1497 in d8996bd

return_data = web3.eth.call(call_transaction)

and below should probably also be wrapped with a try-except-clause and throw BadFunctionCallOutput or another more specific error.

Another and maybe even the better option might be, to replace all the ValueError and KeyError in https://github.com/ethereum/web3.py/blob/master/web3/manager.py with some kind of server side response error right away.

[ml31415]

I'd be happy to put a PR together, but I'd like to hear the developers view on that first.

[kclowes]

Sorry, @ml31415 I lost track of this. I'm for changing ValueError to be something more specific to web3. I think the best option would be to raise something other than ValueError in manager.py. In order to keep this backwards compatible, however, we'd need to inherit from ValueError in v5. Once we have a v6 branch going, we can remove the inheritance. I'm not sure what we'd call the error class... Maybe Web3ClientError, ClientResponseError, Web3ResponseError? /cc master namer @marcgarreau

[ml31415]

Thanks for coming back to that! From a user point of view, I'm always a big fan if a library provides a "root" exception, from where all purposefully thrown errors derive, so that I can catch for only that error type, if I'm not sure what exactly might go wrong in some library call. Or to put it in other words: It's great to be able to freely choose the level of granularity when catching an exception. E.g. as seen in manager.py if it's a server response KeyError or ValueError, I might love to catch for both of them generically. I'd suggest a hierarchy like that:

class Web3Error(Exception):
    pass

# other Web3Error derived errors
# maybe Web3ClientError

class ResponseError(Web3Error):  # or Web3ClientError
    pass

class ResponseValueError(ValueError, ResponseError):
    pass

class ResponseNotFound(KeyError, ResponseError): 
    # as seen in https://github.com/ethereum/web3.py/blob/125c6aedc618a008f07069f09ddc3430002af0df/web3/manager.py#L197
    pass

[kclowes]

Great minds! That is one of the features we're planning on implementing in v6. Tracked in #1478

[ml31415]

Sorry for having created a duplicate then. Had missed that unfortunately.

[kclowes]

No worries! I think this deserves to be open still since it's a separate request to have a different error come back than ValueError.

[LefterisJP]

No worries! I think this deserves to be open still since it's a separate request to have a different error come back than ValueError.

Hey @kclowes this is still open.

We are upgrading rotki's codebase to web3.py v6.

After #1839 I thought we would no longer see ValueErrors but I still see them in some places. Example below:

ERROR    rotkehlchen.greenlets.manager:manager.py:81 Greenlet with id 139772959211136: Attempt connection to base node public node died with exception: {'code': -32000, 'message': 'missing trie node 7a8d16362e6dbe9cd83ec5c1b865f687e66aaa70
4eb7e2fdc823518a650174af (path ) state 0x7a8d16362e6dbe9cd83ec5c1b865f687e66aaa704eb7e2fdc823518a650174af is not available, not found'}.
Exception Name: <class 'ValueError'>      
Exception Info: {'code': -32000, 'message': 'missing trie node 7a8d16362e6dbe9cd83ec5c1b865f687e66aaa704eb7e2fdc823518a650174af (path ) state 0x7a8d16362e6dbe9cd83ec5c1b865f687e66aaa704eb7e2fdc823518a650174af is not available, not found'}
Traceback:       
   File "src/gevent/greenlet.py", line 908, in gevent._gevent_cgreenlet.Greenlet.run
  File "/home/lefteris/w/rotkehlchen/rotkehlchen/chain/evm/node_inquirer.py", line 476, in attempt_connect
    is_pruned, is_archive = self.determine_capabilities(web3)                       
  File "/home/lefteris/w/rotkehlchen/rotkehlchen/chain/evm/node_inquirer.py", line 1306, in determine_capabilities
    is_archive = self._have_archive(web3)                                                                                                                                                                                                        File "/home/lefteris/w/rotkehlchen/rotkehlchen/chain/evm/node_inquirer.py", line 1365, in _have_archive
    balance = self.get_historical_balance(                                                                                                                                                                                                       File "/home/lefteris/w/rotkehlchen/rotkehlchen/chain/evm/node_inquirer.py", line 347, in get_historical_balance
    result = web3.eth.get_balance(address, block_identifier=block_number)                                                                                                                                                                        File "/home/lefteris/.virtualenvs/rotkipy310/lib/python3.10/site-packages/web3/eth/eth.py", line 423, in get_balance
    return self._get_balance(account, block_identifier)
  File "/home/lefteris/.virtualenvs/rotkipy310/lib/python3.10/site-packages/web3/module.py", line 75, in caller
    result = w3.manager.request_blocking(                                                                                                                                                                                                      
  File "/home/lefteris/.virtualenvs/rotkipy310/lib/python3.10/site-packages/web3/manager.py", line 325, in request_blocking
    return self.formatted_response(                                                                                                                                                                                                            
  File "/home/lefteris/.virtualenvs/rotkipy310/lib/python3.10/site-packages/web3/manager.py", line 288, in formatted_response 
    raise ValueError(error)   
    ```

That one is querying historical state for balance of an address, and node is pruned.

So I guess this is not yet finished in v6?

[kclowes]

🤔 Hmm, yes it is. I think it just got missed in v6. We moved all of our custom exceptions that previously inherited from ValueError to inherit from Web3Exception, but we left Python ValueErrors. I'm not opposed to raising a custom web3.exceptions.ValueError (name TBD). I'll add to our v7 tracking issue, but @fselmo, @pacrob, @reedsa - let me know if you think this is a bad idea and we can continue the conversation.

[LefterisJP]

I'm not opposed to raising a custom web3.exceptions.ValueError (name TBD)

yeah anything common like this so we have clean handling of library exception at all places we use web3, otherwise we end up having to catch both this and ValueError.

Thanks!

[fselmo]

Closed by #3300 and #3359 (raise Web3RPCError).