From 0a0f30dccd1934669604ca9a3ed69985d692c70f Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:09:45 -0700 Subject: [PATCH 01/15] move oepratorforwarder out of dev/ and add more test cases --- .../operatorforwarder.gas-snapshot | 18 +- .../{dev => }/AuthorizedForwarder.sol | 2 +- .../{dev => }/AuthorizedReceiver.sol | 0 .../{dev => }/LinkTokenReceiver.sol | 0 .../operatorforwarder/{dev => }/Operator.sol | 12 +- .../{dev => }/OperatorFactory.sol | 0 .../AuthorizedReceiverInterface.sol | 0 .../interfaces/WithdrawalInterface.sol | 0 .../contributedTests/helpers/Deployer.sol | 46 ++++ .../contributedTests/helpers/MockReceiver.sol | 14 ++ .../test/contributedTests/test/Factory.t.sol | 85 ++++++++ .../contributedTests/test/Forwarder.t.sol | 200 ++++++++++++++++++ .../contributedTests/test/FuzzOperator.t.sol | 104 +++++++++ .../test/contributedTests/test/Operator.t.sol | 163 ++++++++++++++ .../{dev => }/test/operator.t.sol | 2 +- .../test/testhelpers/BasicConsumer.sol | 0 .../testhelpers/ChainlinkClientHelper.sol | 2 +- .../test/testhelpers/Chainlinked.sol | 2 +- .../{dev => }/test/testhelpers/Consumer.sol | 4 +- .../test/testhelpers/EmptyOracle.sol | 4 +- .../test/testhelpers/GasGuzzlingConsumer.sol | 2 +- .../test/testhelpers/GetterSetter.sol | 0 .../test/testhelpers/MaliciousChainlink.sol | 4 +- .../test/testhelpers/MaliciousChainlinked.sol | 2 +- .../test/testhelpers/MaliciousConsumer.sol | 0 .../MaliciousMultiWordConsumer.sol | 4 +- .../test/testhelpers/MaliciousRequester.sol | 2 +- .../test/testhelpers/MultiWordConsumer.sol | 4 +- 28 files changed, 651 insertions(+), 25 deletions(-) rename contracts/src/v0.8/operatorforwarder/{dev => }/AuthorizedForwarder.sol (97%) rename contracts/src/v0.8/operatorforwarder/{dev => }/AuthorizedReceiver.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/LinkTokenReceiver.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/Operator.sol (97%) rename contracts/src/v0.8/operatorforwarder/{dev => }/OperatorFactory.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/interfaces/AuthorizedReceiverInterface.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/interfaces/WithdrawalInterface.sol (100%) create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol rename contracts/src/v0.8/operatorforwarder/{dev => }/test/operator.t.sol (98%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/BasicConsumer.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/ChainlinkClientHelper.sol (90%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/Chainlinked.sol (98%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/Consumer.sol (94%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/EmptyOracle.sol (83%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/GasGuzzlingConsumer.sol (96%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/GetterSetter.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MaliciousChainlink.sol (91%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MaliciousChainlinked.sol (97%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MaliciousConsumer.sol (100%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MaliciousMultiWordConsumer.sol (94%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MaliciousRequester.sol (95%) rename contracts/src/v0.8/operatorforwarder/{dev => }/test/testhelpers/MultiWordConsumer.sol (97%) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index 964c1a91b8d..b0698d35290 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -1,2 +1,16 @@ -Operator_cancelRequest:test_Success(uint96) (runs: 256, μ: 306103, ~: 306096) -Operator_cancelRequest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 384781, ~: 389554) \ No newline at end of file +FactoryTest:testDeployNewForwarder() (gas: 1050792) +FactoryTest:testDeployNewForwarderAndTransferOwnership() (gas: 1064662) +FactoryTest:testDeployNewOperator() (gas: 3023070) +FactoryTest:testDeployNewOperatorAndForwarder() (gas: 4071877) +ForwarderTest:testForward(uint256) (runs: 256, μ: 235226, ~: 236315) +ForwarderTest:testMultiForward(uint256,uint256) (runs: 256, μ: 266861, ~: 268105) +ForwarderTest:testOwnerForward() (gas: 32246) +ForwarderTest:testSetAuthorizedSenders() (gas: 168042) +ForwarderTest:testTransferOwnershipWithMessage() (gas: 38881) +FuzzTokenReceiver:testFuzzOnTokenTransferFromLink(address,uint256,address,uint256,bytes32,address,bytes4,uint256,uint256,bytes) (runs: 256, μ: 272702, ~: 277658) +OperatorTest:testCancelOracleRequest() (gas: 110758) +OperatorTest:testFulfillOracleRequest() (gas: 198351) +OperatorTest:testOracleRequestFlow() (gas: 89666) +OperatorTest:testUnauthorizedFulfillment() (gas: 88254) +Operator_cancelRequest:test_Success(uint96) (runs: 256, μ: 306105, ~: 306096) +Operator_cancelRequest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 385443, ~: 389554) \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol b/contracts/src/v0.8/operatorforwarder/AuthorizedForwarder.sol similarity index 97% rename from contracts/src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol rename to contracts/src/v0.8/operatorforwarder/AuthorizedForwarder.sol index 824ffce6f0f..ffb09248701 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol +++ b/contracts/src/v0.8/operatorforwarder/AuthorizedForwarder.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.19; -import {ConfirmedOwnerWithProposal} from "../../shared/access/ConfirmedOwnerWithProposal.sol"; +import {ConfirmedOwnerWithProposal} from "../shared/access/ConfirmedOwnerWithProposal.sol"; import {AuthorizedReceiver} from "./AuthorizedReceiver.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; diff --git a/contracts/src/v0.8/operatorforwarder/dev/AuthorizedReceiver.sol b/contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/AuthorizedReceiver.sol rename to contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/LinkTokenReceiver.sol b/contracts/src/v0.8/operatorforwarder/LinkTokenReceiver.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/LinkTokenReceiver.sol rename to contracts/src/v0.8/operatorforwarder/LinkTokenReceiver.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/Operator.sol b/contracts/src/v0.8/operatorforwarder/Operator.sol similarity index 97% rename from contracts/src/v0.8/operatorforwarder/dev/Operator.sol rename to contracts/src/v0.8/operatorforwarder/Operator.sol index e68df5fd075..d18c62a6f9f 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/Operator.sol +++ b/contracts/src/v0.8/operatorforwarder/Operator.sol @@ -3,14 +3,14 @@ pragma solidity 0.8.19; import {AuthorizedReceiver} from "./AuthorizedReceiver.sol"; import {LinkTokenReceiver} from "./LinkTokenReceiver.sol"; -import {ConfirmedOwner} from "../../shared/access/ConfirmedOwner.sol"; -import {LinkTokenInterface} from "../../shared/interfaces/LinkTokenInterface.sol"; +import {ConfirmedOwner} from "../shared/access/ConfirmedOwner.sol"; +import {LinkTokenInterface} from "../shared/interfaces/LinkTokenInterface.sol"; import {AuthorizedReceiverInterface} from "./interfaces/AuthorizedReceiverInterface.sol"; -import {OperatorInterface} from "../../interfaces/OperatorInterface.sol"; -import {IOwnable} from "../../shared/interfaces/IOwnable.sol"; +import {OperatorInterface} from "../interfaces/OperatorInterface.sol"; +import {IOwnable} from "../shared/interfaces/IOwnable.sol"; import {WithdrawalInterface} from "./interfaces/WithdrawalInterface.sol"; -import {OracleInterface} from "../../interfaces/OracleInterface.sol"; -import {SafeCast} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/math/SafeCast.sol"; +import {OracleInterface} from "../interfaces/OracleInterface.sol"; +import {SafeCast} from "../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/math/SafeCast.sol"; // @title The Chainlink Operator contract // @notice Node operators can deploy this contract to fulfill requests sent to them diff --git a/contracts/src/v0.8/operatorforwarder/dev/OperatorFactory.sol b/contracts/src/v0.8/operatorforwarder/OperatorFactory.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/OperatorFactory.sol rename to contracts/src/v0.8/operatorforwarder/OperatorFactory.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/interfaces/AuthorizedReceiverInterface.sol b/contracts/src/v0.8/operatorforwarder/interfaces/AuthorizedReceiverInterface.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/interfaces/AuthorizedReceiverInterface.sol rename to contracts/src/v0.8/operatorforwarder/interfaces/AuthorizedReceiverInterface.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/interfaces/WithdrawalInterface.sol b/contracts/src/v0.8/operatorforwarder/interfaces/WithdrawalInterface.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/interfaces/WithdrawalInterface.sol rename to contracts/src/v0.8/operatorforwarder/interfaces/WithdrawalInterface.sol diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol new file mode 100644 index 00000000000..692e1862e9f --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../../../AuthorizedForwarder.sol"; +import "../../../Operator.sol"; +import "../../../OperatorFactory.sol"; + +import "./MockReceiver.sol"; + +import "../../../../mocks/MockLinkToken.sol"; + +abstract contract Deployer is Test { + + OperatorFactory factory; + Operator operator; + AuthorizedForwarder forwarder; + + MockLinkToken public link; + MockReceiver mockReceiver; + + address owner = address(uint160(uint256(keccak256("owner")))); + address alice = address(uint160(uint256(keccak256("alice")))); + address bob = address(uint160(uint256(keccak256("bob")))); + + address sender1 = address(uint160(uint256(keccak256("sender1")))); + address sender2 = address(uint160(uint256(keccak256("sender2")))); + address sender3 = address(uint160(uint256(keccak256("sender3")))); + + function _setUp() internal { + _deploy(); + } + + function _deploy() internal { + vm.startPrank(owner); + + link = new MockLinkToken(); + factory = new OperatorFactory(address(link)); + + mockReceiver = new MockReceiver(); + + vm.stopPrank(); + + } +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol new file mode 100644 index 00000000000..107bf6d1361 --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +contract MockReceiver { + uint256 public value; + + function receiveData(uint256 _value) public { + value = _value; + } + + function revertMessage() pure public { + revert("test revert message"); + } +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol new file mode 100644 index 00000000000..647c7d8dcc9 --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../helpers/Deployer.sol"; + +contract FactoryTest is Deployer { + + function setUp() public { + _setUp(); + } + + /** + * @dev Test the deployment of a new operator. + */ + function testDeployNewOperator() public { + vm.prank(alice); + // Deploy a new operator using the factory. + address newOperator = factory.deployNewOperator(); + // Assert that the new operator was indeed created by the factory. + assertTrue(factory.created(newOperator)); + // Ensure that Alice is the owner of the newly deployed operator. + require(Operator(newOperator).owner() == alice); + } + + /** + * @dev Test the deployment of a new operator and a new forwarder. + */ + function testDeployNewOperatorAndForwarder() public { + vm.prank(alice); + // Deploy both a new operator and a new forwarder using the factory. + (address newOperator, address newForwarder) = factory.deployNewOperatorAndForwarder(); + + // Assert that the new operator and the new forwarder were indeed created by the factory. + assertTrue(factory.created(newOperator)); + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is the owner of the newly deployed operator. + require(Operator(newOperator).owner() == alice); + + //Operator to accept ownership + vm.prank(newOperator); + AuthorizedForwarder(newForwarder).acceptOwnership(); + + // Ensure that the newly deployed operator is the owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == newOperator, "operator is not the owner"); + } + + /** + * @dev Test the deployment of a new forwarder. + */ + function testDeployNewForwarder() public { + vm.prank(alice); + // Deploy a new forwarder using the factory. + address newForwarder = factory.deployNewForwarder(); + // Assert that the new forwarder was indeed created by the factory. + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is the owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == alice); + } + + /** + * @dev Test the deployment of a new forwarder and then transfer its ownership. + */ + function testDeployNewForwarderAndTransferOwnership() public { + vm.prank(alice); + // Deploy a new forwarder with a proposal to transfer its ownership to Bob. + address newForwarder = factory.deployNewForwarderAndTransferOwnership(bob, new bytes(0)); + // Assert that the new forwarder was indeed created by the factory. + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is still the current owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == alice); + + // Only proposed owner can call acceptOwnership() + vm.expectRevert("Must be proposed owner"); + AuthorizedForwarder(newForwarder).acceptOwnership(); + + vm.prank(bob); + // Let Bob accept the ownership. + AuthorizedForwarder(newForwarder).acceptOwnership(); + // Ensure that Bob is now the owner of the forwarder after the transfer. + require(AuthorizedForwarder(newForwarder).owner() == bob); + } + +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol new file mode 100644 index 00000000000..762bce06e52 --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../helpers/Deployer.sol"; + +contract ForwarderTest is Deployer { + + function setUp() public { + _setUp(); + + vm.prank(alice); + forwarder = AuthorizedForwarder(factory.deployNewForwarder()); + } + + /** + * @dev Tests the functionality of setting authorized senders. + */ + function testSetAuthorizedSenders() public { + address[] memory senders; + + // Expect a revert when trying to set an empty list of authorized senders + vm.expectRevert("Cannot set authorized senders"); + forwarder.setAuthorizedSenders(senders); + + vm.prank(alice); + // Expect a revert because the sender list is empty + vm.expectRevert("Must have at least 1 sender"); + forwarder.setAuthorizedSenders(senders); + + // Create a list with two identical sender addresses + senders = new address[](2); + senders[0] = sender1; + senders[1] = sender1; + + vm.prank(alice); + // Expect a revert because the sender list has duplicates + vm.expectRevert("Must not have duplicate senders"); + forwarder.setAuthorizedSenders(senders); + + // Set the second sender to a different address + senders[1] = sender2; + + vm.prank(alice); + // Update the authorized senders list + forwarder.setAuthorizedSenders(senders); + + // Check if both sender1 and sender2 are now authorized + assertTrue(forwarder.isAuthorizedSender(sender1)); + assertTrue(forwarder.isAuthorizedSender(sender2)); + + // Fetch the authorized senders and verify they match the set addresses + address[] memory returnedSenders = forwarder.getAuthorizedSenders(); + require(returnedSenders[0] == senders[0]); + require(returnedSenders[1] == senders[1]); + + // Create a new list with only sender3 + senders = new address[](1); + senders[0] = sender3; + + // Prank 'alice' and update the authorized senders to just sender3 + vm.prank(alice); + forwarder.setAuthorizedSenders(senders); + + // Ensure sender1 and sender2 are no longer authorized + assertFalse(forwarder.isAuthorizedSender(sender1)); + assertFalse(forwarder.isAuthorizedSender(sender2)); + + // Check that sender3 is now the only authorized sender + assertTrue(forwarder.isAuthorizedSender(sender3)); + returnedSenders = forwarder.getAuthorizedSenders(); + require(returnedSenders[0] == senders[0]); + } + + /** + * @dev Tests the behavior of single forward + */ + function testForward(uint256 _value) public { + _addSenders(); + + vm.expectRevert("Not authorized sender"); + forwarder.forward(address(0), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Cannot forward to Link token"); + forwarder.forward(address(link), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Must forward to a contract"); + forwarder.forward(address(0), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.forward(address(mockReceiver), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("test revert message"); + forwarder.forward(address(mockReceiver), abi.encodeWithSignature("revertMessage()")); + + vm.prank(sender1); + forwarder.forward(address(mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); + + require(mockReceiver.value() == _value); + } + + function testMultiForward(uint256 _value1, uint256 _value2) public { + _addSenders(); + + address[] memory tos; + bytes[] memory datas; + + vm.expectRevert("Not authorized sender"); + forwarder.multiForward(tos, datas); + + tos = new address[](2); + datas = new bytes[](1); + + vm.prank(sender1); + vm.expectRevert("Arrays must have the same length"); + forwarder.multiForward(tos, datas); + + datas = new bytes[](2); + + vm.prank(sender1); + vm.expectRevert("Must forward to a contract"); + forwarder.multiForward(tos, datas); + + tos[0] = address(mockReceiver); + tos[1] = address(link); + + vm.prank(sender1); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.multiForward(tos, datas); + + datas[0] = abi.encodeWithSignature("receiveData(uint256)", _value1); + datas[1] = abi.encodeWithSignature("receiveData(uint256)", _value2); + + vm.prank(sender1); + vm.expectRevert("Cannot forward to Link token"); + forwarder.multiForward(tos, datas); + + tos[1] = address(mockReceiver); + + vm.prank(sender1); + forwarder.multiForward(tos, datas); + + require(mockReceiver.value() == _value2); + } + + /** + * @dev tests the difference between ownerForward and forward + * specifically owner can forward to link token + */ + function testOwnerForward() public { + vm.expectRevert("Only callable by owner"); + forwarder.ownerForward(address(0), new bytes(0)); + + vm.prank(alice); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.ownerForward(address(link), new bytes(0)); + + vm.prank(alice); + forwarder.ownerForward(address(link), abi.encodeWithSignature("balanceOf(address)", address(0))); + } + + /** + * @dev Tests the behavior of transfer and accept ownership of the contract. + */ + function testTransferOwnershipWithMessage() public { + vm.prank(bob); + vm.expectRevert("Only callable by owner"); + forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + + vm.prank(alice); + forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + + vm.expectRevert("Must be proposed owner"); + forwarder.acceptOwnership(); + + vm.prank(bob); + forwarder.acceptOwnership(); + + require(forwarder.owner() == bob); + } + + /** + * @dev Helper function to setup senders + */ + function _addSenders() internal { + address[] memory senders = new address[](3); + senders[0] = sender1; + senders[1] = sender2; + senders[2] = sender3; + + vm.prank(alice); + forwarder.setAuthorizedSenders(senders); + } + +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol new file mode 100644 index 00000000000..7dfc61d41ba --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol @@ -0,0 +1,104 @@ +pragma solidity ^0.8.19; + +import "../helpers/Deployer.sol"; +import "../../../AuthorizedReceiver.sol"; +import "../../../Operator.sol"; + +contract FuzzTokenReceiver is Test { + + bytes4 private constant ORACLE_REQUEST_SELECTOR = Operator.oracleRequest.selector; + bytes4 private constant OPERATOR_REQUEST_SELECTOR = Operator.operatorRequest.selector; + uint256 private constant SELECTOR_LENGTH = 4; + uint256 private constant EXPECTED_REQUEST_WORDS = 2; + uint256 private constant MINIMUM_REQUEST_LENGTH = SELECTOR_LENGTH + (32 * EXPECTED_REQUEST_WORDS); + bytes receivedData; + + address _spoofedLinkToken = address(0xCCBBAA); + + function testFuzzOnTokenTransferFromLink( + address _actualSender, + uint256 _actualAmount, + address _sender, + uint256 _amount, + bytes32 _specId, + address _callbackAddress, + bytes4 _callbackFunc, + uint256 _nonce, + uint256 _dataVersion, + bytes calldata _dataBytes + ) public { + + + bytes memory tokenTransferData = abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + _sender, + _amount, + _specId, + _callbackAddress, + _callbackFunc, + _nonce, + _dataVersion, + _dataBytes + ); + + bytes memory expectedData = abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + _actualSender, + _actualAmount, + _specId, + _callbackAddress, + _callbackFunc, + _nonce, + _dataVersion, + _dataBytes + ); + + _onTokenTransfer(_actualSender, _actualAmount, tokenTransferData); + + assertEq(receivedData, expectedData, "data does not match expected"); + + } + + function _onTokenTransfer( + address sender, + uint256 amount, + bytes memory data + ) private permittedFunctionsForLINK(data) { + assembly { + // solhint-disable-next-line avoid-low-level-calls + mstore(add(data, 36), sender) // ensure correct sender is passed + // solhint-disable-next-line avoid-low-level-calls + mstore(add(data, 68), amount) // ensure correct amount is passed0.8.19 + } + // solhint-disable-next-line avoid-low-level-calls + (bool success, ) = address(this).delegatecall(data); // calls oracleRequest + require(success, "Unable to create request"); + } + + // @dev Reverts if the given data does not begin with the `oracleRequest` function selector + // @param data The data payload of the request + modifier permittedFunctionsForLINK(bytes memory data) { + bytes4 funcSelector; + assembly { + // solhint-disable-next-line avoid-low-level-calls + funcSelector := mload(add(data, 32)) + } + _validateTokenTransferAction(funcSelector, data); + _; + } + + // @notice Require that the token transfer action is valid + // @dev OPERATOR_REQUEST_SELECTOR = multiword, ORACLE_REQUEST_SELECTOR = singleword + function _validateTokenTransferAction(bytes4 funcSelector, bytes memory data) internal pure { + require(data.length >= MINIMUM_REQUEST_LENGTH, "Invalid request length"); + require( + funcSelector == OPERATOR_REQUEST_SELECTOR || funcSelector == ORACLE_REQUEST_SELECTOR, + "Must use whitelisted functions" + ); + } + + // Callback function for oracle request fulfillment + fallback(bytes calldata _receivedData) external returns (bytes memory) { + receivedData = _receivedData; + } +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol new file mode 100644 index 00000000000..43723355f8a --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol @@ -0,0 +1,163 @@ +pragma solidity ^0.8.19; + +import "../helpers/Deployer.sol"; +import "../../../AuthorizedReceiver.sol"; + +contract OperatorTest is Deployer, AuthorizedReceiver { + address operatorAddr; + uint256 private dataReceived; + address operatorOwner = address(0xAABBCC); + + + + function setUp() public { + _setUp(); + operator = new Operator(address(link), operatorOwner); + operatorAddr = address(operator); + dataReceived = 0; + } + + // Callback function for oracle request fulfillment + function callback(bytes32 _requestId) public { + require(msg.sender == operatorAddr, "Only Operator can call this function"); + dataReceived += 1; + } + + // @notice concrete implementation of AuthorizedReceiver + // @return bool of whether sender is authorized + function _canSetAuthorizedSenders() internal view override returns (bool) { + return true; + } + + + function testOracleRequestFlow() public { + // Define some mock values + bytes32 specId = keccak256("testSpec"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 0; + uint256 dataVersion = 1; + bytes memory data = ""; + + uint256 initialLinkBalance = link.balanceOf(operatorAddr); + uint256 payment = 1 ether; // Mock payment value + + uint256 withdrawableBefore = operator.withdrawable(); + + // Send LINK tokens to the Operator contract using `transferAndCall` + link.setBalance(address(alice), 1 ether); + assertEq(link.balanceOf(address(alice)), 1 ether, "balance update failed"); + + vm.prank(alice); + link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, data)); + + // Check that the LINK tokens were transferred to the Operator contract + assertEq(link.balanceOf(operatorAddr), initialLinkBalance + payment); + // No withdrawable LINK as it's all locked + assertEq(operator.withdrawable(), withdrawableBefore); + } + + function testFulfillOracleRequest() public { + // This test file is the callback target and actual sender contract + // so we should enable it to set Authorised senders to itself + address[] memory senders = new address[](2); + senders[0] = address(this); + senders[0] = address(bob); + + vm.prank(address(operatorOwner)); + operator.setAuthorizedSenders(senders); + + uint256 withdrawableBefore = operator.withdrawable(); + + // Define mock values for creating a new oracle request + bytes32 specId = keccak256("testSpecForFulfill"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 1; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + // Convert bytes to bytes32 + bytes32 data = bytes32(uint256(keccak256(dataBytes))); + + // Send LINK tokens to the Operator contract using `transferAndCall` + link.setBalance(address(bob), 1 ether); + vm.prank(bob); + link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); + + + // Fulfill the request using the operator + bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); + vm.prank(bob); + operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, data); + + assertEq(dataReceived, 1, "Oracle request was not fulfilled"); + + // Withdrawable balance + assertEq(operator.withdrawable(), withdrawableBefore + payment, "Internal accounting not updated correctly"); + } + + function testCancelOracleRequest() public { + // Define mock values for creating a new oracle request + bytes32 specId = keccak256("testSpecForCancel"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 2; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + uint256 withdrawableBefore = operator.withdrawable(); + + // Convert bytes to bytes32 + bytes32 data = bytes32(uint256(keccak256(dataBytes))); + + // Send LINK tokens to the Operator contract using `transferAndCall` + link.setBalance(address(bob), 1 ether); + vm.prank(bob); + link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(bob), payment, specId, address(bob), callbackFunctionId, nonce, dataVersion, dataBytes)); + + // No withdrawable balance as it's all locked + assertEq(operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); + + bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); + + vm.startPrank(alice); + vm.expectRevert(bytes("Params do not match request ID")); + operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + vm.stopPrank(); + + vm.startPrank(bob); + vm.expectRevert(bytes("Request is not expired")); + operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + + vm.warp(expiration); + operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + vm.stopPrank(); + + // Check if the LINK tokens were refunded to the sender (bob in this case) + assertEq(link.balanceOf(address(bob)), 1 ether, "Oracle request was not canceled properly"); + + assertEq(operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); + } + + function testUnauthorizedFulfillment() public { + bytes32 specId = keccak256("unauthorizedFulfillSpec"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 5; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + link.setBalance(address(alice), payment); + vm.prank(alice); + link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(alice), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); + + bytes32 requestId = keccak256(abi.encodePacked(alice, nonce)); + + vm.prank(address(bob)); + vm.expectRevert(bytes("Not authorized sender")); + operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, bytes32(uint256(keccak256(dataBytes)))); + } +} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol similarity index 98% rename from contracts/src/v0.8/operatorforwarder/dev/test/operator.t.sol rename to contracts/src/v0.8/operatorforwarder/test/operator.t.sol index 96975a2baf4..d061f9a40d3 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.19; import {Test} from "forge-std/Test.sol"; import {Operator} from "../Operator.sol"; import {ChainlinkClientHelper} from "./testhelpers/ChainlinkClientHelper.sol"; -import {LinkToken} from "../../../shared/token/ERC677/LinkToken.sol"; +import {LinkToken} from "../../shared/token/ERC677/LinkToken.sol"; contract Operator_cancelRequest is Test { address public s_link; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/BasicConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/BasicConsumer.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/BasicConsumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/BasicConsumer.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/ChainlinkClientHelper.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol similarity index 90% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/ChainlinkClientHelper.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol index d15eb07c8c9..37acf3bd5df 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/ChainlinkClientHelper.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import {ChainlinkClient} from "../../../../ChainlinkClient.sol"; +import {ChainlinkClient} from "../../../ChainlinkClient.sol"; contract ChainlinkClientHelper is ChainlinkClient { bytes4 public constant FULFILSELECTOR = this.fulfill.selector; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Chainlinked.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Chainlinked.sol similarity index 98% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Chainlinked.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/Chainlinked.sol index 86dc474e8a6..dba5d407623 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Chainlinked.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Chainlinked.sol @@ -1,6 +1,6 @@ pragma solidity ^0.8.0; -import {ChainlinkClient, Chainlink} from "../../../../ChainlinkClient.sol"; +import {ChainlinkClient, Chainlink} from "../../../ChainlinkClient.sol"; /** * @title The Chainlinked contract diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Consumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol similarity index 94% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Consumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol index 0d01778e19e..3b62353b83c 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/Consumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol @@ -1,8 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import {ChainlinkClient, ChainlinkRequestInterface, LinkTokenInterface} from "../../../../ChainlinkClient.sol"; -import {Chainlink} from "../../../../Chainlink.sol"; +import {ChainlinkClient, ChainlinkRequestInterface, LinkTokenInterface} from "../../../ChainlinkClient.sol"; +import {Chainlink} from "../../../Chainlink.sol"; contract Consumer is ChainlinkClient { using Chainlink for Chainlink.Request; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/EmptyOracle.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/EmptyOracle.sol similarity index 83% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/EmptyOracle.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/EmptyOracle.sol index 2abe393151e..f278791d2bb 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/EmptyOracle.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/EmptyOracle.sol @@ -1,8 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import {ChainlinkRequestInterface} from "../../../../interfaces/ChainlinkRequestInterface.sol"; -import {OracleInterface} from "../../../../interfaces/OracleInterface.sol"; +import {ChainlinkRequestInterface} from "../../../interfaces/ChainlinkRequestInterface.sol"; +import {OracleInterface} from "../../../interfaces/OracleInterface.sol"; /* solhint-disable no-empty-blocks */ contract EmptyOracle is ChainlinkRequestInterface, OracleInterface { diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/GasGuzzlingConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GasGuzzlingConsumer.sol similarity index 96% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/GasGuzzlingConsumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/GasGuzzlingConsumer.sol index 54ff0e30e66..029102018b0 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/GasGuzzlingConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GasGuzzlingConsumer.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.0; import {Consumer} from "./Consumer.sol"; -import {Chainlink} from "../../../../Chainlink.sol"; +import {Chainlink} from "../../../Chainlink.sol"; contract GasGuzzlingConsumer is Consumer { using Chainlink for Chainlink.Request; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/GetterSetter.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/GetterSetter.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlink.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlink.sol similarity index 91% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlink.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlink.sol index 5cc343aa7f4..11c863fbb98 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlink.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlink.sol @@ -1,7 +1,7 @@ pragma solidity ^0.8.0; -import {CBORChainlink as CBOR_Chainlink} from "../../../../vendor/CBORChainlink.sol"; -import {BufferChainlink as Buffer_Chainlink} from "../../../../vendor/BufferChainlink.sol"; +import {CBORChainlink as CBOR_Chainlink} from "../../../vendor/CBORChainlink.sol"; +import {BufferChainlink as Buffer_Chainlink} from "../../../vendor/BufferChainlink.sol"; // solhint-disable library MaliciousChainlink { diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlinked.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol similarity index 97% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlinked.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol index 722fbdd599b..8ca02c34c62 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousChainlinked.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.0; import {MaliciousChainlink} from "./MaliciousChainlink.sol"; import {Chainlinked, Chainlink} from "./Chainlinked.sol"; -import {LinkTokenInterface} from "../../../../shared/interfaces/LinkTokenInterface.sol"; +import {LinkTokenInterface} from "../../../shared/interfaces/LinkTokenInterface.sol"; // solhint-disable contract MaliciousChainlinked is Chainlinked { diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousConsumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousMultiWordConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousMultiWordConsumer.sol similarity index 94% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousMultiWordConsumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousMultiWordConsumer.sol index 272361f2dda..d9d14cb3d43 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousMultiWordConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousMultiWordConsumer.sol @@ -1,8 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import {ChainlinkClient} from "../../../../ChainlinkClient.sol"; -import {Chainlink} from "../../../../Chainlink.sol"; +import {ChainlinkClient} from "../../../ChainlinkClient.sol"; +import {Chainlink} from "../../../Chainlink.sol"; contract MaliciousMultiWordConsumer is ChainlinkClient { uint256 private constant ORACLE_PAYMENT = 1 ether; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousRequester.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousRequester.sol similarity index 95% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousRequester.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousRequester.sol index 9b19653722c..c01c8a60bb7 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousRequester.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousRequester.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.0; import {MaliciousChainlink} from "./MaliciousChainlink.sol"; import {MaliciousChainlinked, Chainlink} from "./MaliciousChainlinked.sol"; -import {ChainlinkRequestInterface} from "../../../../interfaces/ChainlinkRequestInterface.sol"; +import {ChainlinkRequestInterface} from "../../../interfaces/ChainlinkRequestInterface.sol"; contract MaliciousRequester is MaliciousChainlinked { uint256 private constant ORACLE_PAYMENT = 1 ether; diff --git a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MultiWordConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol similarity index 97% rename from contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MultiWordConsumer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol index ce2bf1907c5..95575c41a78 100644 --- a/contracts/src/v0.8/operatorforwarder/dev/test/testhelpers/MultiWordConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol @@ -1,7 +1,7 @@ pragma solidity ^0.8.0; -import {ChainlinkClient, ChainlinkRequestInterface, LinkTokenInterface} from "../../../../ChainlinkClient.sol"; -import {Chainlink} from "../../../../Chainlink.sol"; +import {ChainlinkClient, ChainlinkRequestInterface, LinkTokenInterface} from "../../../ChainlinkClient.sol"; +import {Chainlink} from "../../../Chainlink.sol"; contract MultiWordConsumer is ChainlinkClient { using Chainlink for Chainlink.Request; From 16dad2bf6781726fa9e00027ba72aebff7af38f1 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:33:18 -0700 Subject: [PATCH 02/15] Update test locations --- .../{contributedTests/test => }/Factory.t.sol | 10 +- .../test => }/Forwarder.t.sol | 12 +- .../test => }/FuzzOperator.t.sol | 8 +- .../test/contributedTests/test/Operator.t.sol | 163 ------------------ .../operatorforwarder/test/operator.t.sol | 153 +++++++++++++++- .../helpers => testhelpers}/Deployer.sol | 8 +- .../helpers => testhelpers}/MockReceiver.sol | 0 7 files changed, 170 insertions(+), 184 deletions(-) rename contracts/src/v0.8/operatorforwarder/test/{contributedTests/test => }/Factory.t.sol (92%) rename contracts/src/v0.8/operatorforwarder/test/{contributedTests/test => }/Forwarder.t.sol (95%) rename contracts/src/v0.8/operatorforwarder/test/{contributedTests/test => }/FuzzOperator.t.sol (95%) delete mode 100644 contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol rename contracts/src/v0.8/operatorforwarder/test/{contributedTests/helpers => testhelpers}/Deployer.sol (86%) rename contracts/src/v0.8/operatorforwarder/test/{contributedTests/helpers => testhelpers}/MockReceiver.sol (100%) diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol similarity index 92% rename from contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol rename to contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index 647c7d8dcc9..04aed6f73b3 100644 --- a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "../helpers/Deployer.sol"; +import "./testhelpers/Deployer.sol"; contract FactoryTest is Deployer { @@ -14,7 +14,7 @@ contract FactoryTest is Deployer { /** * @dev Test the deployment of a new operator. */ - function testDeployNewOperator() public { + function test_DeployNewOperator() public { vm.prank(alice); // Deploy a new operator using the factory. address newOperator = factory.deployNewOperator(); @@ -27,7 +27,7 @@ contract FactoryTest is Deployer { /** * @dev Test the deployment of a new operator and a new forwarder. */ - function testDeployNewOperatorAndForwarder() public { + function test_DeployNewOperatorAndForwarder() public { vm.prank(alice); // Deploy both a new operator and a new forwarder using the factory. (address newOperator, address newForwarder) = factory.deployNewOperatorAndForwarder(); @@ -49,7 +49,7 @@ contract FactoryTest is Deployer { /** * @dev Test the deployment of a new forwarder. */ - function testDeployNewForwarder() public { + function test_DeployNewForwarder() public { vm.prank(alice); // Deploy a new forwarder using the factory. address newForwarder = factory.deployNewForwarder(); @@ -62,7 +62,7 @@ contract FactoryTest is Deployer { /** * @dev Test the deployment of a new forwarder and then transfer its ownership. */ - function testDeployNewForwarderAndTransferOwnership() public { + function test_DeployNewForwarderAndTransferOwnership() public { vm.prank(alice); // Deploy a new forwarder with a proposal to transfer its ownership to Bob. address newForwarder = factory.deployNewForwarderAndTransferOwnership(bob, new bytes(0)); diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol similarity index 95% rename from contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol rename to contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index 762bce06e52..5e7b4fc1794 100644 --- a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "../helpers/Deployer.sol"; +import "./testhelpers/Deployer.sol"; contract ForwarderTest is Deployer { @@ -17,7 +17,7 @@ contract ForwarderTest is Deployer { /** * @dev Tests the functionality of setting authorized senders. */ - function testSetAuthorizedSenders() public { + function test_SetAuthorizedSenders() public { address[] memory senders; // Expect a revert when trying to set an empty list of authorized senders @@ -76,7 +76,7 @@ contract ForwarderTest is Deployer { /** * @dev Tests the behavior of single forward */ - function testForward(uint256 _value) public { + function test_Forward(uint256 _value) public { _addSenders(); vm.expectRevert("Not authorized sender"); @@ -104,7 +104,7 @@ contract ForwarderTest is Deployer { require(mockReceiver.value() == _value); } - function testMultiForward(uint256 _value1, uint256 _value2) public { + function test_MultiForward(uint256 _value1, uint256 _value2) public { _addSenders(); address[] memory tos; @@ -152,7 +152,7 @@ contract ForwarderTest is Deployer { * @dev tests the difference between ownerForward and forward * specifically owner can forward to link token */ - function testOwnerForward() public { + function test_OwnerForward() public { vm.expectRevert("Only callable by owner"); forwarder.ownerForward(address(0), new bytes(0)); @@ -167,7 +167,7 @@ contract ForwarderTest is Deployer { /** * @dev Tests the behavior of transfer and accept ownership of the contract. */ - function testTransferOwnershipWithMessage() public { + function test_TransferOwnershipWithMessage() public { vm.prank(bob); vm.expectRevert("Only callable by owner"); forwarder.transferOwnershipWithMessage(bob, new bytes(0)); diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol similarity index 95% rename from contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol rename to contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol index 7dfc61d41ba..80f913c9c15 100644 --- a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/FuzzOperator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol @@ -1,8 +1,8 @@ pragma solidity ^0.8.19; -import "../helpers/Deployer.sol"; -import "../../../AuthorizedReceiver.sol"; -import "../../../Operator.sol"; +import "./testhelpers/Deployer.sol"; +import "../AuthorizedReceiver.sol"; +import "../Operator.sol"; contract FuzzTokenReceiver is Test { @@ -15,7 +15,7 @@ contract FuzzTokenReceiver is Test { address _spoofedLinkToken = address(0xCCBBAA); - function testFuzzOnTokenTransferFromLink( + function test_FuzzOnTokenTransferFromLink( address _actualSender, uint256 _actualAmount, address _sender, diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol deleted file mode 100644 index 43723355f8a..00000000000 --- a/contracts/src/v0.8/operatorforwarder/test/contributedTests/test/Operator.t.sol +++ /dev/null @@ -1,163 +0,0 @@ -pragma solidity ^0.8.19; - -import "../helpers/Deployer.sol"; -import "../../../AuthorizedReceiver.sol"; - -contract OperatorTest is Deployer, AuthorizedReceiver { - address operatorAddr; - uint256 private dataReceived; - address operatorOwner = address(0xAABBCC); - - - - function setUp() public { - _setUp(); - operator = new Operator(address(link), operatorOwner); - operatorAddr = address(operator); - dataReceived = 0; - } - - // Callback function for oracle request fulfillment - function callback(bytes32 _requestId) public { - require(msg.sender == operatorAddr, "Only Operator can call this function"); - dataReceived += 1; - } - - // @notice concrete implementation of AuthorizedReceiver - // @return bool of whether sender is authorized - function _canSetAuthorizedSenders() internal view override returns (bool) { - return true; - } - - - function testOracleRequestFlow() public { - // Define some mock values - bytes32 specId = keccak256("testSpec"); - bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); - uint256 nonce = 0; - uint256 dataVersion = 1; - bytes memory data = ""; - - uint256 initialLinkBalance = link.balanceOf(operatorAddr); - uint256 payment = 1 ether; // Mock payment value - - uint256 withdrawableBefore = operator.withdrawable(); - - // Send LINK tokens to the Operator contract using `transferAndCall` - link.setBalance(address(alice), 1 ether); - assertEq(link.balanceOf(address(alice)), 1 ether, "balance update failed"); - - vm.prank(alice); - link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, data)); - - // Check that the LINK tokens were transferred to the Operator contract - assertEq(link.balanceOf(operatorAddr), initialLinkBalance + payment); - // No withdrawable LINK as it's all locked - assertEq(operator.withdrawable(), withdrawableBefore); - } - - function testFulfillOracleRequest() public { - // This test file is the callback target and actual sender contract - // so we should enable it to set Authorised senders to itself - address[] memory senders = new address[](2); - senders[0] = address(this); - senders[0] = address(bob); - - vm.prank(address(operatorOwner)); - operator.setAuthorizedSenders(senders); - - uint256 withdrawableBefore = operator.withdrawable(); - - // Define mock values for creating a new oracle request - bytes32 specId = keccak256("testSpecForFulfill"); - bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); - uint256 nonce = 1; - uint256 dataVersion = 1; - bytes memory dataBytes = ""; - uint256 payment = 1 ether; - uint256 expiration = block.timestamp + 5 minutes; - - // Convert bytes to bytes32 - bytes32 data = bytes32(uint256(keccak256(dataBytes))); - - // Send LINK tokens to the Operator contract using `transferAndCall` - link.setBalance(address(bob), 1 ether); - vm.prank(bob); - link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); - - - // Fulfill the request using the operator - bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); - vm.prank(bob); - operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, data); - - assertEq(dataReceived, 1, "Oracle request was not fulfilled"); - - // Withdrawable balance - assertEq(operator.withdrawable(), withdrawableBefore + payment, "Internal accounting not updated correctly"); - } - - function testCancelOracleRequest() public { - // Define mock values for creating a new oracle request - bytes32 specId = keccak256("testSpecForCancel"); - bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); - uint256 nonce = 2; - uint256 dataVersion = 1; - bytes memory dataBytes = ""; - uint256 payment = 1 ether; - uint256 expiration = block.timestamp + 5 minutes; - - uint256 withdrawableBefore = operator.withdrawable(); - - // Convert bytes to bytes32 - bytes32 data = bytes32(uint256(keccak256(dataBytes))); - - // Send LINK tokens to the Operator contract using `transferAndCall` - link.setBalance(address(bob), 1 ether); - vm.prank(bob); - link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(bob), payment, specId, address(bob), callbackFunctionId, nonce, dataVersion, dataBytes)); - - // No withdrawable balance as it's all locked - assertEq(operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); - - bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); - - vm.startPrank(alice); - vm.expectRevert(bytes("Params do not match request ID")); - operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - vm.stopPrank(); - - vm.startPrank(bob); - vm.expectRevert(bytes("Request is not expired")); - operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - - vm.warp(expiration); - operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - vm.stopPrank(); - - // Check if the LINK tokens were refunded to the sender (bob in this case) - assertEq(link.balanceOf(address(bob)), 1 ether, "Oracle request was not canceled properly"); - - assertEq(operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); - } - - function testUnauthorizedFulfillment() public { - bytes32 specId = keccak256("unauthorizedFulfillSpec"); - bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); - uint256 nonce = 5; - uint256 dataVersion = 1; - bytes memory dataBytes = ""; - uint256 payment = 1 ether; - uint256 expiration = block.timestamp + 5 minutes; - - link.setBalance(address(alice), payment); - vm.prank(alice); - link.transferAndCall(operatorAddr, payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(alice), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); - - bytes32 requestId = keccak256(abi.encodePacked(alice, nonce)); - - vm.prank(address(bob)); - vm.expectRevert(bytes("Not authorized sender")); - operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, bytes32(uint256(keccak256(dataBytes)))); - } -} \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index d061f9a40d3..b69f1e67b0a 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -6,12 +6,17 @@ import {Operator} from "../Operator.sol"; import {ChainlinkClientHelper} from "./testhelpers/ChainlinkClientHelper.sol"; import {LinkToken} from "../../shared/token/ERC677/LinkToken.sol"; -contract Operator_cancelRequest is Test { +import "./testhelpers/Deployer.sol"; +import "../AuthorizedReceiver.sol"; + +contract OperatorTest is Deployer, AuthorizedReceiver { address public s_link; + uint256 private dataReceived; ChainlinkClientHelper public s_client; Operator public s_operator; function setUp() public { + _setUp(); s_link = address(new LinkToken()); s_client = new ChainlinkClientHelper(s_link); @@ -19,12 +24,26 @@ contract Operator_cancelRequest is Test { auth[0] = address(this); s_operator = new Operator(s_link, address(this)); s_operator.setAuthorizedSenders(auth); + + dataReceived = 0; + } + + // Callback function for oracle request fulfillment + function callback(bytes32 _requestId) public { + require(msg.sender == address(s_operator), "Only Operator can call this function"); + dataReceived += 1; + } + + // @notice concrete implementation of AuthorizedReceiver + // @return bool of whether sender is authorized + function _canSetAuthorizedSenders() internal view override returns (bool) { + return true; } function test_Success(uint96 payment) public { payment = uint96(bound(payment, 1, type(uint96).max)); deal(s_link, address(s_client), payment); - // We're going to cancel one request and fulfil the other + // We're going to cancel one request and fulfill the other bytes32 requestIdToCancel = s_client.sendRequest(address(s_operator), payment); // Nothing withdrawable @@ -97,4 +116,134 @@ contract Operator_cancelRequest is Test { assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 0); assertEq(LinkToken(s_link).balanceOf(address(s_client)), 2 * payment); } + + function test_oracleRequestFlow() public { + // Define some mock values + bytes32 specId = keccak256("testSpec"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 0; + uint256 dataVersion = 1; + bytes memory data = ""; + + uint256 initialLinkBalance = LinkToken(s_link).balanceOf(address(s_operator)); + uint256 payment = 1 ether; // Mock payment value + + uint256 withdrawableBefore = s_operator.withdrawable(); + + // Send LINK tokens to the Operator contract using `transferAndCall` + deal(s_link, address(alice), payment); + assertEq(LinkToken(s_link).balanceOf(address(alice)), 1 ether, "balance update failed"); + + vm.prank(alice); + LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, data)); + + // Check that the LINK tokens were transferred to the Operator contract + assertEq(LinkToken(s_link).balanceOf(address(s_operator)), initialLinkBalance + payment); + // No withdrawable LINK as it's all locked + assertEq(s_operator.withdrawable(), withdrawableBefore); + } + + function test_fulfillOracleRequest() public { + // This test file is the callback target and actual sender contract + // so we should enable it to set Authorised senders to itself + address[] memory senders = new address[](2); + senders[0] = address(this); + senders[0] = address(bob); + + s_operator.setAuthorizedSenders(senders); + + uint256 withdrawableBefore = s_operator.withdrawable(); + + // Define mock values for creating a new oracle request + bytes32 specId = keccak256("testSpecForFulfill"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 1; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + // Convert bytes to bytes32 + bytes32 data = bytes32(uint256(keccak256(dataBytes))); + + // Send LINK tokens to the Operator contract using `transferAndCall` + deal(s_link, address(bob), payment); + vm.prank(bob); + LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); + + + // Fulfill the request using the operator + bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); + vm.prank(bob); + s_operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, data); + + assertEq(dataReceived, 1, "Oracle request was not fulfilled"); + + // Withdrawable balance + assertEq(s_operator.withdrawable(), withdrawableBefore + payment, "Internal accounting not updated correctly"); + } + + function test_cancelOracleRequest() public { + // Define mock values for creating a new oracle request + bytes32 specId = keccak256("testSpecForCancel"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 2; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + uint256 withdrawableBefore = s_operator.withdrawable(); + + // Convert bytes to bytes32 + bytes32 data = bytes32(uint256(keccak256(dataBytes))); + + // Send LINK tokens to the Operator contract using `transferAndCall` + deal(s_link, address(bob), payment); + vm.prank(bob); + LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(bob), payment, specId, address(bob), callbackFunctionId, nonce, dataVersion, dataBytes)); + + // No withdrawable balance as it's all locked + assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); + + bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); + + vm.startPrank(alice); + vm.expectRevert(bytes("Params do not match request ID")); + s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + vm.stopPrank(); + + vm.startPrank(bob); + vm.expectRevert(bytes("Request is not expired")); + s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + + vm.warp(expiration); + s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); + vm.stopPrank(); + + // Check if the LINK tokens were refunded to the sender (bob in this case) + assertEq(LinkToken(s_link).balanceOf(address(bob)), 1 ether, "Oracle request was not canceled properly"); + + assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); + } + + function test_unauthorizedFulfillment() public { + bytes32 specId = keccak256("unauthorizedFulfillSpec"); + bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); + uint256 nonce = 5; + uint256 dataVersion = 1; + bytes memory dataBytes = ""; + uint256 payment = 1 ether; + uint256 expiration = block.timestamp + 5 minutes; + + deal(s_link, address(alice), payment); + vm.prank(alice); + LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(alice), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); + + bytes32 requestId = keccak256(abi.encodePacked(alice, nonce)); + + vm.prank(address(bob)); + vm.expectRevert(bytes("Not authorized sender")); + s_operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, bytes32(uint256(keccak256(dataBytes)))); + } } diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol similarity index 86% rename from contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol index 692e1862e9f..d4045ab5267 100644 --- a/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/Deployer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol @@ -3,13 +3,13 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "../../../AuthorizedForwarder.sol"; -import "../../../Operator.sol"; -import "../../../OperatorFactory.sol"; +import "../../AuthorizedForwarder.sol"; +import "../../Operator.sol"; +import "../../OperatorFactory.sol"; import "./MockReceiver.sol"; -import "../../../../mocks/MockLinkToken.sol"; +import "../../../mocks/MockLinkToken.sol"; abstract contract Deployer is Test { diff --git a/contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/test/contributedTests/helpers/MockReceiver.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol From 05ef5111f71d8081dc1bb8771006240b0e519faf Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:35:08 -0700 Subject: [PATCH 03/15] Update gas snapshot --- .../operatorforwarder.gas-snapshot | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index b0698d35290..2ffcc3d261a 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -1,16 +1,16 @@ -FactoryTest:testDeployNewForwarder() (gas: 1050792) -FactoryTest:testDeployNewForwarderAndTransferOwnership() (gas: 1064662) -FactoryTest:testDeployNewOperator() (gas: 3023070) -FactoryTest:testDeployNewOperatorAndForwarder() (gas: 4071877) -ForwarderTest:testForward(uint256) (runs: 256, μ: 235226, ~: 236315) -ForwarderTest:testMultiForward(uint256,uint256) (runs: 256, μ: 266861, ~: 268105) -ForwarderTest:testOwnerForward() (gas: 32246) -ForwarderTest:testSetAuthorizedSenders() (gas: 168042) -ForwarderTest:testTransferOwnershipWithMessage() (gas: 38881) -FuzzTokenReceiver:testFuzzOnTokenTransferFromLink(address,uint256,address,uint256,bytes32,address,bytes4,uint256,uint256,bytes) (runs: 256, μ: 272702, ~: 277658) -OperatorTest:testCancelOracleRequest() (gas: 110758) -OperatorTest:testFulfillOracleRequest() (gas: 198351) -OperatorTest:testOracleRequestFlow() (gas: 89666) -OperatorTest:testUnauthorizedFulfillment() (gas: 88254) -Operator_cancelRequest:test_Success(uint96) (runs: 256, μ: 306105, ~: 306096) -Operator_cancelRequest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 385443, ~: 389554) \ No newline at end of file +FactoryTest:test_DeployNewForwarder() (gas: 1050792) +FactoryTest:test_DeployNewForwarderAndTransferOwnership() (gas: 1064683) +FactoryTest:test_DeployNewOperator() (gas: 3023070) +FactoryTest:test_DeployNewOperatorAndForwarder() (gas: 4071878) +ForwarderTest:test_Forward(uint256) (runs: 256, μ: 235183, ~: 236272) +ForwarderTest:test_MultiForward(uint256,uint256) (runs: 256, μ: 266882, ~: 268126) +ForwarderTest:test_OwnerForward() (gas: 32267) +ForwarderTest:test_SetAuthorizedSenders() (gas: 168077) +ForwarderTest:test_TransferOwnershipWithMessage() (gas: 38863) +FuzzTokenReceiver:test_FuzzOnTokenTransferFromLink(address,uint256,address,uint256,bytes32,address,bytes4,uint256,uint256,bytes) (runs: 256, μ: 272414, ~: 277681) +OperatorTest:test_Success(uint96) (runs: 256, μ: 306108, ~: 306102) +OperatorTest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 385291, ~: 389533) +OperatorTest:test_cancelOracleRequest() (gas: 278994) +OperatorTest:test_fulfillOracleRequest() (gas: 326881) +OperatorTest:test_oracleRequestFlow() (gas: 250165) +OperatorTest:test_unauthorizedFulfillment() (gas: 248625) \ No newline at end of file From 499f7dbc3ace86611bde545c983f8c0c72b60d9c Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:38:20 -0700 Subject: [PATCH 04/15] Update solc compile --- .../scripts/native_solc_compile_all_operatorforwarder | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/contracts/scripts/native_solc_compile_all_operatorforwarder b/contracts/scripts/native_solc_compile_all_operatorforwarder index 2d455994813..c791d50c1d6 100755 --- a/contracts/scripts/native_solc_compile_all_operatorforwarder +++ b/contracts/scripts/native_solc_compile_all_operatorforwarder @@ -28,8 +28,9 @@ compileContract () { } # Contracts -compileContract operatorforwarder/dev/AuthorizedForwarder.sol -compileContract operatorforwarder/dev/AuthorizedReceiver.sol -compileContract operatorforwarder/dev/Operator.sol -compileContract operatorforwarder/dev/OperatorFactory.sol +compileContract operatorforwarder/AuthorizedForwarder.sol +compileContract operatorforwarder/AuthorizedReceiver.sol +compileContract operatorforwarder/LinkTokenReceiver.sol +compileContract operatorforwarder/Operator.sol +compileContract operatorforwarder/OperatorFactory.sol From 678b0bdb01e0082b4b88b5991302b56793ba43a2 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:39:44 -0700 Subject: [PATCH 05/15] Changeset --- contracts/.changeset/small-paws-crash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 contracts/.changeset/small-paws-crash.md diff --git a/contracts/.changeset/small-paws-crash.md b/contracts/.changeset/small-paws-crash.md new file mode 100644 index 00000000000..3a3efa36805 --- /dev/null +++ b/contracts/.changeset/small-paws-crash.md @@ -0,0 +1,5 @@ +--- +"@chainlink/contracts": patch +--- + +Update operatorforwarder tests and pull out of dev/ From fbf7860483feb31733ec1845893b2cde8b7d14f2 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 12:48:12 -0700 Subject: [PATCH 06/15] Prettier linting + hardhat test updates --- .../v0.8/operatorforwarder/test/Factory.t.sol | 134 ++++--- .../operatorforwarder/test/Forwarder.t.sol | 374 +++++++++--------- .../operatorforwarder/test/FuzzOperator.t.sol | 164 ++++---- .../operatorforwarder/test/operator.t.sol | 84 +++- .../test/testhelpers/Deployer.sol | 46 ++- .../test/testhelpers/MockReceiver.sol | 16 +- contracts/test/v0.8/ChainlinkClient.test.ts | 6 +- .../AuthorizedForwarder.test.ts | 4 +- .../v0.8/operatorforwarder/Operator.test.ts | 18 +- .../operatorforwarder/OperatorFactory.test.ts | 6 +- 10 files changed, 450 insertions(+), 402 deletions(-) diff --git a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index 04aed6f73b3..c93874b2637 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -6,80 +6,78 @@ import "forge-std/Test.sol"; import "./testhelpers/Deployer.sol"; contract FactoryTest is Deployer { + function setUp() public { + _setUp(); + } - function setUp() public { - _setUp(); - } + /** + * @dev Test the deployment of a new operator. + */ + function test_DeployNewOperator() public { + vm.prank(alice); + // Deploy a new operator using the factory. + address newOperator = factory.deployNewOperator(); + // Assert that the new operator was indeed created by the factory. + assertTrue(factory.created(newOperator)); + // Ensure that Alice is the owner of the newly deployed operator. + require(Operator(newOperator).owner() == alice); + } - /** - * @dev Test the deployment of a new operator. - */ - function test_DeployNewOperator() public { - vm.prank(alice); - // Deploy a new operator using the factory. - address newOperator = factory.deployNewOperator(); - // Assert that the new operator was indeed created by the factory. - assertTrue(factory.created(newOperator)); - // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == alice); - } + /** + * @dev Test the deployment of a new operator and a new forwarder. + */ + function test_DeployNewOperatorAndForwarder() public { + vm.prank(alice); + // Deploy both a new operator and a new forwarder using the factory. + (address newOperator, address newForwarder) = factory.deployNewOperatorAndForwarder(); - /** - * @dev Test the deployment of a new operator and a new forwarder. - */ - function test_DeployNewOperatorAndForwarder() public { - vm.prank(alice); - // Deploy both a new operator and a new forwarder using the factory. - (address newOperator, address newForwarder) = factory.deployNewOperatorAndForwarder(); + // Assert that the new operator and the new forwarder were indeed created by the factory. + assertTrue(factory.created(newOperator)); + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is the owner of the newly deployed operator. + require(Operator(newOperator).owner() == alice); - // Assert that the new operator and the new forwarder were indeed created by the factory. - assertTrue(factory.created(newOperator)); - assertTrue(factory.created(newForwarder)); - // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == alice); + //Operator to accept ownership + vm.prank(newOperator); + AuthorizedForwarder(newForwarder).acceptOwnership(); - //Operator to accept ownership - vm.prank(newOperator); - AuthorizedForwarder(newForwarder).acceptOwnership(); + // Ensure that the newly deployed operator is the owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == newOperator, "operator is not the owner"); + } - // Ensure that the newly deployed operator is the owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == newOperator, "operator is not the owner"); - } + /** + * @dev Test the deployment of a new forwarder. + */ + function test_DeployNewForwarder() public { + vm.prank(alice); + // Deploy a new forwarder using the factory. + address newForwarder = factory.deployNewForwarder(); + // Assert that the new forwarder was indeed created by the factory. + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is the owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == alice); + } - /** - * @dev Test the deployment of a new forwarder. - */ - function test_DeployNewForwarder() public { - vm.prank(alice); - // Deploy a new forwarder using the factory. - address newForwarder = factory.deployNewForwarder(); - // Assert that the new forwarder was indeed created by the factory. - assertTrue(factory.created(newForwarder)); - // Ensure that Alice is the owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == alice); - } + /** + * @dev Test the deployment of a new forwarder and then transfer its ownership. + */ + function test_DeployNewForwarderAndTransferOwnership() public { + vm.prank(alice); + // Deploy a new forwarder with a proposal to transfer its ownership to Bob. + address newForwarder = factory.deployNewForwarderAndTransferOwnership(bob, new bytes(0)); + // Assert that the new forwarder was indeed created by the factory. + assertTrue(factory.created(newForwarder)); + // Ensure that Alice is still the current owner of the newly deployed forwarder. + require(AuthorizedForwarder(newForwarder).owner() == alice); - /** - * @dev Test the deployment of a new forwarder and then transfer its ownership. - */ - function test_DeployNewForwarderAndTransferOwnership() public { - vm.prank(alice); - // Deploy a new forwarder with a proposal to transfer its ownership to Bob. - address newForwarder = factory.deployNewForwarderAndTransferOwnership(bob, new bytes(0)); - // Assert that the new forwarder was indeed created by the factory. - assertTrue(factory.created(newForwarder)); - // Ensure that Alice is still the current owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == alice); + // Only proposed owner can call acceptOwnership() + vm.expectRevert("Must be proposed owner"); + AuthorizedForwarder(newForwarder).acceptOwnership(); - // Only proposed owner can call acceptOwnership() - vm.expectRevert("Must be proposed owner"); - AuthorizedForwarder(newForwarder).acceptOwnership(); - - vm.prank(bob); - // Let Bob accept the ownership. - AuthorizedForwarder(newForwarder).acceptOwnership(); - // Ensure that Bob is now the owner of the forwarder after the transfer. - require(AuthorizedForwarder(newForwarder).owner() == bob); - } - -} \ No newline at end of file + vm.prank(bob); + // Let Bob accept the ownership. + AuthorizedForwarder(newForwarder).acceptOwnership(); + // Ensure that Bob is now the owner of the forwarder after the transfer. + require(AuthorizedForwarder(newForwarder).owner() == bob); + } +} diff --git a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index 5e7b4fc1794..939c18bad23 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -6,195 +6,193 @@ import "forge-std/Test.sol"; import "./testhelpers/Deployer.sol"; contract ForwarderTest is Deployer { + function setUp() public { + _setUp(); - function setUp() public { - _setUp(); - - vm.prank(alice); - forwarder = AuthorizedForwarder(factory.deployNewForwarder()); - } - - /** - * @dev Tests the functionality of setting authorized senders. - */ - function test_SetAuthorizedSenders() public { - address[] memory senders; + vm.prank(alice); + forwarder = AuthorizedForwarder(factory.deployNewForwarder()); + } - // Expect a revert when trying to set an empty list of authorized senders - vm.expectRevert("Cannot set authorized senders"); - forwarder.setAuthorizedSenders(senders); + /** + * @dev Tests the functionality of setting authorized senders. + */ + function test_SetAuthorizedSenders() public { + address[] memory senders; - vm.prank(alice); - // Expect a revert because the sender list is empty - vm.expectRevert("Must have at least 1 sender"); - forwarder.setAuthorizedSenders(senders); + // Expect a revert when trying to set an empty list of authorized senders + vm.expectRevert("Cannot set authorized senders"); + forwarder.setAuthorizedSenders(senders); - // Create a list with two identical sender addresses - senders = new address[](2); - senders[0] = sender1; - senders[1] = sender1; - - vm.prank(alice); - // Expect a revert because the sender list has duplicates - vm.expectRevert("Must not have duplicate senders"); - forwarder.setAuthorizedSenders(senders); - - // Set the second sender to a different address - senders[1] = sender2; - - vm.prank(alice); - // Update the authorized senders list - forwarder.setAuthorizedSenders(senders); - - // Check if both sender1 and sender2 are now authorized - assertTrue(forwarder.isAuthorizedSender(sender1)); - assertTrue(forwarder.isAuthorizedSender(sender2)); - - // Fetch the authorized senders and verify they match the set addresses - address[] memory returnedSenders = forwarder.getAuthorizedSenders(); - require(returnedSenders[0] == senders[0]); - require(returnedSenders[1] == senders[1]); - - // Create a new list with only sender3 - senders = new address[](1); - senders[0] = sender3; - - // Prank 'alice' and update the authorized senders to just sender3 - vm.prank(alice); - forwarder.setAuthorizedSenders(senders); - - // Ensure sender1 and sender2 are no longer authorized - assertFalse(forwarder.isAuthorizedSender(sender1)); - assertFalse(forwarder.isAuthorizedSender(sender2)); - - // Check that sender3 is now the only authorized sender - assertTrue(forwarder.isAuthorizedSender(sender3)); - returnedSenders = forwarder.getAuthorizedSenders(); - require(returnedSenders[0] == senders[0]); - } - - /** - * @dev Tests the behavior of single forward - */ - function test_Forward(uint256 _value) public { - _addSenders(); - - vm.expectRevert("Not authorized sender"); - forwarder.forward(address(0), new bytes(0)); - - vm.prank(sender1); - vm.expectRevert("Cannot forward to Link token"); - forwarder.forward(address(link), new bytes(0)); - - vm.prank(sender1); - vm.expectRevert("Must forward to a contract"); - forwarder.forward(address(0), new bytes(0)); - - vm.prank(sender1); - vm.expectRevert("Forwarded call reverted without reason"); - forwarder.forward(address(mockReceiver), new bytes(0)); - - vm.prank(sender1); - vm.expectRevert("test revert message"); - forwarder.forward(address(mockReceiver), abi.encodeWithSignature("revertMessage()")); - - vm.prank(sender1); - forwarder.forward(address(mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); - - require(mockReceiver.value() == _value); - } - - function test_MultiForward(uint256 _value1, uint256 _value2) public { - _addSenders(); - - address[] memory tos; - bytes[] memory datas; - - vm.expectRevert("Not authorized sender"); - forwarder.multiForward(tos, datas); - - tos = new address[](2); - datas = new bytes[](1); - - vm.prank(sender1); - vm.expectRevert("Arrays must have the same length"); - forwarder.multiForward(tos, datas); - - datas = new bytes[](2); - - vm.prank(sender1); - vm.expectRevert("Must forward to a contract"); - forwarder.multiForward(tos, datas); - - tos[0] = address(mockReceiver); - tos[1] = address(link); - - vm.prank(sender1); - vm.expectRevert("Forwarded call reverted without reason"); - forwarder.multiForward(tos, datas); - - datas[0] = abi.encodeWithSignature("receiveData(uint256)", _value1); - datas[1] = abi.encodeWithSignature("receiveData(uint256)", _value2); - - vm.prank(sender1); - vm.expectRevert("Cannot forward to Link token"); - forwarder.multiForward(tos, datas); - - tos[1] = address(mockReceiver); - - vm.prank(sender1); - forwarder.multiForward(tos, datas); - - require(mockReceiver.value() == _value2); - } - - /** - * @dev tests the difference between ownerForward and forward - * specifically owner can forward to link token - */ - function test_OwnerForward() public { - vm.expectRevert("Only callable by owner"); - forwarder.ownerForward(address(0), new bytes(0)); - - vm.prank(alice); - vm.expectRevert("Forwarded call reverted without reason"); - forwarder.ownerForward(address(link), new bytes(0)); - - vm.prank(alice); - forwarder.ownerForward(address(link), abi.encodeWithSignature("balanceOf(address)", address(0))); - } - - /** - * @dev Tests the behavior of transfer and accept ownership of the contract. - */ - function test_TransferOwnershipWithMessage() public { - vm.prank(bob); - vm.expectRevert("Only callable by owner"); - forwarder.transferOwnershipWithMessage(bob, new bytes(0)); - - vm.prank(alice); - forwarder.transferOwnershipWithMessage(bob, new bytes(0)); - - vm.expectRevert("Must be proposed owner"); - forwarder.acceptOwnership(); - - vm.prank(bob); - forwarder.acceptOwnership(); - - require(forwarder.owner() == bob); - } - - /** - * @dev Helper function to setup senders - */ - function _addSenders() internal { - address[] memory senders = new address[](3); - senders[0] = sender1; - senders[1] = sender2; - senders[2] = sender3; - - vm.prank(alice); - forwarder.setAuthorizedSenders(senders); - } - -} \ No newline at end of file + vm.prank(alice); + // Expect a revert because the sender list is empty + vm.expectRevert("Must have at least 1 sender"); + forwarder.setAuthorizedSenders(senders); + + // Create a list with two identical sender addresses + senders = new address[](2); + senders[0] = sender1; + senders[1] = sender1; + + vm.prank(alice); + // Expect a revert because the sender list has duplicates + vm.expectRevert("Must not have duplicate senders"); + forwarder.setAuthorizedSenders(senders); + + // Set the second sender to a different address + senders[1] = sender2; + + vm.prank(alice); + // Update the authorized senders list + forwarder.setAuthorizedSenders(senders); + + // Check if both sender1 and sender2 are now authorized + assertTrue(forwarder.isAuthorizedSender(sender1)); + assertTrue(forwarder.isAuthorizedSender(sender2)); + + // Fetch the authorized senders and verify they match the set addresses + address[] memory returnedSenders = forwarder.getAuthorizedSenders(); + require(returnedSenders[0] == senders[0]); + require(returnedSenders[1] == senders[1]); + + // Create a new list with only sender3 + senders = new address[](1); + senders[0] = sender3; + + // Prank 'alice' and update the authorized senders to just sender3 + vm.prank(alice); + forwarder.setAuthorizedSenders(senders); + + // Ensure sender1 and sender2 are no longer authorized + assertFalse(forwarder.isAuthorizedSender(sender1)); + assertFalse(forwarder.isAuthorizedSender(sender2)); + + // Check that sender3 is now the only authorized sender + assertTrue(forwarder.isAuthorizedSender(sender3)); + returnedSenders = forwarder.getAuthorizedSenders(); + require(returnedSenders[0] == senders[0]); + } + + /** + * @dev Tests the behavior of single forward + */ + function test_Forward(uint256 _value) public { + _addSenders(); + + vm.expectRevert("Not authorized sender"); + forwarder.forward(address(0), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Cannot forward to Link token"); + forwarder.forward(address(link), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Must forward to a contract"); + forwarder.forward(address(0), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.forward(address(mockReceiver), new bytes(0)); + + vm.prank(sender1); + vm.expectRevert("test revert message"); + forwarder.forward(address(mockReceiver), abi.encodeWithSignature("revertMessage()")); + + vm.prank(sender1); + forwarder.forward(address(mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); + + require(mockReceiver.value() == _value); + } + + function test_MultiForward(uint256 _value1, uint256 _value2) public { + _addSenders(); + + address[] memory tos; + bytes[] memory datas; + + vm.expectRevert("Not authorized sender"); + forwarder.multiForward(tos, datas); + + tos = new address[](2); + datas = new bytes[](1); + + vm.prank(sender1); + vm.expectRevert("Arrays must have the same length"); + forwarder.multiForward(tos, datas); + + datas = new bytes[](2); + + vm.prank(sender1); + vm.expectRevert("Must forward to a contract"); + forwarder.multiForward(tos, datas); + + tos[0] = address(mockReceiver); + tos[1] = address(link); + + vm.prank(sender1); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.multiForward(tos, datas); + + datas[0] = abi.encodeWithSignature("receiveData(uint256)", _value1); + datas[1] = abi.encodeWithSignature("receiveData(uint256)", _value2); + + vm.prank(sender1); + vm.expectRevert("Cannot forward to Link token"); + forwarder.multiForward(tos, datas); + + tos[1] = address(mockReceiver); + + vm.prank(sender1); + forwarder.multiForward(tos, datas); + + require(mockReceiver.value() == _value2); + } + + /** + * @dev tests the difference between ownerForward and forward + * specifically owner can forward to link token + */ + function test_OwnerForward() public { + vm.expectRevert("Only callable by owner"); + forwarder.ownerForward(address(0), new bytes(0)); + + vm.prank(alice); + vm.expectRevert("Forwarded call reverted without reason"); + forwarder.ownerForward(address(link), new bytes(0)); + + vm.prank(alice); + forwarder.ownerForward(address(link), abi.encodeWithSignature("balanceOf(address)", address(0))); + } + + /** + * @dev Tests the behavior of transfer and accept ownership of the contract. + */ + function test_TransferOwnershipWithMessage() public { + vm.prank(bob); + vm.expectRevert("Only callable by owner"); + forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + + vm.prank(alice); + forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + + vm.expectRevert("Must be proposed owner"); + forwarder.acceptOwnership(); + + vm.prank(bob); + forwarder.acceptOwnership(); + + require(forwarder.owner() == bob); + } + + /** + * @dev Helper function to setup senders + */ + function _addSenders() internal { + address[] memory senders = new address[](3); + senders[0] = sender1; + senders[1] = sender2; + senders[2] = sender3; + + vm.prank(alice); + forwarder.setAuthorizedSenders(senders); + } +} diff --git a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol index 80f913c9c15..c1dcc9d0f09 100644 --- a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol @@ -5,100 +5,92 @@ import "../AuthorizedReceiver.sol"; import "../Operator.sol"; contract FuzzTokenReceiver is Test { + bytes4 private constant ORACLE_REQUEST_SELECTOR = Operator.oracleRequest.selector; + bytes4 private constant OPERATOR_REQUEST_SELECTOR = Operator.operatorRequest.selector; + uint256 private constant SELECTOR_LENGTH = 4; + uint256 private constant EXPECTED_REQUEST_WORDS = 2; + uint256 private constant MINIMUM_REQUEST_LENGTH = SELECTOR_LENGTH + (32 * EXPECTED_REQUEST_WORDS); + bytes receivedData; - bytes4 private constant ORACLE_REQUEST_SELECTOR = Operator.oracleRequest.selector; - bytes4 private constant OPERATOR_REQUEST_SELECTOR = Operator.operatorRequest.selector; - uint256 private constant SELECTOR_LENGTH = 4; - uint256 private constant EXPECTED_REQUEST_WORDS = 2; - uint256 private constant MINIMUM_REQUEST_LENGTH = SELECTOR_LENGTH + (32 * EXPECTED_REQUEST_WORDS); - bytes receivedData; + address _spoofedLinkToken = address(0xCCBBAA); - address _spoofedLinkToken = address(0xCCBBAA); - - function test_FuzzOnTokenTransferFromLink( - address _actualSender, - uint256 _actualAmount, - address _sender, - uint256 _amount, - bytes32 _specId, - address _callbackAddress, - bytes4 _callbackFunc, - uint256 _nonce, - uint256 _dataVersion, - bytes calldata _dataBytes - ) public { - + function test_FuzzOnTokenTransferFromLink( + address _actualSender, + uint256 _actualAmount, + address _sender, + uint256 _amount, + bytes32 _specId, + address _callbackAddress, + bytes4 _callbackFunc, + uint256 _nonce, + uint256 _dataVersion, + bytes calldata _dataBytes + ) public { + bytes memory tokenTransferData = abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + _sender, + _amount, + _specId, + _callbackAddress, + _callbackFunc, + _nonce, + _dataVersion, + _dataBytes + ); - bytes memory tokenTransferData = abi.encodeWithSignature( - "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - _sender, - _amount, - _specId, - _callbackAddress, - _callbackFunc, - _nonce, - _dataVersion, - _dataBytes - ); + bytes memory expectedData = abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + _actualSender, + _actualAmount, + _specId, + _callbackAddress, + _callbackFunc, + _nonce, + _dataVersion, + _dataBytes + ); - bytes memory expectedData = abi.encodeWithSignature( - "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - _actualSender, - _actualAmount, - _specId, - _callbackAddress, - _callbackFunc, - _nonce, - _dataVersion, - _dataBytes - ); - - _onTokenTransfer(_actualSender, _actualAmount, tokenTransferData); + _onTokenTransfer(_actualSender, _actualAmount, tokenTransferData); - assertEq(receivedData, expectedData, "data does not match expected"); + assertEq(receivedData, expectedData, "data does not match expected"); + } + function _onTokenTransfer(address sender, uint256 amount, bytes memory data) private permittedFunctionsForLINK(data) { + assembly { + // solhint-disable-next-line avoid-low-level-calls + mstore(add(data, 36), sender) // ensure correct sender is passed + // solhint-disable-next-line avoid-low-level-calls + mstore(add(data, 68), amount) // ensure correct amount is passed0.8.19 } + // solhint-disable-next-line avoid-low-level-calls + (bool success, ) = address(this).delegatecall(data); // calls oracleRequest + require(success, "Unable to create request"); + } - function _onTokenTransfer( - address sender, - uint256 amount, - bytes memory data - ) private permittedFunctionsForLINK(data) { - assembly { - // solhint-disable-next-line avoid-low-level-calls - mstore(add(data, 36), sender) // ensure correct sender is passed - // solhint-disable-next-line avoid-low-level-calls - mstore(add(data, 68), amount) // ensure correct amount is passed0.8.19 - } - // solhint-disable-next-line avoid-low-level-calls - (bool success, ) = address(this).delegatecall(data); // calls oracleRequest - require(success, "Unable to create request"); + // @dev Reverts if the given data does not begin with the `oracleRequest` function selector + // @param data The data payload of the request + modifier permittedFunctionsForLINK(bytes memory data) { + bytes4 funcSelector; + assembly { + // solhint-disable-next-line avoid-low-level-calls + funcSelector := mload(add(data, 32)) } + _validateTokenTransferAction(funcSelector, data); + _; + } - // @dev Reverts if the given data does not begin with the `oracleRequest` function selector - // @param data The data payload of the request - modifier permittedFunctionsForLINK(bytes memory data) { - bytes4 funcSelector; - assembly { - // solhint-disable-next-line avoid-low-level-calls - funcSelector := mload(add(data, 32)) - } - _validateTokenTransferAction(funcSelector, data); - _; - } - - // @notice Require that the token transfer action is valid - // @dev OPERATOR_REQUEST_SELECTOR = multiword, ORACLE_REQUEST_SELECTOR = singleword - function _validateTokenTransferAction(bytes4 funcSelector, bytes memory data) internal pure { - require(data.length >= MINIMUM_REQUEST_LENGTH, "Invalid request length"); - require( - funcSelector == OPERATOR_REQUEST_SELECTOR || funcSelector == ORACLE_REQUEST_SELECTOR, - "Must use whitelisted functions" - ); - } + // @notice Require that the token transfer action is valid + // @dev OPERATOR_REQUEST_SELECTOR = multiword, ORACLE_REQUEST_SELECTOR = singleword + function _validateTokenTransferAction(bytes4 funcSelector, bytes memory data) internal pure { + require(data.length >= MINIMUM_REQUEST_LENGTH, "Invalid request length"); + require( + funcSelector == OPERATOR_REQUEST_SELECTOR || funcSelector == ORACLE_REQUEST_SELECTOR, + "Must use whitelisted functions" + ); + } - // Callback function for oracle request fulfillment - fallback(bytes calldata _receivedData) external returns (bytes memory) { - receivedData = _receivedData; - } -} \ No newline at end of file + // Callback function for oracle request fulfillment + fallback(bytes calldata _receivedData) external returns (bytes memory) { + receivedData = _receivedData; + } +} diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index b69f1e67b0a..de137149d95 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -135,7 +135,21 @@ contract OperatorTest is Deployer, AuthorizedReceiver { assertEq(LinkToken(s_link).balanceOf(address(alice)), 1 ether, "balance update failed"); vm.prank(alice); - LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, data)); + LinkToken(s_link).transferAndCall( + address(s_operator), + payment, + abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + address(this), + payment, + specId, + address(this), + callbackFunctionId, + nonce, + dataVersion, + data + ) + ); // Check that the LINK tokens were transferred to the Operator contract assertEq(LinkToken(s_link).balanceOf(address(s_operator)), initialLinkBalance + payment); @@ -157,9 +171,9 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // Define mock values for creating a new oracle request bytes32 specId = keccak256("testSpecForFulfill"); bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); - uint256 nonce = 1; + uint256 nonce = 1; uint256 dataVersion = 1; - bytes memory dataBytes = ""; + bytes memory dataBytes = ""; uint256 payment = 1 ether; uint256 expiration = block.timestamp + 5 minutes; @@ -169,8 +183,21 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // Send LINK tokens to the Operator contract using `transferAndCall` deal(s_link, address(bob), payment); vm.prank(bob); - LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(this), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); - + LinkToken(s_link).transferAndCall( + address(s_operator), + payment, + abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + address(this), + payment, + specId, + address(this), + callbackFunctionId, + nonce, + dataVersion, + dataBytes + ) + ); // Fulfill the request using the operator bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); @@ -191,8 +218,8 @@ contract OperatorTest is Deployer, AuthorizedReceiver { uint256 dataVersion = 1; bytes memory dataBytes = ""; uint256 payment = 1 ether; - uint256 expiration = block.timestamp + 5 minutes; - + uint256 expiration = block.timestamp + 5 minutes; + uint256 withdrawableBefore = s_operator.withdrawable(); // Convert bytes to bytes32 @@ -201,7 +228,21 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // Send LINK tokens to the Operator contract using `transferAndCall` deal(s_link, address(bob), payment); vm.prank(bob); - LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(bob), payment, specId, address(bob), callbackFunctionId, nonce, dataVersion, dataBytes)); + LinkToken(s_link).transferAndCall( + address(s_operator), + payment, + abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + address(bob), + payment, + specId, + address(bob), + callbackFunctionId, + nonce, + dataVersion, + dataBytes + ) + ); // No withdrawable balance as it's all locked assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); @@ -212,7 +253,7 @@ contract OperatorTest is Deployer, AuthorizedReceiver { vm.expectRevert(bytes("Params do not match request ID")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); vm.stopPrank(); - + vm.startPrank(bob); vm.expectRevert(bytes("Request is not expired")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); @@ -238,12 +279,33 @@ contract OperatorTest is Deployer, AuthorizedReceiver { deal(s_link, address(alice), payment); vm.prank(alice); - LinkToken(s_link).transferAndCall(address(s_operator), payment, abi.encodeWithSignature("oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", address(alice), payment, specId, address(this), callbackFunctionId, nonce, dataVersion, dataBytes)); + LinkToken(s_link).transferAndCall( + address(s_operator), + payment, + abi.encodeWithSignature( + "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", + address(alice), + payment, + specId, + address(this), + callbackFunctionId, + nonce, + dataVersion, + dataBytes + ) + ); bytes32 requestId = keccak256(abi.encodePacked(alice, nonce)); vm.prank(address(bob)); vm.expectRevert(bytes("Not authorized sender")); - s_operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, bytes32(uint256(keccak256(dataBytes)))); + s_operator.fulfillOracleRequest( + requestId, + payment, + address(this), + callbackFunctionId, + expiration, + bytes32(uint256(keccak256(dataBytes))) + ); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol index d4045ab5267..bf60af9fa48 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol @@ -12,35 +12,33 @@ import "./MockReceiver.sol"; import "../../../mocks/MockLinkToken.sol"; abstract contract Deployer is Test { + OperatorFactory factory; + Operator operator; + AuthorizedForwarder forwarder; - OperatorFactory factory; - Operator operator; - AuthorizedForwarder forwarder; + MockLinkToken public link; + MockReceiver mockReceiver; - MockLinkToken public link; - MockReceiver mockReceiver; + address owner = address(uint160(uint256(keccak256("owner")))); + address alice = address(uint160(uint256(keccak256("alice")))); + address bob = address(uint160(uint256(keccak256("bob")))); - address owner = address(uint160(uint256(keccak256("owner")))); - address alice = address(uint160(uint256(keccak256("alice")))); - address bob = address(uint160(uint256(keccak256("bob")))); + address sender1 = address(uint160(uint256(keccak256("sender1")))); + address sender2 = address(uint160(uint256(keccak256("sender2")))); + address sender3 = address(uint160(uint256(keccak256("sender3")))); - address sender1 = address(uint160(uint256(keccak256("sender1")))); - address sender2 = address(uint160(uint256(keccak256("sender2")))); - address sender3 = address(uint160(uint256(keccak256("sender3")))); + function _setUp() internal { + _deploy(); + } - function _setUp() internal { - _deploy(); - } + function _deploy() internal { + vm.startPrank(owner); - function _deploy() internal { - vm.startPrank(owner); + link = new MockLinkToken(); + factory = new OperatorFactory(address(link)); - link = new MockLinkToken(); - factory = new OperatorFactory(address(link)); + mockReceiver = new MockReceiver(); - mockReceiver = new MockReceiver(); - - vm.stopPrank(); - - } -} \ No newline at end of file + vm.stopPrank(); + } +} diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol index 107bf6d1361..b84339971d4 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol @@ -2,13 +2,13 @@ pragma solidity ^0.8.0; contract MockReceiver { - uint256 public value; + uint256 public value; - function receiveData(uint256 _value) public { - value = _value; - } + function receiveData(uint256 _value) public { + value = _value; + } - function revertMessage() pure public { - revert("test revert message"); - } -} \ No newline at end of file + function revertMessage() public pure { + revert("test revert message"); + } +} diff --git a/contracts/test/v0.8/ChainlinkClient.test.ts b/contracts/test/v0.8/ChainlinkClient.test.ts index b483e890a6b..c5691211c1a 100644 --- a/contracts/test/v0.8/ChainlinkClient.test.ts +++ b/contracts/test/v0.8/ChainlinkClient.test.ts @@ -27,15 +27,15 @@ before(async () => { roles.defaultAccount, ) emptyOracleFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/EmptyOracle.sol:EmptyOracle', + 'src/v0.8/operatorforwarder/test/testhelpers/EmptyOracle.sol:EmptyOracle', roles.defaultAccount, ) getterSetterFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/GetterSetter.sol:GetterSetter', + 'src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol:GetterSetter', roles.defaultAccount, ) operatorFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/Operator.sol:Operator', + 'src/v0.8/operatorforwarder/Operator.sol:Operator', roles.defaultAccount, ) linkTokenFactory = await ethers.getContractFactory( diff --git a/contracts/test/v0.8/operatorforwarder/AuthorizedForwarder.test.ts b/contracts/test/v0.8/operatorforwarder/AuthorizedForwarder.test.ts index 2d6329e221d..d4e1918c976 100644 --- a/contracts/test/v0.8/operatorforwarder/AuthorizedForwarder.test.ts +++ b/contracts/test/v0.8/operatorforwarder/AuthorizedForwarder.test.ts @@ -18,7 +18,7 @@ before(async () => { roles = users.roles getterSetterFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/GetterSetter.sol:GetterSetter', + 'src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol:GetterSetter', roles.defaultAccount, ) brokenFactory = await ethers.getContractFactory( @@ -26,7 +26,7 @@ before(async () => { roles.defaultAccount, ) forwarderFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol:AuthorizedForwarder', + 'src/v0.8/operatorforwarder/AuthorizedForwarder.sol:AuthorizedForwarder', roles.defaultAccount, ) linkTokenFactory = await ethers.getContractFactory( diff --git a/contracts/test/v0.8/operatorforwarder/Operator.test.ts b/contracts/test/v0.8/operatorforwarder/Operator.test.ts index 0d75d8530a4..f15701ca82a 100644 --- a/contracts/test/v0.8/operatorforwarder/Operator.test.ts +++ b/contracts/test/v0.8/operatorforwarder/Operator.test.ts @@ -50,31 +50,31 @@ before(async () => { roles = users.roles basicConsumerFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/BasicConsumer.sol:BasicConsumer', + 'src/v0.8/operatorforwarder/test/testhelpers/BasicConsumer.sol:BasicConsumer', ) multiWordConsumerFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/MultiWordConsumer.sol:MultiWordConsumer', + 'src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol:MultiWordConsumer', ) gasGuzzlingConsumerFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/GasGuzzlingConsumer.sol:GasGuzzlingConsumer', + 'src/v0.8/operatorforwarder/test/testhelpers/GasGuzzlingConsumer.sol:GasGuzzlingConsumer', ) getterSetterFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/GetterSetter.sol:GetterSetter', + 'src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol:GetterSetter', ) maliciousRequesterFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousRequester.sol:MaliciousRequester', + 'src/v0.8/operatorforwarder/test/testhelpers/MaliciousRequester.sol:MaliciousRequester', ) maliciousConsumerFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousConsumer.sol:MaliciousConsumer', + 'src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol:MaliciousConsumer', ) maliciousMultiWordConsumerFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/test/testhelpers/MaliciousMultiWordConsumer.sol:MaliciousMultiWordConsumer', + 'src/v0.8/operatorforwarder/test/testhelpers/MaliciousMultiWordConsumer.sol:MaliciousMultiWordConsumer', ) operatorFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/Operator.sol:Operator', + 'src/v0.8/operatorforwarder/Operator.sol:Operator', ) forwarderFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol:AuthorizedForwarder', + 'src/v0.8/operatorforwarder/AuthorizedForwarder.sol:AuthorizedForwarder', ) linkTokenFactory = await ethers.getContractFactory( 'src/v0.8/shared/test/helpers/LinkTokenTestHelper.sol:LinkTokenTestHelper', diff --git a/contracts/test/v0.8/operatorforwarder/OperatorFactory.test.ts b/contracts/test/v0.8/operatorforwarder/OperatorFactory.test.ts index b9a0fe508b0..b54a75c2c0d 100644 --- a/contracts/test/v0.8/operatorforwarder/OperatorFactory.test.ts +++ b/contracts/test/v0.8/operatorforwarder/OperatorFactory.test.ts @@ -20,15 +20,15 @@ before(async () => { roles.defaultAccount, ) operatorGeneratorFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/OperatorFactory.sol:OperatorFactory', + 'src/v0.8/operatorforwarder/OperatorFactory.sol:OperatorFactory', roles.defaultAccount, ) operatorFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/Operator.sol:Operator', + 'src/v0.8/operatorforwarder/Operator.sol:Operator', roles.defaultAccount, ) forwarderFactory = await ethers.getContractFactory( - 'src/v0.8/operatorforwarder/dev/AuthorizedForwarder.sol:AuthorizedForwarder', + 'src/v0.8/operatorforwarder/AuthorizedForwarder.sol:AuthorizedForwarder', roles.defaultAccount, ) }) From 8d9205cbf68ea7eb95944620d456970624a34294 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 13:00:35 -0700 Subject: [PATCH 07/15] Interface naming and test updates --- .../operatorforwarder/AuthorizedReceiver.sol | 4 +-- .../src/v0.8/operatorforwarder/Operator.sol | 12 +++---- ...rInterface.sol => IAuthorizedReceiver.sol} | 2 +- ...ithdrawalInterface.sol => IWithdrawal.sol} | 2 +- .../test/testhelpers/Deployer.sol | 32 +++++++++---------- 5 files changed, 26 insertions(+), 26 deletions(-) rename contracts/src/v0.8/operatorforwarder/interfaces/{AuthorizedReceiverInterface.sol => IAuthorizedReceiver.sol} (87%) rename contracts/src/v0.8/operatorforwarder/interfaces/{WithdrawalInterface.sol => IWithdrawal.sol} (93%) diff --git a/contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol b/contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol index b741118895f..919602b5acf 100644 --- a/contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol +++ b/contracts/src/v0.8/operatorforwarder/AuthorizedReceiver.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.19; -import {AuthorizedReceiverInterface} from "./interfaces/AuthorizedReceiverInterface.sol"; +import {IAuthorizedReceiver} from "./interfaces/IAuthorizedReceiver.sol"; // solhint-disable gas-custom-errors -abstract contract AuthorizedReceiver is AuthorizedReceiverInterface { +abstract contract AuthorizedReceiver is IAuthorizedReceiver { mapping(address sender => bool authorized) private s_authorizedSenders; address[] private s_authorizedSenderList; diff --git a/contracts/src/v0.8/operatorforwarder/Operator.sol b/contracts/src/v0.8/operatorforwarder/Operator.sol index d18c62a6f9f..64882e43cda 100644 --- a/contracts/src/v0.8/operatorforwarder/Operator.sol +++ b/contracts/src/v0.8/operatorforwarder/Operator.sol @@ -5,17 +5,17 @@ import {AuthorizedReceiver} from "./AuthorizedReceiver.sol"; import {LinkTokenReceiver} from "./LinkTokenReceiver.sol"; import {ConfirmedOwner} from "../shared/access/ConfirmedOwner.sol"; import {LinkTokenInterface} from "../shared/interfaces/LinkTokenInterface.sol"; -import {AuthorizedReceiverInterface} from "./interfaces/AuthorizedReceiverInterface.sol"; +import {IAuthorizedReceiver} from "./interfaces/IAuthorizedReceiver.sol"; import {OperatorInterface} from "../interfaces/OperatorInterface.sol"; import {IOwnable} from "../shared/interfaces/IOwnable.sol"; -import {WithdrawalInterface} from "./interfaces/WithdrawalInterface.sol"; +import {IWithdrawal} from "./interfaces/IWithdrawal.sol"; import {OracleInterface} from "../interfaces/OracleInterface.sol"; import {SafeCast} from "../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/math/SafeCast.sol"; // @title The Chainlink Operator contract // @notice Node operators can deploy this contract to fulfill requests sent to them // solhint-disable gas-custom-errors -contract Operator is AuthorizedReceiver, ConfirmedOwner, LinkTokenReceiver, OperatorInterface, WithdrawalInterface { +contract Operator is AuthorizedReceiver, ConfirmedOwner, LinkTokenReceiver, OperatorInterface, IWithdrawal { struct Commitment { bytes31 paramsHash; uint8 dataVersion; @@ -241,7 +241,7 @@ contract Operator is AuthorizedReceiver, ConfirmedOwner, LinkTokenReceiver, Oper emit TargetsUpdatedAuthorizedSenders(targets, senders, msg.sender); for (uint256 i = 0; i < targets.length; ++i) { - AuthorizedReceiverInterface(targets[i]).setAuthorizedSenders(senders); + IAuthorizedReceiver(targets[i]).setAuthorizedSenders(senders); } } @@ -266,14 +266,14 @@ contract Operator is AuthorizedReceiver, ConfirmedOwner, LinkTokenReceiver, Oper function withdraw( address recipient, uint256 amount - ) external override(OracleInterface, WithdrawalInterface) onlyOwner validateAvailableFunds(amount) { + ) external override(OracleInterface, IWithdrawal) onlyOwner validateAvailableFunds(amount) { assert(i_linkToken.transfer(recipient, amount)); } // @notice Displays the amount of LINK that is available for the node operator to withdraw // @dev We use `ONE_FOR_CONSISTENT_GAS_COST` in place of 0 in storage // @return The amount of withdrawable LINK on the contract - function withdrawable() external view override(OracleInterface, WithdrawalInterface) returns (uint256) { + function withdrawable() external view override(OracleInterface, IWithdrawal) returns (uint256) { return _fundsAvailable(); } diff --git a/contracts/src/v0.8/operatorforwarder/interfaces/AuthorizedReceiverInterface.sol b/contracts/src/v0.8/operatorforwarder/interfaces/IAuthorizedReceiver.sol similarity index 87% rename from contracts/src/v0.8/operatorforwarder/interfaces/AuthorizedReceiverInterface.sol rename to contracts/src/v0.8/operatorforwarder/interfaces/IAuthorizedReceiver.sol index 28b20b14f33..78140d58682 100644 --- a/contracts/src/v0.8/operatorforwarder/interfaces/AuthorizedReceiverInterface.sol +++ b/contracts/src/v0.8/operatorforwarder/interfaces/IAuthorizedReceiver.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -interface AuthorizedReceiverInterface { +interface IAuthorizedReceiver { function isAuthorizedSender(address sender) external view returns (bool); function getAuthorizedSenders() external returns (address[] memory); diff --git a/contracts/src/v0.8/operatorforwarder/interfaces/WithdrawalInterface.sol b/contracts/src/v0.8/operatorforwarder/interfaces/IWithdrawal.sol similarity index 93% rename from contracts/src/v0.8/operatorforwarder/interfaces/WithdrawalInterface.sol rename to contracts/src/v0.8/operatorforwarder/interfaces/IWithdrawal.sol index c064b0627b5..433738d406f 100644 --- a/contracts/src/v0.8/operatorforwarder/interfaces/WithdrawalInterface.sol +++ b/contracts/src/v0.8/operatorforwarder/interfaces/IWithdrawal.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -interface WithdrawalInterface { +interface IWithdrawal { // @notice transfer LINK held by the contract belonging to msg.sender to // another address // @param recipient is the address to send the LINK to diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol index bf60af9fa48..3a0374f77e5 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol @@ -1,31 +1,31 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; -import "forge-std/Test.sol"; +import {Test} from "forge-std/Test.sol"; -import "../../AuthorizedForwarder.sol"; -import "../../Operator.sol"; -import "../../OperatorFactory.sol"; +import {AuthorizedForwarder} from "../../AuthorizedForwarder.sol"; +import {Operator} from "../../Operator.sol"; +import {OperatorFactory} from "../../OperatorFactory.sol"; -import "./MockReceiver.sol"; +import {MockReceiver} from "./MockReceiver.sol"; -import "../../../mocks/MockLinkToken.sol"; +import {MockLinkToken} from "../../../mocks/MockLinkToken.sol"; abstract contract Deployer is Test { - OperatorFactory factory; - Operator operator; - AuthorizedForwarder forwarder; + OperatorFactory public factory; + Operator public operator; + AuthorizedForwarder public forwarder; MockLinkToken public link; - MockReceiver mockReceiver; + MockReceiver public mockReceiver; - address owner = address(uint160(uint256(keccak256("owner")))); - address alice = address(uint160(uint256(keccak256("alice")))); - address bob = address(uint160(uint256(keccak256("bob")))); + address public owner = address(uint160(uint256(keccak256("owner")))); + address public alice = address(uint160(uint256(keccak256("alice")))); + address public bob = address(uint160(uint256(keccak256("bob")))); - address sender1 = address(uint160(uint256(keccak256("sender1")))); - address sender2 = address(uint160(uint256(keccak256("sender2")))); - address sender3 = address(uint160(uint256(keccak256("sender3")))); + address public sender1 = address(uint160(uint256(keccak256("sender1")))); + address public sender2 = address(uint160(uint256(keccak256("sender2")))); + address public sender3 = address(uint160(uint256(keccak256("sender3")))); function _setUp() internal { _deploy(); From 194ada1c1f2c5bd2e5a7ff605217caca309a4e6f Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 25 Apr 2024 14:07:07 -0700 Subject: [PATCH 08/15] Update Deployer.sol => Deployer.t.sol --- contracts/src/v0.8/operatorforwarder/test/Factory.t.sol | 2 +- contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol | 2 +- contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol | 2 +- contracts/src/v0.8/operatorforwarder/test/operator.t.sol | 2 +- .../test/testhelpers/{Deployer.sol => Deployer.t.sol} | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename contracts/src/v0.8/operatorforwarder/test/testhelpers/{Deployer.sol => Deployer.t.sol} (100%) diff --git a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index c93874b2637..70425340154 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "./testhelpers/Deployer.sol"; +import "./testhelpers/Deployer.t.sol"; contract FactoryTest is Deployer { function setUp() public { diff --git a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index 939c18bad23..5b80f6644fc 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "./testhelpers/Deployer.sol"; +import "./testhelpers/Deployer.t.sol"; contract ForwarderTest is Deployer { function setUp() public { diff --git a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol index c1dcc9d0f09..5c258058058 100644 --- a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol @@ -1,6 +1,6 @@ pragma solidity ^0.8.19; -import "./testhelpers/Deployer.sol"; +import "./testhelpers/Deployer.t.sol"; import "../AuthorizedReceiver.sol"; import "../Operator.sol"; diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index de137149d95..5503e3d3d34 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -6,7 +6,7 @@ import {Operator} from "../Operator.sol"; import {ChainlinkClientHelper} from "./testhelpers/ChainlinkClientHelper.sol"; import {LinkToken} from "../../shared/token/ERC677/LinkToken.sol"; -import "./testhelpers/Deployer.sol"; +import "./testhelpers/Deployer.t.sol"; import "../AuthorizedReceiver.sol"; contract OperatorTest is Deployer, AuthorizedReceiver { diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol similarity index 100% rename from contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.sol rename to contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol From e7e7862412d6b17914a43940313caa0bd81607ae Mon Sep 17 00:00:00 2001 From: Austin Born Date: Wed, 1 May 2024 07:00:43 -0700 Subject: [PATCH 09/15] Rework OperatorForwarder tests --- .github/workflows/solidity-foundry.yml | 2 +- contracts/GNUmakefile | 2 +- .../v0.8/operatorforwarder/test/Factory.t.sol | 38 +++-- .../operatorforwarder/test/Forwarder.t.sol | 147 ++++++++--------- .../operatorforwarder/test/FuzzOperator.t.sol | 96 ----------- .../operatorforwarder/test/operator.t.sol | 152 ++++++++---------- .../test/testhelpers/Callback.sol | 21 +++ .../testhelpers/ChainlinkClientHelper.sol | 4 +- .../test/testhelpers/Consumer.sol | 2 +- .../test/testhelpers/Deployer.t.sol | 37 ++--- .../test/testhelpers/GetterSetter.sol | 8 +- .../test/testhelpers/MaliciousChainlinked.sol | 28 ++-- .../test/testhelpers/MaliciousConsumer.sol | 6 +- .../test/testhelpers/MockReceiver.sol | 8 +- .../test/testhelpers/MultiWordConsumer.sol | 14 +- .../v0.8/operatorforwarder/Operator.test.ts | 4 +- 16 files changed, 237 insertions(+), 332 deletions(-) delete mode 100644 contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol create mode 100644 contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol diff --git a/.github/workflows/solidity-foundry.yml b/.github/workflows/solidity-foundry.yml index 2d62ef864a5..5e2d95ea9d1 100644 --- a/.github/workflows/solidity-foundry.yml +++ b/.github/workflows/solidity-foundry.yml @@ -31,7 +31,7 @@ jobs: strategy: fail-fast: false matrix: - product: [vrf, automation, llo-feeds, l2ep, functions, keystone, shared] + product: [automation, functions, keystone, l2ep, llo-feeds, operatorforwarder, shared, vrf] needs: [changes] name: Foundry Tests ${{ matrix.product }} # See https://github.com/foundry-rs/foundry/issues/3827 diff --git a/contracts/GNUmakefile b/contracts/GNUmakefile index 4ec8057b975..ba11b396e89 100644 --- a/contracts/GNUmakefile +++ b/contracts/GNUmakefile @@ -1,6 +1,6 @@ # ALL_FOUNDRY_PRODUCTS contains a list of all products that have a foundry # profile defined and use the Foundry snapshots. -ALL_FOUNDRY_PRODUCTS = l2ep llo-feeds functions keystone shared transmission +ALL_FOUNDRY_PRODUCTS = functions keystone l2ep llo-feeds operatorforwarder shared transmission # To make a snapshot for a specific product, either set the `FOUNDRY_PROFILE` env var # or call the target with `FOUNDRY_PROFILE=product` diff --git a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index 70425340154..d8489b61693 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -8,37 +8,37 @@ import "./testhelpers/Deployer.t.sol"; contract FactoryTest is Deployer { function setUp() public { _setUp(); + + vm.startPrank(s_alice); } /** * @dev Test the deployment of a new operator. */ function test_DeployNewOperator() public { - vm.prank(alice); // Deploy a new operator using the factory. - address newOperator = factory.deployNewOperator(); + address newOperator = s_factory.deployNewOperator(); // Assert that the new operator was indeed created by the factory. - assertTrue(factory.created(newOperator)); + assertTrue(s_factory.created(newOperator)); // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == alice); + require(Operator(newOperator).owner() == s_alice); } /** * @dev Test the deployment of a new operator and a new forwarder. */ function test_DeployNewOperatorAndForwarder() public { - vm.prank(alice); // Deploy both a new operator and a new forwarder using the factory. - (address newOperator, address newForwarder) = factory.deployNewOperatorAndForwarder(); + (address newOperator, address newForwarder) = s_factory.deployNewOperatorAndForwarder(); // Assert that the new operator and the new forwarder were indeed created by the factory. - assertTrue(factory.created(newOperator)); - assertTrue(factory.created(newForwarder)); + assertTrue(s_factory.created(newOperator)); + assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == alice); + require(Operator(newOperator).owner() == s_alice); //Operator to accept ownership - vm.prank(newOperator); + vm.startPrank(newOperator); AuthorizedForwarder(newForwarder).acceptOwnership(); // Ensure that the newly deployed operator is the owner of the newly deployed forwarder. @@ -49,35 +49,33 @@ contract FactoryTest is Deployer { * @dev Test the deployment of a new forwarder. */ function test_DeployNewForwarder() public { - vm.prank(alice); // Deploy a new forwarder using the factory. - address newForwarder = factory.deployNewForwarder(); + address newForwarder = s_factory.deployNewForwarder(); // Assert that the new forwarder was indeed created by the factory. - assertTrue(factory.created(newForwarder)); + assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is the owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == alice); + require(AuthorizedForwarder(newForwarder).owner() == s_alice); } /** * @dev Test the deployment of a new forwarder and then transfer its ownership. */ function test_DeployNewForwarderAndTransferOwnership() public { - vm.prank(alice); // Deploy a new forwarder with a proposal to transfer its ownership to Bob. - address newForwarder = factory.deployNewForwarderAndTransferOwnership(bob, new bytes(0)); + address newForwarder = s_factory.deployNewForwarderAndTransferOwnership(s_bob, new bytes(0)); // Assert that the new forwarder was indeed created by the factory. - assertTrue(factory.created(newForwarder)); + assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is still the current owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == alice); + require(AuthorizedForwarder(newForwarder).owner() == s_alice); // Only proposed owner can call acceptOwnership() vm.expectRevert("Must be proposed owner"); AuthorizedForwarder(newForwarder).acceptOwnership(); - vm.prank(bob); + vm.startPrank(s_bob); // Let Bob accept the ownership. AuthorizedForwarder(newForwarder).acceptOwnership(); // Ensure that Bob is now the owner of the forwarder after the transfer. - require(AuthorizedForwarder(newForwarder).owner() == bob); + require(AuthorizedForwarder(newForwarder).owner() == s_bob); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index 5b80f6644fc..65d6a09abc3 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -4,13 +4,16 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; import "./testhelpers/Deployer.t.sol"; +import {AuthorizedForwarder} from "../AuthorizedForwarder.sol"; contract ForwarderTest is Deployer { + AuthorizedForwarder internal s_forwarder; + function setUp() public { _setUp(); - vm.prank(alice); - forwarder = AuthorizedForwarder(factory.deployNewForwarder()); + vm.prank(s_alice); + s_forwarder = AuthorizedForwarder(s_factory.deployNewForwarder()); } /** @@ -21,54 +24,54 @@ contract ForwarderTest is Deployer { // Expect a revert when trying to set an empty list of authorized senders vm.expectRevert("Cannot set authorized senders"); - forwarder.setAuthorizedSenders(senders); + s_forwarder.setAuthorizedSenders(senders); - vm.prank(alice); + vm.prank(s_alice); // Expect a revert because the sender list is empty vm.expectRevert("Must have at least 1 sender"); - forwarder.setAuthorizedSenders(senders); + s_forwarder.setAuthorizedSenders(senders); // Create a list with two identical sender addresses senders = new address[](2); - senders[0] = sender1; - senders[1] = sender1; + senders[0] = s_sender1; + senders[1] = s_sender1; - vm.prank(alice); + vm.prank(s_alice); // Expect a revert because the sender list has duplicates vm.expectRevert("Must not have duplicate senders"); - forwarder.setAuthorizedSenders(senders); + s_forwarder.setAuthorizedSenders(senders); // Set the second sender to a different address - senders[1] = sender2; + senders[1] = s_sender2; - vm.prank(alice); + vm.prank(s_alice); // Update the authorized senders list - forwarder.setAuthorizedSenders(senders); + s_forwarder.setAuthorizedSenders(senders); - // Check if both sender1 and sender2 are now authorized - assertTrue(forwarder.isAuthorizedSender(sender1)); - assertTrue(forwarder.isAuthorizedSender(sender2)); + // Check if both s_sender1 and s_sender2 are now authorized + assertTrue(s_forwarder.isAuthorizedSender(s_sender1)); + assertTrue(s_forwarder.isAuthorizedSender(s_sender2)); // Fetch the authorized senders and verify they match the set addresses - address[] memory returnedSenders = forwarder.getAuthorizedSenders(); + address[] memory returnedSenders = s_forwarder.getAuthorizedSenders(); require(returnedSenders[0] == senders[0]); require(returnedSenders[1] == senders[1]); - // Create a new list with only sender3 + // Create a new list with only s_sender3 senders = new address[](1); - senders[0] = sender3; + senders[0] = s_sender3; - // Prank 'alice' and update the authorized senders to just sender3 - vm.prank(alice); - forwarder.setAuthorizedSenders(senders); + // Prank 'alice' and update the authorized senders to just s_sender3 + vm.prank(s_alice); + s_forwarder.setAuthorizedSenders(senders); - // Ensure sender1 and sender2 are no longer authorized - assertFalse(forwarder.isAuthorizedSender(sender1)); - assertFalse(forwarder.isAuthorizedSender(sender2)); + // Ensure s_sender1 and s_sender2 are no longer authorized + assertFalse(s_forwarder.isAuthorizedSender(s_sender1)); + assertFalse(s_forwarder.isAuthorizedSender(s_sender2)); - // Check that sender3 is now the only authorized sender - assertTrue(forwarder.isAuthorizedSender(sender3)); - returnedSenders = forwarder.getAuthorizedSenders(); + // Check that s_sender3 is now the only authorized sender + assertTrue(s_forwarder.isAuthorizedSender(s_sender3)); + returnedSenders = s_forwarder.getAuthorizedSenders(); require(returnedSenders[0] == senders[0]); } @@ -79,28 +82,28 @@ contract ForwarderTest is Deployer { _addSenders(); vm.expectRevert("Not authorized sender"); - forwarder.forward(address(0), new bytes(0)); + s_forwarder.forward(address(0), new bytes(0)); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Cannot forward to Link token"); - forwarder.forward(address(link), new bytes(0)); + s_forwarder.forward(address(s_link), new bytes(0)); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Must forward to a contract"); - forwarder.forward(address(0), new bytes(0)); + s_forwarder.forward(address(0), new bytes(0)); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Forwarded call reverted without reason"); - forwarder.forward(address(mockReceiver), new bytes(0)); + s_forwarder.forward(address(s_mockReceiver), new bytes(0)); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("test revert message"); - forwarder.forward(address(mockReceiver), abi.encodeWithSignature("revertMessage()")); + s_forwarder.forward(address(s_mockReceiver), abi.encodeWithSignature("revertMessage()")); - vm.prank(sender1); - forwarder.forward(address(mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); + vm.prank(s_sender1); + s_forwarder.forward(address(s_mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); - require(mockReceiver.value() == _value); + require(s_mockReceiver.getValue() == _value); } function test_MultiForward(uint256 _value1, uint256 _value2) public { @@ -110,41 +113,41 @@ contract ForwarderTest is Deployer { bytes[] memory datas; vm.expectRevert("Not authorized sender"); - forwarder.multiForward(tos, datas); + s_forwarder.multiForward(tos, datas); tos = new address[](2); datas = new bytes[](1); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Arrays must have the same length"); - forwarder.multiForward(tos, datas); + s_forwarder.multiForward(tos, datas); datas = new bytes[](2); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Must forward to a contract"); - forwarder.multiForward(tos, datas); + s_forwarder.multiForward(tos, datas); - tos[0] = address(mockReceiver); - tos[1] = address(link); + tos[0] = address(s_mockReceiver); + tos[1] = address(s_link); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Forwarded call reverted without reason"); - forwarder.multiForward(tos, datas); + s_forwarder.multiForward(tos, datas); datas[0] = abi.encodeWithSignature("receiveData(uint256)", _value1); datas[1] = abi.encodeWithSignature("receiveData(uint256)", _value2); - vm.prank(sender1); + vm.prank(s_sender1); vm.expectRevert("Cannot forward to Link token"); - forwarder.multiForward(tos, datas); + s_forwarder.multiForward(tos, datas); - tos[1] = address(mockReceiver); + tos[1] = address(s_mockReceiver); - vm.prank(sender1); - forwarder.multiForward(tos, datas); + vm.prank(s_sender1); + s_forwarder.multiForward(tos, datas); - require(mockReceiver.value() == _value2); + require(s_mockReceiver.getValue() == _value2); } /** @@ -153,34 +156,34 @@ contract ForwarderTest is Deployer { */ function test_OwnerForward() public { vm.expectRevert("Only callable by owner"); - forwarder.ownerForward(address(0), new bytes(0)); + s_forwarder.ownerForward(address(0), new bytes(0)); - vm.prank(alice); + vm.prank(s_alice); vm.expectRevert("Forwarded call reverted without reason"); - forwarder.ownerForward(address(link), new bytes(0)); + s_forwarder.ownerForward(address(s_link), new bytes(0)); - vm.prank(alice); - forwarder.ownerForward(address(link), abi.encodeWithSignature("balanceOf(address)", address(0))); + vm.prank(s_alice); + s_forwarder.ownerForward(address(s_link), abi.encodeWithSignature("balanceOf(address)", address(0))); } /** * @dev Tests the behavior of transfer and accept ownership of the contract. */ function test_TransferOwnershipWithMessage() public { - vm.prank(bob); + vm.prank(s_bob); vm.expectRevert("Only callable by owner"); - forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + s_forwarder.transferOwnershipWithMessage(s_bob, new bytes(0)); - vm.prank(alice); - forwarder.transferOwnershipWithMessage(bob, new bytes(0)); + vm.prank(s_alice); + s_forwarder.transferOwnershipWithMessage(s_bob, new bytes(0)); vm.expectRevert("Must be proposed owner"); - forwarder.acceptOwnership(); + s_forwarder.acceptOwnership(); - vm.prank(bob); - forwarder.acceptOwnership(); + vm.prank(s_bob); + s_forwarder.acceptOwnership(); - require(forwarder.owner() == bob); + require(s_forwarder.owner() == s_bob); } /** @@ -188,11 +191,11 @@ contract ForwarderTest is Deployer { */ function _addSenders() internal { address[] memory senders = new address[](3); - senders[0] = sender1; - senders[1] = sender2; - senders[2] = sender3; + senders[0] = s_sender1; + senders[1] = s_sender2; + senders[2] = s_sender3; - vm.prank(alice); - forwarder.setAuthorizedSenders(senders); + vm.prank(s_alice); + s_forwarder.setAuthorizedSenders(senders); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol b/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol deleted file mode 100644 index 5c258058058..00000000000 --- a/contracts/src/v0.8/operatorforwarder/test/FuzzOperator.t.sol +++ /dev/null @@ -1,96 +0,0 @@ -pragma solidity ^0.8.19; - -import "./testhelpers/Deployer.t.sol"; -import "../AuthorizedReceiver.sol"; -import "../Operator.sol"; - -contract FuzzTokenReceiver is Test { - bytes4 private constant ORACLE_REQUEST_SELECTOR = Operator.oracleRequest.selector; - bytes4 private constant OPERATOR_REQUEST_SELECTOR = Operator.operatorRequest.selector; - uint256 private constant SELECTOR_LENGTH = 4; - uint256 private constant EXPECTED_REQUEST_WORDS = 2; - uint256 private constant MINIMUM_REQUEST_LENGTH = SELECTOR_LENGTH + (32 * EXPECTED_REQUEST_WORDS); - bytes receivedData; - - address _spoofedLinkToken = address(0xCCBBAA); - - function test_FuzzOnTokenTransferFromLink( - address _actualSender, - uint256 _actualAmount, - address _sender, - uint256 _amount, - bytes32 _specId, - address _callbackAddress, - bytes4 _callbackFunc, - uint256 _nonce, - uint256 _dataVersion, - bytes calldata _dataBytes - ) public { - bytes memory tokenTransferData = abi.encodeWithSignature( - "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - _sender, - _amount, - _specId, - _callbackAddress, - _callbackFunc, - _nonce, - _dataVersion, - _dataBytes - ); - - bytes memory expectedData = abi.encodeWithSignature( - "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - _actualSender, - _actualAmount, - _specId, - _callbackAddress, - _callbackFunc, - _nonce, - _dataVersion, - _dataBytes - ); - - _onTokenTransfer(_actualSender, _actualAmount, tokenTransferData); - - assertEq(receivedData, expectedData, "data does not match expected"); - } - - function _onTokenTransfer(address sender, uint256 amount, bytes memory data) private permittedFunctionsForLINK(data) { - assembly { - // solhint-disable-next-line avoid-low-level-calls - mstore(add(data, 36), sender) // ensure correct sender is passed - // solhint-disable-next-line avoid-low-level-calls - mstore(add(data, 68), amount) // ensure correct amount is passed0.8.19 - } - // solhint-disable-next-line avoid-low-level-calls - (bool success, ) = address(this).delegatecall(data); // calls oracleRequest - require(success, "Unable to create request"); - } - - // @dev Reverts if the given data does not begin with the `oracleRequest` function selector - // @param data The data payload of the request - modifier permittedFunctionsForLINK(bytes memory data) { - bytes4 funcSelector; - assembly { - // solhint-disable-next-line avoid-low-level-calls - funcSelector := mload(add(data, 32)) - } - _validateTokenTransferAction(funcSelector, data); - _; - } - - // @notice Require that the token transfer action is valid - // @dev OPERATOR_REQUEST_SELECTOR = multiword, ORACLE_REQUEST_SELECTOR = singleword - function _validateTokenTransferAction(bytes4 funcSelector, bytes memory data) internal pure { - require(data.length >= MINIMUM_REQUEST_LENGTH, "Invalid request length"); - require( - funcSelector == OPERATOR_REQUEST_SELECTOR || funcSelector == ORACLE_REQUEST_SELECTOR, - "Must use whitelisted functions" - ); - } - - // Callback function for oracle request fulfillment - fallback(bytes calldata _receivedData) external returns (bytes memory) { - receivedData = _receivedData; - } -} diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index 5503e3d3d34..b6cd43d975f 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -3,46 +3,30 @@ pragma solidity 0.8.19; import {Test} from "forge-std/Test.sol"; import {Operator} from "../Operator.sol"; +import {Callback} from "./testhelpers/Callback.sol"; import {ChainlinkClientHelper} from "./testhelpers/ChainlinkClientHelper.sol"; -import {LinkToken} from "../../shared/token/ERC677/LinkToken.sol"; +import {Deployer} from "./testhelpers/Deployer.t.sol"; -import "./testhelpers/Deployer.t.sol"; -import "../AuthorizedReceiver.sol"; - -contract OperatorTest is Deployer, AuthorizedReceiver { - address public s_link; - uint256 private dataReceived; - ChainlinkClientHelper public s_client; - Operator public s_operator; +contract OperatorTest is Deployer { + ChainlinkClientHelper private s_client; + Callback private s_callback; + Operator private s_operator; function setUp() public { _setUp(); - s_link = address(new LinkToken()); - s_client = new ChainlinkClientHelper(s_link); + s_client = new ChainlinkClientHelper(address(s_link)); address[] memory auth = new address[](1); auth[0] = address(this); - s_operator = new Operator(s_link, address(this)); + s_operator = new Operator(address(s_link), address(this)); s_operator.setAuthorizedSenders(auth); - dataReceived = 0; - } - - // Callback function for oracle request fulfillment - function callback(bytes32 _requestId) public { - require(msg.sender == address(s_operator), "Only Operator can call this function"); - dataReceived += 1; - } - - // @notice concrete implementation of AuthorizedReceiver - // @return bool of whether sender is authorized - function _canSetAuthorizedSenders() internal view override returns (bool) { - return true; + s_callback = new Callback(address(s_operator)); } - function test_Success(uint96 payment) public { - payment = uint96(bound(payment, 1, type(uint96).max)); - deal(s_link, address(s_client), payment); + function test_sendRequest(uint96 payment) public { + vm.assume(payment > 0); + deal(address(s_link), address(s_client), payment); // We're going to cancel one request and fulfill the other bytes32 requestIdToCancel = s_client.sendRequest(address(s_operator), payment); @@ -50,8 +34,8 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // 1 payment in escrow // Client has zero link assertEq(s_operator.withdrawable(), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), payment); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), 0); + assertEq(s_link.balanceOf(address(s_operator)), payment); + assertEq(s_link.balanceOf(address(s_client)), 0); // Advance time so we can cancel uint256 expiration = block.timestamp + s_operator.EXPIRYTIME(); @@ -60,20 +44,22 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // 1 payment has been returned due to the cancellation. assertEq(s_operator.withdrawable(), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), payment); + assertEq(s_link.balanceOf(address(s_operator)), 0); + assertEq(s_link.balanceOf(address(s_client)), payment); } function test_afterSuccessfulRequestSucess(uint96 payment) public { - payment = uint96(bound(payment, 1, type(uint96).max) / 2); - deal(s_link, address(s_client), 2 * payment); + vm.assume(payment > 1); + payment /= payment; + + deal(address(s_link), address(s_client), 2 * payment); // Initial state, client has 2 payments, zero in escrow, zero in the operator, zeero withdrawable assertEq(s_operator.withdrawable(), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), 2 * payment); + assertEq(s_link.balanceOf(address(s_operator)), 0); + assertEq(s_link.balanceOf(address(s_client)), 2 * payment); - // We're going to cancel one request and fulfil the other + // We're going to cancel one request and fulfill the other bytes32 requestId = s_client.sendRequest(address(s_operator), payment); bytes32 requestIdToCancel = s_client.sendRequest(address(s_operator), payment); @@ -81,8 +67,8 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // Operator now has the 2 payments in escrow // Client has zero payments assertEq(s_operator.withdrawable(), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 2 * payment); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), 0); + assertEq(s_link.balanceOf(address(s_operator)), 2 * payment); + assertEq(s_link.balanceOf(address(s_client)), 0); // Fulfill one request uint256 expiration = block.timestamp + s_operator.EXPIRYTIME(); @@ -90,14 +76,14 @@ contract OperatorTest is Deployer, AuthorizedReceiver { requestId, payment, address(s_client), - s_client.FULFILSELECTOR(), + s_client.FULFILL_SELECTOR(), expiration, bytes32(hex"01") ); // 1 payment withdrawable from fulfilling `requestId`, 1 payment in escrow assertEq(s_operator.withdrawable(), payment); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 2 * payment); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), 0); + assertEq(s_link.balanceOf(address(s_operator)), 2 * payment); + assertEq(s_link.balanceOf(address(s_client)), 0); // Advance time so we can cancel vm.warp(expiration + 1); @@ -105,16 +91,16 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // 1 payment has been returned due to the cancellation, 1 payment should be withdrawable assertEq(s_operator.withdrawable(), payment); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), payment); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), payment); + assertEq(s_link.balanceOf(address(s_operator)), payment); + assertEq(s_link.balanceOf(address(s_client)), payment); // Withdraw the remaining payment s_operator.withdraw(address(s_client), payment); // End state is exactly the same as the initial state. assertEq(s_operator.withdrawable(), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), 0); - assertEq(LinkToken(s_link).balanceOf(address(s_client)), 2 * payment); + assertEq(s_link.balanceOf(address(s_operator)), 0); + assertEq(s_link.balanceOf(address(s_client)), 2 * payment); } function test_oracleRequestFlow() public { @@ -125,17 +111,17 @@ contract OperatorTest is Deployer, AuthorizedReceiver { uint256 dataVersion = 1; bytes memory data = ""; - uint256 initialLinkBalance = LinkToken(s_link).balanceOf(address(s_operator)); + uint256 initialLinkBalance = s_link.balanceOf(address(s_operator)); uint256 payment = 1 ether; // Mock payment value uint256 withdrawableBefore = s_operator.withdrawable(); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(s_link, address(alice), payment); - assertEq(LinkToken(s_link).balanceOf(address(alice)), 1 ether, "balance update failed"); + deal(address(s_link), s_alice, payment); + assertEq(s_link.balanceOf(s_alice), 1 ether, "balance update failed"); - vm.prank(alice); - LinkToken(s_link).transferAndCall( + vm.prank(s_alice); + s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( @@ -143,7 +129,7 @@ contract OperatorTest is Deployer, AuthorizedReceiver { address(this), payment, specId, - address(this), + address(s_callback), callbackFunctionId, nonce, dataVersion, @@ -152,7 +138,7 @@ contract OperatorTest is Deployer, AuthorizedReceiver { ); // Check that the LINK tokens were transferred to the Operator contract - assertEq(LinkToken(s_link).balanceOf(address(s_operator)), initialLinkBalance + payment); + assertEq(s_link.balanceOf(address(s_operator)), initialLinkBalance + payment); // No withdrawable LINK as it's all locked assertEq(s_operator.withdrawable(), withdrawableBefore); } @@ -162,7 +148,7 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // so we should enable it to set Authorised senders to itself address[] memory senders = new address[](2); senders[0] = address(this); - senders[0] = address(bob); + senders[0] = s_bob; s_operator.setAuthorizedSenders(senders); @@ -178,12 +164,12 @@ contract OperatorTest is Deployer, AuthorizedReceiver { uint256 expiration = block.timestamp + 5 minutes; // Convert bytes to bytes32 - bytes32 data = bytes32(uint256(keccak256(dataBytes))); + bytes32 data = bytes32(keccak256(dataBytes)); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(s_link, address(bob), payment); - vm.prank(bob); - LinkToken(s_link).transferAndCall( + deal(address(s_link), s_bob, payment); + vm.prank(s_bob); + s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( @@ -191,7 +177,7 @@ contract OperatorTest is Deployer, AuthorizedReceiver { address(this), payment, specId, - address(this), + address(s_callback), callbackFunctionId, nonce, dataVersion, @@ -200,11 +186,11 @@ contract OperatorTest is Deployer, AuthorizedReceiver { ); // Fulfill the request using the operator - bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); - vm.prank(bob); - s_operator.fulfillOracleRequest(requestId, payment, address(this), callbackFunctionId, expiration, data); + bytes32 requestId = keccak256(abi.encodePacked(s_bob, nonce)); + vm.prank(s_bob); + s_operator.fulfillOracleRequest(requestId, payment, address(s_callback), callbackFunctionId, expiration, data); - assertEq(dataReceived, 1, "Oracle request was not fulfilled"); + assertEq(s_callback.getCallbacksReceived(), 1, "Oracle request was not fulfilled"); // Withdrawable balance assertEq(s_operator.withdrawable(), withdrawableBefore + payment, "Internal accounting not updated correctly"); @@ -223,20 +209,20 @@ contract OperatorTest is Deployer, AuthorizedReceiver { uint256 withdrawableBefore = s_operator.withdrawable(); // Convert bytes to bytes32 - bytes32 data = bytes32(uint256(keccak256(dataBytes))); + bytes32 data = bytes32(keccak256(dataBytes)); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(s_link, address(bob), payment); - vm.prank(bob); - LinkToken(s_link).transferAndCall( + deal(address(s_link), s_bob, payment); + vm.prank(s_bob); + s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - address(bob), + s_bob, payment, specId, - address(bob), + s_bob, callbackFunctionId, nonce, dataVersion, @@ -247,23 +233,21 @@ contract OperatorTest is Deployer, AuthorizedReceiver { // No withdrawable balance as it's all locked assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); - bytes32 requestId = keccak256(abi.encodePacked(bob, nonce)); + bytes32 requestId = keccak256(abi.encodePacked(s_bob, nonce)); - vm.startPrank(alice); + vm.startPrank(s_alice); vm.expectRevert(bytes("Params do not match request ID")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - vm.stopPrank(); - vm.startPrank(bob); + vm.startPrank(s_bob); vm.expectRevert(bytes("Request is not expired")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); vm.warp(expiration); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - vm.stopPrank(); // Check if the LINK tokens were refunded to the sender (bob in this case) - assertEq(LinkToken(s_link).balanceOf(address(bob)), 1 ether, "Oracle request was not canceled properly"); + assertEq(s_link.balanceOf(s_bob), 1 ether, "Oracle request was not canceled properly"); assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); } @@ -277,17 +261,17 @@ contract OperatorTest is Deployer, AuthorizedReceiver { uint256 payment = 1 ether; uint256 expiration = block.timestamp + 5 minutes; - deal(s_link, address(alice), payment); - vm.prank(alice); - LinkToken(s_link).transferAndCall( + deal(address(s_link), s_alice, payment); + vm.prank(s_alice); + s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - address(alice), + s_alice, payment, specId, - address(this), + address(s_callback), callbackFunctionId, nonce, dataVersion, @@ -295,17 +279,17 @@ contract OperatorTest is Deployer, AuthorizedReceiver { ) ); - bytes32 requestId = keccak256(abi.encodePacked(alice, nonce)); + bytes32 requestId = keccak256(abi.encodePacked(s_alice, nonce)); - vm.prank(address(bob)); + vm.prank(s_bob); vm.expectRevert(bytes("Not authorized sender")); s_operator.fulfillOracleRequest( requestId, payment, - address(this), + address(s_callback), callbackFunctionId, expiration, - bytes32(uint256(keccak256(dataBytes))) + bytes32(keccak256(dataBytes)) ); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol new file mode 100644 index 00000000000..0f62c4fbc44 --- /dev/null +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract Callback { + address private operator; + uint256 private callbacksReceived = 0; + + constructor(address _operator) { + operator = _operator; + } + + // Callback function for oracle request fulfillment + function callback(bytes32) public { + require(msg.sender == operator, "Only Operator can call this function"); + callbacksReceived += 1; + } + + function getCallbacksReceived() public view returns (uint256) { + return callbacksReceived; + } +} diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol index 37acf3bd5df..9b6ba6bb432 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/ChainlinkClientHelper.sol @@ -4,14 +4,14 @@ pragma solidity ^0.8.0; import {ChainlinkClient} from "../../../ChainlinkClient.sol"; contract ChainlinkClientHelper is ChainlinkClient { - bytes4 public constant FULFILSELECTOR = this.fulfill.selector; + bytes4 public constant FULFILL_SELECTOR = this.fulfill.selector; constructor(address link) { _setChainlinkToken(link); } function sendRequest(address op, uint256 payment) external returns (bytes32) { - return _sendChainlinkRequestTo(op, _buildOperatorRequest(bytes32(hex"10"), FULFILSELECTOR), payment); + return _sendChainlinkRequestTo(op, _buildOperatorRequest(bytes32(hex"10"), FULFILL_SELECTOR), payment); } function cancelRequest(bytes32 requestId, uint256 payment, uint256 expiration) external { diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol index 3b62353b83c..da8e4a6e4a9 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol @@ -8,7 +8,7 @@ contract Consumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes32 public currentPrice; + bytes32 private currentPrice; event RequestFulfilled( bytes32 indexed requestId, // User-defined ID diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol index 3a0374f77e5..31dcfb9c3b2 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol @@ -3,42 +3,33 @@ pragma solidity ^0.8.0; import {Test} from "forge-std/Test.sol"; +import {MockReceiver} from "./MockReceiver.sol"; import {AuthorizedForwarder} from "../../AuthorizedForwarder.sol"; import {Operator} from "../../Operator.sol"; import {OperatorFactory} from "../../OperatorFactory.sol"; - -import {MockReceiver} from "./MockReceiver.sol"; - -import {MockLinkToken} from "../../../mocks/MockLinkToken.sol"; +import {LinkToken} from "../../../shared/token/ERC677/LinkToken.sol"; abstract contract Deployer is Test { - OperatorFactory public factory; - Operator public operator; - AuthorizedForwarder public forwarder; + OperatorFactory internal s_factory; + LinkToken internal s_link; + MockReceiver internal s_mockReceiver; - MockLinkToken public link; - MockReceiver public mockReceiver; + address private s_owner = makeAddr("owner"); + address internal s_alice = makeAddr("alice"); + address internal s_bob = makeAddr("bob"); - address public owner = address(uint160(uint256(keccak256("owner")))); - address public alice = address(uint160(uint256(keccak256("alice")))); - address public bob = address(uint160(uint256(keccak256("bob")))); - - address public sender1 = address(uint160(uint256(keccak256("sender1")))); - address public sender2 = address(uint160(uint256(keccak256("sender2")))); - address public sender3 = address(uint160(uint256(keccak256("sender3")))); + address public s_sender1 = makeAddr("sender1"); + address public s_sender2 = makeAddr("sender2"); + address public s_sender3 = makeAddr("sender3"); function _setUp() internal { _deploy(); } function _deploy() internal { - vm.startPrank(owner); - - link = new MockLinkToken(); - factory = new OperatorFactory(address(link)); - - mockReceiver = new MockReceiver(); + s_link = new LinkToken(); + s_factory = new OperatorFactory(address(s_link)); - vm.stopPrank(); + s_mockReceiver = new MockReceiver(); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol index 494da582e1b..a61405f05a5 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol @@ -3,10 +3,10 @@ pragma solidity ^0.8.0; // GetterSetter is a contract to aid debugging and testing during development. // solhint-disable contract GetterSetter { - bytes32 public getBytes32; - uint256 public getUint256; - bytes32 public requestId; - bytes public getBytes; + bytes32 private getBytes32; + uint256 private getUint256; + bytes32 private requestId; + bytes private getBytes; event SetBytes32(address indexed from, bytes32 indexed value); event SetUint256(address indexed from, uint256 indexed value); diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol index 8ca02c34c62..989c39c18be 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousChainlinked.sol @@ -10,8 +10,8 @@ contract MaliciousChainlinked is Chainlinked { using MaliciousChainlink for MaliciousChainlink.WithdrawRequest; using Chainlink for Chainlink.Request; - uint256 private maliciousRequests = 1; - mapping(bytes32 => address) private maliciousPendingRequests; + uint256 private s_maliciousRequests = 1; + mapping(bytes32 => address) private s_maliciousPendingRequests; function newWithdrawRequest( bytes32 _specId, @@ -27,31 +27,31 @@ contract MaliciousChainlinked is Chainlinked { Chainlink.Request memory _req, uint256 _amount ) internal returns (bytes32 requestId) { - requestId = keccak256(abi.encodePacked(_target, maliciousRequests)); - _req.nonce = maliciousRequests; - maliciousPendingRequests[requestId] = oracleAddress(); + requestId = keccak256(abi.encodePacked(_target, s_maliciousRequests)); + _req.nonce = s_maliciousRequests; + s_maliciousPendingRequests[requestId] = oracleAddress(); emit ChainlinkRequested(requestId); LinkTokenInterface link = LinkTokenInterface(chainlinkToken()); require( link.transferAndCall(oracleAddress(), _amount, encodeTargetRequest(_req)), "Unable to transferAndCall to oracle" ); - maliciousRequests += 1; + s_maliciousRequests += 1; return requestId; } function chainlinkPriceRequest(Chainlink.Request memory _req, uint256 _amount) internal returns (bytes32 requestId) { - requestId = keccak256(abi.encodePacked(this, maliciousRequests)); - _req.nonce = maliciousRequests; - maliciousPendingRequests[requestId] = oracleAddress(); + requestId = keccak256(abi.encodePacked(this, s_maliciousRequests)); + _req.nonce = s_maliciousRequests; + s_maliciousPendingRequests[requestId] = oracleAddress(); emit ChainlinkRequested(requestId); LinkTokenInterface link = LinkTokenInterface(chainlinkToken()); require( link.transferAndCall(oracleAddress(), _amount, encodePriceRequest(_req)), "Unable to transferAndCall to oracle" ); - maliciousRequests += 1; + s_maliciousRequests += 1; return requestId; } @@ -60,16 +60,16 @@ contract MaliciousChainlinked is Chainlinked { MaliciousChainlink.WithdrawRequest memory _req, uint256 _wei ) internal returns (bytes32 requestId) { - requestId = keccak256(abi.encodePacked(this, maliciousRequests)); - _req.nonce = maliciousRequests; - maliciousPendingRequests[requestId] = oracleAddress(); + requestId = keccak256(abi.encodePacked(this, s_maliciousRequests)); + _req.nonce = s_maliciousRequests; + s_maliciousPendingRequests[requestId] = oracleAddress(); emit ChainlinkRequested(requestId); LinkTokenInterface link = LinkTokenInterface(chainlinkToken()); require( link.transferAndCall(oracleAddress(), _wei, encodeWithdrawRequest(_req)), "Unable to transferAndCall to oracle" ); - maliciousRequests += 1; + s_maliciousRequests += 1; return requestId; } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol index 003e628880f..842eec90542 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MaliciousConsumer.sol @@ -5,7 +5,7 @@ import {Chainlinked, Chainlink} from "./Chainlinked.sol"; // solhint-disable contract MaliciousConsumer is Chainlinked { uint256 private constant ORACLE_PAYMENT = 1 ether; - uint256 private expiration; + uint256 private s_expiration; constructor(address _link, address _oracle) public payable { setLinkToken(_link); @@ -16,7 +16,7 @@ contract MaliciousConsumer is Chainlinked { function requestData(bytes32 _id, bytes memory _callbackFunc) public { Chainlink.Request memory req = newRequest(_id, address(this), bytes4(keccak256(_callbackFunc))); - expiration = block.timestamp + 5 minutes; + s_expiration = block.timestamp + 5 minutes; chainlinkRequest(req, ORACLE_PAYMENT); } @@ -25,7 +25,7 @@ contract MaliciousConsumer is Chainlinked { } function cancelRequestOnFulfill(bytes32 _requestId, bytes32) public { - _cancelChainlinkRequest(_requestId, ORACLE_PAYMENT, this.cancelRequestOnFulfill.selector, expiration); + _cancelChainlinkRequest(_requestId, ORACLE_PAYMENT, this.cancelRequestOnFulfill.selector, s_expiration); } function remove() public { diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol index b84339971d4..4e825b4505f 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MockReceiver.sol @@ -2,13 +2,17 @@ pragma solidity ^0.8.0; contract MockReceiver { - uint256 public value; + uint256 private s_value; function receiveData(uint256 _value) public { - value = _value; + s_value = _value; } function revertMessage() public pure { revert("test revert message"); } + + function getValue() external view returns (uint256) { + return s_value; + } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol index 95575c41a78..267e31a2d9b 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol @@ -7,15 +7,15 @@ contract MultiWordConsumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes public currentPrice; + bytes private currentPrice; - bytes32 public usd; - bytes32 public eur; - bytes32 public jpy; + bytes32 private usd; + bytes32 private eur; + bytes32 private jpy; - uint256 public usdInt; - uint256 public eurInt; - uint256 public jpyInt; + uint256 private usdInt; + uint256 private eurInt; + uint256 private jpyInt; event RequestFulfilled( bytes32 indexed requestId, // User-defined ID diff --git a/contracts/test/v0.8/operatorforwarder/Operator.test.ts b/contracts/test/v0.8/operatorforwarder/Operator.test.ts index f15701ca82a..1ef7d127b7a 100644 --- a/contracts/test/v0.8/operatorforwarder/Operator.test.ts +++ b/contracts/test/v0.8/operatorforwarder/Operator.test.ts @@ -1419,7 +1419,7 @@ describe('Operator', () => { }) describe('#fulfillOracleRequest2', () => { - describe('single word fulfils', () => { + describe('single word fulfills', () => { const response = 'Hi mom!' const responseTypes = ['bytes32'] const responseValues = [toBytes32String(response)] @@ -2024,7 +2024,7 @@ describe('Operator', () => { }) }) - describe('multi word fulfils', () => { + describe('multi word fulfills', () => { describe('one bytes parameter', () => { const response = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.\ From 0b1d3e5608af57bd21ec7c1449d9b06bed2cb6f6 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Wed, 1 May 2024 07:04:50 -0700 Subject: [PATCH 10/15] Update gas snapshot --- .../operatorforwarder.gas-snapshot | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index 2ffcc3d261a..938f34d26da 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -1,16 +1,15 @@ -FactoryTest:test_DeployNewForwarder() (gas: 1050792) -FactoryTest:test_DeployNewForwarderAndTransferOwnership() (gas: 1064683) -FactoryTest:test_DeployNewOperator() (gas: 3023070) -FactoryTest:test_DeployNewOperatorAndForwarder() (gas: 4071878) -ForwarderTest:test_Forward(uint256) (runs: 256, μ: 235183, ~: 236272) -ForwarderTest:test_MultiForward(uint256,uint256) (runs: 256, μ: 266882, ~: 268126) -ForwarderTest:test_OwnerForward() (gas: 32267) -ForwarderTest:test_SetAuthorizedSenders() (gas: 168077) -ForwarderTest:test_TransferOwnershipWithMessage() (gas: 38863) -FuzzTokenReceiver:test_FuzzOnTokenTransferFromLink(address,uint256,address,uint256,bytes32,address,bytes4,uint256,uint256,bytes) (runs: 256, μ: 272414, ~: 277681) -OperatorTest:test_Success(uint96) (runs: 256, μ: 306108, ~: 306102) -OperatorTest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 385291, ~: 389533) -OperatorTest:test_cancelOracleRequest() (gas: 278994) -OperatorTest:test_fulfillOracleRequest() (gas: 326881) -OperatorTest:test_oracleRequestFlow() (gas: 250165) -OperatorTest:test_unauthorizedFulfillment() (gas: 248625) \ No newline at end of file +FactoryTest:test_DeployNewForwarder() (gas: 1050322) +FactoryTest:test_DeployNewForwarderAndTransferOwnership() (gas: 1064241) +FactoryTest:test_DeployNewOperator() (gas: 3022623) +FactoryTest:test_DeployNewOperatorAndForwarder() (gas: 4071431) +ForwarderTest:test_Forward(uint256) (runs: 256, μ: 235167, ~: 236256) +ForwarderTest:test_MultiForward(uint256,uint256) (runs: 256, μ: 266833, ~: 268077) +ForwarderTest:test_OwnerForward() (gas: 32320) +ForwarderTest:test_SetAuthorizedSenders() (gas: 168060) +ForwarderTest:test_TransferOwnershipWithMessage() (gas: 38891) +OperatorTest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 387086, ~: 387090) +OperatorTest:test_cancelOracleRequest() (gas: 278413) +OperatorTest:test_fulfillOracleRequest() (gas: 332649) +OperatorTest:test_oracleRequestFlow() (gas: 251875) +OperatorTest:test_sendRequest(uint96) (runs: 256, μ: 303612, ~: 303620) +OperatorTest:test_unauthorizedFulfillment() (gas: 250438) \ No newline at end of file From ed6e023b99008a305bc20e471df966fb387e851d Mon Sep 17 00:00:00 2001 From: Austin Born Date: Wed, 1 May 2024 07:09:01 -0700 Subject: [PATCH 11/15] prettier --- .../test/testhelpers/Callback.sol | 12 ++++---- .../test/testhelpers/Consumer.sol | 4 +-- .../test/testhelpers/MultiWordConsumer.sol | 28 +++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol index 0f62c4fbc44..9dccfed428a 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Callback.sol @@ -2,20 +2,20 @@ pragma solidity ^0.8.0; contract Callback { - address private operator; - uint256 private callbacksReceived = 0; + address private s_operator; + uint256 private s_callbacksReceived = 0; constructor(address _operator) { - operator = _operator; + s_operator = _operator; } // Callback function for oracle request fulfillment function callback(bytes32) public { - require(msg.sender == operator, "Only Operator can call this function"); - callbacksReceived += 1; + require(msg.sender == s_operator, "Only Operator can call this function"); + s_callbacksReceived += 1; } function getCallbacksReceived() public view returns (uint256) { - return callbacksReceived; + return s_callbacksReceived; } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol index da8e4a6e4a9..2f0d79a1d16 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol @@ -8,7 +8,7 @@ contract Consumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes32 private currentPrice; + bytes32 private s_currentPrice; event RequestFulfilled( bytes32 indexed requestId, // User-defined ID @@ -50,6 +50,6 @@ contract Consumer is ChainlinkClient { function fulfill(bytes32 _requestId, bytes32 _price) public recordChainlinkFulfillment(_requestId) { emit RequestFulfilled(_requestId, _price); - currentPrice = _price; + s_currentPrice = _price; } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol index 267e31a2d9b..957527fbbf4 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol @@ -7,15 +7,15 @@ contract MultiWordConsumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes private currentPrice; + bytes private s_currentPrice; - bytes32 private usd; - bytes32 private eur; - bytes32 private jpy; + bytes32 private s_usd; + bytes32 private s_eur; + bytes32 private s_jpy; - uint256 private usdInt; - uint256 private eurInt; - uint256 private jpyInt; + uint256 private s_usdInt; + uint256 private s_eurInt; + uint256 private s_jpyInt; event RequestFulfilled( bytes32 indexed requestId, // User-defined ID @@ -100,9 +100,9 @@ contract MultiWordConsumer is ChainlinkClient { bytes32 _jpy ) public recordChainlinkFulfillment(_requestId) { emit RequestMultipleFulfilled(_requestId, _usd, _eur, _jpy); - usd = _usd; - eur = _eur; - jpy = _jpy; + s_usd = _usd; + s_eur = _eur; + s_jpy = _jpy; } function fulfillMultipleParametersWithCustomURLs( @@ -112,14 +112,14 @@ contract MultiWordConsumer is ChainlinkClient { uint256 _jpy ) public recordChainlinkFulfillment(_requestId) { emit RequestMultipleFulfilledWithCustomURLs(_requestId, _usd, _eur, _jpy); - usdInt = _usd; - eurInt = _eur; - jpyInt = _jpy; + s_usdInt = _usd; + s_eurInt = _eur; + s_jpyInt = _jpy; } function fulfillBytes(bytes32 _requestId, bytes memory _price) public recordChainlinkFulfillment(_requestId) { emit RequestFulfilled(_requestId, _price); - currentPrice = _price; + s_currentPrice = _price; } function publicGetNextRequestCount() external view returns (uint256) { From 701540df735711ad8e5ab320c812e3b8fad592f6 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Wed, 1 May 2024 08:35:50 -0700 Subject: [PATCH 12/15] Fix Hardhat tests --- .../test/testhelpers/Consumer.sol | 6 ++++- .../test/testhelpers/GetterSetter.sol | 24 +++++++++++-------- .../test/testhelpers/MultiWordConsumer.sol | 18 +++++++++++++- .../v0.8/operatorforwarder/Operator.test.ts | 12 +++++----- 4 files changed, 42 insertions(+), 18 deletions(-) diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol index 2f0d79a1d16..82709d3def8 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Consumer.sol @@ -8,7 +8,7 @@ contract Consumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes32 private s_currentPrice; + bytes32 internal s_currentPrice; event RequestFulfilled( bytes32 indexed requestId, // User-defined ID @@ -52,4 +52,8 @@ contract Consumer is ChainlinkClient { emit RequestFulfilled(_requestId, _price); s_currentPrice = _price; } + + function getCurrentPrice() public view returns (bytes32) { + return s_currentPrice; + } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol index a61405f05a5..722362bc4aa 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/GetterSetter.sol @@ -3,10 +3,10 @@ pragma solidity ^0.8.0; // GetterSetter is a contract to aid debugging and testing during development. // solhint-disable contract GetterSetter { - bytes32 private getBytes32; - uint256 private getUint256; - bytes32 private requestId; - bytes private getBytes; + bytes32 private s_getBytes32; + uint256 private s_getUint256; + bytes32 private s_requestId; + bytes private s_getBytes; event SetBytes32(address indexed from, bytes32 indexed value); event SetUint256(address indexed from, uint256 indexed value); @@ -15,32 +15,36 @@ contract GetterSetter { event Output(bytes32 b32, uint256 u256, bytes32 b322); function setBytes32(bytes32 _value) public { - getBytes32 = _value; + s_getBytes32 = _value; emit SetBytes32(msg.sender, _value); } function requestedBytes32(bytes32 _requestId, bytes32 _value) public { - requestId = _requestId; + s_requestId = _requestId; setBytes32(_value); } function setBytes(bytes memory _value) public { - getBytes = _value; + s_getBytes = _value; emit SetBytes(msg.sender, _value); } + function getBytes() public view returns (bytes memory _value) { + return s_getBytes; + } + function requestedBytes(bytes32 _requestId, bytes memory _value) public { - requestId = _requestId; + s_requestId = _requestId; setBytes(_value); } function setUint256(uint256 _value) public { - getUint256 = _value; + s_getUint256 = _value; emit SetUint256(msg.sender, _value); } function requestedUint256(bytes32 _requestId, uint256 _value) public { - requestId = _requestId; + s_requestId = _requestId; setUint256(_value); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol index 957527fbbf4..fe249831fef 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/MultiWordConsumer.sol @@ -7,7 +7,7 @@ contract MultiWordConsumer is ChainlinkClient { using Chainlink for Chainlink.Request; bytes32 internal s_specId; - bytes private s_currentPrice; + bytes internal s_currentPrice; bytes32 private s_usd; bytes32 private s_eur; @@ -125,4 +125,20 @@ contract MultiWordConsumer is ChainlinkClient { function publicGetNextRequestCount() external view returns (uint256) { return _getNextRequestCount(); } + + function getCurrentPrice() public view returns (bytes memory _value) { + return s_currentPrice; + } + + function usd() public view returns (bytes32 _value) { + return s_usd; + } + + function eur() public view returns (bytes32 _value) { + return s_eur; + } + + function jpy() public view returns (bytes32 _value) { + return s_jpy; + } } diff --git a/contracts/test/v0.8/operatorforwarder/Operator.test.ts b/contracts/test/v0.8/operatorforwarder/Operator.test.ts index 1ef7d127b7a..6fb8768f54a 100644 --- a/contracts/test/v0.8/operatorforwarder/Operator.test.ts +++ b/contracts/test/v0.8/operatorforwarder/Operator.test.ts @@ -1076,7 +1076,7 @@ describe('Operator', () => { .connect(roles.oracleNode) .fulfillOracleRequest(...convertFufillParams(request, response)) - const currentValue = await basicConsumer.currentPrice() + const currentValue = await basicConsumer.getCurrentPrice() assert.equal(response, ethers.utils.parseBytes32String(currentValue)) }) @@ -1105,7 +1105,7 @@ describe('Operator', () => { .fulfillOracleRequest(...convertFufillParams(request, response2)), ) - const currentValue = await basicConsumer.currentPrice() + const currentValue = await basicConsumer.getCurrentPrice() assert.equal(response, ethers.utils.parseBytes32String(currentValue)) }) }) @@ -1528,7 +1528,7 @@ describe('Operator', () => { ), ) - const currentValue = await basicConsumer.currentPrice() + const currentValue = await basicConsumer.getCurrentPrice() assert.equal( response, ethers.utils.parseBytes32String(currentValue), @@ -1576,7 +1576,7 @@ describe('Operator', () => { ), ) - const currentValue = await basicConsumer.currentPrice() + const currentValue = await basicConsumer.getCurrentPrice() assert.equal( response, ethers.utils.parseBytes32String(currentValue), @@ -2149,7 +2149,7 @@ describe('Operator', () => { ), ) - const currentValue = await multiConsumer.currentPrice() + const currentValue = await multiConsumer.getCurrentPrice() assert.equal(response, ethers.utils.toUtf8String(currentValue)) }) @@ -2195,7 +2195,7 @@ describe('Operator', () => { ), ) - const currentValue = await multiConsumer.currentPrice() + const currentValue = await multiConsumer.getCurrentPrice() assert.equal(response, ethers.utils.toUtf8String(currentValue)) }) }) From 8cf21c55722b81120108fd1f5f748c7c0b96b9b4 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 2 May 2024 01:14:28 -0700 Subject: [PATCH 13/15] More style maintenance --- .../operatorforwarder.gas-snapshot | 30 +++++++++---------- .../v0.8/operatorforwarder/test/Factory.t.sol | 26 +++++----------- .../operatorforwarder/test/Forwarder.t.sol | 30 ++++--------------- .../operatorforwarder/test/operator.t.sol | 16 ++++------ 4 files changed, 34 insertions(+), 68 deletions(-) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index 938f34d26da..504acd0bc7c 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -1,15 +1,15 @@ -FactoryTest:test_DeployNewForwarder() (gas: 1050322) -FactoryTest:test_DeployNewForwarderAndTransferOwnership() (gas: 1064241) -FactoryTest:test_DeployNewOperator() (gas: 3022623) -FactoryTest:test_DeployNewOperatorAndForwarder() (gas: 4071431) -ForwarderTest:test_Forward(uint256) (runs: 256, μ: 235167, ~: 236256) -ForwarderTest:test_MultiForward(uint256,uint256) (runs: 256, μ: 266833, ~: 268077) -ForwarderTest:test_OwnerForward() (gas: 32320) -ForwarderTest:test_SetAuthorizedSenders() (gas: 168060) -ForwarderTest:test_TransferOwnershipWithMessage() (gas: 38891) -OperatorTest:test_afterSuccessfulRequestSucess(uint96) (runs: 256, μ: 387086, ~: 387090) -OperatorTest:test_cancelOracleRequest() (gas: 278413) -OperatorTest:test_fulfillOracleRequest() (gas: 332649) -OperatorTest:test_oracleRequestFlow() (gas: 251875) -OperatorTest:test_sendRequest(uint96) (runs: 256, μ: 303612, ~: 303620) -OperatorTest:test_unauthorizedFulfillment() (gas: 250438) \ No newline at end of file +FactoryTest:test_DeployNewForwarderAndTransferOwnership_Success() (gas: 1064232) +FactoryTest:test_DeployNewForwarder_Success() (gas: 1050356) +FactoryTest:test_DeployNewOperatorAndForwarder_Success() (gas: 4071429) +FactoryTest:test_DeployNewOperator_Success() (gas: 3022611) +ForwarderTest:test_Forward_Success(uint256) (runs: 256, μ: 235165, ~: 236254) +ForwarderTest:test_MultiForward_Success(uint256,uint256) (runs: 256, μ: 266877, ~: 268121) +ForwarderTest:test_OwnerForward_Success() (gas: 32343) +ForwarderTest:test_SetAuthorizedSenders_Success() (gas: 168079) +ForwarderTest:test_TransferOwnershipWithMessage_Success() (gas: 38909) +OperatorTest:test_CancelOracleRequest_Success() (gas: 278388) +OperatorTest:test_FulfillOracleRequest_Success() (gas: 332668) +OperatorTest:test_NotAuthorizedSender_Revert() (gas: 250404) +OperatorTest:test_OracleRequest_Success() (gas: 251892) +OperatorTest:test_SendRequestAndCancelRequest_Success(uint96) (runs: 256, μ: 387102, ~: 387107) +OperatorTest:test_SendRequest_Success(uint96) (runs: 256, μ: 303593, ~: 303601) \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index d8489b61693..1785138b7bf 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; -import "forge-std/Test.sol"; - -import "./testhelpers/Deployer.t.sol"; +import {Deployer} from "./testhelpers/Deployer.t.sol"; +import {AuthorizedForwarder} from "../AuthorizedForwarder.sol"; +import {Operator} from "../Operator.sol"; contract FactoryTest is Deployer { function setUp() public { @@ -12,10 +12,7 @@ contract FactoryTest is Deployer { vm.startPrank(s_alice); } - /** - * @dev Test the deployment of a new operator. - */ - function test_DeployNewOperator() public { + function test_DeployNewOperator_Success() public { // Deploy a new operator using the factory. address newOperator = s_factory.deployNewOperator(); // Assert that the new operator was indeed created by the factory. @@ -24,10 +21,7 @@ contract FactoryTest is Deployer { require(Operator(newOperator).owner() == s_alice); } - /** - * @dev Test the deployment of a new operator and a new forwarder. - */ - function test_DeployNewOperatorAndForwarder() public { + function test_DeployNewOperatorAndForwarder_Success() public { // Deploy both a new operator and a new forwarder using the factory. (address newOperator, address newForwarder) = s_factory.deployNewOperatorAndForwarder(); @@ -45,10 +39,7 @@ contract FactoryTest is Deployer { require(AuthorizedForwarder(newForwarder).owner() == newOperator, "operator is not the owner"); } - /** - * @dev Test the deployment of a new forwarder. - */ - function test_DeployNewForwarder() public { + function test_DeployNewForwarder_Success() public { // Deploy a new forwarder using the factory. address newForwarder = s_factory.deployNewForwarder(); // Assert that the new forwarder was indeed created by the factory. @@ -57,10 +48,7 @@ contract FactoryTest is Deployer { require(AuthorizedForwarder(newForwarder).owner() == s_alice); } - /** - * @dev Test the deployment of a new forwarder and then transfer its ownership. - */ - function test_DeployNewForwarderAndTransferOwnership() public { + function test_DeployNewForwarderAndTransferOwnership_Success() public { // Deploy a new forwarder with a proposal to transfer its ownership to Bob. address newForwarder = s_factory.deployNewForwarderAndTransferOwnership(s_bob, new bytes(0)); // Assert that the new forwarder was indeed created by the factory. diff --git a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index 65d6a09abc3..b2b5de479b0 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -1,9 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; -import "forge-std/Test.sol"; - -import "./testhelpers/Deployer.t.sol"; +import {Deployer} from "./testhelpers/Deployer.t.sol"; import {AuthorizedForwarder} from "../AuthorizedForwarder.sol"; contract ForwarderTest is Deployer { @@ -16,10 +14,7 @@ contract ForwarderTest is Deployer { s_forwarder = AuthorizedForwarder(s_factory.deployNewForwarder()); } - /** - * @dev Tests the functionality of setting authorized senders. - */ - function test_SetAuthorizedSenders() public { + function test_SetAuthorizedSenders_Success() public { address[] memory senders; // Expect a revert when trying to set an empty list of authorized senders @@ -75,10 +70,7 @@ contract ForwarderTest is Deployer { require(returnedSenders[0] == senders[0]); } - /** - * @dev Tests the behavior of single forward - */ - function test_Forward(uint256 _value) public { + function test_Forward_Success(uint256 _value) public { _addSenders(); vm.expectRevert("Not authorized sender"); @@ -106,7 +98,7 @@ contract ForwarderTest is Deployer { require(s_mockReceiver.getValue() == _value); } - function test_MultiForward(uint256 _value1, uint256 _value2) public { + function test_MultiForward_Success(uint256 _value1, uint256 _value2) public { _addSenders(); address[] memory tos; @@ -150,11 +142,7 @@ contract ForwarderTest is Deployer { require(s_mockReceiver.getValue() == _value2); } - /** - * @dev tests the difference between ownerForward and forward - * specifically owner can forward to link token - */ - function test_OwnerForward() public { + function test_OwnerForward_Success() public { vm.expectRevert("Only callable by owner"); s_forwarder.ownerForward(address(0), new bytes(0)); @@ -166,10 +154,7 @@ contract ForwarderTest is Deployer { s_forwarder.ownerForward(address(s_link), abi.encodeWithSignature("balanceOf(address)", address(0))); } - /** - * @dev Tests the behavior of transfer and accept ownership of the contract. - */ - function test_TransferOwnershipWithMessage() public { + function test_TransferOwnershipWithMessage_Success() public { vm.prank(s_bob); vm.expectRevert("Only callable by owner"); s_forwarder.transferOwnershipWithMessage(s_bob, new bytes(0)); @@ -186,9 +171,6 @@ contract ForwarderTest is Deployer { require(s_forwarder.owner() == s_bob); } - /** - * @dev Helper function to setup senders - */ function _addSenders() internal { address[] memory senders = new address[](3); senders[0] = s_sender1; diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index b6cd43d975f..e3c9a2a11df 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.19; -import {Test} from "forge-std/Test.sol"; import {Operator} from "../Operator.sol"; import {Callback} from "./testhelpers/Callback.sol"; import {ChainlinkClientHelper} from "./testhelpers/ChainlinkClientHelper.sol"; @@ -24,7 +23,7 @@ contract OperatorTest is Deployer { s_callback = new Callback(address(s_operator)); } - function test_sendRequest(uint96 payment) public { + function test_SendRequest_Success(uint96 payment) public { vm.assume(payment > 0); deal(address(s_link), address(s_client), payment); // We're going to cancel one request and fulfill the other @@ -48,7 +47,7 @@ contract OperatorTest is Deployer { assertEq(s_link.balanceOf(address(s_client)), payment); } - function test_afterSuccessfulRequestSucess(uint96 payment) public { + function test_SendRequestAndCancelRequest_Success(uint96 payment) public { vm.assume(payment > 1); payment /= payment; @@ -103,7 +102,7 @@ contract OperatorTest is Deployer { assertEq(s_link.balanceOf(address(s_client)), 2 * payment); } - function test_oracleRequestFlow() public { + function test_OracleRequest_Success() public { // Define some mock values bytes32 specId = keccak256("testSpec"); bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); @@ -143,7 +142,7 @@ contract OperatorTest is Deployer { assertEq(s_operator.withdrawable(), withdrawableBefore); } - function test_fulfillOracleRequest() public { + function test_FulfillOracleRequest_Success() public { // This test file is the callback target and actual sender contract // so we should enable it to set Authorised senders to itself address[] memory senders = new address[](2); @@ -196,7 +195,7 @@ contract OperatorTest is Deployer { assertEq(s_operator.withdrawable(), withdrawableBefore + payment, "Internal accounting not updated correctly"); } - function test_cancelOracleRequest() public { + function test_CancelOracleRequest_Success() public { // Define mock values for creating a new oracle request bytes32 specId = keccak256("testSpecForCancel"); bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); @@ -208,9 +207,6 @@ contract OperatorTest is Deployer { uint256 withdrawableBefore = s_operator.withdrawable(); - // Convert bytes to bytes32 - bytes32 data = bytes32(keccak256(dataBytes)); - // Send LINK tokens to the Operator contract using `transferAndCall` deal(address(s_link), s_bob, payment); vm.prank(s_bob); @@ -252,7 +248,7 @@ contract OperatorTest is Deployer { assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); } - function test_unauthorizedFulfillment() public { + function test_NotAuthorizedSender_Revert() public { bytes32 specId = keccak256("unauthorizedFulfillSpec"); bytes4 callbackFunctionId = bytes4(keccak256("callback(bytes32)")); uint256 nonce = 5; From 92af97378cd112b36fb26b73efd71e39e90df3d0 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 2 May 2024 01:22:11 -0700 Subject: [PATCH 14/15] Update constant variables in Deployer.t.sol --- .../operatorforwarder.gas-snapshot | 30 +++---- .../v0.8/operatorforwarder/test/Factory.t.sol | 16 ++-- .../operatorforwarder/test/Forwarder.t.sol | 82 +++++++++---------- .../operatorforwarder/test/operator.t.sol | 42 +++++----- .../test/testhelpers/Deployer.t.sol | 12 ++- 5 files changed, 90 insertions(+), 92 deletions(-) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index 504acd0bc7c..f4e6306e768 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -1,15 +1,15 @@ -FactoryTest:test_DeployNewForwarderAndTransferOwnership_Success() (gas: 1064232) -FactoryTest:test_DeployNewForwarder_Success() (gas: 1050356) -FactoryTest:test_DeployNewOperatorAndForwarder_Success() (gas: 4071429) -FactoryTest:test_DeployNewOperator_Success() (gas: 3022611) -ForwarderTest:test_Forward_Success(uint256) (runs: 256, μ: 235165, ~: 236254) -ForwarderTest:test_MultiForward_Success(uint256,uint256) (runs: 256, μ: 266877, ~: 268121) -ForwarderTest:test_OwnerForward_Success() (gas: 32343) -ForwarderTest:test_SetAuthorizedSenders_Success() (gas: 168079) -ForwarderTest:test_TransferOwnershipWithMessage_Success() (gas: 38909) -OperatorTest:test_CancelOracleRequest_Success() (gas: 278388) -OperatorTest:test_FulfillOracleRequest_Success() (gas: 332668) -OperatorTest:test_NotAuthorizedSender_Revert() (gas: 250404) -OperatorTest:test_OracleRequest_Success() (gas: 251892) -OperatorTest:test_SendRequestAndCancelRequest_Success(uint96) (runs: 256, μ: 387102, ~: 387107) -OperatorTest:test_SendRequest_Success(uint96) (runs: 256, μ: 303593, ~: 303601) \ No newline at end of file +FactoryTest:test_DeployNewForwarderAndTransferOwnership_Success() (gas: 1059722) +FactoryTest:test_DeployNewForwarder_Success() (gas: 1048209) +FactoryTest:test_DeployNewOperatorAndForwarder_Success() (gas: 4069305) +FactoryTest:test_DeployNewOperator_Success() (gas: 3020464) +ForwarderTest:test_Forward_Success(uint256) (runs: 256, μ: 226200, ~: 227289) +ForwarderTest:test_MultiForward_Success(uint256,uint256) (runs: 256, μ: 257876, ~: 259120) +ForwarderTest:test_OwnerForward_Success() (gas: 30118) +ForwarderTest:test_SetAuthorizedSenders_Success() (gas: 160524) +ForwarderTest:test_TransferOwnershipWithMessage_Success() (gas: 35123) +OperatorTest:test_CancelOracleRequest_Success() (gas: 274436) +OperatorTest:test_FulfillOracleRequest_Success() (gas: 330603) +OperatorTest:test_NotAuthorizedSender_Revert() (gas: 246716) +OperatorTest:test_OracleRequest_Success() (gas: 250019) +OperatorTest:test_SendRequestAndCancelRequest_Success(uint96) (runs: 256, μ: 387120, ~: 387124) +OperatorTest:test_SendRequest_Success(uint96) (runs: 256, μ: 303611, ~: 303620) \ No newline at end of file diff --git a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol index 1785138b7bf..d54dc620460 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Factory.t.sol @@ -9,7 +9,7 @@ contract FactoryTest is Deployer { function setUp() public { _setUp(); - vm.startPrank(s_alice); + vm.startPrank(ALICE); } function test_DeployNewOperator_Success() public { @@ -18,7 +18,7 @@ contract FactoryTest is Deployer { // Assert that the new operator was indeed created by the factory. assertTrue(s_factory.created(newOperator)); // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == s_alice); + require(Operator(newOperator).owner() == ALICE); } function test_DeployNewOperatorAndForwarder_Success() public { @@ -29,7 +29,7 @@ contract FactoryTest is Deployer { assertTrue(s_factory.created(newOperator)); assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is the owner of the newly deployed operator. - require(Operator(newOperator).owner() == s_alice); + require(Operator(newOperator).owner() == ALICE); //Operator to accept ownership vm.startPrank(newOperator); @@ -45,25 +45,25 @@ contract FactoryTest is Deployer { // Assert that the new forwarder was indeed created by the factory. assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is the owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == s_alice); + require(AuthorizedForwarder(newForwarder).owner() == ALICE); } function test_DeployNewForwarderAndTransferOwnership_Success() public { // Deploy a new forwarder with a proposal to transfer its ownership to Bob. - address newForwarder = s_factory.deployNewForwarderAndTransferOwnership(s_bob, new bytes(0)); + address newForwarder = s_factory.deployNewForwarderAndTransferOwnership(BOB, new bytes(0)); // Assert that the new forwarder was indeed created by the factory. assertTrue(s_factory.created(newForwarder)); // Ensure that Alice is still the current owner of the newly deployed forwarder. - require(AuthorizedForwarder(newForwarder).owner() == s_alice); + require(AuthorizedForwarder(newForwarder).owner() == ALICE); // Only proposed owner can call acceptOwnership() vm.expectRevert("Must be proposed owner"); AuthorizedForwarder(newForwarder).acceptOwnership(); - vm.startPrank(s_bob); + vm.startPrank(BOB); // Let Bob accept the ownership. AuthorizedForwarder(newForwarder).acceptOwnership(); // Ensure that Bob is now the owner of the forwarder after the transfer. - require(AuthorizedForwarder(newForwarder).owner() == s_bob); + require(AuthorizedForwarder(newForwarder).owner() == BOB); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol index b2b5de479b0..ba6ce1c17c1 100644 --- a/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/Forwarder.t.sol @@ -10,7 +10,7 @@ contract ForwarderTest is Deployer { function setUp() public { _setUp(); - vm.prank(s_alice); + vm.prank(ALICE); s_forwarder = AuthorizedForwarder(s_factory.deployNewForwarder()); } @@ -21,51 +21,51 @@ contract ForwarderTest is Deployer { vm.expectRevert("Cannot set authorized senders"); s_forwarder.setAuthorizedSenders(senders); - vm.prank(s_alice); + vm.prank(ALICE); // Expect a revert because the sender list is empty vm.expectRevert("Must have at least 1 sender"); s_forwarder.setAuthorizedSenders(senders); // Create a list with two identical sender addresses senders = new address[](2); - senders[0] = s_sender1; - senders[1] = s_sender1; + senders[0] = SENDER_1; + senders[1] = SENDER_1; - vm.prank(s_alice); + vm.prank(ALICE); // Expect a revert because the sender list has duplicates vm.expectRevert("Must not have duplicate senders"); s_forwarder.setAuthorizedSenders(senders); // Set the second sender to a different address - senders[1] = s_sender2; + senders[1] = SENDER_2; - vm.prank(s_alice); + vm.prank(ALICE); // Update the authorized senders list s_forwarder.setAuthorizedSenders(senders); - // Check if both s_sender1 and s_sender2 are now authorized - assertTrue(s_forwarder.isAuthorizedSender(s_sender1)); - assertTrue(s_forwarder.isAuthorizedSender(s_sender2)); + // Check if both SENDER_1 and SENDER_2 are now authorized + assertTrue(s_forwarder.isAuthorizedSender(SENDER_1)); + assertTrue(s_forwarder.isAuthorizedSender(SENDER_2)); // Fetch the authorized senders and verify they match the set addresses address[] memory returnedSenders = s_forwarder.getAuthorizedSenders(); require(returnedSenders[0] == senders[0]); require(returnedSenders[1] == senders[1]); - // Create a new list with only s_sender3 + // Create a new list with only SENDER_3 senders = new address[](1); - senders[0] = s_sender3; + senders[0] = SENDER_3; - // Prank 'alice' and update the authorized senders to just s_sender3 - vm.prank(s_alice); + // Prank 'alice' and update the authorized senders to just SENDER_3 + vm.prank(ALICE); s_forwarder.setAuthorizedSenders(senders); - // Ensure s_sender1 and s_sender2 are no longer authorized - assertFalse(s_forwarder.isAuthorizedSender(s_sender1)); - assertFalse(s_forwarder.isAuthorizedSender(s_sender2)); + // Ensure SENDER_1 and SENDER_2 are no longer authorized + assertFalse(s_forwarder.isAuthorizedSender(SENDER_1)); + assertFalse(s_forwarder.isAuthorizedSender(SENDER_2)); - // Check that s_sender3 is now the only authorized sender - assertTrue(s_forwarder.isAuthorizedSender(s_sender3)); + // Check that SENDER_3 is now the only authorized sender + assertTrue(s_forwarder.isAuthorizedSender(SENDER_3)); returnedSenders = s_forwarder.getAuthorizedSenders(); require(returnedSenders[0] == senders[0]); } @@ -76,23 +76,23 @@ contract ForwarderTest is Deployer { vm.expectRevert("Not authorized sender"); s_forwarder.forward(address(0), new bytes(0)); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Cannot forward to Link token"); s_forwarder.forward(address(s_link), new bytes(0)); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Must forward to a contract"); s_forwarder.forward(address(0), new bytes(0)); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Forwarded call reverted without reason"); s_forwarder.forward(address(s_mockReceiver), new bytes(0)); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("test revert message"); s_forwarder.forward(address(s_mockReceiver), abi.encodeWithSignature("revertMessage()")); - vm.prank(s_sender1); + vm.prank(SENDER_1); s_forwarder.forward(address(s_mockReceiver), abi.encodeWithSignature("receiveData(uint256)", _value)); require(s_mockReceiver.getValue() == _value); @@ -110,33 +110,33 @@ contract ForwarderTest is Deployer { tos = new address[](2); datas = new bytes[](1); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Arrays must have the same length"); s_forwarder.multiForward(tos, datas); datas = new bytes[](2); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Must forward to a contract"); s_forwarder.multiForward(tos, datas); tos[0] = address(s_mockReceiver); tos[1] = address(s_link); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Forwarded call reverted without reason"); s_forwarder.multiForward(tos, datas); datas[0] = abi.encodeWithSignature("receiveData(uint256)", _value1); datas[1] = abi.encodeWithSignature("receiveData(uint256)", _value2); - vm.prank(s_sender1); + vm.prank(SENDER_1); vm.expectRevert("Cannot forward to Link token"); s_forwarder.multiForward(tos, datas); tos[1] = address(s_mockReceiver); - vm.prank(s_sender1); + vm.prank(SENDER_1); s_forwarder.multiForward(tos, datas); require(s_mockReceiver.getValue() == _value2); @@ -146,38 +146,38 @@ contract ForwarderTest is Deployer { vm.expectRevert("Only callable by owner"); s_forwarder.ownerForward(address(0), new bytes(0)); - vm.prank(s_alice); + vm.prank(ALICE); vm.expectRevert("Forwarded call reverted without reason"); s_forwarder.ownerForward(address(s_link), new bytes(0)); - vm.prank(s_alice); + vm.prank(ALICE); s_forwarder.ownerForward(address(s_link), abi.encodeWithSignature("balanceOf(address)", address(0))); } function test_TransferOwnershipWithMessage_Success() public { - vm.prank(s_bob); + vm.prank(BOB); vm.expectRevert("Only callable by owner"); - s_forwarder.transferOwnershipWithMessage(s_bob, new bytes(0)); + s_forwarder.transferOwnershipWithMessage(BOB, new bytes(0)); - vm.prank(s_alice); - s_forwarder.transferOwnershipWithMessage(s_bob, new bytes(0)); + vm.prank(ALICE); + s_forwarder.transferOwnershipWithMessage(BOB, new bytes(0)); vm.expectRevert("Must be proposed owner"); s_forwarder.acceptOwnership(); - vm.prank(s_bob); + vm.prank(BOB); s_forwarder.acceptOwnership(); - require(s_forwarder.owner() == s_bob); + require(s_forwarder.owner() == BOB); } function _addSenders() internal { address[] memory senders = new address[](3); - senders[0] = s_sender1; - senders[1] = s_sender2; - senders[2] = s_sender3; + senders[0] = SENDER_1; + senders[1] = SENDER_2; + senders[2] = SENDER_3; - vm.prank(s_alice); + vm.prank(ALICE); s_forwarder.setAuthorizedSenders(senders); } } diff --git a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol index e3c9a2a11df..6c4a7c2ae1a 100644 --- a/contracts/src/v0.8/operatorforwarder/test/operator.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/operator.t.sol @@ -116,10 +116,10 @@ contract OperatorTest is Deployer { uint256 withdrawableBefore = s_operator.withdrawable(); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(address(s_link), s_alice, payment); - assertEq(s_link.balanceOf(s_alice), 1 ether, "balance update failed"); + deal(address(s_link), ALICE, payment); + assertEq(s_link.balanceOf(ALICE), 1 ether, "balance update failed"); - vm.prank(s_alice); + vm.prank(ALICE); s_link.transferAndCall( address(s_operator), payment, @@ -147,7 +147,7 @@ contract OperatorTest is Deployer { // so we should enable it to set Authorised senders to itself address[] memory senders = new address[](2); senders[0] = address(this); - senders[0] = s_bob; + senders[0] = BOB; s_operator.setAuthorizedSenders(senders); @@ -166,8 +166,8 @@ contract OperatorTest is Deployer { bytes32 data = bytes32(keccak256(dataBytes)); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(address(s_link), s_bob, payment); - vm.prank(s_bob); + deal(address(s_link), BOB, payment); + vm.prank(BOB); s_link.transferAndCall( address(s_operator), payment, @@ -185,8 +185,8 @@ contract OperatorTest is Deployer { ); // Fulfill the request using the operator - bytes32 requestId = keccak256(abi.encodePacked(s_bob, nonce)); - vm.prank(s_bob); + bytes32 requestId = keccak256(abi.encodePacked(BOB, nonce)); + vm.prank(BOB); s_operator.fulfillOracleRequest(requestId, payment, address(s_callback), callbackFunctionId, expiration, data); assertEq(s_callback.getCallbacksReceived(), 1, "Oracle request was not fulfilled"); @@ -208,17 +208,17 @@ contract OperatorTest is Deployer { uint256 withdrawableBefore = s_operator.withdrawable(); // Send LINK tokens to the Operator contract using `transferAndCall` - deal(address(s_link), s_bob, payment); - vm.prank(s_bob); + deal(address(s_link), BOB, payment); + vm.prank(BOB); s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - s_bob, + BOB, payment, specId, - s_bob, + BOB, callbackFunctionId, nonce, dataVersion, @@ -229,13 +229,13 @@ contract OperatorTest is Deployer { // No withdrawable balance as it's all locked assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); - bytes32 requestId = keccak256(abi.encodePacked(s_bob, nonce)); + bytes32 requestId = keccak256(abi.encodePacked(BOB, nonce)); - vm.startPrank(s_alice); + vm.startPrank(ALICE); vm.expectRevert(bytes("Params do not match request ID")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); - vm.startPrank(s_bob); + vm.startPrank(BOB); vm.expectRevert(bytes("Request is not expired")); s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); @@ -243,7 +243,7 @@ contract OperatorTest is Deployer { s_operator.cancelOracleRequest(requestId, payment, callbackFunctionId, expiration); // Check if the LINK tokens were refunded to the sender (bob in this case) - assertEq(s_link.balanceOf(s_bob), 1 ether, "Oracle request was not canceled properly"); + assertEq(s_link.balanceOf(BOB), 1 ether, "Oracle request was not canceled properly"); assertEq(s_operator.withdrawable(), withdrawableBefore, "Internal accounting not updated correctly"); } @@ -257,14 +257,14 @@ contract OperatorTest is Deployer { uint256 payment = 1 ether; uint256 expiration = block.timestamp + 5 minutes; - deal(address(s_link), s_alice, payment); - vm.prank(s_alice); + deal(address(s_link), ALICE, payment); + vm.prank(ALICE); s_link.transferAndCall( address(s_operator), payment, abi.encodeWithSignature( "oracleRequest(address,uint256,bytes32,address,bytes4,uint256,uint256,bytes)", - s_alice, + ALICE, payment, specId, address(s_callback), @@ -275,9 +275,9 @@ contract OperatorTest is Deployer { ) ); - bytes32 requestId = keccak256(abi.encodePacked(s_alice, nonce)); + bytes32 requestId = keccak256(abi.encodePacked(ALICE, nonce)); - vm.prank(s_bob); + vm.prank(BOB); vm.expectRevert(bytes("Not authorized sender")); s_operator.fulfillOracleRequest( requestId, diff --git a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol index 31dcfb9c3b2..da746c7ff8c 100644 --- a/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol +++ b/contracts/src/v0.8/operatorforwarder/test/testhelpers/Deployer.t.sol @@ -14,13 +14,11 @@ abstract contract Deployer is Test { LinkToken internal s_link; MockReceiver internal s_mockReceiver; - address private s_owner = makeAddr("owner"); - address internal s_alice = makeAddr("alice"); - address internal s_bob = makeAddr("bob"); - - address public s_sender1 = makeAddr("sender1"); - address public s_sender2 = makeAddr("sender2"); - address public s_sender3 = makeAddr("sender3"); + address internal constant ALICE = address(0x101); + address internal constant BOB = address(0x102); + address internal constant SENDER_1 = address(0x103); + address internal constant SENDER_2 = address(0x104); + address internal constant SENDER_3 = address(0x105); function _setUp() internal { _deploy(); From 2286dace358dc6cf6c285dde99937c454c686e5e Mon Sep 17 00:00:00 2001 From: Austin Born Date: Thu, 2 May 2024 01:22:55 -0700 Subject: [PATCH 15/15] Update gas snapshot --- contracts/gas-snapshots/operatorforwarder.gas-snapshot | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/gas-snapshots/operatorforwarder.gas-snapshot b/contracts/gas-snapshots/operatorforwarder.gas-snapshot index f4e6306e768..ee6c063f2f3 100644 --- a/contracts/gas-snapshots/operatorforwarder.gas-snapshot +++ b/contracts/gas-snapshots/operatorforwarder.gas-snapshot @@ -8,8 +8,14 @@ ForwarderTest:test_OwnerForward_Success() (gas: 30118) ForwarderTest:test_SetAuthorizedSenders_Success() (gas: 160524) ForwarderTest:test_TransferOwnershipWithMessage_Success() (gas: 35123) OperatorTest:test_CancelOracleRequest_Success() (gas: 274436) +OperatorTest:test_CancelOracleRequest_Success() (gas: 274436) +OperatorTest:test_FulfillOracleRequest_Success() (gas: 330603) OperatorTest:test_FulfillOracleRequest_Success() (gas: 330603) OperatorTest:test_NotAuthorizedSender_Revert() (gas: 246716) +OperatorTest:test_NotAuthorizedSender_Revert() (gas: 246716) OperatorTest:test_OracleRequest_Success() (gas: 250019) +OperatorTest:test_OracleRequest_Success() (gas: 250019) +OperatorTest:test_SendRequestAndCancelRequest_Success(uint96) (runs: 256, μ: 387120, ~: 387124) OperatorTest:test_SendRequestAndCancelRequest_Success(uint96) (runs: 256, μ: 387120, ~: 387124) +OperatorTest:test_SendRequest_Success(uint96) (runs: 256, μ: 303611, ~: 303620) OperatorTest:test_SendRequest_Success(uint96) (runs: 256, μ: 303611, ~: 303620) \ No newline at end of file