From 2908c5259a2613878f28bd6dd2ebefadd401e0b3 Mon Sep 17 00:00:00 2001 From: Arthur Gousset <46296830+arthurgousset@users.noreply.github.com> Date: Wed, 5 Jun 2024 12:41:49 +0100 Subject: [PATCH] Integration tests for Anvil migrations (#11002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(protocol/migrations_sol): adds existing unit tests * test(test-sol/integration): adds e2e test for GoldToken This is a test run, to see how `setUp()` and contract definitions work. 1. run devchain ```sh ./migrations_sol/create_and_migrate_anvil_devchain.sh ``` 2. run test against devchain ```sh forge test \ --match-path test-sol/integration/Integration.t.sol \ --match-contract GoldTokenTest_General \ -vvv \ --fork-url http://127.0.0.1:8546 ``` Tests pass Output: ```sh [⠰] Compiling... No files changed, compilation skipped Running 3 tests for test-sol/integration/Integration.t.sol:GoldTokenTest_General [PASS] test_decimals() (gas: 10837) Logs: GoldToken address is: 0xfE8CbC1cFA1b3b8256f310bdfd40E60597083448 [PASS] test_name() (gas: 12537) Logs: GoldToken address is: 0xfE8CbC1cFA1b3b8256f310bdfd40E60597083448 [PASS] test_symbol() (gas: 12579) Logs: GoldToken address is: 0xfE8CbC1cFA1b3b8256f310bdfd40E60597083448 Test result: ok. 3 passed; 0 failed; 0 skipped; finished in 135.40ms Ran 1 test suites: 3 tests passed, 0 failed, 0 skipped (3 total tests) ``` * test(test-sol/integration): adds loop to assert contract is in Registry * chore(test-sol/integration): adds TODO comment about config file * test(test-sol/integration): upgrades to solidity 0.8 This is to allow me to use the `.code` property on deployed smart contracts, which is only supported on Solidity 0.8 and above. * nit(test-sol/integration): updates constructor visibility In Solidity versions 0.7.0 and later, constructors are implicitly internal, meaning they can only be called within the contract itself or derived contracts, making the visibility keyword redundant. * nit(test-sol/integration): updates function state mutability The state mutability of a function can be restricted to view if the function does not modify the state of the blockchain. A view function promises not to alter the state, which allows the Ethereum Virtual Machine (EVM) to optimize how it handles the function. The function does not modify any state; it only reads from the registry and logs some information. * test(test-sol/integration): adds MVP assertion that bytecode matches * fix(test-sol/integration): use implementation address and deployedBytecode * refactor(test-sol): adds registry array in `constants.sol` The goal is to use that in both the migration script (`Migration.s.sol`) and the integration test (`Integration.t.sol`). * refactor(protocol/migration_sol): uses registry array from `constants.sol` * fix(test-sol/integration): adds back `Test.sol` import Mistakenly deleted earlier * refactor(migrations_sol): simplifies script that runs integration tests * chore(test-sol/integration): commits WIP on failing bytecode test * test(test-sol): adds helper to remove metadata from bytecode Also adds docstring for better readability. * chore(test-sol/integration): excludes failing contracts from bytecode test * chor(test-sol/integration): adds code comment for readability * chore(test-sol/integration): removing unused code comments * fix(workflows/protocol_tests): excludes integration tests * fix(workflows/protocol_tests): excludes integration tests more explicitly * nit(test-sol/constants): improves code comment for context * fix(workflows/protocol_tests): downgrades foundry version The latest version has a regression that leads to a failing integration test. But, fixing a previous nightly build fixed the error. * fix(workflows/protocol_tests): excludes integration tests correctly * chore(test-sol/utils): reverts `deployCodeTo` edit in `ECDSAHelper` Because I'm downgrading to a previous foundry version, I have to undo the change to this line made in this PR: https://github.com/celo-org/celo-monorepo/pull/10997 * style(protocol): linting I always forget to run `yarn prettify` in the top-level directory * docs(workflows/protocol_tests): adds better code comment To explain what flags and arguments are used. * refactor(test-sol/integration): rename variables for better readability * refactor(test-sol/integration): take constants out of `for`-loop * style(protocol): linting Using `yarn prettify` in the top-level directory * fix(test-sol/integration): add contract name back into `for`-loop * nit(test-sol/integration): rename variable for better readability * refactor(migrations_sol/constants): creates new `constants.sol` * refactor(migrations_sol&test-sol): removes `Utils` import Adds function in `Integration.t.sol` directly to avoid 0.5.x version problems. * test(test-sol/integration): bumps solidity version to `>=0.8.7 <0.8.20` * refactor(test-sol/integration): use `.code` property instead of helper function Because the script is now on 0.8, we can use the `.code` property to fetch the runtime bytecode of a smart contract deployed on the devchain. Previously, we needed to load the bytecode from storage with inline assembly. * refactor(foundry): simplifies configs to exclude tests Functionally this should be the same, but the configs and flags are more readable. --- .github/workflows/protocol_tests.yml | 7 +- packages/protocol/foundry.toml | 10 +- .../protocol/migrations_sol/Migration.s.sol | 40 ++---- .../protocol/migrations_sol/constants.sol | 30 +++++ .../migrations_sol/integration_tests.sh | 4 - .../run_integration_tests_in_anvil.sh | 11 +- .../test-sol/integration/Integration.t.sol | 117 ++++++++++++++++-- .../protocol/test-sol/utils/ECDSAHelper.sol | 2 +- 8 files changed, 166 insertions(+), 55 deletions(-) create mode 100644 packages/protocol/migrations_sol/constants.sol delete mode 100755 packages/protocol/migrations_sol/integration_tests.sh diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 6d05c627da0..10eff4b0881 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -50,6 +50,8 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 + with: + version: "nightly-f625d0fa7c51e65b4bf1e8f7931cd1c6e2e285e9" - name: Install forge dependencies run: forge install @@ -94,8 +96,9 @@ jobs: - name: Run Everything just in case something was missed # can't use gas limit because some setUp function use more than the limit - run: forge test -vvv #TODO this should ignore integration tests - + # Excludes integration tests, because they require an anvil devchain running on the localhost. + run: forge test -vvv --no-match-contract RegistryIntegrationTest + - name: Generate migrations if: success() || failure() run: ./migrations_sol/run_integration_tests_in_anvil.sh diff --git a/packages/protocol/foundry.toml b/packages/protocol/foundry.toml index f309779040c..b6a500daa6e 100644 --- a/packages/protocol/foundry.toml +++ b/packages/protocol/foundry.toml @@ -14,12 +14,16 @@ remappings = [ 'forge-std-8/=lib/celo-foundry-8/lib/forge-std/src/', '@celo-contracts-8=contracts-0.8/', '@openzeppelin/contracts8/=lib/openzeppelin-contracts8/contracts/', - '@celo-contracts/=contracts/' + '@celo-contracts/=contracts/', + '@celo-migrations/=migrations_sol/' ] -no_match_contract = "RandomTest" no_match_test = "skip" -no_match_path = "contracts/common/libraries/test/BLS12Passthrough.sol" # tested from celo-blockain repo + +# `BLS12Passthrough.sol` is excluded, because it's tested in the celo-blockain repo as +# described here: https://github.com/celo-org/celo-monorepo/pull/10240 +# `Random.sol` is excluded, but I'm not sure why. It was already excluded so I'm leaving it here. +no_match_path = "*test/{BLS12Passthrough.sol,RandomTest.sol}" fs_permissions = [{ access = "read", path = "./out"}, { access = "read", path = "./migrations_sol/migrationsConfig.json"}, { access = "read", path = "./governanceConstitution.json"}] diff --git a/packages/protocol/migrations_sol/Migration.s.sol b/packages/protocol/migrations_sol/Migration.s.sol index 859fccf10ac..dfae2c739b3 100644 --- a/packages/protocol/migrations_sol/Migration.s.sol +++ b/packages/protocol/migrations_sol/Migration.s.sol @@ -3,6 +3,7 @@ pragma solidity >=0.8.7 <0.8.20; // Note: This script should not include any cheatcode so that it can run in production import { Script } from "forge-std-8/Script.sol"; + import "forge-std/console.sol"; import "forge-std/StdJson.sol"; @@ -46,6 +47,8 @@ import "@openzeppelin/contracts8/utils/math/Math.sol"; import "@celo-contracts-8/common/UsingRegistry.sol"; +import { Constants } from "@celo-migrations/constants.sol"; + contract ForceTx { // event to trigger so a tx can be processed event VanillaEvent(string); @@ -57,7 +60,7 @@ contract ForceTx { } } -contract Migration is Script, UsingRegistry { +contract Migration is Script, UsingRegistry, Constants { using stdJson for string; /** @@ -941,38 +944,9 @@ contract Migration is Script, UsingRegistry { (bool) ); if (!skipTransferOwnership) { - // TODO move this list somewhere else - - string[23] memory fixedStringArray = [ - "Accounts", - // 'Attestations', - // BlockchainParameters ownership transitioned to governance in a follow-up script.? - "BlockchainParameters", - "DoubleSigningSlasher", - "DowntimeSlasher", - "Election", - "EpochRewards", - "Escrow", - "FederatedAttestations", - "FeeCurrencyWhitelist", - "FeeCurrencyDirectory", - "Freezer", - "FeeHandler", - "GoldToken", - "Governance", - "GovernanceSlasher", - "LockedGold", - "OdisPayments", - "Random", - "Registry", - "SortedOracles", - "UniswapFeeHandlerSeller", - "MentoFeeHandlerSeller", - "Validators" - ]; - - for (uint256 i = 0; i < fixedStringArray.length; i++) { - string memory contractToTransfer = fixedStringArray[i]; + // BlockchainParameters ownership transitioned to governance in a follow-up script.? + for (uint256 i = 0; i < contractsInRegistry.length; i++) { + string memory contractToTransfer = contractsInRegistry[i]; console.log("Transfering ownership of: ", contractToTransfer); IProxy proxy = IProxy(registry.getAddressForStringOrDie(contractToTransfer)); proxy._transferOwnership(governanceAddress); diff --git a/packages/protocol/migrations_sol/constants.sol b/packages/protocol/migrations_sol/constants.sol new file mode 100644 index 00000000000..81bee276509 --- /dev/null +++ b/packages/protocol/migrations_sol/constants.sol @@ -0,0 +1,30 @@ +pragma solidity >=0.8.7 <0.8.20; + +contract Constants { + // List of contracts that are expected to be in Registry.sol + string[23] contractsInRegistry = [ + "Accounts", + "BlockchainParameters", + "DoubleSigningSlasher", + "DowntimeSlasher", + "Election", + "EpochRewards", + "Escrow", + "FederatedAttestations", + "FeeCurrencyWhitelist", + "FeeCurrencyDirectory", + "Freezer", + "FeeHandler", + "GoldToken", + "Governance", + "GovernanceSlasher", + "LockedGold", + "OdisPayments", + "Random", + "Registry", + "SortedOracles", + "UniswapFeeHandlerSeller", + "MentoFeeHandlerSeller", + "Validators" + ]; +} diff --git a/packages/protocol/migrations_sol/integration_tests.sh b/packages/protocol/migrations_sol/integration_tests.sh deleted file mode 100755 index ae2e54025b5..00000000000 --- a/packages/protocol/migrations_sol/integration_tests.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -forge test --fork-url http://127.0.0.1:$ANVIL_PORT --match-contract=IntegrationTest -vvv # || echo "Test failed" # TODO for some reason the echo didn't work \ No newline at end of file diff --git a/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh b/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh index 60f2cb82b80..af25e4b069a 100755 --- a/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh +++ b/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh @@ -1,13 +1,16 @@ #!/usr/bin/env bash set -euo pipefail - -# generate devchain +# Generate and run devchain +echo "Generating and running devchain before running integration tests..." source $PWD/migrations_sol/create_and_migrate_anvil_devchain.sh # Run integration tests -source $PWD/migrations_sol/integration_tests.sh - +echo "Running integration tests..." +forge test \ +-vvv \ +--match-contract RegistryIntegrationTest \ +--fork-url http://127.0.0.1:$ANVIL_PORT # helper kill anvil # kill $(lsof -i tcp:$ANVIL_PORT | tail -n 1 | awk '{print $2}') diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 5446db353dc..e601de7ff46 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,19 +1,120 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.5.13; +pragma solidity >=0.8.7 <0.8.20; + +import { Test } from "forge-std-8/Test.sol"; +import "forge-std-8/console2.sol"; + +import { Constants } from "@celo-migrations/constants.sol"; -import "celo-foundry/Test.sol"; -import "@celo-contracts/common/GoldToken.sol"; -// import "@celo-contracts/common/test/MockGoldToken.sol"; -import "forge-std/console.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; +import "@celo-contracts/common/interfaces/IProxy.sol"; contract IntegrationTest is Test { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); - address account1 = actor("account1"); - address account2 = actor("account2"); IRegistry registry = IRegistry(registryAddress); function setUp() public {} - function test_dummy() public {} + /** + * @notice Removes CBOR encoded metadata from the tail of the deployedBytecode. + * @param data Bytecode including the CBOR encoded tail. + * @return Bytecode without the CBOR encoded metadata. + */ + function removeMetadataFromBytecode(bytes memory data) public pure returns (bytes memory) { + // Ensure the data length is at least enough to contain the length specifier + require(data.length >= 2, "Data too short to contain a valid CBOR length specifier"); + + // Calculate the length of the CBOR encoded section from the last two bytes + uint16 cborLength = uint16(uint8(data[data.length - 2])) * + 256 + + uint16(uint8(data[data.length - 1])); + + // Ensure the length is valid (not greater than the data array length minus 2 bytes for the length field) + require(cborLength <= data.length - 2, "Invalid CBOR length"); + + // Calculate the new length of the data without the CBOR section + uint newLength = data.length - 2 - cborLength; + + // Create a new byte array for the result + bytes memory result = new bytes(newLength); + + // Copy data from the original byte array to the new one, excluding the CBOR section and its length field + for (uint i = 0; i < newLength; i++) { + result[i] = data[i]; + } + + return result; + } +} + +contract RegistryIntegrationTest is IntegrationTest, Constants { + IProxy proxy; + + function test_shouldHaveAddressInRegistry() public view { + for (uint256 i = 0; i < contractsInRegistry.length; i++) { + string memory contractName = contractsInRegistry[i]; + address contractAddress = registry.getAddressFor(keccak256(abi.encodePacked(contractName))); + console2.log(contractName, "address in Registry is: ", contractAddress); + assert(contractAddress != address(0)); + } + } + + function test_shouldHaveCorrectBytecode() public { + // Converting contract names to hashes for comparison + bytes32 hashAccount = keccak256(abi.encodePacked("Accounts")); + bytes32 hashElection = keccak256(abi.encodePacked("Election")); + bytes32 hashEscrow = keccak256(abi.encodePacked("Escrow")); + bytes32 hashFederatedAttestations = keccak256(abi.encodePacked("FederatedAttestations")); + bytes32 hashGovernance = keccak256(abi.encodePacked("Governance")); + bytes32 hashSortedOracles = keccak256(abi.encodePacked("SortedOracles")); + bytes32 hashValidators = keccak256(abi.encodePacked("Validators")); + + for (uint256 i = 0; i < contractsInRegistry.length; i++) { + // Read name from list of core contracts + string memory contractName = contractsInRegistry[i]; + console2.log("Checking bytecode of:", contractName); + + // Skipping test for contracts that depend on linked libraries + // This is a known limitation in Foundry at the moment: + // Source: https://github.com/foundry-rs/foundry/issues/6120 + bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); + if ( + hashContractName != hashAccount && + hashContractName != hashElection && + hashContractName != hashEscrow && + hashContractName != hashFederatedAttestations && + hashContractName != hashGovernance && + hashContractName != hashSortedOracles && + hashContractName != hashValidators + ) { + // Get proxy address registered in the Registry + address proxyAddress = registry.getAddressForStringOrDie(contractName); + proxy = IProxy(address(uint160(proxyAddress))); + + // Get implementation address + address implementationAddress = proxy._getImplementation(); + + // Get bytecode from deployed contract + bytes memory actualBytecodeWithMetadataOnDevchain = implementationAddress.code; + bytes memory actualBytecodeOnDevchain = removeMetadataFromBytecode( + actualBytecodeWithMetadataOnDevchain + ); + + // Get bytecode from build artifacts + bytes memory expectedBytecodeWithMetadataFromArtifacts = vm.getDeployedCode( + string(abi.encodePacked(contractName, ".sol")) + ); + bytes memory expectedBytecodeFromArtifacts = removeMetadataFromBytecode( + expectedBytecodeWithMetadataFromArtifacts + ); + + // Compare the bytecodes + assertEq( + actualBytecodeOnDevchain, + expectedBytecodeFromArtifacts, + "Bytecode does not match" + ); + } + } + } } diff --git a/packages/protocol/test-sol/utils/ECDSAHelper.sol b/packages/protocol/test-sol/utils/ECDSAHelper.sol index aaa7c1119ae..cd85d52cccd 100644 --- a/packages/protocol/test-sol/utils/ECDSAHelper.sol +++ b/packages/protocol/test-sol/utils/ECDSAHelper.sol @@ -13,7 +13,7 @@ contract ECDSAHelper is Test { bytes32 _s ) public returns (bytes memory) { address SECP256K1Address = actor("SECP256K1Address"); - deployCodeTo("SECP256K1.sol:SECP256K1", SECP256K1Address); + deployCodeTo("out/SECP256K1.sol/SECP256K1.0.5.17.json", SECP256K1Address); sECP256K1 = ISECP256K1(SECP256K1Address); string memory header = "\x19Ethereum Signed Message:\n32";