refactor: use constants across test-sol/ and migrations_sol/
#11143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To avoid import clashes when importing `TestConstants` from `test-sol` as well. Previously, both classes were called `Constants`, which leads to import clashes.
To avoid import clashes when importing `MigrationsConstants` from `migrations_sol` as well. Previously, both classes were called `Constants`, which leads to import clashes.
…move-reused-addresses-to-constants
…ts' of github.com:celo-org/celo-monorepo into arthurgousset/refactor/move-reused-addresses-to-constants
arthurgousset
commented
Jul 16, 2024
packages/protocol/test-sol/unit/common/CeloDistributionSchedule.t.sol
Outdated
Show resolved
Hide resolved
arthurgousset
commented
Jul 16, 2024
packages/protocol/test-sol/unit/common/CeloDistributionSchedule.t.sol
Outdated
Show resolved
Hide resolved
arthurgousset
commented
Jul 16, 2024
packages/protocol/test-sol/integration/RevokeCeloAfterL2Transition.sol
Outdated
Show resolved
Hide resolved
arthurgousset
commented
Jul 16, 2024
packages/protocol/test-sol/unit/common/CeloDistributionSchedule.t.sol
Outdated
Show resolved
Hide resolved
… test This test fails with this change ```diff - celoDistributionSchedule.initialize(registryAddress); + celoDistributionSchedule.initialize(REGISTRY_ADDRESS); ``` ```sh forge test -vvv \ --match-path "test-sol/unit/*" Failing tests: Encountered 1 failing test in test-sol/unit/common/CeloDistributionSchedule.t.sol:CeloDistributionScheduleTest_activate [FAIL. Reason: call reverted as expected, but without data] test_Reverts_WhenRegistryNotUpdated() (gas: 1877202) Encountered a total of 1 failing tests, 2202 tests succeeded ``` But, passes with this change: ```diff - celoDistributionSchedule.initialize(registryAddress); + celoDistributionSchedule.initialize(PROXY_ADMIN_ADDRESS); ``` ```sh $ forge test -vvv \ --match-path "test-sol/unit/*" # ... Ran 360 test suites in 4.60s (11.71s CPU time): 2203 tests passed, 0 failed, 0 skipped (2203 total tests) ``` I can sort of see why that's the case. In the current `setUp()` function the L2 is activated by arbitrarily deploying `Registry.sol` bytecode to the `proxyAdminAddress`. https://github.com/celo-org/celo-monorepo/blob/9d0276d9fd47471667ff577165f92d45e76a18c8/packages/protocol/test-sol/unit/common/CeloDistributionSchedule.t.sol#L68-L75 My hunch is that this _empty_ Registry is then used in this test. Since it's empty these two `UsingRegistry()` getters fails as expected by this test https://github.com/celo-org/celo-monorepo/blob/9d0276d9fd47471667ff577165f92d45e76a18c8/packages/protocol/contracts-0.8/common/CeloDistributionSchedule.sol#L77-L79 https://github.com/celo-org/celo-monorepo/blob/9d0276d9fd47471667ff577165f92d45e76a18c8/packages/protocol/contracts-0.8/common/UsingRegistry.sol#L82-L85 https://github.com/celo-org/celo-monorepo/blob/9d0276d9fd47471667ff577165f92d45e76a18c8/packages/protocol/contracts/common/Registry.sol#L49-L53 In my opinion, this is a janky way to test that the contract call reverts. My reasoning and things I'd change: 1. `proxyAdminAddress` will not necessarily have `Registry` bytecode. _Any_ arbitrary bytecode activates the L2. So using that `proxyAdminAddress` is a valid `Registry` is not a good assumption. 2. I'd create a new purposely empty Registry in this test, so it's obvious to the reader why the test fails. 3. I'd update the test name since it's not obvious which contract the `Registry` is missing. Presumably it's the `CeloToken`, but it's not immediately obvious from the test name. I'll probably fix this in a separate PR.
arthurgousset
changed the title
test-sol/ and migrations_sol/test-sol/ and migrations_sol/
arthurgousset
force-pushed
the
arthurgousset/refactor/move-reused-addresses-to-constants
branch
from
f42b8dc
to
47effb8
Compare
All unit tests pass ```sh forge test -vvv \ --match-path "test-sol/unit/*" ```
arthurgousset
force-pushed
the
arthurgousset/refactor/move-reused-addresses-to-constants
branch
from
e643660
to
046bf59
Compare
To avoid redefining them and having a variable name clash at compilation: ```sh Error (9097): Identifier already declared. --> test-sol/constants.sol:28:3: | 28 | address constant REGISTRY_ADDRESS = 0x000000000000000000000000000000000000ce10; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Note: The previous declaration is here: --> migrations_sol/constants.sol:33:3: | 33 | address constant REGISTRY_ADDRESS = 0x000000000000000000000000000000000000ce10; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ```
arthurgousset
changed the title
test-sol/ and migrations_sol/test-sol/ and migrations_sol/
|
CI is failing because of a compilation error: Error: Identifier not found or not unique.
test-sol/unit/governance/network/Governance.t.sol:61:34: DeclarationError: Identifier not found or not unique.
contract GovernanceTest is Test, Constants, Utils {
^-------^Source: CI It's strange because I can't see the suggested line in my diffs or the source code as it is currently committed: But it seems to be here: Edit: I wasn't looking at the right commit after merging |
…move-reused-addresses-to-constants
pahor167
approved these changes
Jul 19, 2024
…move-reused-addresses-to-constants
…onflict I made a mistake when resolving a conflict earlier and committed too much. This should fix the error, the test is all green now.
arthurgousset
deleted the
arthurgousset/refactor/move-reused-addresses-to-constants
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Housekeeping PR:
defines
REGISTRY_ADDRESSandPROXY_ADMIN_ADDRESSintest-sol/constant.solcelo-monorepo/packages/protocol/test-sol/constants.sol
Lines 28 to 29 in d43293b
refactors all test and migration files to import and use these two constants instead of defining them every time.
Other changes
None
Tested
Yes, on CI. All unit tests pass and all migration scripts work as before.
Related issues
None.
Backwards compatibility
Yes, entirely backwards compatible.
Documentation
None.