From 4aa76469b8aa3c99f61f0f0c75308492660090d6 Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Mon, 16 Sep 2024 16:10:00 +0200 Subject: [PATCH 1/3] test: use hookShouldRevert pattern in `Escrow.t.sol` --- test/Escrow.t.sol | 36 ++++++++++++++---------- test/base/DummyDAppControl.sol | 50 ++++++++++++++++++++++------------ 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/test/Escrow.t.sol b/test/Escrow.t.sol index 705144068..d6035cced 100644 --- a/test/Escrow.t.sol +++ b/test/Escrow.t.sol @@ -143,7 +143,7 @@ contract EscrowTest is BaseTest { .withAllowAllocateValueFailure(true) // Allow the value allocation to fail .build() ); - executeHookCase(false, block.timestamp * 2, noError); + executeHookCase(block.timestamp * 2, noError); } // Ensure metacall reverts with the proper error when the preOps hook reverts. @@ -154,7 +154,8 @@ contract EscrowTest is BaseTest { .withReuseUserOp(true) // Allow metacall to revert .build() ); - executeHookCase(true, 0, AtlasErrors.PreOpsFail.selector); + dAppControl.setPreOpsShouldRevert(true); + executeHookCase(0, AtlasErrors.PreOpsFail.selector); } // Ensure the user operation executes successfully. To ensure the operation's returned data is as expected, we @@ -168,7 +169,7 @@ contract EscrowTest is BaseTest { .withAllowAllocateValueFailure(true) // Allow the value allocation to fail .build() ); - executeHookCase(false, block.timestamp * 3, noError); + executeHookCase(block.timestamp * 3, noError); } // Ensure metacall reverts with the proper error when the user operation reverts. @@ -178,7 +179,8 @@ contract EscrowTest is BaseTest { .withReuseUserOp(true) // Allow metacall to revert .build() ); - executeHookCase(true, 0, AtlasErrors.UserOpFail.selector); + dAppControl.setUserOpShouldRevert(true); + executeHookCase(0, AtlasErrors.UserOpFail.selector); } // Ensure metacall reverts with the proper error when the allocateValue hook reverts. @@ -194,7 +196,7 @@ contract EscrowTest is BaseTest { dAppControl.setAllocateValueShouldRevert(true); - executeHookCase(false, 1, AtlasErrors.AllocateValueFail.selector); + executeHookCase(1, AtlasErrors.AllocateValueFail.selector); } // Ensure the postOps hook is successfully called. No return data is expected from the postOps hook, so we do not @@ -205,7 +207,7 @@ contract EscrowTest is BaseTest { .withRequirePostOps(true) // Execute the postOps hook .build() ); - executeHookCase(false, 0, noError); + executeHookCase(0, noError); } // Ensure metacall reverts with the proper error when the postOps hook reverts. @@ -219,7 +221,8 @@ contract EscrowTest is BaseTest { .withAllowAllocateValueFailure(true) // Allow the value allocation to fail .build() ); - executeHookCase(false, 1, AtlasErrors.PostOpsFail.selector); + dAppControl.setPostOpsShouldRevert(true); + executeHookCase(1, AtlasErrors.PostOpsFail.selector); } // Ensure the allocateValue hook is successfully called. No return data is expected from the allocateValue hook, so @@ -237,17 +240,16 @@ contract EscrowTest is BaseTest { vm.expectEmit(false, false, false, true, executionEnvironment); emit MEVPaymentSuccess(address(0), defaultBidAmount); - this.executeHookCase(false, 0, noError); + this.executeHookCase(0, noError); } - function executeHookCase(bool hookShouldRevert, uint256 expectedHookReturnValue, bytes4 expectedError) public { + function executeHookCase(uint256 expectedHookReturnValue, bytes4 expectedError) public { bool revertExpected = expectedError != noError; UserOperation memory userOp = validUserOperation(address(dAppControl)) .withData( abi.encodeWithSelector( dAppControl.userOperationCall.selector, - hookShouldRevert, expectedHookReturnValue ) ) @@ -385,7 +387,7 @@ contract EscrowTest is BaseTest { ); UserOperation memory userOp = validUserOperation(address(dAppControl)) - .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, false, 1)) + .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, 1)) .signAndBuild(address(atlasVerification), userPK); SolverOperation[] memory solverOps = new SolverOperation[](1); @@ -394,6 +396,8 @@ contract EscrowTest is BaseTest { .signAndBuild(address(atlasVerification), solverOnePK); uint256 result = (1 << uint256(SolverOutcome.PreSolverFailed)); + dAppControl.setPreSolverShouldRevert(true); + executeSolverOperationCase(userOp, solverOps, false, false, result, true); } @@ -409,7 +413,7 @@ contract EscrowTest is BaseTest { ); UserOperation memory userOp = validUserOperation(address(dAppControl)) - .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, false, 1)) + .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, 1)) .signAndBuild(address(atlasVerification), userPK); SolverOperation[] memory solverOps = new SolverOperation[](1); @@ -418,6 +422,8 @@ contract EscrowTest is BaseTest { .signAndBuild(address(atlasVerification), solverOnePK); uint256 result = (1 << uint256(SolverOutcome.PostSolverFailed)); + dAppControl.setPostSolverShouldRevert(true); + executeSolverOperationCase(userOp, solverOps, true, false, result, true); } @@ -508,7 +514,7 @@ contract EscrowTest is BaseTest { ); userOp = validUserOperation(address(dAppControl)) - .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, false, expectedDataValue)) + .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, expectedDataValue)) .signAndBuild(address(atlasVerification), userPK); solverOps[0] = validSolverOperation(userOp) @@ -539,7 +545,7 @@ contract EscrowTest is BaseTest { ); userOp = validUserOperation(address(dAppControl)) - .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, false, dataValue)) + .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, dataValue)) .signAndBuild(address(atlasVerification), userPK); solverOps[0] = validSolverOperation(userOp) @@ -565,7 +571,7 @@ contract EscrowTest is BaseTest { defaultAtlasWithCallConfig(callConfig); userOp = validUserOperation(address(dAppControl)) - .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, false, 0)) + .withData(abi.encodeWithSelector(dAppControl.userOperationCall.selector, 0)) .signAndBuild(address(atlasVerification), userPK); solverOps = new SolverOperation[](1); diff --git a/test/base/DummyDAppControl.sol b/test/base/DummyDAppControl.sol index e7ce04018..34341e813 100644 --- a/test/base/DummyDAppControl.sol +++ b/test/base/DummyDAppControl.sol @@ -21,6 +21,8 @@ contract DummyDAppControl is DAppControl { bool public allocateValueShouldRevert; bool public postOpsShouldRevert; + // TODO add storage vars to store input data for each hook, then test hooks were passed the correct data + event MEVPaymentSuccess(address bidToken, uint256 bidAmount); constructor( @@ -38,38 +40,49 @@ contract DummyDAppControl is DAppControl { function _checkUserOperation(UserOperation memory) internal pure virtual override { } function _preOpsCall(UserOperation calldata userOp) internal virtual override returns (bytes memory) { - if (userOp.data.length == 0) { - return new bytes(0); - } + bool shouldRevert = DummyDAppControl(CONTROL).preOpsShouldRevert(); + require(!shouldRevert, "_preOpsCall revert requested"); - (bool success, bytes memory data) = address(userOp.dapp).call(userOp.data); - require(success, "_preOpsCall reverted"); + if (userOp.data.length == 0) return new bytes(0); + + (, bytes memory data) = address(userOp.dapp).call(userOp.data); return data; } - function _postOpsCall(bool, bytes calldata data) internal pure virtual override { + function _postOpsCall(bool, bytes calldata data) internal view virtual override { + bool shouldRevert = DummyDAppControl(CONTROL).postOpsShouldRevert(); + require(!shouldRevert, "_postOpsCall revert requested"); if (data.length == 0) return; - (bool shouldRevert) = abi.decode(data, (bool)); - require(!shouldRevert, "_postOpsCall revert requested"); + // TODO store input data and check it was passed correctly in Escrow.t.sol + // (bool shouldRevert) = abi.decode(data, (bool)); + // require(!shouldRevert, "_postOpsCall revert requested"); } function _preSolverCall(SolverOperation calldata, bytes calldata returnData) internal view virtual override { + bool shouldRevert = DummyDAppControl(CONTROL).preSolverShouldRevert(); + require(!shouldRevert, "_preSolverCall revert requested"); + if (returnData.length == 0) { return; } - (bool shouldRevert) = abi.decode(returnData, (bool)); - require(!shouldRevert, "_preSolverCall revert requested"); + // TODO store input data and check it was passed correctly in Escrow.t.sol + // (bool shouldRevert) = abi.decode(returnData, (bool)); + // require(!shouldRevert, "_preSolverCall revert requested"); } - function _postSolverCall(SolverOperation calldata, bytes calldata returnData) internal pure virtual override { + function _postSolverCall(SolverOperation calldata, bytes calldata returnData) internal view virtual override { + bool shouldRevert = DummyDAppControl(CONTROL).postSolverShouldRevert(); + require(!shouldRevert, "_postSolverCall revert requested"); + if (returnData.length == 0) { return; } - (bool shouldRevert) = abi.decode(returnData, (bool)); - require(!shouldRevert, "_postSolverCall revert requested"); + // TODO store input data and check it was passed correctly in Escrow.t.sol + // (bool shouldRevert) = abi.decode(returnData, (bool)); + // require(!shouldRevert, "_postSolverCall revert requested"); } function _allocateValueCall( @@ -81,12 +94,10 @@ contract DummyDAppControl is DAppControl { virtual override { - if (data.length == 0) { - return; - } - bool shouldRevert = DummyDAppControl(CONTROL).allocateValueShouldRevert(); require(!shouldRevert, "_allocateValueCall revert requested"); + + // TODO store input data and check it was passed correctly in Escrow.t.sol emit MEVPaymentSuccess(bidToken, winningAmount); } @@ -100,8 +111,11 @@ contract DummyDAppControl is DAppControl { // Custom functions // **************************************** - function userOperationCall(bool shouldRevert, uint256 returnValue) public pure returns (uint256) { + function userOperationCall(uint256 returnValue) public view returns (uint256) { + bool shouldRevert = DummyDAppControl(CONTROL).userOpShouldRevert(); require(!shouldRevert, "userOperationCall revert requested"); + + // TODO store input data and check it was passed correctly in Escrow.t.sol return returnValue; } From ffba055fc7f2db66a33ffb84ffa73cd7b4d3a06a Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:31:29 +0200 Subject: [PATCH 2/3] test: improve DummyDAppControl, Escrow tests --- test/Escrow.t.sol | 31 +++++++++++------ test/base/DummyDAppControl.sol | 61 ++++++++++++++++++++-------------- 2 files changed, 56 insertions(+), 36 deletions(-) diff --git a/test/Escrow.t.sol b/test/Escrow.t.sol index d6035cced..cacbf8040 100644 --- a/test/Escrow.t.sol +++ b/test/Escrow.t.sol @@ -143,7 +143,10 @@ contract EscrowTest is BaseTest { .withAllowAllocateValueFailure(true) // Allow the value allocation to fail .build() ); - executeHookCase(block.timestamp * 2, noError); + + (UserOperation memory userOp,,) = executeHookCase(block.timestamp * 2, noError); + bytes memory expectedInput = abi.encode(userOp); + assertEq(expectedInput, dAppControl.preOpsInputData(), "preOpsInputData should match expectedInput"); } // Ensure metacall reverts with the proper error when the preOps hook reverts. @@ -170,6 +173,8 @@ contract EscrowTest is BaseTest { .build() ); executeHookCase(block.timestamp * 3, noError); + bytes memory expectedInput = abi.encode(block.timestamp * 3); + assertEq(expectedInput, dAppControl.userOpInputData(), "userOpInputData should match expectedInput"); } // Ensure metacall reverts with the proper error when the user operation reverts. @@ -195,7 +200,6 @@ contract EscrowTest is BaseTest { ); dAppControl.setAllocateValueShouldRevert(true); - executeHookCase(1, AtlasErrors.AllocateValueFail.selector); } @@ -208,6 +212,8 @@ contract EscrowTest is BaseTest { .build() ); executeHookCase(0, noError); + bytes memory expectedInput = abi.encode(true, new bytes(0)); + assertEq(expectedInput, dAppControl.postOpsInputData(), "postOpsInputData should match expectedInput"); } // Ensure metacall reverts with the proper error when the postOps hook reverts. @@ -234,19 +240,22 @@ contract EscrowTest is BaseTest { .withTrackUserReturnData(true) // Track the user operation's return data .build() ); + uint256 userOpArg = 321; - vm.prank(userEOA); - address executionEnvironment = atlas.createExecutionEnvironment(userEOA, address(dAppControl)); + executeHookCase(userOpArg, noError); - vm.expectEmit(false, false, false, true, executionEnvironment); - emit MEVPaymentSuccess(address(0), defaultBidAmount); - this.executeHookCase(0, noError); + bytes memory expectedInput = abi.encode(address(0), defaultBidAmount, abi.encode(userOpArg)); + assertEq(expectedInput, dAppControl.allocateValueInputData(), "allocateValueInputData should match expectedInput"); } - function executeHookCase(uint256 expectedHookReturnValue, bytes4 expectedError) public { + function executeHookCase(uint256 expectedHookReturnValue, bytes4 expectedError) public returns( + UserOperation memory userOp, + SolverOperation[] memory solverOps, + DAppOperation memory dappOp + ) { bool revertExpected = expectedError != noError; - UserOperation memory userOp = validUserOperation(address(dAppControl)) + userOp = validUserOperation(address(dAppControl)) .withData( abi.encodeWithSelector( dAppControl.userOperationCall.selector, @@ -255,13 +264,13 @@ contract EscrowTest is BaseTest { ) .signAndBuild(address(atlasVerification), userPK); - SolverOperation[] memory solverOps = new SolverOperation[](1); + solverOps = new SolverOperation[](1); solverOps[0] = validSolverOperation(userOp) .withBidAmount(defaultBidAmount) .withData(abi.encode(expectedHookReturnValue)) .signAndBuild(address(atlasVerification), solverOnePK); - DAppOperation memory dappOp = validDAppOperation(userOp, solverOps).build(); + dappOp = validDAppOperation(userOp, solverOps).build(); if (revertExpected) { vm.expectRevert(expectedError); diff --git a/test/base/DummyDAppControl.sol b/test/base/DummyDAppControl.sol index 34341e813..42975c5cd 100644 --- a/test/base/DummyDAppControl.sol +++ b/test/base/DummyDAppControl.sol @@ -22,6 +22,12 @@ contract DummyDAppControl is DAppControl { bool public postOpsShouldRevert; // TODO add storage vars to store input data for each hook, then test hooks were passed the correct data + bytes public preOpsInputData; + bytes public userOpInputData; + bytes public preSolverInputData; + bytes public postSolverInputData; + bytes public allocateValueInputData; + bytes public postOpsInputData; event MEVPaymentSuccess(address bidToken, uint256 bidAmount); @@ -43,46 +49,34 @@ contract DummyDAppControl is DAppControl { bool shouldRevert = DummyDAppControl(CONTROL).preOpsShouldRevert(); require(!shouldRevert, "_preOpsCall revert requested"); + DummyDAppControl(CONTROL).setInputData(abi.encode(userOp), 0); + console.logBytes(abi.encode(userOp)); + if (userOp.data.length == 0) return new bytes(0); (, bytes memory data) = address(userOp.dapp).call(userOp.data); return data; } - function _postOpsCall(bool, bytes calldata data) internal view virtual override { + function _postOpsCall(bool solved, bytes calldata data) internal virtual override { bool shouldRevert = DummyDAppControl(CONTROL).postOpsShouldRevert(); require(!shouldRevert, "_postOpsCall revert requested"); - if (data.length == 0) return; - // TODO store input data and check it was passed correctly in Escrow.t.sol - // (bool shouldRevert) = abi.decode(data, (bool)); - // require(!shouldRevert, "_postOpsCall revert requested"); + DummyDAppControl(CONTROL).setInputData(abi.encode(solved, data), 5); } - function _preSolverCall(SolverOperation calldata, bytes calldata returnData) internal view virtual override { + function _preSolverCall(SolverOperation calldata solverOp, bytes calldata returnData) internal virtual override { bool shouldRevert = DummyDAppControl(CONTROL).preSolverShouldRevert(); require(!shouldRevert, "_preSolverCall revert requested"); - if (returnData.length == 0) { - return; - } - - // TODO store input data and check it was passed correctly in Escrow.t.sol - // (bool shouldRevert) = abi.decode(returnData, (bool)); - // require(!shouldRevert, "_preSolverCall revert requested"); + DummyDAppControl(CONTROL).setInputData(abi.encode(solverOp, returnData), 2); } - function _postSolverCall(SolverOperation calldata, bytes calldata returnData) internal view virtual override { + function _postSolverCall(SolverOperation calldata solverOp, bytes calldata returnData) internal virtual override { bool shouldRevert = DummyDAppControl(CONTROL).postSolverShouldRevert(); require(!shouldRevert, "_postSolverCall revert requested"); - if (returnData.length == 0) { - return; - } - - // TODO store input data and check it was passed correctly in Escrow.t.sol - // (bool shouldRevert) = abi.decode(returnData, (bool)); - // require(!shouldRevert, "_postSolverCall revert requested"); + DummyDAppControl(CONTROL).setInputData(abi.encode(solverOp, returnData), 3); } function _allocateValueCall( @@ -97,8 +91,9 @@ contract DummyDAppControl is DAppControl { bool shouldRevert = DummyDAppControl(CONTROL).allocateValueShouldRevert(); require(!shouldRevert, "_allocateValueCall revert requested"); - // TODO store input data and check it was passed correctly in Escrow.t.sol - emit MEVPaymentSuccess(bidToken, winningAmount); + DummyDAppControl(CONTROL).setInputData(abi.encode(bidToken, winningAmount, data), 4); + + // emit MEVPaymentSuccess(bidToken, winningAmount); } function getBidValue(SolverOperation calldata solverOp) public view virtual override returns (uint256) { @@ -111,11 +106,12 @@ contract DummyDAppControl is DAppControl { // Custom functions // **************************************** - function userOperationCall(uint256 returnValue) public view returns (uint256) { + function userOperationCall(uint256 returnValue) public returns (uint256) { bool shouldRevert = DummyDAppControl(CONTROL).userOpShouldRevert(); require(!shouldRevert, "userOperationCall revert requested"); - // TODO store input data and check it was passed correctly in Escrow.t.sol + DummyDAppControl(CONTROL).setInputData(abi.encode(returnValue), 1); + return returnValue; } @@ -144,4 +140,19 @@ contract DummyDAppControl is DAppControl { function setPostOpsShouldRevert(bool _postOpsShouldRevert) public { postOpsShouldRevert = _postOpsShouldRevert; } + + // Called by the EE to save input data for testing after the metacall ends + function setInputData( + bytes memory inputData, + uint256 hook // 0: preOps, 1: userOp, 2: preSolver, 3: postSolver, 4: allocateValue, 5: postOps + ) + public + { + if (hook == 0) preOpsInputData = inputData; + if (hook == 1) userOpInputData = inputData; + if (hook == 2) preSolverInputData = inputData; + if (hook == 3) postSolverInputData = inputData; + if (hook == 4) allocateValueInputData = inputData; + if (hook == 5) postOpsInputData = inputData; + } } From 1c476d9ec4f8e98b2199a85ddd47e78a98f4720c Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Wed, 18 Sep 2024 16:00:36 +0200 Subject: [PATCH 3/3] chore: remove TODO --- test/base/DummyDAppControl.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/base/DummyDAppControl.sol b/test/base/DummyDAppControl.sol index 42975c5cd..b33af3cc9 100644 --- a/test/base/DummyDAppControl.sol +++ b/test/base/DummyDAppControl.sol @@ -21,7 +21,6 @@ contract DummyDAppControl is DAppControl { bool public allocateValueShouldRevert; bool public postOpsShouldRevert; - // TODO add storage vars to store input data for each hook, then test hooks were passed the correct data bytes public preOpsInputData; bytes public userOpInputData; bytes public preSolverInputData;