Fix encoding & hardhat test

[tommyrharper]

Description

Add a hardhat test for conditional order signatures, and fix a tiny bug in the SC ConditionalOrderHashLib.hash function.

Related PR

Fix/addition to PR

Motivation and Context

Front-end was struggling to get signatures to match, reached out for help, I got pulled down the rabbit hole and decided to try and debug.

I found these two stack-overflow posts with the same issue:

• https://stackoverflow.com/questions/75019935/how-to-define-typehash-for-eip712-typed-data-signing-with-nested-struct-in-solid
• https://ethereum.stackexchange.com/questions/151513/eip712-typeddata-encoding-with-nested-array-of-structs-returning-wrong-signer

And also another example that uses the new approach:

• https://gist.github.com/sambacha/d2fb1d45b2a8cf0696c2297125006a6d

In both of those posts, the solution used encodePacked for the array of hashes before hashing that array instead of encode. Honestly I couldn't tell this myself from reading the EIP, so I wrote a hardhat test to verify, and it fails without this change, and passes with it.

This thread questions the reasoning behind this. bb_terk says:

Looking at the example in EIP-712 (https://github.com/ethereum/EIPs/blob/master/assets/eip-712/Example.js 599), I see that dynamic arrays/strings are not padded, instead of static types that are encoded with padding. Maybe the reason is that a dynamic array could be always different from another one, instead of a static type that could have the same rappresentation in memory. I don’t find another reason for that.

That's the closest I could come to a reason for using encodePacked other than the fact it works and others use it in this scenario. The actual EIP itself says the following:

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

This phraseology is pretty opaque to me, if anything it makes me think to use encode over encodePacked. Hence it is possible that Viem and ethers have incorrectly interpreted the EIP. It is either that, or I am not reading it correctly or am missing some details in it.

Note the following change was due to a reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index) error when testing an array of conditions with hardhat:

bytes32[] memory hashedConditions = new bytes32[](co.conditions.length);

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• Ran forge fmt?
• Ran forge snapshot?
• Ran forge test?

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6513c2f) 74.10% compared to head (e20490a) 74.10%.

Files	Patch %	Lines
src/libraries/ConditionalOrderHashLib.sol	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           fix-encoding      #18   +/-   ##
=============================================
  Coverage         74.10%   74.10%           
=============================================
  Files                 5        5           
  Lines               139      139           
  Branches             28       28           
=============================================
  Hits                103      103           
  Misses               30       30           
  Partials              6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JaredBorders]

very nicely done ☑️

[JaredBorders]

I had a single question, but otherwise, everything LGTM ✨