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

Event ProcessReceipt fails for events with same id but different indexed attributes #1351

Closed
Uxio0 opened this issue May 10, 2019 · 3 comments

Comments

@Uxio0
Copy link
Contributor

Uxio0 commented May 10, 2019

  • Version: 4.9.2
  • Python: 3.7
  • OS: linux

What was wrong?

When I use rich_logs = erc721.events.Transfer().processReceipt(tx_receipt) I expect the relevant logs to be decoded and the others ignored. But if I try to process a tx with mixed ERC20 and ERC721 Transfer events (ERC721 has the third parameter of the event indexed, ERC20 not) it will fail decoding the event.

ERC20 Transfer Event event Transfer(address indexed from, address indexed to, uint256 value); = 3 topics
ERC721 Transfer Event event Transfer(address indexed from, address indexed to, uint256 indexed tokenId); = 4 topics

Problem is that signature for Transfer is the same, but as tokenId is indexed for ERC721 it will have one more log topic than ERC20. When web3.py tries to decode an ERC20 event with the ERC721 contract, it will fail as it's expecting one topic more.

in
----> 1 erc721.events.Transfer().processReceipt(tx_receipt)

~/.virtualenvs/gnosis-py/lib/python3.7/site-packages/web3/utils/decorators.py in _wrapper(*args, **kwargs)
12 def _wrapper(*args, **kwargs):
13 if obj is not None:
---> 14 return self.method(obj, *args, **kwargs)
15 else:
16 return self.method(objtype, *args, **kwargs)

~/.virtualenvs/gnosis-py/lib/python3.7/site-packages/web3/contract.py in processReceipt(self, txn_receipt)
1277 @combomethod
1278 def processReceipt(self, txn_receipt):
-> 1279 return self._parse_logs(txn_receipt)
1280
1281 @to_tuple

~/.virtualenvs/gnosis-py/lib/python3.7/site-packages/eth_utils/functional.py in inner(*args, **kwargs)
44 @functools.wraps(fn)
45 def inner(*args, **kwargs) -> T: # type: ignore
---> 46 return callback(fn(*args, **kwargs))
47
48 return inner

~/.virtualenvs/gnosis-py/lib/python3.7/site-packages/web3/contract.py in _parse_logs(self, txn_receipt)
1283 for log in txn_receipt['logs']:
1284 try:
-> 1285 decoded_log = get_event_data(self.abi, log)
1286 except MismatchedABI:
1287 continue

~/.virtualenvs/gnosis-py/lib/python3.7/site-packages/web3/utils/events.py in get_event_data(event_abi, log_entry)
168 raise ValueError("Expected {0} log topics. Got {1}".format(
169 len(log_topic_types),
--> 170 len(log_topics),
171 ))
172

ValueError: Expected 3 log topics. Got 2

You can test it easily with rinkeby tx-hash 0x21381484d8f69dcd782560d1fd3cd818e743c79767985d01aec7e61c2a7f1de9, for example.

How can it be fixed?

  • Skip incorrect decoded logs, as they are not complaint with the contract interface
  • At least allow a flag in processReceipt to skip not complaint logs
@Uxio0 Uxio0 changed the title Process Receipt tries to process different topics Event ProcessReceipt fails for events with same id but different indexed attributes May 10, 2019
@pipermerriam
Copy link
Member

Thanks for raising this and explaining it well.

I think that we could address this using a similar pattern to codecs.encode and decode which take an errors keyword argument to decide what should be done on missing logs. The simple implementation might take any of the following static values:

  • ignore: return the log, in place, without decoding
  • discard: in the event of a decoding error, discard the log.
  • strict: propagate the exception if an exception occurs.

I'm thinking it could be useful to allow a custom error handler function to be provided as well, but I see a route for us to do that later without needing to introduce a breaking change. Instead of using strings for ignore/discard/strict lets make these values that are provided by the web3 library. This would allow us to swap these out in the future to change the internal implementation without changing the external API.

Also, I'm not set on those three names so feel free to explore better/alternate names.

@pipermerriam
Copy link
Member

I think this also suggests we're lacking an API to do erc721.events.Transfer().processLog(tx_receipt.logs[0]) which should be very easy to implement. (and maybe backport to v4 since it should be very simple)

@Uxio0
Copy link
Contributor Author

Uxio0 commented May 10, 2019

Agree with the ignore/discard/strict.

As you say too, it would be very useful to have a method that allows to process a single log, instead of the full receipt

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

No branches or pull requests

3 participants