From 12c4e0a4fac65411513d4c89d68990e2f3d9eb97 Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Thu, 25 Jan 2024 22:50:47 -0500 Subject: [PATCH] re-write automation forwarder tests with foundry and remove old tests (#11889) --- .../automation/test/AutomationForwarder.t.sol | 105 ++++++------- .../automation/AutomationForwarder.test.ts | 146 ------------------ 2 files changed, 45 insertions(+), 206 deletions(-) delete mode 100644 contracts/test/v0.8/automation/AutomationForwarder.test.ts diff --git a/contracts/src/v0.8/automation/test/AutomationForwarder.t.sol b/contracts/src/v0.8/automation/test/AutomationForwarder.t.sol index aee2fcfbd29..d9a8c3bcfd3 100644 --- a/contracts/src/v0.8/automation/test/AutomationForwarder.t.sol +++ b/contracts/src/v0.8/automation/test/AutomationForwarder.t.sol @@ -1,89 +1,74 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.16; -import {IAutomationRegistryConsumer} from "../interfaces/IAutomationRegistryConsumer.sol"; import {IAutomationForwarder} from "../interfaces/IAutomationForwarder.sol"; import {AutomationForwarder} from "../AutomationForwarder.sol"; import {AutomationForwarderLogic} from "../AutomationForwarderLogic.sol"; -import {MockKeeperRegistry2_1} from "../mocks/MockKeeperRegistry2_1.sol"; -import {UpkeepCounter} from "../testhelpers/UpkeepCounter.sol"; -import {BaseTest} from "./BaseTest.t.sol"; +import "forge-std/Test.sol"; // in contracts directory, run // forge test --match-path src/v0.8/automation/test/AutomationForwarder.t.sol -contract AutomationForwarderSetUp is BaseTest { - IAutomationForwarder internal forwarder; - AutomationForwarderLogic internal logicContract; - IAutomationRegistryConsumer internal default_registry; - UpkeepCounter internal default_target; - uint256 constant GAS = 1e18; +contract Target { + function handler() external pure {} - function setUp() public override { - // BaseTest.setUp() not called since we want calls to iniatiate from default_registry, not from some predefined owner - default_registry = IAutomationRegistryConsumer(new MockKeeperRegistry2_1()); - default_target = new UpkeepCounter(10000, 1); - vm.startPrank(address(default_registry)); - logicContract = new AutomationForwarderLogic(); - forwarder = IAutomationForwarder( - address(new AutomationForwarder(address(default_target), address(default_registry), address(logicContract))) - ); - // OWNER not necessary? - OWNER = address(default_registry); + function handlerRevert() external pure { + revert("revert"); } +} - function getSelector(string memory _func, bytes memory myData) public pure returns (bytes memory) { - bytes4 functionSignature = bytes4(keccak256(bytes(_func))); - return abi.encodeWithSelector(functionSignature, myData); +contract AutomationForwarderTestSetUp is Test { + address internal constant REGISTRY = 0x3e19ef5Aaa2606655f5A677A97E085cf3811067c; + address internal constant STRANGER = 0x618fae5d04963B2CEf533F247Eb2C46Bf1801D3b; + + IAutomationForwarder internal forwarder; + address internal TARGET; + + function setUp() public { + TARGET = address(new Target()); + AutomationForwarderLogic logicContract = new AutomationForwarderLogic(); + forwarder = IAutomationForwarder(address(new AutomationForwarder(TARGET, REGISTRY, address(logicContract)))); } } -contract AutomationForwarder_forward is AutomationForwarderSetUp { - function testBasicSuccess() public { - uint256 prevCount = default_target.counter(); - bytes memory selector = getSelector("performUpkeep(bytes)", "performDataHere"); - (bool val, uint256 gasUsed) = forwarder.forward(GAS, selector); - assertEq(val, true); - assertGt(gasUsed, 0); - uint256 newCount = default_target.counter(); - assertEq(newCount, prevCount + 1); +contract AutomationForwarderTest_constructor is AutomationForwarderTestSetUp { + function testInitialValues() external { + assertEq(address(forwarder.getRegistry()), REGISTRY); + assertEq(forwarder.getTarget(), TARGET); } - function testWrongFunctionSelectorSuccess() public { - uint256 prevCount = default_target.counter(); - bytes memory selector = getSelector("performUpkeep(bytes calldata data)", ""); - (bool val, uint256 gasUsed) = forwarder.forward(GAS, selector); - assertFalse(val); - assertGt(gasUsed, 0); - uint256 newCount = default_target.counter(); - assertEq(newCount, prevCount); + function testTypeAndVersion() external { + assertEq(forwarder.typeAndVersion(), "AutomationForwarder 1.0.0"); } +} - function testNotAuthorizedReverts() public { - uint256 prevCount = default_target.counter(); - bytes memory selector = getSelector("performUpkeep(bytes)", ""); - changePrank(STRANGER); +contract AutomationForwarderTest_forward is AutomationForwarderTestSetUp { + function testOnlyCallableByTheRegistry() external { + vm.prank(REGISTRY); + forwarder.forward(100000, abi.encodeWithSelector(Target.handler.selector)); + vm.prank(STRANGER); vm.expectRevert(); - (bool val, uint256 gasUsed) = forwarder.forward(GAS, selector); - assertFalse(val); - assertEq(gasUsed, 0); - uint256 newCount = default_target.counter(); - assertEq(newCount, prevCount); + forwarder.forward(100000, abi.encodeWithSelector(Target.handler.selector)); } -} -contract AutomationForwarder_updateRegistry is AutomationForwarderSetUp { - function testBasicSuccess() public { - address newRegistry = address(1); - forwarder.updateRegistry(address(newRegistry)); - IAutomationRegistryConsumer newReg = forwarder.getRegistry(); - assertEq(address(newReg), newRegistry); + function testReturnsSuccessValueAndGasUsed() external { + vm.startPrank(REGISTRY); + (bool success, uint256 gasUsed) = forwarder.forward(100000, abi.encodeWithSelector(Target.handler.selector)); + assertTrue(success); + assertGt(gasUsed, 0); + (success, gasUsed) = forwarder.forward(100000, abi.encodeWithSelector(Target.handlerRevert.selector)); + assertFalse(success); + assertGt(gasUsed, 0); } +} - function testNotFromRegistryNotAuthorizedReverts() public { +contract AutomationForwarderTest_updateRegistry is AutomationForwarderTestSetUp { + function testOnlyCallableByTheActiveRegistry() external { address newRegistry = address(1); - changePrank(STRANGER); + vm.startPrank(REGISTRY); + forwarder.updateRegistry(newRegistry); + assertEq(address(forwarder.getRegistry()), newRegistry); vm.expectRevert(); - forwarder.updateRegistry(address(newRegistry)); + forwarder.updateRegistry(REGISTRY); } } diff --git a/contracts/test/v0.8/automation/AutomationForwarder.test.ts b/contracts/test/v0.8/automation/AutomationForwarder.test.ts deleted file mode 100644 index 634bfb61ea9..00000000000 --- a/contracts/test/v0.8/automation/AutomationForwarder.test.ts +++ /dev/null @@ -1,146 +0,0 @@ -import { expect } from 'chai' -import { ethers } from 'hardhat' -import { getUsers, Roles } from '../../test-helpers/setup' -import { IAutomationForwarder } from '../../../typechain/IAutomationForwarder' -import { IAutomationForwarder__factory as IAutomationForwarderFactory } from '../../../typechain/factories/IAutomationForwarder__factory' -import { loadFixture } from '@nomicfoundation/hardhat-network-helpers' -import { - deployMockContract, - MockContract, -} from '@ethereum-waffle/mock-contract' - -/** - * @dev note there are two types of factories in this test: the AutomationForwarderFactory contract, which - * deploys new instances of the AutomationForwarder, and the ethers-js javascript factory, which deploys new - * contracts of any type from JS. Therefore, the forwarderFactoryFactory (below) is a javascript object that deploys - * the contract factory, which deploys forwarder instances :) - */ - -const CUSTOM_REVERT = 'this is a custom revert message' - -let roles: Roles -let defaultAddress: string -let forwarder: IAutomationForwarder -let target: MockContract - -const targetABI = [ - 'function handler()', // 0xc80916d4 - 'function handlerUint(uint256) returns(uint256)', // 0x28da6d29 - 'function iRevert()', // 0xc07d0f94 -] - -const HANDLER = '0xc80916d4' -const HANDLER_UINT = - '0x28da6d290000000000000000000000000000000000000000000000000000000000000001' -const HANDLER_BYTES = '0x100e0465' -const HANDLER_REVERT = '0xc07d0f94' - -const newRegistry = ethers.Wallet.createRandom().address - -const setup = async () => { - roles = (await getUsers()).roles - defaultAddress = await roles.defaultAccount.getAddress() - target = await deployMockContract(roles.defaultAccount, targetABI) - await target.deployed() - await target.mock.handler.returns() - await target.mock.handlerUint.returns(100) - await target.mock.iRevert.revertsWithReason(CUSTOM_REVERT) - const logicFactory = await ethers.getContractFactory( - 'AutomationForwarderLogic', - ) - const logicContract = await logicFactory - .connect(roles.defaultAccount) - .deploy() - const factory = await ethers.getContractFactory('AutomationForwarder') - const forwarderContract = await factory - .connect(roles.defaultAccount) - .deploy( - target.address, - await roles.defaultAccount.getAddress(), - logicContract.address, - ) - forwarder = IAutomationForwarderFactory.connect( - forwarderContract.address, - roles.defaultAccount, - ) -} - -describe('AutomationForwarder', () => { - beforeEach(async () => { - await loadFixture(setup) - }) - - describe('constructor()', () => { - it('sets the initial values', async () => { - expect(await forwarder.getRegistry()).to.equal(defaultAddress) - expect(await forwarder.getTarget()).to.equal(target.address) - }) - }) - - describe('typeAndVersion()', () => { - it('has the correct type and version', async () => { - expect(await forwarder.typeAndVersion()).to.equal( - 'AutomationForwarder 1.0.0', - ) - }) - }) - - describe('forward()', () => { - const gas = 100_000 - it('is only callable by the registry', async () => { - await expect( - forwarder.connect(roles.stranger).forward(gas, HANDLER), - ).to.be.revertedWith('') - await forwarder.connect(roles.defaultAccount).forward(gas, HANDLER) - }) - - it('forwards the call to the target', async () => { - await forwarder.connect(roles.defaultAccount).forward(gas, HANDLER) - await forwarder.connect(roles.defaultAccount).forward(gas, HANDLER_UINT) - await forwarder.connect(roles.defaultAccount).forward(gas, HANDLER_BYTES) - }) - - it('returns the success value & gas used by the target call', async () => { - const result = await forwarder - .connect(roles.defaultAccount) - .callStatic.forward(gas, HANDLER) - expect(result.success).to.be.true - expect(result.gasUsed.toNumber()).to.be.greaterThan(0) - - const result2 = await forwarder - .connect(roles.defaultAccount) - .callStatic.forward(gas, HANDLER_UINT) - expect(result2.success).to.be.true - expect(result2.gasUsed.toNumber()).to.be.greaterThan(0) - - const result3 = await forwarder - .connect(roles.defaultAccount) - .callStatic.forward(gas, HANDLER_REVERT) - expect(result3.success).to.be.false - expect(result3.gasUsed.toNumber()).to.be.greaterThan(0) - }) - - it('reverts if too little gas is supplied', async () => { - await expect( - forwarder - .connect(roles.defaultAccount) - .forward(100_000, HANDLER, { gasLimit: 99_999 }), - ).to.be.reverted - }) - }) - - describe('updateRegistry()', () => { - it('is only callable by the existing registry', async () => { - await expect( - forwarder.connect(roles.stranger).updateRegistry(newRegistry), - ).to.be.revertedWith('') - await forwarder.connect(roles.defaultAccount).updateRegistry(newRegistry) - }) - - it('is updates the registry', async () => { - expect(await forwarder.getRegistry()).to.equal(defaultAddress) - await forwarder.connect(roles.defaultAccount).updateRegistry(newRegistry) - expect(await forwarder.getRegistry()).to.equal(newRegistry) - }) - }) -})