Skip to content

Commit

Permalink
feat: narrow ABI encoding errors down to target problem argument/field (
Browse files Browse the repository at this point in the history
#4798)

# Description

## Problem\*

Resolves #3560 
Resolves #4778 

## Summary\*

This PR updates `InputValue.matches_abi` to
`InputValue.find_type_mismatch` which returns an error related to the
type which is failing to be abi encoded.

## Additional Context

This should help @alexghr with debugging #4778 

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Apr 12, 2024
1 parent d71d5de commit e412e6e
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 41 deletions.
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

0 comments on commit e412e6e

Please sign in to comment.