search

celo-org / celo-monorepo

Official repository for core projects comprising the Celo platform
https://celo.org
Apache License 2.0
687 stars 362 forks source link

test: add integration tests for Anvil migrations #11002

Closed arthurgousset closed 1 month ago

arthurgousset commented 2 months ago

Description

  1. adds test to assert that all core contracts on the devchain have a registered address in the Registry.
  2. adds test to assert that all core contracts on the devchain have the correct runtime bytecode, excluding those that depend on linked libraries.

[!NOTE]
We cannot currently test contracts that depend on linked libraries because vm.getDeployedCode() cannot fetch bytecode of contracts that depend on linked libraries. I created a follow-up issue with context on why this can't be done in this PR:

Other changes

  1. adds a constant in constants.sol that defines the list of contracts that are expected to be in Registry.sol. Refactors the migration and integration test scripts to import that list instead of defining the same list twice.
  2. deletes integration_tests.sh, and refactors the integration test script (run_integration_tests_in_anvil.sh) to include commands from integration_tests.sh instead.
  3. downgrades the foundry version to nightly-f625d0fa7c51e65b4bf1e8f7931cd1c6e2e285e9 (2024-04-02). This is the last foundry version that doesn't have a regression that affects the integration tests.
  4. reverts how ECDSAHelper.sol is deployed to comply with the downgraded foundry version nightly-f625d0fa7c51e65b4bf1e8f7931cd1c6e2e285e9. This reverts a change made in https://github.com/celo-org/celo-monorepo/pull/10997.

[!NOTE]
For context

  1. nightly-fbad377ab26a432e48444cf324feee1195a30960 (2024-06-04) has the regression ❌
  2. nightly-0fc39763d46b8ffab5fa4eaeb2f65ae078fa07de (2024-06-03) has the regression ❌
  3. nightly-5ac78a9cd4b94dc53d1fe5e0f42372b28b5a7559 (2024-06-02) has the regression ❌
  4. nightly-f479e945c6be78bb902df12f9d683c3bb55e3fb0 (2024-06-01) has the regression ❌
  5. nightly-cafc2606a2187a42b236df4aa65f4e8cdfcea970 (2024-05-02) has the regression ❌
  6. nightly-f625d0fa7c51e65b4bf1e8f7931cd1c6e2e285e9 (2024-04-02) doesn't have the regression ✅

Tested

Yes, these are all tests. The tests run during CI when migrations are generated and separately.

Related issues

Backwards compatibility

Yes, only adds additional tests.

Documentation

Documentation in docstrings in code.

arthurgousset commented 2 months ago

MVP test: Assert that existing tests that are known to pass also pass when run against the devchain.

  1. Run Anvil with migrations (in another terminal)

    ./migrations_sol/create_and_migrate_anvil_devchain.sh

  2. Run existing tests against local Anvil instance with --rpc-url flag. Existing tests can be found in the CI workflow protocol_tests.yml

    forge test \ 
-vvv \ 
--match-path "test-sol/common/*" \
--rpc-url=http://127.0.0.1:8546

# ... test logs
Ran 84 test suites: 409 tests passed, 0 failed, 0 skipped (409 total tests)
    forge test \
-vvv \
--block-gas-limit 50000000 \
--match-path "test-sol/governance/network/*" \
--rpc-url=http://127.0.0.1:8546

Failing tests:
Encountered 1 failing test in test-sol/governance/network/BlockchainParameters.t.sol:BlockchainParametersTest_initialize
[FAIL. Reason: log != expected log] test_Emits_UptimeLookbackWindowSet() (gas: 124192)

Encountered 1 failing test in test-sol/governance/network/BlockchainParameters.t.sol:BlockchainParametersTest_setUptimeLookbackWindow
[FAIL. Reason: log != expected log] test_Emits_UptimeLookbackWindowSet() (gas: 72044)

Encountered a total of 2 failing tests, 413 tests succeeded

This is done in https://github.com/celo-org/celo-monorepo/pull/11002/commits/cd987f334c9422e5e4a817b68a853efa7200aa7f

arthurgousset commented 2 months ago

Ideas for invariants in the integration tests:

  1. assert that the core number or list of contracts are deployed
  2. assert that they are initialized correctly
  3. assert that they are registered in the Registry
arthurgousset commented 1 month ago

As of https://github.com/celo-org/celo-monorepo/pull/11002/commits/ee24877a8482df05c316a5d6c490a5ef4971c0e8, I'm able to run end-to-end tests against a local devchain:

MVP test

This is a test run, to see how setUp() and contract definitions work using GoldToken.sol as an example.

  1. run devchain locally (in a separate terminal)
./migrations_sol/create_and_migrate_anvil_devchain.sh
  1. run MVP test against local devchain
forge test \
--match-path test-sol/integration/Integration.t.sol \
--match-contract GoldTokenTest_General \
-vvv \
--fork-url http://127.0.0.1:8546/

The tests pass

Output:

[⠰] 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)
arthurgousset commented 1 month ago

As of https://github.com/celo-org/celo-monorepo/pull/11002/commits/714bc754d1543dc82ad6320a2a3bae734707d980, I'm asserting that a list of contract names is registered in Registry.sol on the devchain. 

[PASS] test_shouldHaveAddressInRegistry() (gas: 200293)
Logs:
  Accounts address in Registry is:  0x8f24d37a4697e49AECd08d2b197E6968D2f007d3
  BlockchainParameters address in Registry is:  0xFEB14C5787fd63a417612bF8Bc2232e169A7034F
  DoubleSigningSlasher address in Registry is:  0xA920a6fF4249f9A4CDdE3D67ff6275EdA6A1D89f
  DowntimeSlasher address in Registry is:  0xAADB35CAD2f922180106F50BF7ead66C5AD0f101
  Election address in Registry is:  0x5E386A280030a076528aD192BA94D7e31090F461
  EpochRewards address in Registry is:  0xfff723D9B8f466Cd9e11BF4aaca171550aCF18Fc
  Escrow address in Registry is:  0xd773882801F417427aE5C7A032296D93Fcf11DA9
  FederatedAttestations address in Registry is:  0x93f2E9307e3003a0a10A5171478CE18796aA2333
  FeeCurrencyWhitelist address in Registry is:  0x99f389e8a9903aF72ba481e8f000e8E229e529dA
  FeeCurrencyDirectory address in Registry is:  0x42Fe5a2A61ed9705eb2F08a04A58CEB606D22f6a
  Freezer address in Registry is:  0x91dfD4C1B1262fad0f75A38D955B42b4bC586Bc1
  FeeHandler address in Registry is:  0x04dE09026097347E2D540b6fbD9dC4aae3Ab0A90
  GoldToken address in Registry is:  0xfE8CbC1cFA1b3b8256f310bdfd40E60597083448
  Governance address in Registry is:  0xcF636ab1f15d3FB684A82cB1c69977DAfDFc6a0d
  GovernanceSlasher address in Registry is:  0xE41a630372DE5B890a81B148a982b464Aa3B3625
  LockedGold address in Registry is:  0xc7F0E00681356896c06d5c810F0333ab30fBB8D1
  OdisPayments address in Registry is:  0xf7EfD92E61aB144483738B5F0f9cd46E6E9190BC
  Random address in Registry is:  0x8982140ccFb38d7Dc439f953b37829f019A3e6e5
  Registry address in Registry is:  0x000000000000000000000000000000000000ce10
  SortedOracles address in Registry is:  0xa2204011717369e044106e3bC93599E02538d65b
  UniswapFeeHandlerSeller address in Registry is:  0x27D282cBE154FD738de3242F80005cc2ca183981
  MentoFeeHandlerSeller address in Registry is:  0x677e4E735a36A7Ed935D424fCCe57a33831BF0Dc
  Validators address in Registry is:  0xA6aaDC309AA8E134d4a150eb4b58254801353FDc
arthurgousset commented 1 month ago

As of 130523e:

  1. I refactored the migration and integration test scripts, so that both import a list of contract names from constants.sol :white_check_mark:
  2. I refactored the integration test script run_integration_tests_in_anvil.sh and deleted integration_tests.sh :white_check_mark:

The only thing stopping this PR from being ready for review is the failing bytecode test.

arthurgousset commented 1 month ago

As of https://github.com/celo-org/celo-monorepo/pull/11002/commits/14bd4cf12ca4b06f3518f02281a755d92e5f358c, I'm successfully comparing bytecodes of all contracts in the Registry excluding those that depend on linked libraries.

This is a known limitation of Foundry at the moment:

More details on which contracts are failing and why > My current thinking is that these contracts fail because they depend on linked libraries. > > We know the linked libraries are: > > ``` > contracts/common/linkedlists/AddressSortedLinkedListWithMedian.sol > contracts/common/Signatures.sol:Signatures > contracts/common/linkedlists/AddressLinkedList.sol > contracts/common/linkedlists/AddressSortedLinkedList.sol > contracts/common/linkedlists/IntegerSortedLinkedList.sol > contracts/governance/Proposals.sol > ``` > > Source: Shell output from running migrations script. > > We know: > > 1. `Accounts` depends on: > ``` > import "../common/Signatures.sol"; > ``` > 2. `Election` depends on: > ``` > import "../common/linkedlists/AddressSortedLinkedList.sol"; > ``` > 3. `Escrow` depends on: > ``` > import "../common/Signatures.sol"; > ``` > 4. `FederatedAttestations` depends on: > ``` > import "../common/Signatures.sol"; > ``` > 5. `Governance` depends on: > ``` > import "../common/linkedlists/IntegerSortedLinkedList.sol"; > ``` > 6. `SortedOracles` depends on: > ``` > import "../common/linkedlists/AddressSortedLinkedListWithMedian.sol"; > ``` > 7. `Validators` depends on: > ``` > import "../common/linkedlists/AddressLinkedList.sol"; > ``` > > This is a known limitation of Foundry at the moment: > > * [Support library linking in vm.getDeployedCode foundry-rs/foundry#6120](https://github.com/foundry-rs/foundry/issues/6120) Source: https://github.com/celo-org/celo-monorepo/pull/11002#discussion_r1622563949

On that basis, the PR is ready for review pending CI passing.

openzeppelin-code[bot] commented 1 month ago

Integration tests for Anvil migrations

Generated at commit: 255f467731edfcd12ef8de492946c66c59724d2f

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total		 2
3
0
15
42
62
Dependencies Critical
High
Medium
Low
Note
Total		 0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

arthurgousset commented 1 month ago

As of https://github.com/celo-org/celo-monorepo/pull/11002/commits/dcb09e16e60ac9b8f802fed0fc8002f203c40939, the PR is all green and ready for review ✅