Contract event overloading breaks hasattr

[Arachnid]

• Version: 5.0.0a6
• Python: 3.7
• OS: linux

What was wrong?

Calling hasattr on a contract events object should return True or False as appropriate. Instead, it raises the following exception:

Traceback (most recent call last):
  File "tools/get_names.py", line 101, in <module>
    main(parser.parse_args())
  File "tools/get_names.py", line 89, in main
    if hasattr(registrar.events, 'BidRevealed'):
  File "/home/user/.local/lib/python3.7/site-packages/web3/contract.py", line 200, in __getattr__
    "Are you sure you provided the correct contract abi?"
web3.exceptions.MismatchedABI: ("The event 'BidRevealed' was not found in this contract's abi. ", 'Are you sure you provided the correct contract abi?')

How can it be fixed?

Fix the 'helpful' overloading of __getattr__ to raise the exception expected by Python (preferable), or implement __hasattr__.

[pipermerriam]

👍 to making hasattr work as expected. Should be a pretty thing to fix. It is possible we have a similar problem in contract.functions and contract.caller

[adamjsawicki]

@pipermerriam Are we looking to change __getattr__ to raise an AttributeError here and here, or do we want to implement __hasattr__ by checking if some event_name exists in self.__dict__ or self.__dict__['_events']?

[pipermerriam]

@swixx sorry I missed your comment when it came through.

I believe we want to change __getattr_ to raise an AttributeError (with the same exception messages). It would be ideal if hasattr worked as expected too so if we need a custom implementation of that it could go alongside this fix.

[pipermerriam]

cc @kclowes I there is an argument to be made that this qualifies as a breaking change but also a bugfix. Anyone with existing code catching these exception would end up with broken code.

We could:

1. tell ourselves it is ok because this is a bugfix (is it?)
2. make a new interum exception class that inherits from both AttributeError and the existing exception classes that are raised that would then be removed in v6.

If option 2 proves viable which I don't see why it wouldn't I think it's preferrable but I'm curious what you think.

[kclowes]

Yeah, I'd lean toward option 2. I find it really frustrating when a library implements a breaking change on a minor or patch release, unless it definitely is a bug. And I'd argue that this is a functionality change, not a bug. We can add the removal of the interim exception class to the v6 tracking issue here.

[pipermerriam]

@swixx chime in if you'd like to take this on (or just open a PR 😄 )