Skip to content

Commit

Permalink
chore: do not rely on src libraries in test libraries (#210)
Browse files Browse the repository at this point in the history
## Description

We define a broad set of library functions to make testing the contracts
in `src` easier. However, these libraries themselves rely on the tested
contracts. This could lead to issues when the tests pass because both
the libraries and the files in `src` are broken in the same way. To
guarantee that tests are independent of the tested libraries, all use of
`src` libraries has been removed.

## What I really wanted

Unfortunately, it's still possible that we forget about this and use
some function in, e.g., `GPv2Order` in the test library, especially
since we need to import this library to get related types and constants.

I tried to create a Solidity file that reexports everything we need from
the libraries without the functions themselves (this would, for example,
allow us to define a custom linting rule asserting that there's no
import from `src` in any test file) but I was unsuccessful. It was very
easy to reexport contracts (e.g., `GPv2Settlement`) and interfaces
(e.g., `IERC20`), and it was also easy to reexport constants (e.g.,
`GPv2Order.KIND_SELL`), The issue was with reexporting type structs: to
my understanding, this is impossible in Solidity ≤0.8.26, confirmed by
the fact that there is no statement that appears to enable this use case
in the [language
grammar](https://docs.soliditylang.org/en/v0.8.26/grammar.html).
Since neither reexporting everything but the structs nor reexporting the
entire libraries make sense, I decided to keep everything as it is
without further reexporting.

## Test Plan

Nothing broken in CI.
  • Loading branch information
fedgiac authored Sep 10, 2024
1 parent 5957d67 commit 1ff7aef
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 24 deletions.
44 changes: 27 additions & 17 deletions test/libraries/Order.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@
pragma solidity ^0.8;

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

library Order {
using GPv2Order for GPv2Order.Data;
using GPv2Order for bytes;
using GPv2Trade for uint256;
import {Eip712} from "./Eip712.sol";

library Order {
/// Order flags
struct Flags {
bytes32 kind;
Expand Down Expand Up @@ -134,10 +131,24 @@ library Order {
}
}

/// @dev Given a GPv2Trade encoded flags, decode them into a `Flags` struct
function toFlags(uint256 encodedFlags) internal pure returns (Flags memory flags) {
(flags.kind, flags.partiallyFillable, flags.sellTokenBalance, flags.buyTokenBalance,) =
encodedFlags.extractFlags();
/// @dev Given a GPv2Trade encoded flags, decode them into a `Flags` struct.
/// See GPv2Trade.extractFlags for a complete definition of each flag.
function toFlags(uint256 encodedFlags) internal pure returns (Flags memory) {
bytes32 sellTokenBalance;
if (encodedFlags & 0x08 == 0) {
sellTokenBalance = GPv2Order.BALANCE_ERC20;
} else if (encodedFlags & 0x04 == 0) {
sellTokenBalance = GPv2Order.BALANCE_EXTERNAL;
} else {
sellTokenBalance = GPv2Order.BALANCE_INTERNAL;
}

return Flags({
kind: (encodedFlags & 0x01 == 0) ? GPv2Order.KIND_SELL : GPv2Order.KIND_BUY,
partiallyFillable: encodedFlags & 0x02 != 0,
sellTokenBalance: sellTokenBalance,
buyTokenBalance: (encodedFlags & 0x10 == 0) ? GPv2Order.BALANCE_ERC20 : GPv2Order.BALANCE_INTERNAL
});
}

/// @dev Computes the order UID for an order and the given owner
Expand All @@ -146,17 +157,16 @@ library Order {
pure
returns (bytes memory)
{
return computeOrderUid(order.hash(domainSeparator), owner, order.validTo);
return computeOrderUid(hash(order, domainSeparator), owner, order.validTo);
}

/// @dev Computes the order UID for its base components
function computeOrderUid(bytes32 orderHash, address owner, uint32 validTo)
internal
pure
returns (bytes memory orderUid)
{
orderUid = new bytes(GPv2Order.UID_LENGTH);
orderUid.packOrderUidParams(orderHash, owner, validTo);
function computeOrderUid(bytes32 orderHash, address owner, uint32 validTo) internal pure returns (bytes memory) {
return abi.encodePacked(orderHash, owner, validTo);
}

function hash(GPv2Order.Data memory order, bytes32 domainSeparator) internal pure returns (bytes32) {
return Eip712.typedDataHash(Eip712.toEip712SignedStruct(order), domainSeparator);
}

function fuzz(Fuzzed memory params) internal pure returns (GPv2Order.Data memory) {
Expand Down
14 changes: 9 additions & 5 deletions test/libraries/Sign.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ pragma solidity ^0.8;

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

import {EIP1271Verifier, GPv2Order, GPv2Signing, GPv2Trade} from "src/contracts/mixins/GPv2Signing.sol";
import {EIP1271Verifier, GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol";

import {Bytes} from "./Bytes.sol";
import {Order} from "./Order.sol";

type PreSignSignature is address;

library Sign {
using GPv2Order for GPv2Order.Data;
using GPv2Trade for uint256;
using Bytes for bytes;
using Order for GPv2Order.Data;

// Copied from GPv2Signing.sol
uint256 internal constant PRE_SIGNED = uint256(keccak256("GPv2Signing.Scheme.PreSign"));
Expand Down Expand Up @@ -123,8 +123,12 @@ library Sign {
}

/// @dev Given a GPv2Trade encoded flags, decode them into a `GPv2Signing.Scheme`
function toSigningScheme(uint256 encodedFlags) internal pure returns (GPv2Signing.Scheme signingScheme) {
(,,,, signingScheme) = encodedFlags.extractFlags();
function toSigningScheme(uint256 encodedFlags) internal pure returns (GPv2Signing.Scheme) {
uint256 encodedScheme = (encodedFlags >> 5);
if (encodedScheme >= 4) {
revert("invalid flag encoding, trying to use invalid signature scheme");
}
return GPv2Signing.Scheme(encodedScheme);
}

/// @dev Internal helper function for EthSign signatures (non-EIP-712)
Expand Down
1 change: 0 additions & 1 deletion test/libraries/Trade.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {Sign} from "./Sign.sol";
import {GPv2Order, GPv2Signing, GPv2Trade, IERC20} from "src/contracts/mixins/GPv2Signing.sol";

library Trade {
using GPv2Trade for uint256;
using Order for Order.Flags;
using Order for uint256;
using Sign for GPv2Signing.Scheme;
Expand Down
1 change: 0 additions & 1 deletion test/libraries/encoders/SettlementEncoder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {Trade} from "../Trade.sol";
import {Registry, TokenRegistry} from "./TokenRegistry.sol";

library SettlementEncoder {
using GPv2Order for GPv2Order.Data;
using Trade for GPv2Order.Data;
using Sign for Vm;
using TokenRegistry for TokenRegistry.State;
Expand Down

0 comments on commit 1ff7aef

Please sign in to comment.