From e412e6e30910472b9d5f9000370ce5138ad39ce7 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 12 Apr 2024 20:15:40 +0100 Subject: [PATCH] feat: narrow ABI encoding errors down to target problem argument/field (#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. --- tooling/noirc_abi/src/errors.rs | 9 +- tooling/noirc_abi/src/input_parser/mod.rs | 157 +++++++++++++++--- tooling/noirc_abi/src/lib.rs | 10 +- .../test/browser/errors.test.ts | 2 +- .../noirc_abi_wasm/test/node/errors.test.ts | 2 +- 5 files changed, 139 insertions(+), 41 deletions(-) diff --git a/tooling/noirc_abi/src/errors.rs b/tooling/noirc_abi/src/errors.rs index 687fecfcc1d..4209a9e218b 100644 --- a/tooling/noirc_abi/src/errors.rs +++ b/tooling/noirc_abi/src/errors.rs @@ -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; @@ -38,8 +41,8 @@ impl From for InputParserError { pub enum AbiError { #[error("Received parameters not expected by ABI: {0:?}")] UnexpectedParams(Vec), - #[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( diff --git a/tooling/noirc_abi/src/input_parser/mod.rs b/tooling/noirc_abi/src/input_parser/mod.rs index f66e069d487..024cb06007e 100644 --- a/tooling/noirc_abi/src/input_parser/mod.rs +++ b/tooling/noirc_abi/src/input_parser/mod.rs @@ -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; @@ -22,63 +23,165 @@ pub enum InputValue { Struct(BTreeMap), } +#[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 }, + #[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 = + 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 diff --git a/tooling/noirc_abi/src/lib.rs b/tooling/noirc_abi/src/lib.rs index 89a60b0ed26..6ad13500bdd 100644 --- a/tooling/noirc_abi/src/lib.rs +++ b/tooling/noirc_abi/src/lib.rs @@ -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)) }) diff --git a/tooling/noirc_abi_wasm/test/browser/errors.test.ts b/tooling/noirc_abi_wasm/test/browser/errors.test.ts index 429a2d446a3..0f75ff64a3e 100644 --- a/tooling/noirc_abi_wasm/test/browser/errors.test.ts +++ b/tooling/noirc_abi_wasm/test/browser/errors.test.ts @@ -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 }', ); }); diff --git a/tooling/noirc_abi_wasm/test/node/errors.test.ts b/tooling/noirc_abi_wasm/test/node/errors.test.ts index 0d007e64803..fba451b4a8c 100644 --- a/tooling/noirc_abi_wasm/test/node/errors.test.ts +++ b/tooling/noirc_abi_wasm/test/node/errors.test.ts @@ -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 }', ); });