Skip to content

Commit

Permalink
chore: migrate GPv2Order eip712 tests to Foundry (#195)
Browse files Browse the repository at this point in the history
## Description

See title.

This part was particularly painful to port to Solidity; most notably,
there are a lot of missing quality-of-life features when handling
arrays.

Since we don't have `ethers` anymore, I had to build my own EIP712
encoding library (as [recommended by
Foundry](https://book.getfoundry.sh/tutorials/testing-eip712)).
As I could have introduced new bugs in the library, the related tests
have become more aggressive (for example, all flag combinations are
tested for consistency with the contract). There are also a few sanity
checks which have already turned out to be useful during development.

I also sneakily introduced fuzz tests, since they seemed appropriate
here.

Sorry for the horrible nested loops, suggestions on how to get rid of
them are welcome.

The good news: there's a chance that a few things can be simplified with
a future version of the compiler, see comments in code.

## Test Plan

CI.

## Related Issues

#117

---------

Co-authored-by: mfw78 <[email protected]>
  • Loading branch information
fedgiac and mfw78 authored Aug 8, 2024
1 parent 5475208 commit 658b897
Show file tree
Hide file tree
Showing 8 changed files with 272 additions and 52 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ jobs:
run: |
forge --version
if [ "${{ matrix.profile }}" == "solc-0.7.6" ]; then
forge build --sizes --use 0.7.6 --skip 'test/*' --skip 'script/*'
FOUNDRY_PROFILE=ci forge build --sizes --use 0.7.6 --skip 'test/*' --skip 'script/*'
else
forge build --sizes
FOUNDRY_PROFILE=ci forge build --sizes
fi
id: build

- name: Run Forge tests
if: matrix.profile != 'solc-0.7.6'
run: |
forge test -vvv
FOUNDRY_PROFILE=ci forge test -vvv
id: test
5 changes: 4 additions & 1 deletion foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ ignore = [
# We don't want to change the formatting of our main contracts until the
# migration to Foundry is concluded.
"src/contracts/**/*"
]
]

[profile.ci]
fuzz.seed = '0'
9 changes: 0 additions & 9 deletions src/ts/order.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,6 @@ export const ORDER_TYPE_FIELDS = [
{ name: "buyTokenBalance", type: "string" },
];

/**
* The EIP-712 type hash for a Gnosis Protocol v2 order.
*/
export const ORDER_TYPE_HASH = ethers.utils.id(
`Order(${ORDER_TYPE_FIELDS.map(({ name, type }) => `${type} ${name}`).join(
",",
)})`,
);

/**
* Normalizes a timestamp value to a Unix timestamp.
* @param time The timestamp value to normalize.
Expand Down
41 changes: 2 additions & 39 deletions test/GPv2Order.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,9 @@ import { expect } from "chai";
import { Contract, BigNumber } from "ethers";
import { ethers } from "hardhat";

import {
ORDER_TYPE_HASH,
ORDER_UID_LENGTH,
OrderKind,
hashOrder,
packOrderUidParams,
} from "../src/ts";
import { ORDER_UID_LENGTH, packOrderUidParams } from "../src/ts";

import { encodeOrder } from "./encoding";
import { fillBytes, fillDistinctBytes } from "./testHelpers";
import { fillDistinctBytes } from "./testHelpers";

describe("GPv2Order", () => {
let orders: Contract;
Expand All @@ -21,36 +14,6 @@ describe("GPv2Order", () => {
orders = await GPv2Order.deploy();
});

describe("TYPE_HASH", () => {
it("matches the EIP-712 order type hash", async () => {
expect(await orders.typeHashTest()).to.equal(ORDER_TYPE_HASH);
});
});

describe("hash", () => {
it("computes EIP-712 order signing hash", async () => {
const domain = { name: "test" };
const domainSeparator = ethers.utils._TypedDataEncoder.hashDomain(domain);

const order = {
sellToken: fillBytes(20, 0x01),
buyToken: fillBytes(20, 0x02),
receiver: fillBytes(20, 0x03),
sellAmount: ethers.utils.parseEther("42"),
buyAmount: ethers.utils.parseEther("13.37"),
validTo: 0xffffffff,
appData: ethers.constants.HashZero,
feeAmount: ethers.utils.parseEther("1.0"),
kind: OrderKind.SELL,
partiallyFillable: false,
};

expect(
await orders.hashTest(encodeOrder(order), domainSeparator),
).to.equal(hashOrder(domain, order));
});
});

describe("packOrderUidParams", () => {
it("packs the order UID", async () => {
const orderDigest = fillDistinctBytes(32, 1);
Expand Down
18 changes: 18 additions & 0 deletions test/GPv2Order/Helper.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

import {GPv2OrderTestInterface} from "test/src/GPv2OrderTestInterface.sol";

// TODO: move the content of `GPv2OrderTestInterface` here once all tests have been removed.
// solhint-disable-next-line no-empty-blocks
contract Harness is GPv2OrderTestInterface {}

contract Helper is Test {
Harness internal executor;

function setUp() public {
executor = new Harness();
}
}
59 changes: 59 additions & 0 deletions test/GPv2Order/Order.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8;

import {Helper} from "./Helper.sol";

import {GPv2Order, IERC20} from "src/contracts/libraries/GPv2Order.sol";

import {Helper} from "./Helper.sol";
import {Eip712} from "test/libraries/Eip712.sol";
import {Order as OrderLib} from "test/libraries/Order.sol";

contract Order is Helper {
using Eip712 for GPv2Order.Data;
using Eip712 for Eip712.Order;

struct Fuzzed {
address sellToken;
address buyToken;
address receiver;
uint256 sellAmount;
uint256 buyAmount;
uint32 validTo;
bytes32 appData;
uint256 feeAmount;
}

// Keep track of which order hashes have been seen.
mapping(bytes32 orderHash => bool) seen;

function test_TYPE_HASH_matches_the_EIP_712_order_type_hash() public view {
assertEq(executor.typeHashTest(), Eip712.ORDER_TYPE_HASH());
}

function testFuzz_computes_EIP_712_order_signing_hash(Fuzzed memory fuzzed) public {
bytes32 domainSeparator = keccak256("test domain separator");
OrderLib.Flags[] memory flags = OrderLib.ALL_FLAGS();
for (uint256 i = 0; i < flags.length; i++) {
GPv2Order.Data memory order = GPv2Order.Data({
sellToken: IERC20(fuzzed.sellToken),
buyToken: IERC20(fuzzed.buyToken),
receiver: fuzzed.receiver,
sellAmount: fuzzed.sellAmount,
buyAmount: fuzzed.buyAmount,
validTo: fuzzed.validTo,
appData: fuzzed.appData,
feeAmount: fuzzed.feeAmount,
kind: flags[i].kind,
partiallyFillable: flags[i].partiallyFillable,
sellTokenBalance: flags[i].sellTokenBalance,
buyTokenBalance: flags[i].buyTokenBalance
});

bytes32 orderSigningHash = executor.hashTest(order, domainSeparator);
assertEq(orderSigningHash, order.toEip712SignedStruct().typedDataHash(domainSeparator));
require(!seen[orderSigningHash], "different flags led to the same hash");
seen[orderSigningHash] = true;
}
}
}
129 changes: 129 additions & 0 deletions test/libraries/Eip712.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8;

import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol";

library Eip712 {
// This is the struct representing an order that is signed by the user using
// EIP-712.
struct Order {
address sellToken;
address buyToken;
address receiver;
uint256 sellAmount;
uint256 buyAmount;
uint32 validTo;
bytes32 appData;
uint256 feeAmount;
string kind;
bool partiallyFillable;
string sellTokenBalance;
string buyTokenBalance;
}

// Ideally, this would be replaced by type(Order).typehash.
// Progress tracking for this Solidity feature is here:
// https://github.com/ethereum/solidity/issues/14157
function ORDER_TYPE_HASH() internal pure returns (bytes32) {
return keccak256(
bytes(
string.concat(
// Should reflect the definition of the struct `Order`.
"Order(",
"address sellToken,",
"address buyToken,",
"address receiver,",
"uint256 sellAmount,",
"uint256 buyAmount,",
"uint32 validTo,",
"bytes32 appData,",
"uint256 feeAmount,",
"string kind,",
"bool partiallyFillable,",
"string sellTokenBalance,",
"string buyTokenBalance",
")"
)
)
);
}

function toKindString(bytes32 orderKind) internal pure returns (string memory) {
if (orderKind == GPv2Order.KIND_SELL) {
return "sell";
} else if (orderKind == GPv2Order.KIND_BUY) {
return "buy";
} else {
revert("invalid order kind identifier");
}
}

function toSellTokenBalanceString(bytes32 balanceType) private pure returns (string memory) {
return toTokenBalanceString(balanceType, true);
}

function toBuyTokenBalanceString(bytes32 balanceType) private pure returns (string memory) {
return toTokenBalanceString(balanceType, false);
}

function toTokenBalanceString(bytes32 balanceType, bool isSell) internal pure returns (string memory) {
if (balanceType == GPv2Order.BALANCE_ERC20) {
return "erc20";
} else if (balanceType == GPv2Order.BALANCE_EXTERNAL) {
require(isSell, "external order kind is only supported for sell balance");
return "external";
} else if (balanceType == GPv2Order.BALANCE_INTERNAL) {
return "internal";
} else {
revert("invalid order kind identifier");
}
}

function toEip712SignedStruct(GPv2Order.Data memory order) internal pure returns (Order memory) {
return Order({
sellToken: address(order.sellToken),
buyToken: address(order.buyToken),
receiver: order.receiver,
sellAmount: order.sellAmount,
buyAmount: order.buyAmount,
validTo: order.validTo,
appData: order.appData,
feeAmount: order.feeAmount,
kind: toKindString(order.kind),
partiallyFillable: order.partiallyFillable,
sellTokenBalance: toSellTokenBalanceString(order.sellTokenBalance),
buyTokenBalance: toBuyTokenBalanceString(order.buyTokenBalance)
});
}

function hashStruct(Order memory order) internal pure returns (bytes32) {
// Ideally, this would be replaced by `order.hashStruct()`.
// Progress tracking for this Solidity feature is here:
// https://github.com/ethereum/solidity/issues/14208
return keccak256(
// Note: dynamic types are hashed.
abi.encode(
ORDER_TYPE_HASH(),
order.sellToken,
order.buyToken,
order.receiver,
order.sellAmount,
order.buyAmount,
order.validTo,
order.appData,
order.feeAmount,
keccak256(bytes(order.kind)),
order.partiallyFillable,
keccak256(bytes(order.sellTokenBalance)),
keccak256(bytes(order.buyTokenBalance))
)
);
}

// Ideally, this would be replaced by a dedicated function in Solidity.
// This is currently not planned but it could be once `typehash` and
// `hashStruct` are introduced.
function typedDataHash(Order memory order, bytes32 domainSeparator) internal pure returns (bytes32) {
return keccak256(abi.encodePacked("\x19\x01", domainSeparator, hashStruct(order)));
}
}
57 changes: 57 additions & 0 deletions test/libraries/Order.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,63 @@ library Order {
bool partiallyFillable;
}

// I wish I could declare the following as constants and export them as part
// of the library. However, "Only constants of value type and byte array
// type are implemented." and "Library cannot have non-constant state
// variables". So I'm left with defining them as functions.

function ALL_KINDS() internal pure returns (bytes32[2] memory) {
return [GPv2Order.KIND_SELL, GPv2Order.KIND_BUY];
}

function ALL_SELL_TOKEN_BALANCES() internal pure returns (bytes32[3] memory) {
return [GPv2Order.BALANCE_ERC20, GPv2Order.BALANCE_EXTERNAL, GPv2Order.BALANCE_INTERNAL];
}

function ALL_BUY_TOKEN_BALANCES() internal pure returns (bytes32[2] memory) {
return [GPv2Order.BALANCE_ERC20, GPv2Order.BALANCE_INTERNAL];
}

function ALL_FLAGS() internal pure returns (Flags[] memory out) {
uint256 numBools = 1;
uint256 boolLength = 2;
// "out" has as many entries as there are distinct options to fill the
// `Flags` struct.
out = new Flags[](
ALL_KINDS().length * ALL_SELL_TOKEN_BALANCES().length * ALL_BUY_TOKEN_BALANCES().length
* (boolLength * numBools)
);
uint256 offset = 0;
for (uint256 kindI = 0; kindI < ALL_KINDS().length; kindI++) {
for (
uint256 sellTokenBalanceI = 0; sellTokenBalanceI < ALL_SELL_TOKEN_BALANCES().length; sellTokenBalanceI++
) {
for (
uint256 buyTokenBalanceI = 0; buyTokenBalanceI < ALL_BUY_TOKEN_BALANCES().length; buyTokenBalanceI++
) {
bytes32 kind = ALL_KINDS()[kindI];
bytes32 sellTokenBalance = ALL_SELL_TOKEN_BALANCES()[sellTokenBalanceI];
bytes32 buyTokenBalance = ALL_BUY_TOKEN_BALANCES()[buyTokenBalanceI];
out[offset] = Flags({
kind: kind,
sellTokenBalance: sellTokenBalance,
buyTokenBalance: buyTokenBalance,
partiallyFillable: false
});
out[offset + 1] = Flags({
kind: kind,
sellTokenBalance: sellTokenBalance,
buyTokenBalance: buyTokenBalance,
partiallyFillable: true
});
offset += 2;
}
}
}
// Sanity check: we filled all array slots.
require(offset == out.length, "undefined entries in flag array");
}

/// @dev Return an empty sell order
function emptySell() internal pure returns (GPv2Order.Data memory order) {
order.sellToken = IERC20(address(0));
Expand Down

0 comments on commit 658b897

Please sign in to comment.