spec: Validate required divisor fields are not 0

[asymmetric]

This is a replacement for #7882, implementing the check at the the deserialization level with the addition of a #[serde(deserialize_with) attribute on the required Uint divisor fields.

Optional<Uint> divisor fields are not checked at the moment, since I was having trouble getting that to work.

/cc @AndreaSilva @rphmeier

Closes #7482.
Closes #7882.

[parity-cla-bot]

It looks like @asymmetric signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[debris]

thanks!

[debris]

how does printed error look like? Does it say describe which field is expected to have a non-zero value?

Maybe instead of using should_panic you could do:

assert_eq!(serde_json::from_str(s).unwrap_err().to_string(), "error message");

[andresilva]

It doesn't specify which field is failing, only it's position. The error message looks like this:
"invalid value: integer `0`, expected a non-zero value at line 3 column 39". I agree the test can be converted not to use should_panic. It would be nice to show the field name but this is the same behavior we already have for other fields.

[asymmetric]

Why is it better to avoid #[should_panic(expect)]? I thought that using an attribute would make more sense, since it's available.

[debris]

@paritytech/ci @General-Beck last commit - 555e0cb is not run by CI. Could you check why?

[andresilva]

LGTM! 👍

[andresilva]

@asymmetric I think the following will work for Option<Uint>:

pub fn validate_opt_non_zero<'de, D>(d: D) -> Result<Option<Uint>, D::Error> where D: Deserializer<'de> {
	let value: Option<Uint> = Option::deserialize(d)?;

	if let Some(value) = value {
		if value == Uint(U256::from(0)) {
			return Err(Error::invalid_value(Unexpected::Unsigned(value.into()), &"a non-zero value"))
		}
	}

	Ok(value)
}

You then need to enable default when declaring the deserialize_with:

#[serde(rename="difficultyIncrementDivisor")]
#[serde(default, deserialize_with="uint::validate_opt_non_zero")]
pub difficulty_increment_divisor: Option<Uint>,

[asymmetric]

@andresilva Thanks, that seems to have done the trick!

I was declaring the validation function as

pub fn validate_optional_non_zero<'de, D>(d: Option<D>) -> Result<Option<Uint>, D::Error> where D: Deserializer<'de>

(i.e. the argument was an Option<D>), which caused:

error[E0308]: mismatched types
  --> json/src/spec/ethash.rs:23:35
   |
23 | #[derive(Clone, Debug, PartialEq, Deserialize)]
   |                                   ^^^^^^^^^^^
   |                                   |
   |                                   expected enum `std::option::Option`, found type parameter
   |                                   help: try using a variant of the expected type: `serde::export::Some(__deserializer)`
   |
   = note: expected type `std::option::Option<_>`
              found type `__D`

By the way, it compiles even without #[serde(default)]. What does the attribute do, e.g. what's the default deserialization for the field?

[5chdn]

Tests fail :)

[andresilva]

@asymmetric I think when you use deserialize_with it no longer sets default to true, which means missing fields are not deserialized as None and instead you get a missing field error (this is why the tests are failing).