Default strict byte checking

[fselmo]

What was wrong?

closes #1903
closes #1154

How was it fixed?

• Enable strict byte checking by default
• Switch flag to disable strict byte checking
• Improve upon strict encoders, use eth-abi's encoders + decoders where possible. Some places are still not possible because AcceptsHexStrEncoder, for example, resides within web3.py and eth-abi does not have an ExactLengthBytesEncoder.
• Fix tests to account for these changes

Bonus: This fixes the issues we were seeing with strict bytes checking and later solidity versions (#2301)

Todo:

• Add entry to the release notes
• Docs need a revamp to reflect these changes separate PR?

Cute Animal Picture

[fselmo]

@kclowes + @pacrob... some things to think about here that I wanted to bring more direct attention to.

1. The strict bytes type check flag is now able to be toggled on and off. I think this is a plus but open to discussion there as I took that liberty of my own accord. I've added checks to make sure the codec makes sense when it is passed on to other properties. In modules, for example, the codec is inherited from the w3 instance. I think this makes sense but a setter could also easily be added for more module-specific customization.
2. w3.ens inherits the codec and strict_bytes_type_checking flag as well since this module is on the web3 instance itself. Previously, this was not a thing and I'm not sure this was ideal. Let's say a user turned on strict bytes checking for the w3 instance and used the w3.ens. I would expect to have strict bytes checking on for the ens module as well, but, under the hood this would call an internal init_web3 which uses the w3 instances provider and middlewares but that's it. So strict byte checking would not be on. These changes make it so that the w3.ens.w3 property is an exact object reference back to the w3 instance. This means that whatever settings change on the w3 instance itself are directly reflected onto w3.ens.w3 as well since this is tied to the same object reference now.
3. ENS.from_web3 only inherits the codec and strict byte check flag value at the time that the ENS class is instantiate. I think this makes perfect sense since this is a way to instantiate a standalone ENS class object. It does not seem ideal at all to inherit the object reference here since changing ns.w3.strict_bytes_type_checking, for example, would also change w3.strict_bytes_type_checking.

I added tests that check for each of these cases but I'm all ears on any of the above and if there are more test cases that we might want to add.

[kclowes]

Nice! I noticed a few tests in the tests/core/utilities and tests/core/filtering directories where you changed w3_strict_abi -> w3, but not the other way around, so there are some duplicate tests now, I believe. I commented on a few of them, but not all.

1. The strict bytes type check flag is now able to be toggled on and off. I think this is a plus but open to discussion there as I took that liberty of my own accord. I've added checks to make sure the codec makes sense when it is passed on to other properties. In modules, for example, the codec is inherited from the w3 instance. I think this makes sense but a setter could also easily be added for more module-specific customization.

This makes sense to me!

2. w3.ens inherits the codec and strict_bytes_type_checking flag as well since this module is on the web3 instance itself. Previously, this was not a thing and I'm not sure this was ideal. Let's say a user turned on strict bytes checking for the w3 instance and used the w3.ens. I would expect to have strict bytes checking on for the ens module as well, but, under the hood this would call an internal init_web3 which uses the w3 instances provider and middlewares but that's it. So strict byte checking would not be on. These changes make it so that the w3.ens.w3 property is an exact object reference back to the w3 instance. This means that whatever settings change on the w3 instance itself are directly reflected onto w3.ens.w3 as well since this is tied to the same object reference now.
3. ENS.from_web3 only inherits the codec and strict byte check flag value at the time that the ENS class is instantiate. I think this makes perfect sense since this is a way to instantiate a standalone ENS class object. It does not seem ideal at all to inherit the object reference here since changing ns.w3.strict_bytes_type_checking, for example, would also change w3.strict_bytes_type_checking.

Feel free to take or leave since I think either way we leave it will be sort of confusing, but IMO it's unintuitive to have ENS.from_web3(w3).w3 be a different web3 instance than w3.ens.w3. Although I do agree that it’s also confusing to have ns.w3.strict_bytes_type_checking have the side effect of also changing the flag on the w3 instance. I think what’s throwing me off is that the method is called from_web3. Maybe it would be more intuitive to have the strict_bytes_type_checking flag on the ENS instance instead? Or, if we do decide to stick with diverging w3 instances, I think it’s worth adding a note to the from_web3 docs to say that the web3 instance associated with the ns instance is a new instance than the one that was passed to from_web3.

[fselmo]

(@kclowes) IMO it's unintuitive to have ENS.from_web3(w3).w3 be a different web3 instance than w3.ens.w3

I see the from_web3() method as "creating an ENS instance from the current state of the w3 instance". It inherits the properties present on the w3 instance at the time of instantiation of the ns ENS instance. Since this is now a standalone variable (ns = ENS.from_web3(w3)), it does inherit the middlewares and provider from w3 and now it also inherits the strict bytes checking / codec.

I don't think it makes much sense for when we change the w3 middlewares for the ns.w3 middlewares to change and vice-versa and I think it's similar with the provider and now with the strict bytes checking state.

That said... I agree that it's very obscured atm. Apart from the docstring in the from_web3 mentioning the codec, it's unclear that under the hood it is inheriting the state of the codec from web3. Perhaps you're right that this should be made more explicit via ns.enable_strict_bytes_checking or something... which would just be a shortcut to ns.w3.enable_strict_bytes_checking. Does that seem like a reasonable approach, since this is taken as its own separate instance?

---

edit: I tacked it into the latest commit if you want to see what that looks like (no docs yet)

[kclowes]

I don't think it makes much sense for when we change the w3 middlewares for the ns.w3 middlewares to change and vice-versa and I think it's similar with the provider and now with the strict bytes checking state.

That's a good point about middleware that I didn't think about.

Yeah, I like the explicitness of having the method ns.enable_strict_bytes_checking on the ns instance. I think that feels more intuitive. 👍

[fselmo]

Yeah, I like the explicitness of having the method ns.enable_strict_bytes_checking on the ns instance. I think that feels more intuitive. 👍

Sounds good. I'll add some documentation around that before merging 👍🏼