From cd987f334c9422e5e4a817b68a853efa7200aa7f Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 21 May 2024 15:25:32 +0100 Subject: [PATCH 01/36] test(protocol/migrations_sol): adds existing unit tests --- .../migrations_sol/integration_tests.sh | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/packages/protocol/migrations_sol/integration_tests.sh b/packages/protocol/migrations_sol/integration_tests.sh index ae2e54025b5..0510953d470 100755 --- a/packages/protocol/migrations_sol/integration_tests.sh +++ b/packages/protocol/migrations_sol/integration_tests.sh @@ -1,4 +1,66 @@ #!/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 +: ' + The following runs specific integration tests. + ' +# Run integration tests +forge test \ +--match-contract=IntegrationTest \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +: ' + The following tests assert that existing unit tests pass when run against the migrated anvil fork. + These are necessary, but not sufficient requirements. + The idea is that existing unit tests, which are known to pass in a testing environment, should + also pass if they are run against the devchain using the "--fork-url" flag. + ' +# Run tests common +# can't use gas limit because some setUp function use more than the limit +forge test \ +--match-path "test-sol/common/*" \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run tests governance/network +forge test \ +--match-path "test-sol/governance/network/*" \ +--block-gas-limit 50000000 \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run tests governance/validators +forge test \ +--match-path "test-sol/governance/validators/*" \ +--block-gas-limit 50000000 \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run tests governance/voting +# can't use gas limit because some setUp function use more than the limit +forge test \ +--match-path "test-sol/governance/voting/*" \ +--block-gas-limit 50000000 \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run tests stability +forge test \ +--match-path "test-sol/stability/*" \ +--block-gas-limit 50000000 \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run tests identity +forge test \ +--match-path "test-sol/identity/*" \ +--block-gas-limit 50000000 \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT + +# Run Everything just in case something was missed +# can't use gas limit because some setUp function use more than the limit +forge test \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT #TODO this should ignore integration tests From ee24877a8482df05c316a5d6c490a5ef4971c0e8 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 28 May 2024 17:22:22 +0100 Subject: [PATCH 02/36] test(test-sol/integration): adds e2e test for GoldToken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-sol/integration/Integration.t.sol | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 5446db353dc..e4aee685da0 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -3,17 +3,38 @@ pragma solidity ^0.5.13; 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/GoldToken.sol"; +import "@celo-contracts/common/UsingRegistry.sol"; +import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; +import "forge-std/console2.sol"; -contract IntegrationTest is Test { - address constant registryAddress = address(0x000000000000000000000000000000000000ce10); +contract IntegrationTest is Test, UsingRegistry { address account1 = actor("account1"); address account2 = actor("account2"); + address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); + GoldToken goldToken; function setUp() public {} - - function test_dummy() public {} } + +contract GoldTokenTest_General is IntegrationTest { + function setUp() public { + goldToken = GoldToken(registry.getAddressForStringOrDie("GoldToken")); + console2.log("GoldToken address is:", address(goldToken)); + } + + function test_name() public { + assertEq(goldToken.name(), "Celo native asset"); + } + + function test_symbol() public { + assertEq(goldToken.symbol(), "CELO"); + } + + function test_decimals() public { + assertEq(uint256(goldToken.decimals()), 18); + } +} \ No newline at end of file From 714bc754d1543dc82ad6320a2a3bae734707d980 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Wed, 29 May 2024 18:42:10 +0100 Subject: [PATCH 03/36] test(test-sol/integration): adds loop to assert contract is in Registry --- .../test-sol/integration/Integration.t.sol | 68 ++++++++++++------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index e4aee685da0..d67ee348028 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,40 +1,60 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.5.13; +import "forge-std/console2.sol"; + import "celo-foundry/Test.sol"; -import "@celo-contracts/common/GoldToken.sol"; -import "forge-std/console.sol"; +import { Constants } from "@test-sol/constants.sol"; + import "@celo-contracts/common/interfaces/IRegistry.sol"; -import "@celo-contracts/common/GoldToken.sol"; -import "@celo-contracts/common/UsingRegistry.sol"; -import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; -import "forge-std/console2.sol"; -contract IntegrationTest is Test, UsingRegistry { - address account1 = actor("account1"); - address account2 = actor("account2"); +contract IntegrationTest is Test, Constants { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); - GoldToken goldToken; + + address account1 = actor("account1"); + address account2 = actor("account2"); function setUp() public {} } -contract GoldTokenTest_General is IntegrationTest { - function setUp() public { - goldToken = GoldToken(registry.getAddressForStringOrDie("GoldToken")); - console2.log("GoldToken address is:", address(goldToken)); - } - - function test_name() public { - assertEq(goldToken.name(), "Celo native asset"); - } - - function test_symbol() public { - assertEq(goldToken.symbol(), "CELO"); +contract RegistryIntegrationTest is IntegrationTest { + string[23] expectedContractsInRegistry; + + constructor() public { + expectedContractsInRegistry = [ + "Accounts", + "BlockchainParameters", + "DoubleSigningSlasher", + "DowntimeSlasher", + "Election", + "EpochRewards", + "Escrow", + "FederatedAttestations", + "FeeCurrencyWhitelist", + "FeeCurrencyDirectory", + "Freezer", + "FeeHandler", + "GoldToken", + "Governance", + "GovernanceSlasher", + "LockedGold", + "OdisPayments", + "Random", + "Registry", + "SortedOracles", + "UniswapFeeHandlerSeller", + "MentoFeeHandlerSeller", + "Validators" + ]; } - function test_decimals() public { - assertEq(uint256(goldToken.decimals()), 18); + function test_shouldHaveAddressInRegistry() public { + for (uint256 i = 0; i < expectedContractsInRegistry.length; i++) { + string memory contractName = expectedContractsInRegistry[i]; + address contractAddress = registry.getAddressFor(keccak256(abi.encodePacked(contractName))); + console2.log(contractName, "address in Registry is: ", contractAddress); + assert(contractAddress != address(0)); + } } } \ No newline at end of file From 8651f31d9b8eec0f10f3602f64b8555b7c3f8569 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Wed, 29 May 2024 18:43:37 +0100 Subject: [PATCH 04/36] chore(test-sol/integration): adds TODO comment about config file --- packages/protocol/test-sol/integration/Integration.t.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index d67ee348028..568d4dc18e8 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -20,7 +20,9 @@ contract IntegrationTest is Test, Constants { contract RegistryIntegrationTest is IntegrationTest { string[23] expectedContractsInRegistry; - + + // TODO(Arthur): Consider moving this to a config file. Perhaps make the migration depend + // on that file too? constructor() public { expectedContractsInRegistry = [ "Accounts", From eec38636ea43dfde8306cbb5bcf17cc48996371a Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Thu, 30 May 2024 11:26:35 +0100 Subject: [PATCH 05/36] 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. --- .../protocol/test-sol/integration/Integration.t.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 568d4dc18e8..a7730e29e47 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,19 +1,19 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.5.13; +pragma solidity ^0.8.0; import "forge-std/console2.sol"; -import "celo-foundry/Test.sol"; -import { Constants } from "@test-sol/constants.sol"; +// import { Constants } from "@test-sol/constants.sol"; +// import "celo-foundry-8/Test.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; -contract IntegrationTest is Test, Constants { +contract IntegrationTest { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); - address account1 = actor("account1"); - address account2 = actor("account2"); + // address account1 = actor("account1"); + // address account2 = actor("account2"); function setUp() public {} } From c4f55d5586f37eea702c2234b0849c0e7ed11608 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Thu, 30 May 2024 15:59:34 +0100 Subject: [PATCH 06/36] 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. --- packages/protocol/test-sol/integration/Integration.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index a7730e29e47..8b730ebd5a4 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -23,7 +23,7 @@ contract RegistryIntegrationTest is IntegrationTest { // TODO(Arthur): Consider moving this to a config file. Perhaps make the migration depend // on that file too? - constructor() public { + constructor() { expectedContractsInRegistry = [ "Accounts", "BlockchainParameters", From de6b1431a187f56c7e4ca83155ed086b21fe5989 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Thu, 30 May 2024 16:01:01 +0100 Subject: [PATCH 07/36] 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. --- packages/protocol/test-sol/integration/Integration.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 8b730ebd5a4..b89e0d0377e 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -51,7 +51,7 @@ contract RegistryIntegrationTest is IntegrationTest { ]; } - function test_shouldHaveAddressInRegistry() public { + function test_shouldHaveAddressInRegistry() public view { for (uint256 i = 0; i < expectedContractsInRegistry.length; i++) { string memory contractName = expectedContractsInRegistry[i]; address contractAddress = registry.getAddressFor(keccak256(abi.encodePacked(contractName))); From d99a7465129125ecfd2d9eb66763c6e77cc53623 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Thu, 30 May 2024 16:04:00 +0100 Subject: [PATCH 08/36] test(test-sol/integration): adds MVP assertion that bytecode matches --- .../test-sol/integration/Integration.t.sol | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index b89e0d0377e..1db7c7616f4 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,14 +1,14 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; -import "forge-std/console2.sol"; +// import "forge-std/console2.sol"; // import { Constants } from "@test-sol/constants.sol"; -// import "celo-foundry-8/Test.sol"; +import "celo-foundry-8/Test.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; -contract IntegrationTest { +contract IntegrationTest is Test { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); @@ -59,4 +59,19 @@ contract RegistryIntegrationTest is IntegrationTest { assert(contractAddress != address(0)); } } + + function test_shouldHaveCorrectBytecode() public { + ////////////////////DEBUGGING: START///////////////////////// + // SPECIFIC EXAMPLE REGISTRY.SOL BEFORE LOOPING OVER ALL CONTRACTS + string memory contractName = "Registry"; + address contractAddress = registry.getAddressForStringOrDie(contractName); + ////////////////////DEBUGGING: END/////////////////////////// + + // Get the deployed bytecode + bytes memory actualBytecode = contractAddress.code; + bytes memory expectedBytecode = vm.getDeployedCode(string.concat(contractName, ".sol")); + + // Compare the bytecodes + assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); + } } \ No newline at end of file From 4df1e7c514e093852d786d673e1ae976e1f9a92c Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Thu, 30 May 2024 18:22:03 +0100 Subject: [PATCH 09/36] fix(test-sol/integration): use implementation address and deployedBytecode --- .../test-sol/integration/Integration.t.sol | 31 +++++++++++++------ packages/protocol/test-sol/utils.sol | 21 +++++++++++++ 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 1db7c7616f4..73dee963380 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,12 +1,14 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.0; +pragma solidity >=0.5.13 <0.8.20; // import "forge-std/console2.sol"; // import { Constants } from "@test-sol/constants.sol"; -import "celo-foundry-8/Test.sol"; - +import "celo-foundry/Test.sol"; +import { Utils } from "@test-sol/utils.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; +import "@celo-contracts/common/interfaces/IProxy.sol"; + contract IntegrationTest is Test { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); @@ -18,12 +20,13 @@ contract IntegrationTest is Test { function setUp() public {} } -contract RegistryIntegrationTest is IntegrationTest { +contract RegistryIntegrationTest is IntegrationTest, Utils { string[23] expectedContractsInRegistry; + IProxy proxy; // TODO(Arthur): Consider moving this to a config file. Perhaps make the migration depend // on that file too? - constructor() { + constructor() public { expectedContractsInRegistry = [ "Accounts", "BlockchainParameters", @@ -64,12 +67,22 @@ contract RegistryIntegrationTest is IntegrationTest { ////////////////////DEBUGGING: START///////////////////////// // SPECIFIC EXAMPLE REGISTRY.SOL BEFORE LOOPING OVER ALL CONTRACTS string memory contractName = "Registry"; - address contractAddress = registry.getAddressForStringOrDie(contractName); + address proxyAddress = registry.getAddressForStringOrDie(contractName); + proxy = IProxy(address(uint160(proxyAddress))); ////////////////////DEBUGGING: END/////////////////////////// - // Get the deployed bytecode - bytes memory actualBytecode = contractAddress.code; - bytes memory expectedBytecode = vm.getDeployedCode(string.concat(contractName, ".sol")); + address implementationAddress = proxy._getImplementation(); + console2.log("Implementation address is :", implementationAddress); + + // Get bytecode from deployed contract (in Solidity 0.5) + bytes memory actualBytecode = getCodeAt(implementationAddress); // IProxy.sol and Proxy.sol are 0.5 + // Get bytecode from build artifacts + bytes memory expectedBytecode = vm.getDeployedCode("Registry.sol"); + + // // Get bytecode from deployed contract (in Solidity 0.8) + // bytes memory actualBytecode = implementationAddress.code; + // // Get bytecode from build artifacts (in Solidity 0.8) + // bytes memory expectedBytecode = vm.getDeployedCode(string.concat(contractName, ".sol")); // Compare the bytecodes assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); diff --git a/packages/protocol/test-sol/utils.sol b/packages/protocol/test-sol/utils.sol index 91189b89bab..3d95e5d5af2 100644 --- a/packages/protocol/test-sol/utils.sol +++ b/packages/protocol/test-sol/utils.sol @@ -75,4 +75,25 @@ contract Utils is Test { (uint256(keccak256(abi.encodePacked(block.timestamp, block.difficulty, msg.sender, salt))) % (max - min + 1)) + min; } + + // Get the runtime code or "deployedBytecode" at a contract address. + // Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. + // On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. + // This implementation is taken from the Solidity documentation. + // Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example + function getCodeAt(address _addr) public view returns (bytes memory o_code) { + assembly { + // retrieve the size of the code, this needs assembly + let size := extcodesize(_addr) + // allocate output byte array - this could also be done without assembly + // by using o_code = new bytes(size) + o_code := mload(0x40) + // new "memory end" including padding + mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) + // store length in memory + mstore(o_code, size) + // actually retrieve the code, this needs assembly + extcodecopy(_addr, add(o_code, 0x20), 0, size) + } + } } From dc6a6c58b2a1e6bb05b4d8efd48d0f575d47b15f Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 10:56:55 +0100 Subject: [PATCH 10/36] 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`). --- packages/protocol/test-sol/constants.sol | 29 ++++++++++++++++- .../test-sol/integration/Integration.t.sol | 32 +++---------------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/packages/protocol/test-sol/constants.sol b/packages/protocol/test-sol/constants.sol index 45bb187faf5..ad158671a95 100644 --- a/packages/protocol/test-sol/constants.sol +++ b/packages/protocol/test-sol/constants.sol @@ -1,4 +1,4 @@ -pragma solidity ^0.5.13; +pragma solidity >=0.5.13 <0.8.20; // This contract is only required for Solidity 0.5 contract Constants { @@ -21,4 +21,31 @@ contract Constants { string constant LockedGoldContract = "LockedGold"; string constant ValidatorsContract = "Validators"; string constant GovernanceContract = "Governance"; + + // Contracts 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/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 73dee963380..a72e509db8f 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -3,9 +3,9 @@ pragma solidity >=0.5.13 <0.8.20; // import "forge-std/console2.sol"; -// import { Constants } from "@test-sol/constants.sol"; -import "celo-foundry/Test.sol"; import { Utils } from "@test-sol/utils.sol"; +import { Constants } from "@test-sol/constants.sol"; + import "@celo-contracts/common/interfaces/IRegistry.sol"; import "@celo-contracts/common/interfaces/IProxy.sol"; @@ -20,38 +20,14 @@ contract IntegrationTest is Test { function setUp() public {} } -contract RegistryIntegrationTest is IntegrationTest, Utils { +contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { string[23] expectedContractsInRegistry; IProxy proxy; // TODO(Arthur): Consider moving this to a config file. Perhaps make the migration depend // on that file too? constructor() public { - expectedContractsInRegistry = [ - "Accounts", - "BlockchainParameters", - "DoubleSigningSlasher", - "DowntimeSlasher", - "Election", - "EpochRewards", - "Escrow", - "FederatedAttestations", - "FeeCurrencyWhitelist", - "FeeCurrencyDirectory", - "Freezer", - "FeeHandler", - "GoldToken", - "Governance", - "GovernanceSlasher", - "LockedGold", - "OdisPayments", - "Random", - "Registry", - "SortedOracles", - "UniswapFeeHandlerSeller", - "MentoFeeHandlerSeller", - "Validators" - ]; + expectedContractsInRegistry = contractsInRegistry; } function test_shouldHaveAddressInRegistry() public view { From d6389c827bf426a55829a411b1e21e04286877f1 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 12:44:21 +0100 Subject: [PATCH 11/36] refactor(protocol/migration_sol): uses registry array from `constants.sol` --- .../protocol/migrations_sol/Migration.s.sol | 38 +++---------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/packages/protocol/migrations_sol/Migration.s.sol b/packages/protocol/migrations_sol/Migration.s.sol index 859fccf10ac..d97b1ae6401 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 "@test-sol/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]; + 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); From ab7bab9b3d96f8974ef479aaf8386a73da2e0a8e Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 12:45:03 +0100 Subject: [PATCH 12/36] fix(test-sol/integration): adds back `Test.sol` import Mistakenly deleted earlier --- packages/protocol/test-sol/integration/Integration.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index a72e509db8f..28fa98f82dd 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -2,6 +2,7 @@ pragma solidity >=0.5.13 <0.8.20; // import "forge-std/console2.sol"; +import "celo-foundry/Test.sol"; import { Utils } from "@test-sol/utils.sol"; import { Constants } from "@test-sol/constants.sol"; From e372b8a6ebdc4c87204a294ba953383539836d91 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 12:58:04 +0100 Subject: [PATCH 13/36] refactor(migrations_sol): simplifies script that runs integration tests --- .../migrations_sol/integration_tests.sh | 66 ------------------- .../run_integration_tests_in_anvil.sh | 11 ++-- 2 files changed, 7 insertions(+), 70 deletions(-) delete mode 100755 packages/protocol/migrations_sol/integration_tests.sh diff --git a/packages/protocol/migrations_sol/integration_tests.sh b/packages/protocol/migrations_sol/integration_tests.sh deleted file mode 100755 index 0510953d470..00000000000 --- a/packages/protocol/migrations_sol/integration_tests.sh +++ /dev/null @@ -1,66 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -: ' - The following runs specific integration tests. - ' -# Run integration tests -forge test \ ---match-contract=IntegrationTest \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -: ' - The following tests assert that existing unit tests pass when run against the migrated anvil fork. - These are necessary, but not sufficient requirements. - The idea is that existing unit tests, which are known to pass in a testing environment, should - also pass if they are run against the devchain using the "--fork-url" flag. - ' -# Run tests common -# can't use gas limit because some setUp function use more than the limit -forge test \ ---match-path "test-sol/common/*" \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run tests governance/network -forge test \ ---match-path "test-sol/governance/network/*" \ ---block-gas-limit 50000000 \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run tests governance/validators -forge test \ ---match-path "test-sol/governance/validators/*" \ ---block-gas-limit 50000000 \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run tests governance/voting -# can't use gas limit because some setUp function use more than the limit -forge test \ ---match-path "test-sol/governance/voting/*" \ ---block-gas-limit 50000000 \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run tests stability -forge test \ ---match-path "test-sol/stability/*" \ ---block-gas-limit 50000000 \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run tests identity -forge test \ ---match-path "test-sol/identity/*" \ ---block-gas-limit 50000000 \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT - -# Run Everything just in case something was missed -# can't use gas limit because some setUp function use more than the limit -forge test \ --vvv \ ---fork-url http://127.0.0.1:$ANVIL_PORT #TODO this should ignore integration tests 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..3f8577dd9bb 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 \ +--match-path test-sol/integration/Integration.t.sol \ +-vvv \ +--fork-url http://127.0.0.1:$ANVIL_PORT # helper kill anvil # kill $(lsof -i tcp:$ANVIL_PORT | tail -n 1 | awk '{print $2}') From 130523ea2a27e56852ade77e9516916f6c3afc95 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 12:58:49 +0100 Subject: [PATCH 14/36] chore(test-sol/integration): commits WIP on failing bytecode test --- .../protocol/test-sol/integration/Integration.t.sol | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 28fa98f82dd..875fef35dce 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -43,18 +43,20 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { function test_shouldHaveCorrectBytecode() public { ////////////////////DEBUGGING: START///////////////////////// // SPECIFIC EXAMPLE REGISTRY.SOL BEFORE LOOPING OVER ALL CONTRACTS - string memory contractName = "Registry"; - address proxyAddress = registry.getAddressForStringOrDie(contractName); - proxy = IProxy(address(uint160(proxyAddress))); + string memory contractName = "Freezer"; ////////////////////DEBUGGING: END/////////////////////////// + // Get proxy address registered in the Registry + address proxyAddress = registry.getAddressForStringOrDie(contractName); + proxy = IProxy(address(uint160(proxyAddress))); + // Get implementation address address implementationAddress = proxy._getImplementation(); console2.log("Implementation address is :", implementationAddress); // Get bytecode from deployed contract (in Solidity 0.5) bytes memory actualBytecode = getCodeAt(implementationAddress); // IProxy.sol and Proxy.sol are 0.5 // Get bytecode from build artifacts - bytes memory expectedBytecode = vm.getDeployedCode("Registry.sol"); + bytes memory expectedBytecode = vm.getDeployedCode(string(abi.encodePacked(contractName, ".sol"))); // // Get bytecode from deployed contract (in Solidity 0.8) // bytes memory actualBytecode = implementationAddress.code; From 1e61942c5cf03a1addb8a1fe279fe61c567fa20d Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 15:41:52 +0100 Subject: [PATCH 15/36] test(test-sol): adds helper to remove metadata from bytecode Also adds docstring for better readability. --- packages/protocol/test-sol/utils.sol | 43 ++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/protocol/test-sol/utils.sol b/packages/protocol/test-sol/utils.sol index 3d95e5d5af2..e31fd9a0234 100644 --- a/packages/protocol/test-sol/utils.sol +++ b/packages/protocol/test-sol/utils.sol @@ -76,11 +76,15 @@ contract Utils is Test { (max - min + 1)) + min; } - // Get the runtime code or "deployedBytecode" at a contract address. - // Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. - // On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. - // This implementation is taken from the Solidity documentation. - // Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example + /** + * @notice Gets runtime code (or "deployedBytecode") at a contract address. + * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. + * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. + * This implementation is taken from the Solidity documentation. + * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example + * @param _addr Contract address. + * @return Runtime bytecode at contract address. + */ function getCodeAt(address _addr) public view returns (bytes memory o_code) { assembly { // retrieve the size of the code, this needs assembly @@ -96,4 +100,33 @@ contract Utils is Test { extcodecopy(_addr, add(o_code, 0x20), 0, size) } } + + /** + * @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; + } } From e55c973bdd9d3330fbcd06ff58f0765fd3981b76 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 15:42:30 +0100 Subject: [PATCH 16/36] chore(test-sol/integration): excludes failing contracts from bytecode test --- .../test-sol/integration/Integration.t.sol | 79 ++++++++++++------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 875fef35dce..02ecd120481 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -22,48 +22,67 @@ contract IntegrationTest is Test { } contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { - string[23] expectedContractsInRegistry; IProxy proxy; - // TODO(Arthur): Consider moving this to a config file. Perhaps make the migration depend - // on that file too? - constructor() public { - expectedContractsInRegistry = contractsInRegistry; - } - function test_shouldHaveAddressInRegistry() public view { - for (uint256 i = 0; i < expectedContractsInRegistry.length; i++) { - string memory contractName = expectedContractsInRegistry[i]; + 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 { - ////////////////////DEBUGGING: START///////////////////////// - // SPECIFIC EXAMPLE REGISTRY.SOL BEFORE LOOPING OVER ALL CONTRACTS - string memory contractName = "Freezer"; - ////////////////////DEBUGGING: END/////////////////////////// + function test_shouldHaveCorrectBytecode() public { + for (uint256 i = 0; i < contractsInRegistry.length; i++) { + // Read name from list of core contracts + string memory contractName = contractsInRegistry[i]; + console2.log("Contract is:", contractName); - // Get proxy address registered in the Registry - address proxyAddress = registry.getAddressForStringOrDie(contractName); - proxy = IProxy(address(uint160(proxyAddress))); - // Get implementation address - address implementationAddress = proxy._getImplementation(); - console2.log("Implementation address is :", implementationAddress); + /////////// DEBUGGING: Contracts that fail the test /////////////// + // Convert strings to hashes for comparison + bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); + 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")); + + if (hashContractName != hashAccount + && hashContractName != hashElection + && hashContractName != hashEscrow + && hashContractName != hashFederatedAttestations + && hashContractName != hashGovernance + && hashContractName != hashSortedOracles + && hashContractName != hashValidators + ) { + /////////// DEBUGGING /////////////// /////////////// /////////////// + console2.log("Checking bytecode of:", contractName); + // Get proxy address registered in the Registry + address proxyAddress = registry.getAddressForStringOrDie(contractName); + proxy = IProxy(address(uint160(proxyAddress))); - // Get bytecode from deployed contract (in Solidity 0.5) - bytes memory actualBytecode = getCodeAt(implementationAddress); // IProxy.sol and Proxy.sol are 0.5 - // Get bytecode from build artifacts - bytes memory expectedBytecode = vm.getDeployedCode(string(abi.encodePacked(contractName, ".sol"))); + // Get implementation address + address implementationAddress = proxy._getImplementation(); - // // Get bytecode from deployed contract (in Solidity 0.8) - // bytes memory actualBytecode = implementationAddress.code; - // // Get bytecode from build artifacts (in Solidity 0.8) - // bytes memory expectedBytecode = vm.getDeployedCode(string.concat(contractName, ".sol")); + // Get bytecode from deployed contract (in Solidity 0.5) + // Because IProxy.sol and Proxy.sol are 0.5 + bytes memory actualBytecodeWithMetadata = getCodeAt(implementationAddress); // + bytes memory actualBytecode = removeMetadataFromBytecode(actualBytecodeWithMetadata); + + // Get bytecode from build artifacts + bytes memory expectedBytecodeWithMetadata = vm.getDeployedCode(string(abi.encodePacked(contractName, ".sol"))); + bytes memory expectedBytecode = removeMetadataFromBytecode(expectedBytecodeWithMetadata); + + //////////////////// DEBUGGING ////////////////////// + // bool bytecodesMatch = (actualBytecode == expectedBytecode); + //////////////////// DEBUGGING ////////////////////// - // Compare the bytecodes - assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); + // Compare the bytecodes + assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); + } + } } } \ No newline at end of file From 14bd4cf12ca4b06f3518f02281a755d92e5f358c Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 16:08:04 +0100 Subject: [PATCH 17/36] chor(test-sol/integration): adds code comment for readability --- .../protocol/test-sol/integration/Integration.t.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 02ecd120481..1e935bea8bf 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -37,10 +37,9 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { for (uint256 i = 0; i < contractsInRegistry.length; i++) { // Read name from list of core contracts string memory contractName = contractsInRegistry[i]; - console2.log("Contract is:", contractName); + console2.log("Checking bytecode of:", contractName); - /////////// DEBUGGING: Contracts that fail the test /////////////// - // Convert strings to hashes for comparison + // Converting contract names to hashes for comparison bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); bytes32 hashAccount = keccak256(abi.encodePacked("Accounts")); bytes32 hashElection = keccak256(abi.encodePacked("Election")); @@ -49,7 +48,10 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { bytes32 hashGovernance = keccak256(abi.encodePacked("Governance")); bytes32 hashSortedOracles = keccak256(abi.encodePacked("SortedOracles")); bytes32 hashValidators = keccak256(abi.encodePacked("Validators")); - + + // 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 if (hashContractName != hashAccount && hashContractName != hashElection && hashContractName != hashEscrow @@ -58,8 +60,6 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { && hashContractName != hashSortedOracles && hashContractName != hashValidators ) { - /////////// DEBUGGING /////////////// /////////////// /////////////// - console2.log("Checking bytecode of:", contractName); // Get proxy address registered in the Registry address proxyAddress = registry.getAddressForStringOrDie(contractName); proxy = IProxy(address(uint160(proxyAddress))); From 075697e2f46c233c61faeb76ec0d8ceae7114573 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 16:15:27 +0100 Subject: [PATCH 18/36] chore(test-sol/integration): removing unused code comments --- .../protocol/test-sol/integration/Integration.t.sol | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 1e935bea8bf..182db0913d1 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.5.13 <0.8.20; -// import "forge-std/console2.sol"; import "celo-foundry/Test.sol"; import { Utils } from "@test-sol/utils.sol"; @@ -15,9 +14,6 @@ contract IntegrationTest is Test { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); - // address account1 = actor("account1"); - // address account2 = actor("account2"); - function setUp() public {} } @@ -67,18 +63,13 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { // Get implementation address address implementationAddress = proxy._getImplementation(); - // Get bytecode from deployed contract (in Solidity 0.5) - // Because IProxy.sol and Proxy.sol are 0.5 - bytes memory actualBytecodeWithMetadata = getCodeAt(implementationAddress); // + // Get bytecode from deployed contract + bytes memory actualBytecodeWithMetadata = getCodeAt(implementationAddress); bytes memory actualBytecode = removeMetadataFromBytecode(actualBytecodeWithMetadata); // Get bytecode from build artifacts bytes memory expectedBytecodeWithMetadata = vm.getDeployedCode(string(abi.encodePacked(contractName, ".sol"))); bytes memory expectedBytecode = removeMetadataFromBytecode(expectedBytecodeWithMetadata); - - //////////////////// DEBUGGING ////////////////////// - // bool bytecodesMatch = (actualBytecode == expectedBytecode); - //////////////////// DEBUGGING ////////////////////// // Compare the bytecodes assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); From 41bff1b8feefed6a019d2c840557665270c0fbc0 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Fri, 31 May 2024 16:37:20 +0100 Subject: [PATCH 19/36] fix(workflows/protocol_tests): excludes integration tests --- .github/workflows/protocol_tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 6d05c627da0..87158e9890e 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -94,7 +94,7 @@ 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 + run: forge test -vvv --no-match-path "test-sol/integration/Integration.t.sol" - name: Generate migrations if: success() || failure() From c7511a847f59ff9d7d12989db9ebf7c7f17558d4 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Mon, 3 Jun 2024 16:57:25 +0100 Subject: [PATCH 20/36] fix(workflows/protocol_tests): excludes integration tests more explicitly --- .github/workflows/protocol_tests.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 87158e9890e..612ede9678e 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -94,8 +94,8 @@ 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 --no-match-path "test-sol/integration/Integration.t.sol" - + run: forge test -vvv --no-match-path "test-sol/integration/Integration.t.sol" --no-match-contract "RegistryIntegrationTest" + - name: Generate migrations if: success() || failure() run: ./migrations_sol/run_integration_tests_in_anvil.sh From bb386de1ea39db2029c541b2aaedff43f56e85ec Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Mon, 3 Jun 2024 17:00:42 +0100 Subject: [PATCH 21/36] nit(test-sol/constants): improves code comment for context --- packages/protocol/test-sol/constants.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/constants.sol b/packages/protocol/test-sol/constants.sol index ad158671a95..baced966207 100644 --- a/packages/protocol/test-sol/constants.sol +++ b/packages/protocol/test-sol/constants.sol @@ -22,7 +22,7 @@ contract Constants { string constant ValidatorsContract = "Validators"; string constant GovernanceContract = "Governance"; - // Contracts in Registry.sol + // List of contracts that are expected to be in Registry.sol string[23] contractsInRegistry = [ "Accounts", "BlockchainParameters", From 1f7ab42e7fda270f1a05da8e42560fa875fed4ee Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Mon, 3 Jun 2024 18:36:18 +0100 Subject: [PATCH 22/36] 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. --- .github/workflows/protocol_tests.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 612ede9678e..a016c3c91ae 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 From 2f9f8379ae80d5f1a076a5290c936e1bc0c0477c Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 11:00:03 +0100 Subject: [PATCH 23/36] fix(workflows/protocol_tests): excludes integration tests correctly --- .github/workflows/protocol_tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index a016c3c91ae..5324922c083 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -96,7 +96,7 @@ 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 --no-match-path "test-sol/integration/Integration.t.sol" --no-match-contract "RegistryIntegrationTest" + run: forge test -vvv --no-match-contract "RegistryIntegrationTest|RandomTest|BLS12_381Passthrough" - name: Generate migrations if: success() || failure() From 30de987ccb125ed10c92f0e099d9b772a4fb00d1 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 11:06:50 +0100 Subject: [PATCH 24/36] 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 --- packages/protocol/test-sol/utils/ECDSAHelper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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"; From dcb09e16e60ac9b8f802fed0fc8002f203c40939 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 11:07:45 +0100 Subject: [PATCH 25/36] style(protocol): linting I always forget to run `yarn prettify` in the top-level directory --- .../protocol/migrations_sol/Migration.s.sol | 2 +- .../test-sol/integration/Integration.t.sol | 34 ++++++++++--------- packages/protocol/test-sol/utils.sol | 30 ++++++++-------- 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/packages/protocol/migrations_sol/Migration.s.sol b/packages/protocol/migrations_sol/Migration.s.sol index d97b1ae6401..728746b0fd9 100644 --- a/packages/protocol/migrations_sol/Migration.s.sol +++ b/packages/protocol/migrations_sol/Migration.s.sol @@ -944,7 +944,7 @@ contract Migration is Script, UsingRegistry, Constants { (bool) ); if (!skipTransferOwnership) { - // BlockchainParameters ownership transitioned to governance in a follow-up script.? + // 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); diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 182db0913d1..bece00e30a3 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -9,7 +9,6 @@ import { Constants } from "@test-sol/constants.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; import "@celo-contracts/common/interfaces/IProxy.sol"; - contract IntegrationTest is Test { address constant registryAddress = address(0x000000000000000000000000000000000000ce10); IRegistry registry = IRegistry(registryAddress); @@ -21,7 +20,7 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { IProxy proxy; function test_shouldHaveAddressInRegistry() public view { - for (uint256 i = 0; i < contractsInRegistry.length; i++) { + 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); @@ -29,13 +28,13 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { } } - function test_shouldHaveCorrectBytecode() public { - for (uint256 i = 0; i < contractsInRegistry.length; i++) { + function test_shouldHaveCorrectBytecode() public { + 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); - // Converting contract names to hashes for comparison + // Converting contract names to hashes for comparison bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); bytes32 hashAccount = keccak256(abi.encodePacked("Accounts")); bytes32 hashElection = keccak256(abi.encodePacked("Election")); @@ -47,14 +46,15 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { // 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 - if (hashContractName != hashAccount - && hashContractName != hashElection - && hashContractName != hashEscrow - && hashContractName != hashFederatedAttestations - && hashContractName != hashGovernance - && hashContractName != hashSortedOracles - && hashContractName != hashValidators + // Source: https://github.com/foundry-rs/foundry/issues/6120 + 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); @@ -66,9 +66,11 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { // Get bytecode from deployed contract bytes memory actualBytecodeWithMetadata = getCodeAt(implementationAddress); bytes memory actualBytecode = removeMetadataFromBytecode(actualBytecodeWithMetadata); - + // Get bytecode from build artifacts - bytes memory expectedBytecodeWithMetadata = vm.getDeployedCode(string(abi.encodePacked(contractName, ".sol"))); + bytes memory expectedBytecodeWithMetadata = vm.getDeployedCode( + string(abi.encodePacked(contractName, ".sol")) + ); bytes memory expectedBytecode = removeMetadataFromBytecode(expectedBytecodeWithMetadata); // Compare the bytecodes @@ -76,4 +78,4 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { } } } -} \ No newline at end of file +} diff --git a/packages/protocol/test-sol/utils.sol b/packages/protocol/test-sol/utils.sol index e31fd9a0234..b8fe82861d2 100644 --- a/packages/protocol/test-sol/utils.sol +++ b/packages/protocol/test-sol/utils.sol @@ -77,14 +77,14 @@ contract Utils is Test { } /** - * @notice Gets runtime code (or "deployedBytecode") at a contract address. - * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. - * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. - * This implementation is taken from the Solidity documentation. - * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example - * @param _addr Contract address. - * @return Runtime bytecode at contract address. - */ + * @notice Gets runtime code (or "deployedBytecode") at a contract address. + * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. + * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. + * This implementation is taken from the Solidity documentation. + * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example + * @param _addr Contract address. + * @return Runtime bytecode at contract address. + */ function getCodeAt(address _addr) public view returns (bytes memory o_code) { assembly { // retrieve the size of the code, this needs assembly @@ -102,16 +102,18 @@ contract Utils is Test { } /** - * @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. - */ + * @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])); + 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"); @@ -124,7 +126,7 @@ contract Utils is Test { // 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]; + result[i] = data[i]; } return result; From c27a53eaf1cd5729e1b785e90051eac3fb821184 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 11:41:41 +0100 Subject: [PATCH 26/36] docs(workflows/protocol_tests): adds better code comment To explain what flags and arguments are used. --- .github/workflows/protocol_tests.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 5324922c083..716e7a404f9 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -96,6 +96,11 @@ jobs: - name: Run Everything just in case something was missed # can't use gas limit because some setUp function use more than the limit + # Excludes integration tests (`RegistryIntegrationTest`), and other tests that are + # excluded globally in `foundry.toml` like `RandomTest` and `BLS12_381Passthrough`. + # Unfortunately, when you specify a `--no-match-contract` flag, it overrides any global + # exclusions specified in `foundry.toml`. Therefore, somewhat annoyingly, the exclusions here + # have to cover all cases, namely the integration tests and the global exclusions. run: forge test -vvv --no-match-contract "RegistryIntegrationTest|RandomTest|BLS12_381Passthrough" - name: Generate migrations From b9ce12943d5ae699c4867fd61fb8b1ff8bb210ed Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:34:18 +0100 Subject: [PATCH 27/36] refactor(test-sol/integration): rename variables for better readability --- .../protocol/test-sol/integration/Integration.t.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index bece00e30a3..b26e322e08a 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -64,17 +64,17 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { address implementationAddress = proxy._getImplementation(); // Get bytecode from deployed contract - bytes memory actualBytecodeWithMetadata = getCodeAt(implementationAddress); - bytes memory actualBytecode = removeMetadataFromBytecode(actualBytecodeWithMetadata); + bytes memory actualBytecodeWithMetadataOnDevchain = getCodeAt(implementationAddress); + bytes memory actualBytecodeOnDevchain = removeMetadataFromBytecode(actualBytecodeWithMetadataOnDevchain); // Get bytecode from build artifacts - bytes memory expectedBytecodeWithMetadata = vm.getDeployedCode( + bytes memory expectedBytecodeFromArtifactsWithMetadata = vm.getDeployedCode( string(abi.encodePacked(contractName, ".sol")) ); - bytes memory expectedBytecode = removeMetadataFromBytecode(expectedBytecodeWithMetadata); + bytes memory expectedBytecodeFromArtifacts = removeMetadataFromBytecode(expectedBytecodeFromArtifactsWithMetadata); // Compare the bytecodes - assertEq(actualBytecode, expectedBytecode, "Bytecode does not match"); + assertEq(actualBytecodeOnDevchain, expectedBytecodeFromArtifacts, "Bytecode does not match"); } } } From 7a740cbbecd1d607ba9caa0f6882ceb8a3a558b4 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:35:28 +0100 Subject: [PATCH 28/36] refactor(test-sol/integration): take constants out of `for`-loop --- .../test-sol/integration/Integration.t.sol | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index b26e322e08a..b7d933ad309 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -29,20 +29,21 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { } function test_shouldHaveCorrectBytecode() public { + // Converting contract names to hashes for comparison + bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); + 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); - // Converting contract names to hashes for comparison - bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); - 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")); // Skipping test for contracts that depend on linked libraries // This is a known limitation in Foundry at the moment: From 7201463652457a9cc2c08ce3281286d6808187e7 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:37:17 +0100 Subject: [PATCH 29/36] style(protocol): linting Using `yarn prettify` in the top-level directory --- .../test-sol/integration/Integration.t.sol | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index b7d933ad309..03065fc3df8 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -44,7 +44,6 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { 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 @@ -66,16 +65,24 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { // Get bytecode from deployed contract bytes memory actualBytecodeWithMetadataOnDevchain = getCodeAt(implementationAddress); - bytes memory actualBytecodeOnDevchain = removeMetadataFromBytecode(actualBytecodeWithMetadataOnDevchain); + bytes memory actualBytecodeOnDevchain = removeMetadataFromBytecode( + actualBytecodeWithMetadataOnDevchain + ); // Get bytecode from build artifacts bytes memory expectedBytecodeFromArtifactsWithMetadata = vm.getDeployedCode( string(abi.encodePacked(contractName, ".sol")) ); - bytes memory expectedBytecodeFromArtifacts = removeMetadataFromBytecode(expectedBytecodeFromArtifactsWithMetadata); + bytes memory expectedBytecodeFromArtifacts = removeMetadataFromBytecode( + expectedBytecodeFromArtifactsWithMetadata + ); // Compare the bytecodes - assertEq(actualBytecodeOnDevchain, expectedBytecodeFromArtifacts, "Bytecode does not match"); + assertEq( + actualBytecodeOnDevchain, + expectedBytecodeFromArtifacts, + "Bytecode does not match" + ); } } } From 19faa9ba6b7a80590ea4c23173d5aeedd45f891e Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 16:03:43 +0100 Subject: [PATCH 30/36] fix(test-sol/integration): add contract name back into `for`-loop --- packages/protocol/test-sol/integration/Integration.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 03065fc3df8..04bb161f05b 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -30,7 +30,6 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { function test_shouldHaveCorrectBytecode() public { // Converting contract names to hashes for comparison - bytes32 hashContractName = keccak256(abi.encodePacked(contractName)); bytes32 hashAccount = keccak256(abi.encodePacked("Accounts")); bytes32 hashElection = keccak256(abi.encodePacked("Election")); bytes32 hashEscrow = keccak256(abi.encodePacked("Escrow")); @@ -47,6 +46,7 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { // 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 && From d31c336461ea07ddb7a54b42545996b71a512ec5 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 16:35:10 +0100 Subject: [PATCH 31/36] nit(test-sol/integration): rename variable for better readability --- packages/protocol/test-sol/integration/Integration.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 04bb161f05b..6aca2dad962 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -70,11 +70,11 @@ contract RegistryIntegrationTest is IntegrationTest, Utils, Constants { ); // Get bytecode from build artifacts - bytes memory expectedBytecodeFromArtifactsWithMetadata = vm.getDeployedCode( + bytes memory expectedBytecodeWithMetadataFromArtifacts = vm.getDeployedCode( string(abi.encodePacked(contractName, ".sol")) ); bytes memory expectedBytecodeFromArtifacts = removeMetadataFromBytecode( - expectedBytecodeFromArtifactsWithMetadata + expectedBytecodeWithMetadataFromArtifacts ); // Compare the bytecodes From a7f490591dcd07d95544e40819f8ec78a08be49f Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 19:30:42 +0100 Subject: [PATCH 32/36] refactor(migrations_sol/constants): creates new `constants.sol` --- packages/protocol/foundry.toml | 3 +- .../protocol/migrations_sol/constants.sol | 30 +++++++++++++++++++ packages/protocol/test-sol/constants.sol | 29 +----------------- 3 files changed, 33 insertions(+), 29 deletions(-) create mode 100644 packages/protocol/migrations_sol/constants.sol diff --git a/packages/protocol/foundry.toml b/packages/protocol/foundry.toml index f309779040c..40e58f20dba 100644 --- a/packages/protocol/foundry.toml +++ b/packages/protocol/foundry.toml @@ -14,7 +14,8 @@ 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" 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/test-sol/constants.sol b/packages/protocol/test-sol/constants.sol index baced966207..45bb187faf5 100644 --- a/packages/protocol/test-sol/constants.sol +++ b/packages/protocol/test-sol/constants.sol @@ -1,4 +1,4 @@ -pragma solidity >=0.5.13 <0.8.20; +pragma solidity ^0.5.13; // This contract is only required for Solidity 0.5 contract Constants { @@ -21,31 +21,4 @@ contract Constants { string constant LockedGoldContract = "LockedGold"; string constant ValidatorsContract = "Validators"; string constant GovernanceContract = "Governance"; - - // 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" - ]; } From 75a4306f35fa226e6b15bebaa3f5d460d739e719 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 19:35:05 +0100 Subject: [PATCH 33/36] refactor(migrations_sol&test-sol): removes `Utils` import Adds function in `Integration.t.sol` directly to avoid 0.5.x version problems. --- .../protocol/migrations_sol/Migration.s.sol | 2 +- .../test-sol/integration/Integration.t.sol | 64 +++++++++++++++++-- packages/protocol/test-sol/utils.sol | 56 ---------------- 3 files changed, 61 insertions(+), 61 deletions(-) diff --git a/packages/protocol/migrations_sol/Migration.s.sol b/packages/protocol/migrations_sol/Migration.s.sol index 728746b0fd9..dfae2c739b3 100644 --- a/packages/protocol/migrations_sol/Migration.s.sol +++ b/packages/protocol/migrations_sol/Migration.s.sol @@ -47,7 +47,7 @@ import "@openzeppelin/contracts8/utils/math/Math.sol"; import "@celo-contracts-8/common/UsingRegistry.sol"; -import { Constants } from "@test-sol/constants.sol"; +import { Constants } from "@celo-migrations/constants.sol"; contract ForceTx { // event to trigger so a tx can be processed diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 6aca2dad962..94b06ce101a 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.5.13 <0.8.20; -import "celo-foundry/Test.sol"; +import { Test } from "forge-std-8/Test.sol"; +import "forge-std-8/console2.sol"; -import { Utils } from "@test-sol/utils.sol"; -import { Constants } from "@test-sol/constants.sol"; +import { Constants } from "@celo-migrations/constants.sol"; import "@celo-contracts/common/interfaces/IRegistry.sol"; import "@celo-contracts/common/interfaces/IProxy.sol"; @@ -14,9 +14,65 @@ contract IntegrationTest is Test { IRegistry registry = IRegistry(registryAddress); function setUp() public {} + + /** + * @notice Gets runtime code (or "deployedBytecode") at a contract address. + * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. + * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. + * This implementation is taken from the Solidity documentation. + * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example + * @param _addr Contract address. + * @return o_code The runtime bytecode at the specified contract address. + */ + function getCodeAt(address _addr) public view returns (bytes memory o_code) { + assembly { + // retrieve the size of the code, this needs assembly + let size := extcodesize(_addr) + // allocate output byte array - this could also be done without assembly + // by using o_code = new bytes(size) + o_code := mload(0x40) + // new "memory end" including padding + mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) + // store length in memory + mstore(o_code, size) + // actually retrieve the code, this needs assembly + extcodecopy(_addr, add(o_code, 0x20), 0, size) + } + } + + /** + * @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, Utils, Constants { +contract RegistryIntegrationTest is IntegrationTest, Constants { IProxy proxy; function test_shouldHaveAddressInRegistry() public view { diff --git a/packages/protocol/test-sol/utils.sol b/packages/protocol/test-sol/utils.sol index b8fe82861d2..91189b89bab 100644 --- a/packages/protocol/test-sol/utils.sol +++ b/packages/protocol/test-sol/utils.sol @@ -75,60 +75,4 @@ contract Utils is Test { (uint256(keccak256(abi.encodePacked(block.timestamp, block.difficulty, msg.sender, salt))) % (max - min + 1)) + min; } - - /** - * @notice Gets runtime code (or "deployedBytecode") at a contract address. - * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. - * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. - * This implementation is taken from the Solidity documentation. - * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example - * @param _addr Contract address. - * @return Runtime bytecode at contract address. - */ - function getCodeAt(address _addr) public view returns (bytes memory o_code) { - assembly { - // retrieve the size of the code, this needs assembly - let size := extcodesize(_addr) - // allocate output byte array - this could also be done without assembly - // by using o_code = new bytes(size) - o_code := mload(0x40) - // new "memory end" including padding - mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) - // store length in memory - mstore(o_code, size) - // actually retrieve the code, this needs assembly - extcodecopy(_addr, add(o_code, 0x20), 0, size) - } - } - - /** - * @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; - } } From 1e6b230a74030b2ec2c8a830a9b10bcd75f7e14c Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Tue, 4 Jun 2024 19:35:35 +0100 Subject: [PATCH 34/36] test(test-sol/integration): bumps solidity version to `>=0.8.7 <0.8.20` --- packages/protocol/test-sol/integration/Integration.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index 94b06ce101a..c357be99f86 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity >=0.5.13 <0.8.20; +pragma solidity >=0.8.7 <0.8.20; import { Test } from "forge-std-8/Test.sol"; import "forge-std-8/console2.sol"; From 9c913ca0e88c490813786b58850d2ddc08316e13 Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Wed, 5 Jun 2024 10:00:01 +0100 Subject: [PATCH 35/36] 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. --- .../test-sol/integration/Integration.t.sol | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/packages/protocol/test-sol/integration/Integration.t.sol b/packages/protocol/test-sol/integration/Integration.t.sol index c357be99f86..e601de7ff46 100644 --- a/packages/protocol/test-sol/integration/Integration.t.sol +++ b/packages/protocol/test-sol/integration/Integration.t.sol @@ -15,31 +15,6 @@ contract IntegrationTest is Test { function setUp() public {} - /** - * @notice Gets runtime code (or "deployedBytecode") at a contract address. - * Using the `.code` or `.runtime` property on a contract is only available in Solidity 0.8.0 and later. - * On Solity <0.8.0, inline assembly is necessary to retrieve the bytecode of a contract. - * This implementation is taken from the Solidity documentation. - * Source: https://docs.soliditylang.org/en/v0.4.24/assembly.html#example - * @param _addr Contract address. - * @return o_code The runtime bytecode at the specified contract address. - */ - function getCodeAt(address _addr) public view returns (bytes memory o_code) { - assembly { - // retrieve the size of the code, this needs assembly - let size := extcodesize(_addr) - // allocate output byte array - this could also be done without assembly - // by using o_code = new bytes(size) - o_code := mload(0x40) - // new "memory end" including padding - mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) - // store length in memory - mstore(o_code, size) - // actually retrieve the code, this needs assembly - extcodecopy(_addr, add(o_code, 0x20), 0, size) - } - } - /** * @notice Removes CBOR encoded metadata from the tail of the deployedBytecode. * @param data Bytecode including the CBOR encoded tail. @@ -120,7 +95,7 @@ contract RegistryIntegrationTest is IntegrationTest, Constants { address implementationAddress = proxy._getImplementation(); // Get bytecode from deployed contract - bytes memory actualBytecodeWithMetadataOnDevchain = getCodeAt(implementationAddress); + bytes memory actualBytecodeWithMetadataOnDevchain = implementationAddress.code; bytes memory actualBytecodeOnDevchain = removeMetadataFromBytecode( actualBytecodeWithMetadataOnDevchain ); From 255f467731edfcd12ef8de492946c66c59724d2f Mon Sep 17 00:00:00 2001 From: arthurgousset <46296830+arthurgousset@users.noreply.github.com> Date: Wed, 5 Jun 2024 12:05:25 +0100 Subject: [PATCH 36/36] 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 | 8 ++------ packages/protocol/foundry.toml | 7 +++++-- .../migrations_sol/run_integration_tests_in_anvil.sh | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/.github/workflows/protocol_tests.yml b/.github/workflows/protocol_tests.yml index 716e7a404f9..10eff4b0881 100644 --- a/.github/workflows/protocol_tests.yml +++ b/.github/workflows/protocol_tests.yml @@ -96,12 +96,8 @@ jobs: - name: Run Everything just in case something was missed # can't use gas limit because some setUp function use more than the limit - # Excludes integration tests (`RegistryIntegrationTest`), and other tests that are - # excluded globally in `foundry.toml` like `RandomTest` and `BLS12_381Passthrough`. - # Unfortunately, when you specify a `--no-match-contract` flag, it overrides any global - # exclusions specified in `foundry.toml`. Therefore, somewhat annoyingly, the exclusions here - # have to cover all cases, namely the integration tests and the global exclusions. - run: forge test -vvv --no-match-contract "RegistryIntegrationTest|RandomTest|BLS12_381Passthrough" + # 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() diff --git a/packages/protocol/foundry.toml b/packages/protocol/foundry.toml index 40e58f20dba..b6a500daa6e 100644 --- a/packages/protocol/foundry.toml +++ b/packages/protocol/foundry.toml @@ -18,9 +18,12 @@ remappings = [ '@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/run_integration_tests_in_anvil.sh b/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh index 3f8577dd9bb..af25e4b069a 100755 --- a/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh +++ b/packages/protocol/migrations_sol/run_integration_tests_in_anvil.sh @@ -8,8 +8,8 @@ source $PWD/migrations_sol/create_and_migrate_anvil_devchain.sh # Run integration tests echo "Running integration tests..." forge test \ ---match-path test-sol/integration/Integration.t.sol \ -vvv \ +--match-contract RegistryIntegrationTest \ --fork-url http://127.0.0.1:$ANVIL_PORT # helper kill anvil