Skip to content

Get migrations, rebased#11593

Open
martinvol wants to merge 119 commits intorelease/core-contracts/16from
martinvol/gethMigrations
Open

Get migrations, rebased#11593
martinvol wants to merge 119 commits intorelease/core-contracts/16from
martinvol/gethMigrations

Conversation

@martinvol
Copy link
Copy Markdown
Collaborator

Forked from mc01/cr14/geth-migrations-fix-tests

Commit 6ea49d059846e2c96b2b4d375f0c319fbf1fdf08

Mc01 and others added 30 commits August 28, 2025 15:56
* WIP.

* Consitution test that fail with anvil.

* Identify part of migration script that causes tests to fail.

* Wip: fixing migration script.

* Do not rely on ffi mode.

* Fix constitution values in anvil migration.

* Port old governance tests from Truffle.

* Finish porting old governance integration tests to Foundry.

* Fix unit tests.

* Fix CI.

* Fix remappings for CI.

* Fix TS deps.

* Update comment.

Co-authored-by: pahor167 <47992132+pahor167@users.noreply.github.com>

* CR.

* Combine proxy selectors with contract selectors.

* CR.

* Isolate common part.

* Enabled detailed anvil logging in CI by default.

* Fix tests.

* CR fixes.

* Cleanup merging.

---------

Co-authored-by: pahor167 <47992132+pahor167@users.noreply.github.com>
…ns on top of CR14 (#11485)

* Update caching version and add Foundry release scripts

* prettify

* Refactor argument names in make-release scripts for consistency

* Refactor getDefaultValueForSolidityType to handle fixed arrays and improve readability

* prettify
…1488)

* Update README and Foundry configuration for contract verification

- Added Foundry verification instructions alongside Truffle in README.md
- Updated foundry.toml to include a profile for Truffle compatibility with Solidity 0.5.13
- Clarified parameter descriptions in SortedOracles contract

* revert of foundry.toml

* API key

* Update .gitignore and foundry.toml for improved build configuration and compatibility

* Update openzeppelin-contracts8 submodule to v4.9.0

* Refactor foundry.toml: reorganize profiles and update no_match_path and fs_permissions for clarity
Copy link
Copy Markdown
Contributor

@Mc01 Mc01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look on unresolved comments from previous review also 🙏
Resolve conflicts & fix up CI 😌

Except those -> ton of really good work 🙌

- name: Generate migrations on local devchain
if: success() || failure()
run: |
LOAD_STATE=${GITHUB_WORKSPACE}/celo-optimism/packages/contracts-bedrock/anvil-state.json ./scripts/foundry/create_and_migrate_anvil_devchain.sh
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe to be consistent also replace with:

Suggested change
LOAD_STATE=${GITHUB_WORKSPACE}/celo-optimism/packages/contracts-bedrock/anvil-state.json ./scripts/foundry/create_and_migrate_anvil_devchain.sh
LOAD_STATE=${{ github.workspace }}/celo-optimism/packages/contracts-bedrock/anvil-state.json ./scripts/foundry/create_and_migrate_anvil_devchain.sh

?

Comment on lines +984 to +985
// TODO: ForceTx only exists to retrieve Address from PrivateKey -> explore alternatives
address accountAddress = (new ForceTx()).identity();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe attach than following to this TODO: https://github.com/witnet/elliptic-curve-solidity ?
It is probably the only potential direction if we don't want to use cheatcodes.


CONFIG_FILE=$PWD/migrations_sol/migrationsConfig.json

ELECTIONS_ADDRESS=$(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ELECTIONS_ADDRESS=$(
ELECTION_ADDRESS=$(

Comment on lines +21 to +27
# Activate votes:
# Activate Validators
echo "Activating Validators..."
registered_validators=$(cast call \
$VALIDATORS_ADDRESS \
"getRegisteredValidators()(address[])" \
--rpc-url $ANVIL_RPC_URL)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Activate votes:
# Activate Validators
echo "Activating Validators..."
registered_validators=$(cast call \
$VALIDATORS_ADDRESS \
"getRegisteredValidators()(address[])" \
--rpc-url $ANVIL_RPC_URL)
# Activate Validators
echo "Activating Validators..."
registered_validators=$(cast call \
$VALIDATORS_ADDRESS \
"getRegisteredValidators()(address[])" \
--rpc-url $ANVIL_RPC_URL)

$ELECTIONS_ADDRESS \
"getActiveVotes()(uint256)" \
--rpc-url $ANVIL_RPC_URL)
echo "### active votes: $active_votes" No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅:

Suggested change
echo "### active votes: $active_votes"
echo "### active votes: $active_votes"

# Run migrations
echo "Running migration script... "
forge script \
# Not using the --slow flag causes the migrations to randomly hang
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still true in newest foundry version?

Comment on lines +189 to +214
// not responding un Foundry 1.5
// function beforeTestSetup(
// bytes4 _testSelector
// ) public pure virtual returns (bytes[] memory beforeCalldata_) {
// // ensure tests inherit state
// if (_testSelector == this.test_shouldUpvoteProposal.selector) {
// revert("before Test setup called");
// beforeCalldata_ = new bytes[](1);
// beforeCalldata_[0] = abi.encodePacked(this.test_shouldIncrementProposalCount.selector);
// } else if (_testSelector == this.test_shouldApproveProposal.selector) {
// beforeCalldata_ = new bytes[](2);
// beforeCalldata_[0] = abi.encodePacked(this.test_shouldIncrementProposalCount.selector);
// beforeCalldata_[1] = abi.encodePacked(this.test_shouldUpvoteProposal.selector);
// } else if (_testSelector == this.test_shouldIncrementVoteTotals.selector) {
// beforeCalldata_ = new bytes[](3);
// beforeCalldata_[0] = abi.encodePacked(this.test_shouldIncrementProposalCount.selector);
// beforeCalldata_[1] = abi.encodePacked(this.test_shouldUpvoteProposal.selector);
// beforeCalldata_[2] = abi.encodePacked(this.test_shouldApproveProposal.selector);
// } else if (_testSelector == this.test_shouldExecuteProposal.selector) {
// beforeCalldata_ = new bytes[](4);
// beforeCalldata_[0] = abi.encodePacked(this.test_shouldIncrementProposalCount.selector);
// beforeCalldata_[1] = abi.encodePacked(this.test_shouldUpvoteProposal.selector);
// beforeCalldata_[2] = abi.encodePacked(this.test_shouldApproveProposal.selector);
// beforeCalldata_[3] = abi.encodePacked(this.test_shouldIncrementVoteTotals.selector);
// }
// }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's likely a bug 🤔
For sure this should work: https://getfoundry.sh/forge/tests/writing-tests#before-test-setups
Maybe report it as an issue on upstream?

}

function test_shouldExecuteProposal() public virtual override {
function _passProposal() public {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a bit dirty 🥲

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave TODO to fix those up?

* @author cyphered.eth
*/
library SECP256K1 {
contract SECP256K1 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment a bit why such change?

json.parseRaw(".epochManager.newEpochDuration"),
(address)
);
address newEpochDuration = json.readAddress(".epochManager.newEpochDuration");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how this worked before 😶

Suggested change
address newEpochDuration = json.readAddress(".epochManager.newEpochDuration");
uint256 newEpochDuration = json.readUint(".epochManager.newEpochDuration");

// doing a native transfer is not allowed by the unreleased treasury
IERC20(registry.getAddressForStringOrDie("GoldToken")).transfer(
registry.getAddressForStringOrDie("CeloUnreleasedTreasury"),
390_000_000e18
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move to constant? Or migrationsConfig.json?

FOUNDRY_CACHE_KEY: 1
# Supported Foundry version defined at celo-org (GitHub organisation) level, for consistency across workflows.
SUPPORTED_FOUNDRY_VERSION: ${{ vars.SUPPORTED_FOUNDRY_VERSION }}
# Celo Foundry (celo-org/foundry) binaries version used for devchain anvil generation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this comment is referring to at this point?

uses: foundry-rs/foundry-toolchain@8f1998e9878d786675189ef566a2e4bf24869773
with:
version: 'v1.0.0' # TODO: revert back to env var
version: 'v1.5.0'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer want to use an env variable going forward?


- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1
uses: foundry-rs/foundry-toolchain@8f1998e9878d786675189ef566a2e4bf24869773
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a comment detailing where this hash is from and how/when to update it?

uint256 firstEpochBlock,
address[] memory firstElected
) external onlyEpochManagerEnabler {
) external onlyOwner {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contradicts the comment above. Why is the permissioned actor changed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the comment, onlyOwner will pretty much remain useful for deploying, in production this wont ever be used and the EpochManagerEnabler will be deprecated.

Comment on lines 9 to +10
import { console } from "forge-std/console.sol";
import { console2 } from "forge-std/console2.sol";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are two different logging functions necessary?

IProxyFactory proxyFactory;

uint256 proxyNonce = 0;
string json; // configuration file content
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more descriptive variable name, that won't require a comment?

Comment on lines +86 to +87
address DEPLOYER_ACCOUNT;
address SECP256K1Address;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use conventional camelCase for these variable names?

cast rpc evm_increaseTime $MS_TO_ADVANCE --rpc-url $ANVIL_RPC_URL --rpc-timeout 30000
cast rpc anvil_mine $BLOCKS_TO_ADVANCE --rpc-url $ANVIL_RPC_URL --rpc-timeout 30000

# advance 101 seconds
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect comment?

cast rpc anvil_mine $BLOCKS_TO_ADVANCE --rpc-url $ANVIL_RPC_URL --rpc-timeout 30000

# advance 101 seconds
echo `cast block --rpc-url $ANVIL_RPC_URL`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we print the latest block?

Comment on lines +7 to +10
# CUSTOM_FOUNDRY_PATH="$HOME/celo/celo-foundry/"
export FORGE=${CUSTOM_FOUNDRY_PATH:-}"forge"
export CAST=${CUSTOM_FOUNDRY_PATH:-}"cast"
export ANVIL=${CUSTOM_FOUNDRY_PATH:-}"anvil"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the CUTOM_FOUNDRY_PATH stuff be removed?

@martinvol martinvol changed the base branch from release/core-contracts/15 to release/core-contracts/16 February 9, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initialize function of EpochManager should not query the oracle address from the registry Make anvil migrations compatible with op-geth

6 participants