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

feat: narrow ABI encoding errors down to target problem argument/field #4798

Merged
merged 3 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions tooling/noirc_abi/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::{input_parser::InputValue, AbiParameter, AbiType};
use crate::{
input_parser::{InputTypecheckingError, InputValue},
AbiType,
};
use acvm::acir::native_types::Witness;
use thiserror::Error;

Expand Down Expand Up @@ -38,8 +41,8 @@ impl From<serde_json::Error> for InputParserError {
pub enum AbiError {
#[error("Received parameters not expected by ABI: {0:?}")]
UnexpectedParams(Vec<String>),
#[error("The parameter {} is expected to be a {:?} but found incompatible value {value:?}", .param.name, .param.typ)]
TypeMismatch { param: AbiParameter, value: InputValue },
#[error("The value passed for parameter `{}` does not match the specified type:\n{0}", .0.path())]
TypeMismatch(#[from] InputTypecheckingError),
#[error("ABI expects the parameter `{0}`, but this was not found")]
MissingParam(String),
#[error(
Expand Down
157 changes: 130 additions & 27 deletions tooling/noirc_abi/src/input_parser/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use num_bigint::{BigInt, BigUint};
use num_traits::{Num, Zero};
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};
use thiserror::Error;

use acvm::FieldElement;
use serde::Serialize;
Expand All @@ -22,63 +23,165 @@ pub enum InputValue {
Struct(BTreeMap<String, InputValue>),
}

#[derive(Debug, Error)]
pub enum InputTypecheckingError {
#[error("Value {value:?} does not fall within range of allowable values for a {typ:?}")]
OutsideOfValidRange { path: String, typ: AbiType, value: InputValue },
#[error("Type {typ:?} is expected to have length {expected_length} but value {value:?} has length {actual_length}")]
LengthMismatch {
path: String,
typ: AbiType,
value: InputValue,
expected_length: usize,
actual_length: usize,
},
#[error("Could not find value for required field `{expected_field}`. Found values for fields {found_fields:?}")]
MissingField { path: String, expected_field: String, found_fields: Vec<String> },
#[error("Additional unexpected field was provided for type {typ:?}. Found field named `{extra_field}`")]
UnexpectedField { path: String, typ: AbiType, extra_field: String },
#[error("Type {typ:?} and value {value:?} do not match")]
IncompatibleTypes { path: String, typ: AbiType, value: InputValue },
}

impl InputTypecheckingError {
pub(crate) fn path(&self) -> &str {
match self {
InputTypecheckingError::OutsideOfValidRange { path, .. }
| InputTypecheckingError::LengthMismatch { path, .. }
| InputTypecheckingError::MissingField { path, .. }
| InputTypecheckingError::UnexpectedField { path, .. }
| InputTypecheckingError::IncompatibleTypes { path, .. } => path,
}
}
}

impl InputValue {
/// Checks whether the ABI type matches the InputValue type
/// and also their arity
pub fn matches_abi(&self, abi_param: &AbiType) -> bool {
pub(crate) fn find_type_mismatch(
&self,
abi_param: &AbiType,
path: String,
) -> Result<(), InputTypecheckingError> {
match (self, abi_param) {
(InputValue::Field(_), AbiType::Field) => true,
(InputValue::Field(_), AbiType::Field) => Ok(()),
(InputValue::Field(field_element), AbiType::Integer { width, .. }) => {
field_element.num_bits() <= *width
if field_element.num_bits() <= *width {
Ok(())
} else {
Err(InputTypecheckingError::OutsideOfValidRange {
path,
typ: abi_param.clone(),
value: self.clone(),
})
}
}
(InputValue::Field(field_element), AbiType::Boolean) => {
field_element.is_one() || field_element.is_zero()
if field_element.is_one() || field_element.is_zero() {
Ok(())
} else {
Err(InputTypecheckingError::OutsideOfValidRange {
path,
typ: abi_param.clone(),
value: self.clone(),
})
}
}

(InputValue::Vec(array_elements), AbiType::Array { length, typ, .. }) => {
if array_elements.len() != *length as usize {
return false;
return Err(InputTypecheckingError::LengthMismatch {
path,
typ: abi_param.clone(),
value: self.clone(),
expected_length: *length as usize,
actual_length: array_elements.len(),
});
}
// Check that all of the array's elements' values match the ABI as well.
array_elements.iter().all(|input_value| input_value.matches_abi(typ))
for (i, element) in array_elements.iter().enumerate() {
let mut path = path.clone();
path.push_str(&format!("[{i}]"));

element.find_type_mismatch(typ, path)?;
}
Ok(())
}

(InputValue::String(string), AbiType::String { length }) => {
string.len() == *length as usize
if string.len() == *length as usize {
Ok(())
} else {
Err(InputTypecheckingError::LengthMismatch {
path,
typ: abi_param.clone(),
value: self.clone(),
actual_length: string.len(),
expected_length: *length as usize,
})
}
}

(InputValue::Struct(map), AbiType::Struct { fields, .. }) => {
if map.len() != fields.len() {
return false;
for (field_name, field_type) in fields {
if let Some(value) = map.get(field_name) {
let mut path = path.clone();
path.push_str(&format!(".{field_name}"));
value.find_type_mismatch(field_type, path)?;
} else {
return Err(InputTypecheckingError::MissingField {
path,
expected_field: field_name.to_string(),
found_fields: map.keys().cloned().collect(),
});
}
}

let field_types = BTreeMap::from_iter(fields.iter().cloned());
if map.len() > fields.len() {
let expected_fields: HashSet<String> =
fields.iter().map(|(field, _)| field.to_string()).collect();
let extra_field = map.keys().cloned().find(|key| !expected_fields.contains(key)).expect("`map` is larger than the expected type's `fields` so it must contain an unexpected field");
return Err(InputTypecheckingError::UnexpectedField {
path,
typ: abi_param.clone(),
extra_field: extra_field.to_string(),
});
}

// Check that all of the struct's fields' values match the ABI as well.
map.iter().all(|(field_name, field_value)| {
if let Some(field_type) = field_types.get(field_name) {
field_value.matches_abi(field_type)
} else {
false
}
})
Ok(())
}

(InputValue::Vec(vec_elements), AbiType::Tuple { fields }) => {
if vec_elements.len() != fields.len() {
return false;
return Err(InputTypecheckingError::LengthMismatch {
path,
typ: abi_param.clone(),
value: self.clone(),
actual_length: vec_elements.len(),
expected_length: fields.len(),
});
}

vec_elements
.iter()
.zip(fields)
.all(|(input_value, abi_param)| input_value.matches_abi(abi_param))
// Check that all of the array's elements' values match the ABI as well.
for (i, (element, expected_typ)) in vec_elements.iter().zip(fields).enumerate() {
let mut path = path.clone();
path.push_str(&format!(".{i}"));
element.find_type_mismatch(expected_typ, path)?;
}
Ok(())
}

// All other InputValue-AbiType combinations are fundamentally incompatible.
_ => false,
_ => Err(InputTypecheckingError::IncompatibleTypes {
path,
typ: abi_param.clone(),
value: self.clone(),
}),
}
}

/// Checks whether the ABI type matches the InputValue type.
pub fn matches_abi(&self, abi_param: &AbiType) -> bool {
self.find_type_mismatch(abi_param, String::new()).is_ok()
}
}

/// The different formats that are supported when parsing
Expand Down
10 changes: 1 addition & 9 deletions tooling/noirc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,15 +307,7 @@ impl Abi {
.ok_or_else(|| AbiError::MissingParam(param_name.clone()))?
.clone();

if !value.matches_abi(&expected_type) {
let param = self
.parameters
.iter()
.find(|param| param.name == param_name)
.unwrap()
.clone();
return Err(AbiError::TypeMismatch { param, value });
}
value.find_type_mismatch(&expected_type, param_name.clone())?;

Self::encode_value(value, &expected_type).map(|v| (param_name, v))
})
Expand Down
2 changes: 1 addition & 1 deletion tooling/noirc_abi_wasm/test/browser/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ it('errors when an integer input overflows', async () => {
const { abi, inputs } = await import('../shared/uint_overflow');

expect(() => abiEncode(abi, inputs)).to.throw(
'The parameter foo is expected to be a Integer { sign: Unsigned, width: 32 } but found incompatible value Field(2³⁸)',
'The value passed for parameter `foo` does not match the specified type:\nValue Field(2³⁸) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }',
);
});

Expand Down
2 changes: 1 addition & 1 deletion tooling/noirc_abi_wasm/test/node/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ it('errors when an integer input overflows', async () => {
const { abi, inputs } = await import('../shared/uint_overflow');

expect(() => abiEncode(abi, inputs)).to.throw(
'The parameter foo is expected to be a Integer { sign: Unsigned, width: 32 } but found incompatible value Field(2³⁸)',
'The value passed for parameter `foo` does not match the specified type:\nValue Field(2³⁸) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }',
);
});

Expand Down
Loading