release(refactor): integration overhaul #1337

0xClandestine · 2025-04-23T02:29:11Z

Motivation:

This PR refactors the integration test framework to improve performance, maintainability, and organization. The existing integration test structure had grown complex with multiple responsibilities mixed in different files, making it harder to understand and maintain. The goal is to create a more modular, better organized test framework with clear separation of concerns to make writing and maintaining integration tests easier.

Modifications:

Created two new utility files to better organize the codebase:
- IntegrationGetters.t.sol: Centralizes all state-fetching functionality, providing methods to get both current and previous state snapshots.
- IntegrationUtils.t.sol: Contains helper functions for creating users, generating allocation parameters, and other common tasks.
Restructured folder organization:
- Moved deprecated interfaces from deprecatedInterfaces/ to deprecated/.
- Relocated utility files from integration to /test/utils/ folder.
- Moved mocks from integration/mocks/ to test/mocks/.
Renamed test files to follow a more consistent naming convention:
- e.g., ALM_RegisterAndModify.t.sol → Deposit_Delegate_Modify.t.sol.
Refactored the TimeMachine.t.sol contract with improved functionality and moved it to utils/ folder to make it accessible beyond integration tests.
Added a new Constants.t.sol file to centralize commonly used constants.
Added a FOUNDRY_FUZZ_SEED to fork tests that updates weekly (this significantly reduces our RPC usage).
Moved _init into setUp() (this means setup happens once per fuzz run, rather than once per test).
Sort strategies array once, rather than JIT (sorting an array in place is time intensive).
Refactored randomness sourcing to use Vm.

Previously, we were using a uint24 _random variable combined with hashing to generate subsequent random values.
While this pattern simplified test reruns (allowing the reuse of the same _random value for consistency), it has notable drawbacks. Randomness isn't unevenly distributed, leading to modulo bias, and the process is computationally expensive.

Additionally, the same functionality can now be easily achieved through Foundry flags.

Result:

After these changes, the integration test framework will be more modular with clear separation of concerns, making it easier to understand and maintain.

Integration CI times has decreased from 15-25mins -> 5-6mins.

NOTE: There's still quite a bit that could be done, don't want this PR getting too big.

wadealexc · 2025-04-23T14:53:55Z

script/configs/holesky.toml

+
+# NOTE: Use address(0) for any null/undeployed addresses.
+
+[governance]


Hmm.. can we get Zeus to generate these for us somehow? That way it's super easy to update after a deploy/upgrade.

refactor: move time travel getters to separate file chore: wand vacuum src/test/integration --delete refactor: reorganize + rename `IntegrationCheckUtils` -> `IntegrationChecks` chore: make fmt refactor: fixed time machine + beacon chain addresses fix: ci refactor: remove old random getters refactor: randomness refactor: reorganize chore: make fmt fix: ci perf: remove some vm.assumes perf: remove unused user setup return params chore: move `TimeMachine.t.sol` -> `src/test/utils` chore: wand vacuum src/test/integration --delete perf: optimize bitmap conversion + remove shuffle function refactor: update randomization functions + add parsing methods chore: remove unused imports feat: add `Constants.t.sol` refactor: use cheats alias refactor: remove logger from `BeaconChainMock` and `TimeMachine` refactor: renaming refactor: integration deployer + logger fix: ci refactor: rename deprecatedInterfaces -> deprecated refactor: move `src/test/integration/mocks` -> `src/test/mocks` refactor: sort strategies once chore: reduce fuzz runs for time intensive tests chore: forge snapshot refactor: condense storage refactor: cleanup refactor: check_Withdrawal_AsTokens_State chore: weekly fuzz seed reduce rpc usage fix: ci chore: comment out failing tests refactor: use new assertEq array fns chore: make fmt perf: _init is called in setUp (once per run) chore: make fmt chore: only use fuzz seed for fork testing refactor: cleanup `IntegrationDeployer` chore: remove gas snapshot chore: make fmt chore: remove vm.assumes perf: alm multi single run (too expensive) perf: test_multiple_deposits single run (too expensive) perf(refactor): ALM_RegisterAndModify.t.sol chore: make fmt refactor: remove redundant checks chore: make fmt perf: single run for `test_completeCP_withNoAddedShares` (too expensive) refactor: cleanup empty space chore: mv `UpgradeTest` to `integration/tests/upgrade/` chore: mv `TypeImporter` to `interfaces/ICore.sol` chore: remove snapshots fix: tests test: compiles fix: compile errors

**Motivation:** We want to support Zeus config parsing to enable testing of queued deployments. Our original deployment parser lacked the flexibility needed to support Zeus without requiring changes to Zeus itself. Additionally, the `User` and `AVS` contracts deployed by the `IntegrationDeployer` aren't able to properly read config from `IntegrationDeployer` leading to bandaid solutions. **Modifications:** - Removed `InitialDeploymentParser`: About 1k lines -> 100 for parsing. - Added `ConfigParser`: A parser that supports both fast TOML parsing and Zeus-style parsing. - Added `ConfigGetters`: A abstraction that allows contracts deployed by the `IntegrationDeployer` to easily access the current configuration. **Result:** Contracts deployed through the `IntegrationDeployer` can now reliably access their configuration, and Zeus integration is possible without needing to modify Zeus.

**Motivation:** *Explain here the context, and why you're making that change. What is the problem you're trying to solve.* **Modifications:** *Describe the modifications you've done.* **Result:** *After your change, what will change.*

wadealexc · 2025-05-09T15:03:23Z

script/output/devnet/SLASHING_deploy_from_scratch_deployment_data.json

-    "strategyFactoryImplementation": "0x8b1F09f8292fd658Da35b9b3b1d4F7d1C0F3F592",
-    "strategyManager": "0x70f8bC2Da145b434de66114ac539c9756eF64fb3",
-    "strategyManagerImplementation": "0x1562BfE7Cb4644ff030C1dE4aA5A9aBb88a61aeC",
+    "TestToken": "0xcae746B46013Bf4D43B8E2B9e98F9DBE2461532A",


Can we nuke this file? It keeps popping up in places...

What's creating it?

Yeah pretty sure I removed the logic that generates this, think this file just needs to be removed now.

wadealexc · 2025-05-09T15:05:15Z

script/releases/v1.4.1-slashing/2-queueUpgrade.s.sol

 import "../Env.sol";
+import {Deploy} from "./1-deployContracts.s.sol";


Should not have any diffs to prior release scripts.

wadealexc · 2025-05-09T15:05:53Z

snapshots/Integration_ALM_Multi.json

@@ -1,3 +1,3 @@
 {
-  "gasUsed": "47579"
+  "gasUsed": "137087"


What is this file? Can we gitignore snapshots, or figure out what's generating this?

(is it useful/used somewhere?)

This should be fixed as well now, just need to delete this file.

wadealexc · 2025-05-09T15:08:07Z

src/test/Config.t.sol

+
+contract ConfigGettersTest is ConfigGetters, Test {
+    function setUp() public {
+        vm.createSelectFork(vm.rpcUrl("mainnet"), 22_181_590);


Let's pull this block value out into a constant and leave a comment explaining what it is.

(i assume this is post-slashing upgrade?)

Also I think this just gets run with our standard local test suite, right?

That might be annoying if someone's not trying to run fork tests, but this test fails?

Good point, will move/refactor these tests a bit.

0xClandestine · 2025-05-09T15:08:35Z

src/test/ConfigParser.t.sol

+        return proxy;
+    }
+
+    function emptyContract() external returns (address deployed) {


This should be removed, doesn't work.

wadealexc · 2025-05-09T15:11:25Z

src/test/ConfigParser.t.sol

+    using ConfigParser for *;
+
+    function test_ParseTOML() public {
+        vm.createSelectFork(vm.rpcUrl("mainnet"), 22_181_590);


See previous comments about this block value and running this by default

0xClandestine · 2025-05-09T15:12:13Z

TODO: Bump Foundry and forge-std given the latest release (includes new vm.sort and vm.shuffle cheats as well as various large compile time optimizations that ci will benefit from.

wadealexc · 2025-05-09T15:14:57Z

src/test/integration/users/User_M1.t.sol

-import "src/test/integration/deprecatedInterfaces/mainnet/IEigenPod.sol";
-import "src/test/integration/deprecatedInterfaces/mainnet/IEigenPodManager.sol";
-import "src/test/integration/deprecatedInterfaces/mainnet/IStrategyManager.sol";
+import "src/test/integration/deprecated/mainnet/IEigenPod.sol";


Let's delete User_M1 entirely. It's not used anywhere.

wadealexc · 2025-05-09T15:15:41Z

src/test/integration/users/User_M2.t.sol

-import "src/test/integration/deprecatedInterfaces/mainnet/IStrategyManager.sol";
-import "src/test/integration/deprecatedInterfaces/mainnet/IDelegationManager.sol";
-
+import "src/test/integration/deprecated/mainnet/IDelegationManager.sol";


I think we can/should delete User_M2, but it might require just a little bit more work because it's still used (though we don't need to be using it).

Let's mark this work for a followup PR.

wadealexc · 2025-05-09T15:21:56Z

src/test/integration/IntegrationChecks.t.sol

+    AVS avs;
+    SlashingParams slashParams;
+    SlashingParams slashingParams;
+
+    User operator;
+    OperatorSet operatorSet;
+    AllocateParams allocateParams;
+
+    User staker;
+    IERC20[] tokens;
+    IStrategy[] strategies;
+    uint[] initTokenBalances;
+    uint[] initDepositShares;
+    uint[] withdrawableShares;
+    // Withdrawal[] withdrawals; // cannot copy from memory to storage easily
+    bytes32[] withdrawalRoots;
+    uint[] numTokensRemaining;
+
+    uint64 beaconBalanceGwei;
+    uint40[] validators;
+    uint40[] slashedValidators;
+    uint64 slashedGwei;
+    uint64 slashedAmountGwei;


What are all these state variables?

They're shadowed throughout the contract -- can we get rid of them? Tests that need persistent state across setups can just declare it themselves.

(I know we have a bunch of tests that declare similar storage, but people are gonna declare storage variables no matter what - I think it's good to have that at the surface level)

wadealexc · 2025-05-09T15:23:29Z

src/test/integration/IntegrationChecks.t.sol

+    function _completeWithdrawal(
+        User staker,
+        User operator,
+        Withdrawal[] memory withdrawals,
+        IStrategy[] memory strategies,
+        uint[] memory withdrawalShares,
+        uint[] memory expectedTokens
+    ) internal {
+        for (uint i = 0; i < withdrawals.length; i++) {
+            if (_randBool()) {
+                staker.completeWithdrawalAsTokens(withdrawals[i]);
+                check_Withdrawal_AsTokens_State(staker, operator, withdrawals[i], withdrawalShares, expectedTokens);
+            } else {
+                staker.completeWithdrawalAsShares(withdrawals[i]);
+                check_Withdrawal_AsShares_State(staker, operator, withdrawals[i], withdrawals[i].strategies, withdrawalShares);
+            }
+        }
+    }


Is this supposed to be in this contract? It feels out of place

wadealexc · 2025-05-09T15:24:21Z

src/test/integration/IntegrationChecks.t.sol

    /**
     *
     *                              EIGENPOD CHECKS
     *
     */
-    function check_VerifyWC_State(User staker, uint40[] memory validators, uint64 beaconBalanceGwei) internal {
+    function check_VerifyWC_State(User staker, uint40[] memory validators, uint64 beaconBalanceGwei) public {


Putting a pin in this, why are these public now?

ok i figured this meant tests are calling them like checker.check_VerifyWC_State(), but they're not.

could we just keep these internal to keep the diff down, then? maybe im missing where this is needed?

wadealexc · 2025-05-09T15:30:58Z

src/test/integration/IntegrationChecks.t.sol

@@ -245,8 +283,8 @@ contract IntegrationCheckUtils is IntegrationBase {
            staker, strategies, shares, "staker should expected shares in each strategy after depositing"
        );

-        if (delegationManager.isDelegated(address(staker))) {
-            User operator = User(payable(delegationManager.delegatedTo(address(staker))));
+        if (delegationManager().isDelegated(address(staker))) {


Just noting, I feel like making these functions (instead of variables) is a slight downgrade for brevity. But I'm fine with it atm...

wadealexc · 2025-05-09T15:35:47Z

src/test/integration/IntegrationChecks.t.sol

        /// Complete withdrawal(s):
        // The staker will complete the withdrawal as shares
        //
        // ... check that the withdrawal is not pending, that the token balances of the staker and operator are unchanged,
        //     that the withdrawer received the expected shares, and that that the total shares of each o
        //     strategy withdrawn remains unchanged
-        assert_WithdrawalNotPending(delegationManager.calculateWithdrawalRoot(withdrawal), "staker withdrawal should no longer be pending");
+        assert_WithdrawalNotPending(
+            delegationManager().calculateWithdrawalRoot(withdrawal), "staker withdrawal should no longer be pending"


Followup work - we should be able to only pass in withdrawal and have the underlying method get the root.

wadealexc · 2025-05-09T15:55:33Z

src/test/integration/IntegrationDeployer.t.sol

-    function isForktest() public view returns (bool) {
-        return _hash("forktest") == _hash(cheats.envOr(string("FOUNDRY_PROFILE"), string("default")));
+    /// @dev Configures the possible asset and user types for the random users.
+    function _configRand(uint _assetTypes, uint _userTypes) internal virtual {


Seems like this method isn't used anymore - so let's just get rid of it?

wadealexc · 2025-05-09T15:59:41Z

src/test/integration/IntegrationDeployer.t.sol

+        // Whitelist the strategies
+        cheats.prank(strategyManager().strategyWhitelister());
+        strategyManager().addStrategiesToDepositWhitelist(allStrats);


You're adding the beacon chain strat to the deposit whitelist here

wadealexc · 2025-05-09T16:02:38Z

src/test/mocks/BeaconChainMock.t.sol

@@ -7,9 +7,10 @@ import "src/contracts/libraries/BeaconChainProofs.sol";
 import "src/contracts/libraries/Merkle.sol";


Request - let's please move all the beacon chain / EIP mock contracts to a subfolder, for organization's sake.

e.g:

mocks/beacon/ - BeaconChainMock.t.sol - BeaconChainMock_Deneb.t.sol - EIP_4788_Oracle_Mock.t.sol

I'm adding 2 more mocks for Pectra predeploys in MOOCOW, and having all of them in the same place is super helpful. (they're closely related anyway, since the beacon chain deploys and "drives" the predeploys)

oh yeah, and i also did some refactoring of my own and added two libraries:

The libraries also need to sit next to the beacon chain stuff. So, a subfolder is needed :D

wadealexc · 2025-05-09T16:08:36Z

src/test/integration/tests/eigenpod/EigenPod.t.sol

+    function _init() internal virtual override {
+        if (!forkConfig.supportEigenPodTests) cheats.skip(true);
+    }


Why would we not support these?

wadealexc · 2025-05-09T16:09:26Z

src/test/integration/tests/upgrade/EigenPod_Slashing_Migration.t.sol

-        assert_HasExpectedShares(staker, strategies, shares, "shares should decrease by slashed amount");
-    }
-}
+// // SPDX-License-Identifier: BUSL-1.1


Gotta fix this before we merge

wadealexc · 2025-05-09T16:09:36Z

src/test/integration/tests/upgrade/EigenPod.t.sol

-        return expectedTokenDeltas;
-    }
-}
+// // SPDX-License-Identifier: BUSL-1.1


Gotta fix this before we merge

wadealexc · 2025-05-09T16:21:35Z

src/test/integration/IntegrationDeployer.t.sol


-        cheats.createSelectFork(cheats.rpcUrl("mainnet"), mainnetForkBlock);
+    /// @dev Sets up the integration tests for mainnet.
+    function _setUpFork(bool upgrade) public virtual {


This function is pretty messy, can we clean it up?

Confusion between bool upgrade vs forkConfig.upgradeBeforeTesting vs isUpgraded

forkConfig.supportEigenPodTests evaluated in multiple places; let's please keep that to 1 (im not sure what this is for in the first place though)

First part of the method does either _executeTimelockUpgrade or _deployProxies + _upgradeProxies. Later in the method, we might also do _upgradeMainnetContracts. Is that mutually exclusive, or can that happen in addition to the earlier upgrade stuff? Logic should be more clear.

Oh, I realized _upgradeMainnetContracts is just _upgradeProxies, but disguised... let's collapse these definitions and condense upgrade logic pls

wadealexc · 2025-05-09T16:26:12Z

src/test/integration/IntegrationDeployer.t.sol

+        // Deploy ProxyAdmin, PauserRegistry, and executorMultisig.
+        config.governance.proxyAdmin = new ProxyAdmin();
+        config.governance.pauserRegistry = new PauserRegistry(PAUSER.toArray(), UNPAUSER);
+        config.governance.executorMultisig = address(proxyAdmin().owner());

        _deployProxies();


I see we got rid of _deployImplementations here, which is fine - I think moving this to _upgradeProxies is clean.

That said, it feels little inconsistent that the Config contract has fields for "impls" - but I don't think we set them anywhere. IIUC, these are only set if we're doing a fork test and they get read from config?

We should do ONE of the following:

Remove impls from config/parsing entirely. Maybe they're just not needed!

Make sure config is updated with impls no matter what env you're running in

wadealexc · 2025-05-09T16:27:34Z

src/test/integration/IntegrationDeployer.t.sol

+        // if (eq(profile, "default") || eq(profile, "mainnet") || (eq(profile, "forktest") && isUpgraded)) {
+        //     user = userType == DEFAULT ? new User(name) : User(new User_AltMethods(name));
+        // } else if (eq(profile, "forktest") && !isUpgraded) {
+        //     user = User(new User_M2(name));
+        // }


If these are fully disabled lets just completely get rid of em.

wadealexc · 2025-05-09T16:53:04Z

src/test/integration/tests/eigenpod/FullySlashed_EigenPod.t.sol

+        // Ensure the staker has at least 64 ETH to deposit.
+        if (initTokenBalances[0] < 64 ether) {
+            initTokenBalances[0] = 64 ether;
+            cheats.deal(address(staker), 64 ether);
+        }


we just need better "create user" syntax tbh. can leave that for a followup PR though.

wadealexc · 2025-05-09T16:56:59Z

src/test/integration/users/User.t.sol

-
-        _createPod();
+    constructor(string memory name) {
+        config = ConfigGetters(address(msg.sender));


Why not just inherit ConfigGetters?

I'm actually a little lost on what's going on here - are we calling methods on User when we access config.delegationManager()?

wadealexc · 2025-05-09T16:58:18Z

src/test/mocks/BeaconChainMock.t.sol

@@ -176,7 +176,7 @@ contract BeaconChainMock is Logger {
    /// - Setting their current/effective balance
    /// - Assigning them a new, unique index
    function newValidator(bytes memory withdrawalCreds) public payable returns (uint40) {
-        print.method("newValidator");
+        console.log("BeaconChain.newValidator()");


Why get rid of Logger?

Feels like we print inconsistently in a lot of places,,

wadealexc

See questions/comments above.

Overall nice work, there's a lot of good cleanup here.

0xClandestine force-pushed the release-dev/integration-overhaul branch from b942b32 to 69927a1 Compare April 23, 2025 02:30

wadealexc reviewed Apr 23, 2025

View reviewed changes

bowenli86 changed the base branch from main to release-dev/integration-overhaul April 23, 2025 16:33

bowenli86 changed the base branch from release-dev/integration-overhaul to main April 23, 2025 16:34

0xClandestine force-pushed the release-dev/integration-overhaul branch 2 times, most recently from 3432230 to 3130926 Compare April 23, 2025 18:55

0xClandestine added 16 commits April 23, 2025 16:44

chore: remove cache from .gitignore

b2ac381

refactor: lower NUM_LST_STRATS 32 -> 8

05a9d38

fix: tests

957a40e

fix: tests

79e7f40

fix: tests

9a38291

chore: reset json

4683096

chore: comment out failing tests

8741daa

fix: rebase errors

a55ad8b

fix: ci

647a23f

test: queued deployment (Layr-Labs#1307)

1fece5c

**Motivation:** *Explain here the context, and why you're making that change. What is the problem you're trying to solve.* **Modifications:** *Describe the modifications you've done.* **Result:** *After your change, what will change.*

chore: remove workflow file temporarily

21830c8

refactor: reset foundry.yml

c8d8a0b

fix: ci

4b6c636

fix: ci

ce780f8

0xClandestine force-pushed the release-dev/integration-overhaul branch from 9b9e243 to ce780f8 Compare April 23, 2025 20:44

chore: make fmt

70cf80e

0xClandestine requested a review from ypatil12 April 24, 2025 14:30

0xClandestine added 2 commits April 24, 2025 12:28

fix: ci

b21371e

fix: ci

d9c554c

0xClandestine requested review from wadealexc and 0xrajath April 28, 2025 19:20

wadealexc reviewed May 9, 2025

View reviewed changes

0xClandestine commented May 9, 2025

View reviewed changes

wadealexc reviewed May 9, 2025

View reviewed changes

wadealexc requested changes May 9, 2025

View reviewed changes


		# NOTE: Use address(0) for any null/undeployed addresses.

		[governance]

		import "../Env.sol";
		import {Deploy} from "./1-deployContracts.s.sol";

		@@ -7,9 +7,10 @@ import "src/contracts/libraries/BeaconChainProofs.sol";
		import "src/contracts/libraries/Merkle.sol";

release(refactor): integration overhaul #1337

Are you sure you want to change the base?

release(refactor): integration overhaul #1337

Uh oh!

Conversation

0xClandestine commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xClandestine May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadealexc May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xClandestine commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadealexc May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadealexc May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadealexc May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

0xClandestine commented Apr 23, 2025 •

edited

Loading

0xClandestine May 9, 2025 •

edited

Loading

wadealexc May 9, 2025 •

edited

Loading

0xClandestine commented May 9, 2025 •

edited

Loading

wadealexc May 9, 2025 •

edited

Loading

wadealexc May 9, 2025 •

edited

Loading

wadealexc May 9, 2025 •

edited

Loading