Significant dyn-abi fixes :)

crates/dyn-abi/src/parser.rs

@@ -184,25 +184,54 @@ impl<'a> TryFrom<&'a str> for TupleSpecifier<'a> {
         // flexible for `a, b` or `(a, b)`, or `tuple(a, b)`
         // or any missing parenthesis
         let value = value.trim();
-        let value = value.strip_suffix(')').unwrap_or(value);
-        let value = value.strip_prefix("tuple").unwrap_or(value);
-        let value = value.strip_prefix('(').unwrap_or(value);
 
-        let mut types = vec![];
+        // if we strip a trailing paren we MUST strip a leading paren
+        let value = if let Some(val) = value.strip_suffix(')') {
+            val.strip_prefix("tuple")
+                .unwrap_or(val)
+                .strip_prefix('(')
+                .ok_or_else(|| DynAbiError::invalid_type_string(value))?
+        } else {
+            value
+        };
+
+        let value = value
+            .strip_suffix(')')
+            .and_then(|val| val.strip_prefix('('))
+            .and_then(|val| val.strip_prefix("tuple"))
+            .unwrap_or(value);
+
+        // passes over nested tuples
+        let mut types: Vec<TypeSpecifier<'_>> = vec![];
         let mut start = 0;
-        let mut depth = 0;
+        let mut depth: usize = 0;
         for (i, c) in value.char_indices() {
             match c {
                 '(' => depth += 1,
-                ')' => depth -= 1,
+                ')' => {
+                    // handle extra closing paren
+                    depth = depth
+                        .checked_sub(1)
+                        .ok_or_else(|| DynAbiError::invalid_type_string(value))?;
+                }
                 ',' if depth == 0 => {
                     types.push(value[start..i].try_into()?);
                     start = i + 1;
                 }
                 _ => {}
             }
         }
-        types.push(value[start..].try_into()?);
+
+        // handle extra open paren
+        if depth != 0 {
+            return Err(DynAbiError::invalid_type_string(value))
+        }
+
+        // handle trailing commas in tuples
+        let candidate = value[start..].trim();
+        if !candidate.is_empty() {
+            types.push(candidate.try_into()?);
+        }
         Ok(Self { span: value, types })
     }
 }
@@ -348,8 +377,49 @@ impl core::str::FromStr for DynSolType {
 #[cfg(test)]
 mod tests {
     use super::*;
+
+    #[test]
+    fn extra_close_parens() {
+        let test_str = "bool,uint256))";
+        assert_eq!(
+            parse(test_str),
+            Err(DynAbiError::invalid_type_string(test_str))
+        );
+    }
+
+    #[test]
+    fn extra_open_parents() {
+        let test_str = "(bool,uint256";
+        assert_eq!(
+            parse(test_str),
+            Err(DynAbiError::invalid_type_string(test_str))
+        );
+    }
+
+    #[test]
+    fn it_parses_tuples() {
+        assert_eq!(
+            parse("(bool,)").unwrap(),
+            DynSolType::Tuple(vec![DynSolType::Bool])
+        );
+        assert_eq!(
+            parse("(uint256,uint256)").unwrap(),
+            DynSolType::Tuple(vec![DynSolType::Uint(256), DynSolType::Uint(256)])
+        );
+        assert_eq!(
+            parse("(uint256,uint256)[2]").unwrap(),
+            DynSolType::FixedArray(
+                Box::new(DynSolType::Tuple(vec![
+                    DynSolType::Uint(256),
+                    DynSolType::Uint(256)
+                ])),
+                2
+            )
+        );
+    }
+
     #[test]
-    fn it_parses_solidity_types() {
+    fn it_parses_simple_types() {
         assert_eq!(parse("uint256").unwrap(), DynSolType::Uint(256));
         assert_eq!(parse("uint8").unwrap(), DynSolType::Uint(8));
         assert_eq!(parse("uint").unwrap(), DynSolType::Uint(256));
@@ -358,6 +428,10 @@ mod tests {
         assert_eq!(parse("string").unwrap(), DynSolType::String);
         assert_eq!(parse("bytes").unwrap(), DynSolType::Bytes);
         assert_eq!(parse("bytes32").unwrap(), DynSolType::FixedBytes(32));
+    }
+
+    #[test]
+    fn it_parses_complex_solidity_types() {
         assert_eq!(
             parse("uint256[]").unwrap(),
             DynSolType::Array(Box::new(DynSolType::Uint(256)))
@@ -379,20 +453,7 @@ mod tests {
                 Box::new(DynSolType::Uint(256))
             )))))
         );
-        assert_eq!(
-            parse("(uint256,uint256)").unwrap(),
-            DynSolType::Tuple(vec![DynSolType::Uint(256), DynSolType::Uint(256)])
-        );
-        assert_eq!(
-            parse("(uint256,uint256)[2]").unwrap(),
-            DynSolType::FixedArray(
-                Box::new(DynSolType::Tuple(vec![
-                    DynSolType::Uint(256),
-                    DynSolType::Uint(256)
-                ])),
-                2
-            )
-        );
+
         assert_eq!(
             parse(r#"tuple(address,bytes, (bool, (string, uint256)[][3]))[2]"#),
             Ok(DynSolType::FixedArray(

crates/dyn-abi/src/token.rs

@@ -130,33 +130,44 @@ impl<'a> DynToken<'a> {
 
     /// Decodes from a decoder, populating the structure with the decoded data.
     #[inline]
-    pub fn decode_populate(&mut self, dec: &mut Decoder<'a>) -> Result<()> {
+    pub(crate) fn decode_populate(&mut self, dec: &mut Decoder<'a>) -> Result<()> {
         let dynamic = self.is_dynamic();
         match self {
             Self::Word(w) => *w = WordToken::decode_from(dec)?.0,
-            Self::FixedSeq(tokens, size) => {
+            Self::FixedSeq(_, _) => {
                 let mut child = if dynamic {
                     dec.take_indirection()?
                 } else {
                     dec.raw_child()
                 };
-                for token in tokens.to_mut().iter_mut().take(*size) {
-                    token.decode_populate(&mut child)?;
+
+                self.decode_sequence_populate(&mut child)?;
+
+                if !dynamic {
+                    dec.take_offset(child);
                 }
             }
             Self::DynSeq { contents, template } => {
                 let mut child = dec.take_indirection()?;
                 let size = child.take_u32()? as usize;
-                let mut new_tokens: Vec<DynToken<'a>> = Vec::with_capacity(size);
-                for _ in 0..size {
-                    let mut t = if let Some(item) = new_tokens.first() {
-                        item.clone()
-                    } else {
-                        *(template.take().unwrap())
-                    };
-                    t.decode_populate(&mut child)?;
-                    new_tokens.push(t);
-                }
+                // This appears to be an unclarity in the solidity spec. The
+                // spec specifies that offsets are relative to the beginning of
+                // `enc(X)`. But known-good test vectors have it relative to the
+                // word AFTER the array size
+                let mut child = child.raw_child();
+
+                let mut new_tokens: Vec<_> = Vec::with_capacity(size);
+                // This expect is safe because this is only invoked after
+                // `empty_dyn_token()` which always sets template
+                let t = template
+                    .take()
+                    .expect("No template. This is an alloy bug. Please report it.");
+                new_tokens.resize(size, *t);
+
+                new_tokens
+                    .iter_mut()
+                    .for_each(|t| t.decode_populate(&mut child).unwrap());
+
                 *contents = new_tokens.into();
             }
             Self::PackedSeq(buf) => *buf = PackedSeqToken::decode_from(dec)?.0,
@@ -169,8 +180,8 @@ impl<'a> DynToken<'a> {
     #[inline]
     pub(crate) fn decode_sequence_populate(&mut self, dec: &mut Decoder<'a>) -> Result<()> {
         match self {
-            Self::FixedSeq(buf, _) => {
-                for item in buf.to_mut().iter_mut() {
+            Self::FixedSeq(buf, size) => {
+                for item in buf.to_mut().iter_mut().take(*size) {
                     item.decode_populate(dec)?;
                 }
                 Ok(())