diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 00000000..0a63aad2 --- /dev/null +++ b/.prettierignore @@ -0,0 +1 @@ +src/contracts/test/vendor/ diff --git a/bench/single.ts b/bench/single.ts index 565f3606..9b912b98 100644 --- a/bench/single.ts +++ b/bench/single.ts @@ -7,24 +7,33 @@ async function main() { const pad = (x: unknown) => ` ${x} `.padStart(14); console.log(chalk.bold("=== Single Order Gas Benchmarks ===")); - console.log(chalk.gray("--------------+--------------")); + console.log(chalk.gray("--------------+--------------+--------------")); console.log( - ["gasToken", "gas"] + ["refunds", "gasToken", "gas"] .map((header) => chalk.cyan(pad(header))) .join(chalk.gray("|")), ); - console.log(chalk.gray("--------------+--------------")); - for (const gasToken of [0, 5]) { + console.log(chalk.gray("--------------+--------------+--------------")); + for (const [refunds, gasToken] of [ + [0, 0], + [2, 0], + [4, 0], + [8, 0], + [0, 2], + [2, 2], + [4, 2], + [0, 5], + ]) { const { gasUsed } = await fixture.settle({ tokens: 2, trades: 1, interactions: 1, - refunds: 0, + refunds, gasToken, }); console.log( - [pad(gasToken), chalk.yellow(pad(gasUsed.toString()))].join( + [pad(refunds), pad(gasToken), chalk.yellow(pad(gasUsed.toString()))].join( chalk.gray("|"), ), ); diff --git a/src/contracts/GPv2Settlement.sol b/src/contracts/GPv2Settlement.sol index e3b6fa56..c1cd75c2 100644 --- a/src/contracts/GPv2Settlement.sol +++ b/src/contracts/GPv2Settlement.sol @@ -22,12 +22,6 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { using GPv2TradeExecution for GPv2TradeExecution.Data; using SafeMath for uint256; - /// @dev Data used for freeing storage and claiming a gas refund. - struct OrderRefunds { - bytes[] filledAmounts; - bytes[] preSignatures; - } - /// @dev The authenticator is used to determine who can call the settle function. /// That is, only authorised solvers have the ability to invoke settlements. /// Any valid authenticator implements an isSolver method called by the onlySolver @@ -88,6 +82,13 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { _; } + /// @dev Modifier to ensure that an external function is only callable as a + /// settlement interaction. + modifier onlyInteraction { + require(address(this) == msg.sender, "GPv2: not an interaction"); + _; + } + /// @dev Settle the specified orders at a clearing price. Note that it is /// the responsibility of the caller to ensure that all GPv2 invariants are /// upheld for the input settlement, otherwise this call will revert. @@ -110,14 +111,11 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { /// @param interactions Smart contract interactions split into three /// separate lists to be run before the settlement, during the settlement /// and after the settlement respectively. - /// @param orderRefunds Order refunds for clearing storage related to - /// expired orders. function settle( IERC20[] calldata tokens, uint256[] calldata clearingPrices, GPv2Trade.Data[] calldata trades, - GPv2Interaction.Data[][3] calldata interactions, - OrderRefunds calldata orderRefunds + GPv2Interaction.Data[][3] calldata interactions ) external nonReentrant onlySolver { executeInteractions(interactions[0]); @@ -132,8 +130,6 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { executeInteractions(interactions[2]); - claimOrderRefunds(orderRefunds); - emit Settlement(msg.sender); } @@ -150,6 +146,30 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { emit OrderInvalidated(owner, orderUid); } + /// @dev Free storage from the filled amounts of **expired** orders to claim + /// a gas refund. This method can only be called as an interaction. + /// + /// @param orderUids The unique identifiers of the expired order to free + /// storage for. + function freeFilledAmountStorage(bytes[] calldata orderUids) + external + onlyInteraction + { + freeOrderStorage(filledAmount, orderUids); + } + + /// @dev Free storage from the pre signatures of **expired** orders to claim + /// a gas refund. This method can only be called as an interaction. + /// + /// @param orderUids The unique identifiers of the expired order to free + /// storage for. + function freePreSignatureStorage(bytes[] calldata orderUids) + external + onlyInteraction + { + freeOrderStorage(preSignature, orderUids); + } + /// @dev Process all trades one at a time returning the computed net in and /// out transfers for the trades. /// @@ -346,14 +366,6 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { } } - /// @dev Claims order gas refunds. - /// - /// @param orderRefunds Order refund data for freeing storage. - function claimOrderRefunds(OrderRefunds calldata orderRefunds) internal { - freeOrderStorage(orderRefunds.filledAmounts, filledAmount); - freeOrderStorage(orderRefunds.preSignatures, preSignature); - } - /// @dev Claims refund for the specified storage and order UIDs. /// /// This method reverts if any of the orders are still valid. @@ -361,8 +373,8 @@ contract GPv2Settlement is GPv2Signing, ReentrancyGuard, StorageAccessible { /// @param orderUids Order refund data for freeing storage. /// @param orderStorage Order storage mapped on a UID. function freeOrderStorage( - bytes[] calldata orderUids, - mapping(bytes => uint256) storage orderStorage + mapping(bytes => uint256) storage orderStorage, + bytes[] calldata orderUids ) internal { for (uint256 i = 0; i < orderUids.length; i++) { bytes calldata orderUid = orderUids[i]; diff --git a/src/contracts/test/GPv2SettlementTestInterface.sol b/src/contracts/test/GPv2SettlementTestInterface.sol index acdf7be8..2d54d2fb 100644 --- a/src/contracts/test/GPv2SettlementTestInterface.sol +++ b/src/contracts/test/GPv2SettlementTestInterface.sol @@ -55,9 +55,11 @@ contract GPv2SettlementTestInterface is GPv2Settlement { executeInteractions(interactions); } - function claimOrderRefundsTest(OrderRefunds calldata orderRefunds) - external - { - claimOrderRefunds(orderRefunds); + function freeFilledAmountStorageTest(bytes[] calldata orderUids) external { + this.freeFilledAmountStorage(orderUids); + } + + function freePreSignatureStorageTest(bytes[] calldata orderUids) external { + this.freePreSignatureStorage(orderUids); } } diff --git a/src/ts/settlement.ts b/src/ts/settlement.ts index 7cafdd7b..66ae4ed1 100644 --- a/src/ts/settlement.ts +++ b/src/ts/settlement.ts @@ -110,11 +110,6 @@ export interface TradeExecution { feeDiscount: BigNumberish; } -/** - * Table mapping token addresses to their respective clearing prices. - */ -export type Prices = Record; - /** * Order refund data. */ @@ -125,6 +120,11 @@ export interface OrderRefunds { preSignatures: BytesLike[]; } +/** + * Table mapping token addresses to their respective clearing prices. + */ +export type Prices = Record; + /** * Encoded settlement parameters. */ @@ -137,8 +137,6 @@ export type EncodedSettlement = [ Trade[], /** Encoded interactions. */ [Interaction[], Interaction[], Interaction[]], - /** Encoded order refunds. */ - OrderRefunds, ]; /** @@ -260,25 +258,57 @@ export class SettlementEncoder { } /** - * Gets the encoded interactions for the specified stage as a hex-encoded - * string. + * Gets all encoded interactions for all stages. + * + * Note that order refund interactions are included as post-interactions. */ public get interactions(): [Interaction[], Interaction[], Interaction[]] { return [ this._interactions[InteractionStage.PRE].slice(), this._interactions[InteractionStage.INTRA].slice(), - this._interactions[InteractionStage.POST].slice(), + [ + ...this._interactions[InteractionStage.POST], + ...this.encodedOrderRefunds, + ], ]; } /** - * Gets the currently encoded order UIDs for gas refunds. + * Gets the order refunds encoded as interactions. */ - public get orderRefunds(): OrderRefunds { - return { - filledAmounts: this._orderRefunds.filledAmounts.slice(), - preSignatures: this._orderRefunds.preSignatures.slice(), - }; + public get encodedOrderRefunds(): Interaction[] { + const { filledAmounts, preSignatures } = this._orderRefunds; + if (filledAmounts.length + preSignatures.length === 0) { + return []; + } + + const settlement = this.domain.verifyingContract; + if (settlement === undefined) { + throw new Error("domain missing settlement contract address"); + } + + // NOTE: Avoid importing the full GPv2Settlement contract artifact just for + // a tiny snippet of the ABI. Unit and integration tests will catch any + // issues that may arise from this definition becoming out of date. + const iface = new ethers.utils.Interface([ + "function freeFilledAmountStorage(bytes[] orderUids)", + "function freePreSignatureStorage(bytes[] orderUids)", + ]); + + const interactions = []; + for (const [functionName, orderUids] of [ + ["freeFilledAmountStorage", filledAmounts] as const, + ["freePreSignatureStorage", preSignatures] as const, + ].filter(([, orderUids]) => orderUids.length > 0)) { + interactions.push( + normalizeInteraction({ + target: settlement, + callData: iface.encodeFunctionData(functionName, [orderUids]), + }), + ); + } + + return interactions; } /** @@ -381,9 +411,14 @@ export class SettlementEncoder { /** * Encodes order UIDs for gas refunds. * + * @param settlement The address of the settlement contract. * @param orderRefunds The order refunds to encode. */ public encodeOrderRefunds(orderRefunds: Partial): void { + if (this.domain.verifyingContract === undefined) { + throw new Error("domain missing settlement contract address"); + } + const filledAmounts = orderRefunds.filledAmounts ?? []; const preSignatures = orderRefunds.preSignatures ?? []; @@ -408,7 +443,6 @@ export class SettlementEncoder { this.clearingPrices(prices), this.trades, this.interactions, - this.orderRefunds, ]; } diff --git a/test/GPv2Settlement.test.ts b/test/GPv2Settlement.test.ts index ea9fc9a4..a1b7bd59 100644 --- a/test/GPv2Settlement.test.ts +++ b/test/GPv2Settlement.test.ts @@ -8,6 +8,7 @@ import { InteractionStage, OrderFlags, OrderKind, + PRE_SIGNED, SettlementEncoder, SigningScheme, TradeExecution, @@ -19,11 +20,7 @@ import { } from "../src/ts"; import { builtAndDeployedMetadataCoincide } from "./bytecode"; -import { - encodeFilledAmountRefunds, - encodeOutTransfers, - encodePreSignatureRefunds, -} from "./encoding"; +import { encodeOutTransfers } from "./encoding"; function toNumberLossy(value: BigNumber): number { // NOTE: BigNumber throws an exception when if is outside the range of @@ -232,28 +229,16 @@ describe("GPv2Settlement", () => { it("reverts if encoded interactions has incorrect number of stages", async () => { await authenticator.connect(owner).addSolver(solver.address); - const [tokens, clearingPrices, trades, , encodedOrderRefunds] = empty; + const [tokens, clearingPrices, trades] = empty; await expect( settlement .connect(solver) - .settle([ - tokens, - clearingPrices, - trades, - ["0x", "0x"], - encodedOrderRefunds, - ]), + .settle([tokens, clearingPrices, trades, ["0x", "0x"]]), ).to.be.reverted; await expect( settlement .connect(solver) - .settle([ - tokens, - clearingPrices, - trades, - ["0x", "0x", "0x", "0x"], - encodedOrderRefunds, - ]), + .settle([tokens, clearingPrices, trades, ["0x", "0x", "0x", "0x"]]), ).to.be.reverted; }); }); @@ -1146,98 +1131,100 @@ describe("GPv2Settlement", () => { }); }); - describe("claimOrderRefunds", () => { - it("should set filled amount to 0 for all orders", async () => { - const orderUids = [ - packOrderUidParams({ - orderDigest: `0x${"11".repeat(32)}`, - owner: traders[0].address, - validTo: 0, - }), - packOrderUidParams({ - orderDigest: `0x${"22".repeat(32)}`, - owner: traders[0].address, - validTo: 0, - }), - packOrderUidParams({ - orderDigest: `0x${"33".repeat(32)}`, - owner: traders[0].address, - validTo: 0, - }), - ]; + describe("Order Refunds", () => { + const orderUids = [ + packOrderUidParams({ + orderDigest: `0x${"11".repeat(32)}`, + owner: traders[0].address, + validTo: 0, + }), + packOrderUidParams({ + orderDigest: `0x${"22".repeat(32)}`, + owner: traders[0].address, + validTo: 0, + }), + packOrderUidParams({ + orderDigest: `0x${"33".repeat(32)}`, + owner: traders[0].address, + validTo: 0, + }), + ]; - for (const orderUid of orderUids) { - await settlement.connect(traders[0]).invalidateOrder(orderUid); - expect(await settlement.filledAmount(orderUid)).to.not.deep.equal( - ethers.constants.Zero, - ); - } + const commonTests = (freeStorageFunction: string) => { + const testFunction = `${freeStorageFunction}Test`; - await settlement.claimOrderRefundsTest( - encodeFilledAmountRefunds(...orderUids), - ); - for (const orderUid of orderUids) { - expect(await settlement.filledAmount(orderUid)).to.deep.equal( - ethers.constants.Zero, + it("should revert if not called from an interaction", async () => { + await expect(settlement[freeStorageFunction]([])).to.be.revertedWith( + "not an interaction", ); - } - }); + }); - it("should clear pre-signatures", async () => { - const orderUid = packOrderUidParams({ - orderDigest: `0x${"11".repeat(32)}`, - owner: traders[0].address, - validTo: 0, + it("should revert if the encoded order UIDs are malformed", async () => { + const orderUid = packOrderUidParams({ + orderDigest: ethers.constants.HashZero, + owner: ethers.constants.AddressZero, + validTo: 0, + }); + + for (const malformedOrderUid of [ + ethers.utils.hexDataSlice(orderUid, 0, 55), + ethers.utils.hexZeroPad(orderUid, 57), + ]) { + await expect( + settlement[testFunction]([malformedOrderUid]), + ).to.be.revertedWith("invalid uid"); + } }); - await settlement.connect(traders[0]).setPreSignature(orderUid, true); - await settlement.claimOrderRefundsTest( - encodePreSignatureRefunds(orderUid), - ); + it("should revert if the order is still valid", async () => { + const orderUid = packOrderUidParams({ + orderDigest: `0x${"42".repeat(32)}`, + owner: traders[0].address, + validTo: 0xffffffff, + }); - expect(await settlement.preSignature(orderUid)).to.equal( - ethers.constants.Zero, - ); - }); + await expect(settlement[testFunction]([orderUid])).to.be.revertedWith( + "order still valid", + ); + }); + }; - it("should revert if the encoded order UIDs are malformed", async () => { - const orderUid = packOrderUidParams({ - orderDigest: ethers.constants.HashZero, - owner: ethers.constants.AddressZero, - validTo: 0, + describe("freeFilledAmountStorage", () => { + it("should set filled amount to 0 for all orders", async () => { + for (const orderUid of orderUids) { + await settlement.connect(traders[0]).invalidateOrder(orderUid); + expect(await settlement.filledAmount(orderUid)).to.not.deep.equal( + ethers.constants.Zero, + ); + } + + await settlement.freeFilledAmountStorageTest(orderUids); + for (const orderUid of orderUids) { + expect(await settlement.filledAmount(orderUid)).to.equal( + ethers.constants.Zero, + ); + } }); - for (const malformedOrderUid of [ - ethers.utils.hexDataSlice(orderUid, 0, 55), - ethers.utils.hexZeroPad(orderUid, 57), - ]) { - await expect( - settlement.claimOrderRefundsTest( - encodeFilledAmountRefunds(malformedOrderUid), - ), - ).to.be.reverted; - await expect( - settlement.claimOrderRefundsTest( - encodePreSignatureRefunds(malformedOrderUid), - ), - ).to.be.reverted; - } + commonTests("freeFilledAmountStorage"); }); - it("should revert if the order is still valid", async () => { - const orderDigest = "0x" + "11".repeat(32); - const orderUid = packOrderUidParams({ - orderDigest, - owner: traders[0].address, - validTo: 0xffffffff, + describe("freePreSignatureStorage", () => { + it("should clear pre-signatures", async () => { + for (const orderUid of orderUids) { + await settlement.connect(traders[0]).setPreSignature(orderUid, true); + expect(await settlement.preSignature(orderUid)).to.equal(PRE_SIGNED); + } + + await settlement.freePreSignatureStorageTest(orderUids); + for (const orderUid of orderUids) { + expect(await settlement.preSignature(orderUid)).to.equal( + ethers.constants.Zero, + ); + } }); - await expect( - settlement.claimOrderRefundsTest(encodeFilledAmountRefunds(orderUid)), - ).to.be.revertedWith("order still valid"); - await expect( - settlement.claimOrderRefundsTest(encodePreSignatureRefunds(orderUid)), - ).to.be.revertedWith("order still valid"); + commonTests("freePreSignatureStorage"); }); }); }); diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index 80ef4ee6..46344116 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -4,8 +4,8 @@ import { artifacts, ethers, waffle } from "hardhat"; import { EIP1271_MAGICVALUE, - PRE_SIGNED, OrderKind, + PRE_SIGNED, SettlementEncoder, SigningScheme, TypedDataDomain, diff --git a/test/e2e/orderRefunds.test.ts b/test/e2e/orderRefunds.test.ts index 5395b42e..1dce535d 100644 --- a/test/e2e/orderRefunds.test.ts +++ b/test/e2e/orderRefunds.test.ts @@ -167,6 +167,6 @@ describe("E2E: Expired Order Gas Refunds", () => { ); debug(`Gas savings per refund: ${gasSavingsPerRefund}`); - expect(gasSavingsPerRefund.gt(8000)).to.be.true; + expect(gasSavingsPerRefund.gt(4000)).to.be.true; }); }); diff --git a/test/encoding.ts b/test/encoding.ts index 88cb5a74..57df49aa 100644 --- a/test/encoding.ts +++ b/test/encoding.ts @@ -1,7 +1,7 @@ -import { BigNumber, BytesLike } from "ethers"; +import { BigNumber } from "ethers"; import { ethers } from "hardhat"; -import { Order, OrderKind, OrderRefunds, normalizeOrder } from "../src/ts"; +import { Order, OrderKind, normalizeOrder } from "../src/ts"; export type AbiOrder = [ string, @@ -91,15 +91,3 @@ export function encodeOutTransfers(transfers: OutTransfer[]): ExecutedTrade[] { sellAmount: ethers.constants.Zero, })); } - -export function encodeFilledAmountRefunds( - ...filledAmounts: BytesLike[] -): OrderRefunds { - return { filledAmounts, preSignatures: [] }; -} - -export function encodePreSignatureRefunds( - ...preSignatures: BytesLike[] -): OrderRefunds { - return { filledAmounts: [], preSignatures }; -}