Missing ValidationError when calling bytes argument with string value

[palango]

• Version: 5.0.0
• Python: 3.7
• OS: linux/travis
• pip freeze output

See https://travis-ci.org/palango/raiden-contracts/builds/568742398#L398

What was wrong?

I updated the web3.py, eth-tester and further dependencies in a PR of the raiden contracts: raiden-network/raiden-contracts#1153

So far everything looks good, but I get on test failure that looks like a regression to me: https://travis-ci.org/palango/raiden-contracts/builds/568742398#L494

In that test we check that the web3 contract instance throws a ValidationError when the contract is called with an invalid value.

The function in question is here:
https://github.com/raiden-network/raiden-contracts/blob/master/raiden_contracts/data/source/raiden/SecretRegistry.sol#L17

The test code is:

        with pytest.raises(ValidationError):
           secret_registry_contract.functions.registerSecret("")
           Failed: DID NOT RAISE <class 'web3.exceptions.ValidationError'>

How can it be fixed?

This should either raise a ValidationError or it should be documented that strings can now be passed to bytes parameters.

[pipermerriam]

It looks like the offending code is here:

web3.py/web3/_utils/abi.py

Lines 149 to 157 in b899050

	def validate_value(self, value):
	if is_text(value):
	try:
	value = decode_hex(value)
	except binascii.Error:
	self.invalidate_value(
	value,
	msg='invalid hex string',
	)

If the value is text and can be decoded as hex it gets accepted.

My initial thought is that we should only accept `"0x" prefixed hex values. This is complicated since we can't exactly make that change since it would be backwards incompatible.

We might be able to be more strict about just the empty string and require that 0x be present if the string is empty.... cc @carver

[carver]

it should be documented that strings can now be passed to bytes parameters.

Yeah, the short-medium term answer is this ^. Bytes parameters will interpret strings as hex, as Piper pointed out.

On first look, I think I'm good with requiring an 0x prefix in order to interpret a string as hex, but we couldn't make that change until v6, even for the empty string. So it just belongs in the v5 migration docs.

Nope, this is a straight-up bug. Only a 64-character hex-string should be accepted, since the ABI specifies bytes32 in https://github.com/raiden-network/raiden-contracts/blob/master/raiden_contracts/data/source/raiden/SecretRegistry.sol#L17

(the strikethrough text still applies for unbound bytes types)

[kclowes]

Thanks @carver! @pipermerriam and I also talked about having a flag (similar to the enable_unstable_package_management_api flag) that enables stricter checking so that we can throw an error for the empty string case in v5 for people who opt-in without a breaking change.

[carver]

Thanks @carver! @pipermerriam and I also talked about having a flag (similar to the enable_unstable_package_management_api flag) that enables stricter checking so that we can throw an error for the empty string case in v5 for people who opt-in without a breaking change.

Yeah, that seems fine to me.

But even without the flag, the ValidationError from OP should be raised. 👍

[kclowes]

Should be fixed in #1419, but note that if the stricter flag is enabled, it is no longer permitted to have bytestrings shorter than the specified byte length. Thanks for raising this @palango!