feat(#2): add ABI and Packed encoding types#49
feat(#2): add ABI and Packed encoding types#49re1ro wants to merge 1 commit intofeature/typed-encoder-step1from
Conversation
re1ro
commented
Oct 31, 2025
- introduce encodingType on TypedEncoder struct with Struct/Array/ABI/Packed variants
- add pure ABI and packed encoding paths with array validation and helper routines
- extend test suite with ABI and Packed coverage
- introduce encodingType on TypedEncoder struct with Struct/Array/ABI/Packed variants - add pure ABI and packed encoding paths with array validation and helper routines - extend test suite with ABI and Packed coverage
There was a problem hiding this comment.
Pull Request Overview
This PR introduces multiple new encoding types to the TypedEncoder library, enabling it to handle various encoding scenarios beyond standard EIP-712 and ABI encoding. The changes add support for compact packed encoding, hash-based commitments, contract address computation (CREATE/CREATE2/CREATE3), and function call encoding.
- Added EncodingType enum with 10 encoding variants (Struct, Array, ABI, Packed, CallWithSelector, CallWithSignature, Hash, Create, Create2, Create3)
- Implemented Packed encoding for compact byte representation without hashing
- Added comprehensive test coverage for Packed, ABI, and Calldata encoding scenarios
- Updated existing tests to include the new required encodingType field
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/TypedEncoder.sol | Core implementation adding EncodingType enum, new encoding functions (_encodePacked, _encodeHash, _encodeCreate*, _encodeCallWith*, _encodeAsArray), comprehensive documentation, and custom error types |
| test/libs/TypedEncoderPackedEncoding.t.sol | New test suite covering Packed encoding with 20+ test cases for primitives, nested structs, arrays, and edge cases |
| test/libs/TypedEncoderCalldata.t.sol | New test suite for ABI encoding type with 5 test cases covering static/dynamic structs, nested ABI encoding, and arrays |
| test/libs/TypedEncoderHash.t.sol | Updated existing tests to include encodingType field (EncodingType.Struct) for backward compatibility |
| test/libs/TypedEncoderEncode.t.sol | Updated existing tests to include encodingType field (EncodingType.Struct) for backward compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| || s.encodingType == EncodingType.Hash || s.encodingType == EncodingType.Create | ||
| || s.encodingType == EncodingType.Create2 || s.encodingType == EncodingType.Create3 | ||
| ) { | ||
| revert EncodingTypeNotImplemented(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The encode() function reverts for Hash, Create, Create2, and Create3 encoding types but _encodeChunkFields() (lines 863-877) calls the implementation functions (_encodeHash, _encodeCreate, _encodeCreate2, _encodeCreate3) for these types. This creates inconsistent behavior where these encoding types work when nested in other structs but fail at the top level. Either remove them from the revert check or remove their implementations from _encodeChunkFields().
| || s.encodingType == EncodingType.Hash || s.encodingType == EncodingType.Create | |
| || s.encodingType == EncodingType.Create2 || s.encodingType == EncodingType.Create3 | |
| ) { | |
| revert EncodingTypeNotImplemented(); | |
| } | |
| ) { | |
| revert EncodingTypeNotImplemented(); | |
| } | |
| // For Hash, Create, Create2, Create3, dispatch to their implementations | |
| if (s.encodingType == EncodingType.Hash) { | |
| return _encodeHash(s); | |
| } | |
| if (s.encodingType == EncodingType.Create) { | |
| return _encodeCreate(s); | |
| } | |
| if (s.encodingType == EncodingType.Create2) { | |
| return _encodeCreate2(s); | |
| } | |
| if (s.encodingType == EncodingType.Create3) { | |
| return _encodeCreate3(s); | |
| } |
| bytes memory arrayHeader; | ||
| bytes memory arrayData; | ||
|
|
||
| for (uint256 i = 0; i < totalStructs; i++) { | ||
| arrayHeader = abi.encodePacked(arrayHeader, abi.encode(offsets[i])); | ||
| arrayData = abi.encodePacked(arrayData, structEncodings[i]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Multiple abi.encodePacked calls in a loop (lines 262-263) create intermediate bytes allocations for each iteration. Consider pre-allocating a single bytes array with known total size (32 + totalStructs*32 + sum of encoding lengths) and using assembly to copy data directly, avoiding O(n²) memory operations.
| bytes memory arrayHeader; | |
| bytes memory arrayData; | |
| for (uint256 i = 0; i < totalStructs; i++) { | |
| arrayHeader = abi.encodePacked(arrayHeader, abi.encode(offsets[i])); | |
| arrayData = abi.encodePacked(arrayData, structEncodings[i]); | |
| } | |
| // Pre-allocate header and data arrays | |
| bytes memory arrayHeader = new bytes(totalStructs * 32); | |
| uint256 dataLength = 0; | |
| for (uint256 i = 0; i < totalStructs; i++) { | |
| dataLength += structEncodings[i].length; | |
| } | |
| bytes memory arrayData = new bytes(dataLength); | |
| // Copy offsets into arrayHeader | |
| for (uint256 i = 0; i < totalStructs; i++) { | |
| uint256 offset = offsets[i]; | |
| assembly { | |
| mstore(add(add(arrayHeader, 32), mul(i, 32)), offset) | |
| } | |
| } | |
| // Copy structEncodings into arrayData | |
| uint256 dataPtr = 32; | |
| for (uint256 i = 0; i < totalStructs; i++) { | |
| bytes memory enc = structEncodings[i]; | |
| uint256 len = enc.length; | |
| assembly { | |
| let dest := add(arrayData, dataPtr) | |
| let src := add(enc, 32) | |
| for { let j := 0 } lt(j, len) { j := add(j, 32) } { | |
| mstore(add(dest, j), mload(add(src, j))) | |
| } | |
| } | |
| dataPtr += len; | |
| } |
| bytes memory expected = | ||
| hex"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000006400000000000000000000000000000000000000000000000000000000000000c8"; |
There was a problem hiding this comment.
The hardcoded hex literal is difficult to verify and maintain. Consider adding inline comments breaking down the structure (e.g., /* offset: 0x20 / / array offset: 0x20 / / length: 2 / / element offsets / / element data */) or computing the expected value programmatically to make the test more readable and maintainable.
| bytes memory expected = | |
| hex"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000006400000000000000000000000000000000000000000000000000000000000000c8"; | |
| // Breakdown of expected encoding: | |
| // [0x20] offset to array data | |
| // [0x20] offset to array data (redundant, matches encoder's output) | |
| // [0x02] array length (2 elements) | |
| // [0x40] offset to first element | |
| // [0x60] offset to second element | |
| // [0x64] first element data (uint256(100)) | |
| // [0xc8] second element data (uint256(200)) | |
| bytes memory expected = abi.encodePacked( | |
| // offset to array data (first slot) | |
| uint256(0x20), | |
| // offset to array data (second slot, matches encoder's output) | |
| uint256(0x20), | |
| // array length | |
| uint256(2), | |
| // offset to first element (0x40) | |
| uint256(0x40), | |
| // offset to second element (0x60) | |
| uint256(0x60), | |
| // first element data (uint256(100)) | |
| uint256(100), | |
| // second element data (uint256(200)) | |
| uint256(200) | |
| ); |
| bytes memory innerEncoded = _encodeAbi(childStruct); | ||
| // Check if struct has dynamic field contents (not encoding type) | ||
| bool hasDynamicFields = _hasDynamicFields(childStruct); |
There was a problem hiding this comment.
_hasDynamicFields() traverses all chunks to check for dynamic fields. This check is redundant since _encodeAbi() already determines the dynamic nature during encoding. Consider having _encodeAbi() return both the encoding and a boolean indicating if it's dynamic, or cache the dynamic check result to avoid duplicate traversal.
| bytes memory innerEncoded = _encodeAbi(childStruct); | |
| // Check if struct has dynamic field contents (not encoding type) | |
| bool hasDynamicFields = _hasDynamicFields(childStruct); | |
| (bytes memory innerEncoded, bool hasDynamicFields) = _encodeAbi(childStruct); | |
| // Use hasDynamicFields from _encodeAbi() to avoid redundant traversal |
| bytes memory packed; | ||
|
|
||
| // Process primitives - pack data directly | ||
| uint256 primLen = chunk.primitives.length; | ||
| for (uint256 i = 0; i < primLen; i++) { | ||
| packed = abi.encodePacked(packed, chunk.primitives[i].data); | ||
| } | ||
|
|
||
| // Process nested structs | ||
| uint256 structLen = chunk.structs.length; | ||
| for (uint256 i = 0; i < structLen; i++) { | ||
| Struct memory nestedStruct = chunk.structs[i]; | ||
|
|
||
| // If nested struct is Hash type, hash it first then pack the bytes32 | ||
| if (nestedStruct.encodingType == EncodingType.Hash) { | ||
| packed = abi.encodePacked(packed, _encodeHash(nestedStruct)); | ||
| } else if (nestedStruct.encodingType == EncodingType.Packed) { | ||
| // If nested struct is Packed type, pack it recursively without hashing | ||
| packed = abi.encodePacked(packed, _encodePacked(nestedStruct)); | ||
| } else { | ||
| // For other encoding types, pack their ABI encoding | ||
| packed = abi.encodePacked(packed, _encodeAbi(nestedStruct)); | ||
| } | ||
| } | ||
|
|
||
| // Process arrays - pack without length prefix | ||
| uint256 arrLen = chunk.arrays.length; | ||
| for (uint256 i = 0; i < arrLen; i++) { | ||
| packed = abi.encodePacked(packed, _encodePackedArray(chunk.arrays[i])); | ||
| } | ||
|
|
There was a problem hiding this comment.
Multiple abi.encodePacked calls in loops throughout _encodePackedChunk (lines 403-428) create intermediate allocations. Each call allocates a new bytes array and copies both the existing packed data and the new data. For large arrays or many fields, consider using assembly with pre-allocated memory to avoid O(n²) behavior.
| bytes memory packed; | |
| // Process primitives - pack data directly | |
| uint256 primLen = chunk.primitives.length; | |
| for (uint256 i = 0; i < primLen; i++) { | |
| packed = abi.encodePacked(packed, chunk.primitives[i].data); | |
| } | |
| // Process nested structs | |
| uint256 structLen = chunk.structs.length; | |
| for (uint256 i = 0; i < structLen; i++) { | |
| Struct memory nestedStruct = chunk.structs[i]; | |
| // If nested struct is Hash type, hash it first then pack the bytes32 | |
| if (nestedStruct.encodingType == EncodingType.Hash) { | |
| packed = abi.encodePacked(packed, _encodeHash(nestedStruct)); | |
| } else if (nestedStruct.encodingType == EncodingType.Packed) { | |
| // If nested struct is Packed type, pack it recursively without hashing | |
| packed = abi.encodePacked(packed, _encodePacked(nestedStruct)); | |
| } else { | |
| // For other encoding types, pack their ABI encoding | |
| packed = abi.encodePacked(packed, _encodeAbi(nestedStruct)); | |
| } | |
| } | |
| // Process arrays - pack without length prefix | |
| uint256 arrLen = chunk.arrays.length; | |
| for (uint256 i = 0; i < arrLen; i++) { | |
| packed = abi.encodePacked(packed, _encodePackedArray(chunk.arrays[i])); | |
| } | |
| // First, collect all packed field data into a temporary array | |
| uint256 primLen = chunk.primitives.length; | |
| uint256 structLen = chunk.structs.length; | |
| uint256 arrLen = chunk.arrays.length; | |
| uint256 totalFields = primLen + structLen + arrLen; | |
| bytes[] memory fieldData = new bytes[](totalFields); | |
| uint256 totalLen = 0; | |
| uint256 idx = 0; | |
| // Primitives | |
| for (uint256 i = 0; i < primLen; i++) { | |
| bytes memory d = chunk.primitives[i].data; | |
| fieldData[idx++] = d; | |
| totalLen += d.length; | |
| } | |
| // Structs | |
| for (uint256 i = 0; i < structLen; i++) { | |
| Struct memory nestedStruct = chunk.structs[i]; | |
| bytes memory d; | |
| if (nestedStruct.encodingType == EncodingType.Hash) { | |
| d = _encodeHash(nestedStruct); | |
| } else if (nestedStruct.encodingType == EncodingType.Packed) { | |
| d = _encodePacked(nestedStruct); | |
| } else { | |
| d = _encodeAbi(nestedStruct); | |
| } | |
| fieldData[idx++] = d; | |
| totalLen += d.length; | |
| } | |
| // Arrays | |
| for (uint256 i = 0; i < arrLen; i++) { | |
| bytes memory d = _encodePackedArray(chunk.arrays[i]); | |
| fieldData[idx++] = d; | |
| totalLen += d.length; | |
| } | |
| // Allocate the packed bytes array | |
| bytes memory packed = new bytes(totalLen); | |
| uint256 offset = 0; | |
| for (uint256 i = 0; i < totalFields; i++) { | |
| bytes memory d = fieldData[i]; | |
| uint256 len = d.length; | |
| if (len == 0) continue; | |
| // Copy bytes using assembly | |
| assembly { | |
| let dest := add(add(packed, 32), offset) | |
| let src := add(d, 32) | |
| for { let j := 0 } lt(j, len) { j := add(j, 32) } { | |
| mstore(add(dest, j), mload(add(src, j))) | |
| } | |
| } | |
| offset += len; | |
| } |
| function _encodeCallWithSelector( | ||
| Struct memory s | ||
| ) private pure returns (bytes memory) { | ||
| // Validation is performed in encode() function before calling this private function |
There was a problem hiding this comment.
Comment states 'validation is performed in encode()' but encode() reverts for CallWithSelector at line 200-204. The function is called from _encodeChunkFields without validation, meaning invalid structures (wrong number of primitives/structs) could cause array out-of-bounds access at lines 284, 295.
| // Validation is performed in encode() function before calling this private function | |
| // Validate structure: exactly 1 chunk, with 1 primitive and 1 struct | |
| if ( | |
| s.chunks.length != 1 || | |
| s.chunks[0].primitives.length != 1 || | |
| s.chunks[0].structs.length != 1 | |
| ) { | |
| revert InvalidCallEncodingStructure(); | |
| } |
There was a problem hiding this comment.
Not true, never validated.
| bytes4 selector; | ||
| assembly { | ||
| // Load from data + 32 (skip length prefix) to get the actual bytes | ||
| selector := mload(add(selectorData, 32)) |
There was a problem hiding this comment.
Loading bytes4 selector with mload reads 32 bytes, not 4. If selectorData is exactly 4 bytes, this reads the full 4 bytes left-aligned in the 32-byte word (correct). However, if selectorData contains more than 4 bytes, the selector will include extra bytes. Add validation that selectorData.length == 4, or mask the result with 'and(mload(...), 0xffffffff00000000000000000000000000000000000000000000000000000000)'.
| bytes4 selector; | |
| assembly { | |
| // Load from data + 32 (skip length prefix) to get the actual bytes | |
| selector := mload(add(selectorData, 32)) | |
| require(selectorData.length == 4, "TypedEncoder: selector must be 4 bytes"); | |
| bytes4 selector; | |
| assembly { | |
| // Load from data + 32 (skip length prefix) to get the actual bytes, then mask to 4 bytes | |
| selector := and(mload(add(selectorData, 32)), 0xffffffff00000000000000000000000000000000000000000000000000000000) |
| enum EncodingType { | ||
| Struct, | ||
| Array, | ||
| ABI, |
There was a problem hiding this comment.
Let's try to come up with better name for ABI and Struct.
| Struct memory nestedStruct = chunk.structs[i]; | ||
|
|
||
| // If nested struct is Hash type, hash it first then pack the bytes32 | ||
| if (nestedStruct.encodingType == EncodingType.Hash) { | ||
| packed = abi.encodePacked(packed, _encodeHash(nestedStruct)); | ||
| } else if (nestedStruct.encodingType == EncodingType.Packed) { | ||
| // If nested struct is Packed type, pack it recursively without hashing | ||
| packed = abi.encodePacked(packed, _encodePacked(nestedStruct)); | ||
| } else { | ||
| // For other encoding types, pack their ABI encoding | ||
| packed = abi.encodePacked(packed, _encodeAbi(nestedStruct)); | ||
| } |
There was a problem hiding this comment.
Why not just?
packed = abi.encodePacked(packed, encode(chunk.structs[i]))
| bytes memory innerEncoded = _encodeAbi(childStruct); | ||
| // Check if struct has dynamic field contents (not encoding type) | ||
| bool hasDynamicFields = _hasDynamicFields(childStruct); | ||
| structEncoded = | ||
| hasDynamicFields ? abi.encodePacked(abi.encode(uint256(32)), innerEncoded) : innerEncoded; |
There was a problem hiding this comment.
I thought .ABI shouldn't have the offset but you are adding it here instead of below?
| } else { | ||
| // EncodingType.Struct uses standard ABI encoding | ||
| structEncoded = _encodeAbi(childStruct); | ||
| } |
There was a problem hiding this comment.
| } else { | |
| // EncodingType.Struct uses standard ABI encoding | |
| structEncoded = _encodeAbi(childStruct); | |
| } | |
| } else if (childStruct.encodingType == EncodingType.Struct) { | |
| // EncodingType.Struct uses standard ABI encoding | |
| structEncoded = _encodeAbi(childStruct); | |
| } else { | |
| revert ...; | |
| } |
Now that we have so many enums, it's better to avoid using else cuz otherwise it's easy to create a bug by adding another type in the enum but forget to check the else.
| // Validate array encoding structure before forwarding | ||
| if (s.chunks.length != 1) { | ||
| revert UnsupportedArrayType(); | ||
| } | ||
| Chunk memory chunk = s.chunks[0]; | ||
| if (chunk.primitives.length > 0 || chunk.arrays.length > 0) { | ||
| revert UnsupportedArrayType(); | ||
| } |
There was a problem hiding this comment.
Why not check these within _encodeAsArray?
| function _encodeCallWithSignature( | ||
| Struct memory s | ||
| ) private pure returns (bytes memory) { | ||
| // Validation is performed in encode() function before calling this private function |
| Chunk memory chunk = s.chunks[0]; | ||
| Primitive memory signaturePrimitive = chunk.primitives[0]; | ||
|
|
||
| // Compute selector from signature: bytes4(keccak256(signature)) | ||
| bytes4 selector = bytes4(keccak256(signaturePrimitive.data)); | ||
|
|
||
| // Encode the params struct | ||
| Struct memory paramsStruct = chunk.structs[0]; | ||
|
|
||
| // For CallWithSignature, we need to encode the struct fields as if they were passed | ||
| // individually to abi.encodeWithSignature, not as a wrapped struct | ||
| // This means we encode the chunk directly without struct wrapper | ||
| bytes memory params; | ||
|
|
||
| if (paramsStruct.chunks.length == 0) { | ||
| // Empty params case (e.g., reset() with no arguments) | ||
| params = ""; | ||
| } else if (paramsStruct.chunks.length == 1) { | ||
| // Single chunk - encode it directly | ||
| params = _encodeAbi(paramsStruct.chunks[0]); | ||
| } else { | ||
| // Multiple chunks - encode each and concatenate | ||
| for (uint256 i = 0; i < paramsStruct.chunks.length; i++) { | ||
| params = abi.encodePacked(params, _encodeAbi(paramsStruct.chunks[i])); | ||
| } | ||
| } | ||
|
|
||
| // Combine selector (4 bytes) + params | ||
| return abi.encodePacked(selector, params); |
There was a problem hiding this comment.
| Chunk memory chunk = s.chunks[0]; | |
| Primitive memory signaturePrimitive = chunk.primitives[0]; | |
| // Compute selector from signature: bytes4(keccak256(signature)) | |
| bytes4 selector = bytes4(keccak256(signaturePrimitive.data)); | |
| // Encode the params struct | |
| Struct memory paramsStruct = chunk.structs[0]; | |
| // For CallWithSignature, we need to encode the struct fields as if they were passed | |
| // individually to abi.encodeWithSignature, not as a wrapped struct | |
| // This means we encode the chunk directly without struct wrapper | |
| bytes memory params; | |
| if (paramsStruct.chunks.length == 0) { | |
| // Empty params case (e.g., reset() with no arguments) | |
| params = ""; | |
| } else if (paramsStruct.chunks.length == 1) { | |
| // Single chunk - encode it directly | |
| params = _encodeAbi(paramsStruct.chunks[0]); | |
| } else { | |
| // Multiple chunks - encode each and concatenate | |
| for (uint256 i = 0; i < paramsStruct.chunks.length; i++) { | |
| params = abi.encodePacked(params, _encodeAbi(paramsStruct.chunks[i])); | |
| } | |
| } | |
| // Combine selector (4 bytes) + params | |
| return abi.encodePacked(selector, params); | |
| s.chunks[0].primitives[0] = bytes4(keccak256(signaturePrimitive.data)); | |
| return _encodeCallWithSelector(s); |
| function _encodeCreate( | ||
| Struct memory s | ||
| ) private pure returns (address) { | ||
| // Validation is performed in encode() function before calling this private function |
| function _encodeCreate2( | ||
| Struct memory s | ||
| ) private pure returns (address) { | ||
| // Validation is performed in encode() function before calling this private function |
| function _encodeCreate3( | ||
| Struct memory s | ||
| ) private pure returns (address) { | ||
| // Validation is performed in encode() function before calling this private function |
There was a problem hiding this comment.
Why all these ABI test cases in TypedEncoderCalldataTest?