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

add abi.decode strictness flag and catch InsufficientDataBytes error … #3256

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Feb 26, 2024

…when parsing logs

This should use the existing global strictness flag.

What was wrong?

Related to Issue #1441 , if a user is decoding logs with length not a multiple of 32 bytes, it throws an eth-abi InsufficientBytesLength error which is not properly caught.

How was it fixed?

Add catching the error type InsuffiecientDataBytes and allow the passing of strict flag through to eth_abi.decode in process_receipt.

Since the flag is then passed to get_event_data, I considered making the flag available everywhere get_event_data is called, but that would greatly expand the scope and doesn't seem necessary, given the original issue.

Todo:

Cute Animal Picture

image

@pacrob pacrob force-pushed the catch-insufficient-bytes-in-receipt-processing branch from ab73bef to 0f7fbb1 Compare February 27, 2024 18:23
@fselmo
Copy link
Collaborator

fselmo commented Mar 12, 2024

Instead of creating a new flag and passing that flag around, we should let this be determined by the Web3.strict_bytes_type_checking flag. We should use this flag in the decoder for web3 to mark the strict flag value for the abi methods. Thoughts there?

@fselmo
Copy link
Collaborator

fselmo commented Mar 12, 2024

Instead of creating a new flag and passing that flag around, we should let this be determined by the Web3.strict_bytes_type_checking flag. We should use this flag in the decoder for web3 to mark the strict flag value for the abi methods. Thoughts there?

Building on this, it looks like the previous version of the global strict bytes checking only re-builds encoders with is_strict set to False. We could, and maybe should, build decoders with these values as well and override certain areas where the strictness is checked. I'm not sure how much more effort this would take nor that it is the right avenue since it might just re-write a lot of the eth-abi code.

If we can find a good balance to set the strictness at the decoder level, then we can maybe even solve issues like #2489 when we pass in the w3.codec if w3.strict_bytes_type_checking is set to False. This would ensure the decoder itself is already essentially using strict=False when it decodes.

I think we should take a step back and re-evaluate how we want to add this globally since it doesn't seem like the global flag is handling decoding strictness very gracefully.


edit: Looking at the eth-abi code again, I think we just need to override the strict property on each decoder when we build the non-strict registry which should be pretty simple. But don't want to speak too soon.

Curious on your thoughts there.

@pacrob pacrob force-pushed the catch-insufficient-bytes-in-receipt-processing branch from 4d09ec2 to a779467 Compare April 3, 2024 17:54
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

Successfully merging this pull request may close these issues.

None yet

2 participants