noirc_abi cannot parse negative integer strings from JSON

[TomAFrench]

I noticed that we're handling signed integers differently for JSON files compared to TOML.

noir/tooling/noirc_abi/src/input_parser/toml.rs

Lines 129 to 145 in 6167b73

	(
	TomlTypes::String(string),
	AbiType::Field
	| AbiType::Integer { sign: crate::Sign::Unsigned, .. }
	| AbiType::Boolean,
	) => InputValue::Field(parse_str_to_field(&string)?),
	(TomlTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => {
	InputValue::Field(parse_str_to_signed(&string, *width)?)
	}
	(
	TomlTypes::Integer(integer),
	AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean,
	) => {
	let new_value = FieldElement::from(i128::from(integer));
	InputValue::Field(new_value)
	}

noir/tooling/noirc_abi/src/input_parser/json.rs

Lines 146 to 158 in 6167b73

	(
	JsonTypes::String(string),
	AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean,
	) => InputValue::Field(parse_str_to_field(&string)?),
	(
	JsonTypes::Integer(integer),
	AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean,
	) => {
	let new_value = FieldElement::from(i128::from(integer));
	InputValue::Field(new_value)
	}

That is we don't have handling for negative integer strings in JSON. This means that the WASM ABI encoder can't handle these inputs which is bad.

We currently have proptests for ABI encoding but we should probably extend these to cover the input parsing and fix any issues which arise.

[TomAFrench]

I've scoped out the tests necessary in #6638 but I'd appreciate others taking this up.

[asterite]

Oh, I started working on this before it was assigned to @guipublic .

So far what I found is that parse_str_to_signed has a bug in it. In this line:

noir/tooling/noirc_abi/src/input_parser/mod.rs

Line 329 in aa143a7

BigInt::from(2).pow(width) + bigint

we are turning, say, -1, to fit in i8, so we end up with 2*8 - 1 = 255. However the maximum value of i8 is 127, so the math should be BigInt::from(2).pow(width -1) + bigint.

Then the tests signed_integer_serialization_roundtrip will always fail for negative numbers because output_number will always be positive. We need to do a different check when it's negative.

I'll try to send a PR if I manage to fix this.

[TomAFrench]

You've written the same expression as the code you're saying is incorrect but I would say that 255 is correct here as it's the two's complement of -1 which is how we represent signed integers.

[asterite]

Oh, I see what you mean. I'll rollback that commit, or fix it and the fix the test.

[asterite]

I think I got it right this time 🤞

[asterite]

Closed by #6638