Skip to content

Commit 024a674

Browse files
0xDiscotech0xDiscotech
authored
fix: failing tests (#553)
1 parent 652c4cc commit 024a674

File tree

6 files changed

+135
-40
lines changed

6 files changed

+135
-40
lines changed

‎packages/contracts-bedrock/scripts/deploy/DeployConfig.s.sol‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ contract DeployConfig is Script {
8585

8686
bool public useRevenueShare;
8787
address public chainFeesRecipient;
88+
/// @notice This is not read from JSON because it is hardcoded in the deployer. It is overwritten with its setter
89+
/// for testing.
8890
address public l1FeesDepositor;
8991

9092
function read(string memory _path) public {

‎packages/contracts-bedrock/test/L1/FeesDepositor.t.sol‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ import { FeesDepositor } from "src/L1/FeesDepositor.sol";
99
import { IProxyAdmin } from "interfaces/universal/IProxyAdmin.sol";
1010
import { IProxyAdminOwnedBase } from "interfaces/L1/IProxyAdminOwnedBase.sol";
1111
import { Proxy } from "src/universal/Proxy.sol";
12+
import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
13+
import { Features } from "src/libraries/Features.sol";
14+
import { ISystemConfig } from "interfaces/L1/ISystemConfig.sol";
1215

1316
/// @title FeesDepositor_Test
1417
/// @notice Tests all functionality of FeesDepositor including receive, deposit, and setters.
@@ -32,8 +35,7 @@ contract FeesDepositor_Test is CommonTest {
3235

3336
// Deploy FeesDepositor implementation
3437
address implementation = DeployUtils.create1(
35-
"FeesDepositor",
36-
DeployUtils.encodeConstructor(abi.encodeCall(IFeesDepositor.__constructor__, ()))
38+
"FeesDepositor", DeployUtils.encodeConstructor(abi.encodeCall(IFeesDepositor.__constructor__, ()))
3739
);
3840

3941
// Deploy proxy pointing to proxyAdmin
@@ -48,13 +50,14 @@ contract FeesDepositor_Test is CommonTest {
4850

4951
// Initialize through proxy
5052
vm.prank(proxyAdminOwner);
51-
feesDepositor.initialize(
52-
minDepositAmount,
53-
l2Recipient,
54-
optimismPortal2,
55-
gasLimit,
56-
depositData
57-
);
53+
feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit, depositData);
54+
}
55+
56+
/// @notice This contract is excluded from the Initializable.t.sol test because it is not deployed as part of the
57+
/// standard deployment script and instead is deployed manually, that's why we have this test.
58+
function test_cannotReinitialize_succeeds() public {
59+
vm.expectRevert("Initializable: contract is already initialized");
60+
feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit, depositData);
5861
}
5962

6063
function testFuzz_receive_belowThreshold_succeeds(uint256 _amount) external {
@@ -77,7 +80,8 @@ contract FeesDepositor_Test is CommonTest {
7780

7881
assertTrue(success);
7982
assertEq(address(feesDepositor).balance, _amount);
80-
assertEq(address(ethLockbox).balance, 0);
83+
if (systemConfig.isFeatureEnabled(Features.ETH_LOCKBOX)) assertEq(address(ethLockbox).balance, 0);
84+
else assertEq(address(optimismPortal2).balance, 0);
8185
}
8286

8387
function testFuzz_receive_atOrAboveThreshold_succeeds(uint256 _sendAmount) external {
@@ -101,7 +105,8 @@ contract FeesDepositor_Test is CommonTest {
101105

102106
assertTrue(success);
103107
assertEq(address(feesDepositor).balance, 0);
104-
assertEq(address(ethLockbox).balance, _sendAmount);
108+
if (systemConfig.isFeatureEnabled(Features.ETH_LOCKBOX)) assertEq(address(ethLockbox).balance, _sendAmount);
109+
else assertEq(address(optimismPortal2).balance, _sendAmount);
105110
}
106111

107112
function testFuzz_receive_multipleDeposits_succeeds(uint256 _firstAmount, uint256 _secondAmount) external {
@@ -144,7 +149,8 @@ contract FeesDepositor_Test is CommonTest {
144149

145150
// Verify deposit occurred
146151
assertEq(address(feesDepositor).balance, 0);
147-
assertEq(address(ethLockbox).balance, totalAmount);
152+
if (systemConfig.isFeatureEnabled(Features.ETH_LOCKBOX)) assertEq(address(ethLockbox).balance, totalAmount);
153+
else assertEq(address(optimismPortal2).balance, totalAmount);
148154
}
149155

150156
function testFuzz_setMinDepositAmount_asOwner_succeeds(uint96 _newMinDepositAmount) external {

‎packages/contracts-bedrock/test/L2/BaseFeeVault.t.sol‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { IFeeVault } from "interfaces/L2/IFeeVault.sol";
88
import { Types } from "src/libraries/Types.sol";
99
import { Predeploys } from "src/libraries/Predeploys.sol";
1010
import { FeeVault_Test } from "test/L2/FeeVault.t.sol";
11+
import { IProxyAdmin } from "interfaces/universal/IProxyAdmin.sol";
1112

1213
/// @title BaseFeeVault_Test
1314
/// @notice Test contract for the BaseFeeVault contract's functionality
@@ -19,5 +20,7 @@ contract BaseFeeVault_Test is FeeVault_Test {
1920
feeVaultName = "BaseFeeVault";
2021
minWithdrawalAmount = deploy.cfg().baseFeeVaultMinimumWithdrawalAmount();
2122
feeVault = IFeeVault(payable(Predeploys.BASE_FEE_VAULT));
23+
// Current recipient is a contract that reverts when receiving fees, so etching empty bytes to it
24+
vm.etch(recipient, hex"");
2225
}
2326
}

‎packages/contracts-bedrock/test/L2/FeeSplitter.t.sol‎

Lines changed: 103 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ contract FeeSplitter_TestInit is CommonTest {
8787
/// @title FeeSplitter_Initialize_Test
8888
/// @notice Tests the initialization functions of the `FeeSplitter` contract.
8989
contract FeeSplitter_Initialize_Test is FeeSplitter_TestInit {
90+
event Initialized(uint8 version);
91+
9092
/// @notice Test that re-initialization fails on the already-initialized predeploy
9193
function test_reinitialization_reverts() public {
9294
// The FeeSplitter at the predeploy address is already initialized through genesis
@@ -107,6 +109,28 @@ contract FeeSplitter_Initialize_Test is FeeSplitter_TestInit {
107109
assertEq(address(IFeeSplitter(payable(impl)).sharesCalculator()), address(_defaultSharesCalculator));
108110
assertEq(IFeeSplitter(payable(impl)).feeDisbursementInterval(), 1 days);
109111
}
112+
113+
/// @notice Test that the implementation contract disables initializers in the constructor
114+
function test_feeSplitterImplementation_constructorDisablesInitializers() public {
115+
bytes memory creationCode = vm.getCode("FeeSplitter.sol:FeeSplitter");
116+
address implementation;
117+
118+
// Expect the Initialized event to be emitted
119+
vm.expectEmit(true, true, true, true);
120+
emit Initialized(type(uint8).max);
121+
122+
// Deploy the implementation contract
123+
assembly {
124+
implementation := create(0, add(creationCode, 0x20), mload(creationCode))
125+
}
126+
127+
// Verify the implementation contract is not zero address
128+
assertTrue(implementation != address(0));
129+
130+
// Verify re-initialization fails
131+
vm.expectRevert("Initializable: contract is already initialized");
132+
IFeeSplitter(payable(implementation)).initialize(ISharesCalculator(address(_defaultSharesCalculator)));
133+
}
110134
}
111135

112136
/// @title FeeSplitter_Receive_Test
@@ -123,29 +147,27 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
123147

124148
/// @notice Test receive function from non-approved vault reverts even during disbursement
125149
function testFuzz_feeSplitterReceive_WhenNonFeeVault_Reverts(address _caller, uint256 _amount) public {
126-
_amount = bound(_amount, 1 ether, type(uint256).max);
127150
vm.assume(_caller != Predeploys.SEQUENCER_FEE_WALLET);
128151
vm.assume(_caller != Predeploys.BASE_FEE_VAULT);
129152
vm.assume(_caller != Predeploys.OPERATOR_FEE_VAULT);
130153
vm.assume(_caller != Predeploys.L1_FEE_VAULT);
131-
vm.assume(_caller != address(0));
132154

133155
// Mock the _isTransientDisbursing() function to return true
134156
// This allows us to test the sender validation logic
135157
vm.mockCall(address(feeSplitter), abi.encodeWithSignature("_isTransientDisbursing()"), abi.encode(true));
136158

137159
// Setup disbursement conditions but expect revert from non-approved sender
138160
vm.deal(_caller, _amount);
161+
vm.startPrank(_caller);
139162

140-
vm.prank(_caller);
141163
// Now we test the actual sender validation
142164
vm.expectRevert(IFeeSplitter.FeeSplitter_SenderNotApprovedVault.selector);
143165
payable(address(feeSplitter)).call{ value: _amount }("");
144166
}
145167

146168
/// @notice Test receive function works during disbursement from SequencerFeeVault
147169
function test_feeSplitterReceive_SequencerFeeVault_Succeeds(uint256 _amount) public {
148-
_amount = bound(_amount, 1 ether, type(uint256).max);
170+
_amount = bound(_amount, 1, type(uint256).max);
149171

150172
// Setup mocks - only sequencer vault has balance
151173
_mockFeeVaultForSuccessfulWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, _amount);
@@ -179,7 +201,7 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
179201

180202
/// @notice Test receive function works during disbursement from BaseFeeVault
181203
function test_feeSplitterReceive_BaseFeeVault_Succeeds(uint256 _amount) public {
182-
_amount = bound(_amount, 1 ether, type(uint256).max);
204+
_amount = bound(_amount, 1, type(uint256).max);
183205

184206
// Setup mocks - only sequencer vault has balance
185207
_mockFeeVaultForSuccessfulWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, 0);
@@ -213,7 +235,7 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
213235

214236
/// @notice Test receive function works during disbursement from L1FeeVault
215237
function test_feeSplitterReceive_L1FeeVault_Succeeds(uint256 _amount) public {
216-
_amount = bound(_amount, 1 ether, type(uint256).max);
238+
_amount = bound(_amount, 1, type(uint256).max);
217239

218240
// Setup mocks - only sequencer vault has balance
219241
_mockFeeVaultForSuccessfulWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, 0);
@@ -247,7 +269,7 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
247269

248270
/// @notice Test receive function works during disbursement from OperatorFeeVault
249271
function test_feeSplitterReceive_OperatorFeeVault_Succeeds(uint256 _amount) public {
250-
_amount = bound(_amount, 1 ether, type(uint256).max);
272+
_amount = bound(_amount, 1, type(uint256).max);
251273

252274
// Setup mocks - only sequencer vault has balance
253275
_mockFeeVaultForSuccessfulWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, 0);
@@ -296,7 +318,7 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
296318
function test_feeSplitterDisburseFees_WhenNoFeesCollected_Reverts() public {
297319
_setupStandardFeeVaultMocks(0, 0, 0, 0);
298320

299-
vm.warp(block.timestamp + 25 hours);
321+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
300322
vm.expectRevert(IFeeSplitter.FeeSplitter_NoFeesCollected.selector);
301323
feeSplitter.disburseFees();
302324
}
@@ -310,7 +332,7 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
310332
abi.encode(Types.WithdrawalNetwork.L1)
311333
);
312334

313-
vm.warp(block.timestamp + 25 hours);
335+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
314336
vm.expectRevert(IFeeSplitter.FeeSplitter_FeeVaultMustWithdrawToL2.selector);
315337
feeSplitter.disburseFees();
316338
}
@@ -327,7 +349,7 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
327349
Predeploys.SEQUENCER_FEE_WALLET, abi.encodeCall(IFeeVault.recipient, ()), abi.encode(address(0x123))
328350
);
329351

330-
vm.warp(block.timestamp + 25 hours);
352+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
331353
vm.expectRevert(IFeeSplitter.FeeSplitter_FeeVaultMustWithdrawToFeeSplitter.selector);
332354
feeSplitter.disburseFees();
333355
}
@@ -363,7 +385,7 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
363385
);
364386

365387
// Fast forward time to allow disbursement
366-
vm.warp(block.timestamp + 25 hours);
388+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
367389

368390
// Store initial balances
369391
uint256 revenueShareRecipientBalanceBefore = address(_defaultRevenueShareRecipient).balance;
@@ -391,6 +413,68 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
391413
// Verify the fee splitter has no balance
392414
assertEq(address(feeSplitter).balance, 0);
393415
}
416+
417+
/// @notice Test disburseFees reverts when shares calculator returns an empty array
418+
function test_feeSplitterDisburseFees_WhenSharesInfoEmpty_Reverts() public {
419+
uint256 _sequencerAmount = 2 ether;
420+
_setupStandardFeeVaultMocks(_sequencerAmount, 0, 0, 0);
421+
422+
address actualSharesCalculator = address(feeSplitter.sharesCalculator());
423+
ISharesCalculator.ShareInfo[] memory emptyShareInfo = new ISharesCalculator.ShareInfo[](0);
424+
vm.mockCall(
425+
actualSharesCalculator,
426+
abi.encodeCall(ISharesCalculator.getRecipientsAndAmounts, (_sequencerAmount, 0, 0, 0)),
427+
abi.encode(emptyShareInfo)
428+
);
429+
430+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
431+
432+
vm.expectRevert(IFeeSplitter.FeeSplitter_FeeShareInfoEmpty.selector);
433+
feeSplitter.disburseFees();
434+
}
435+
436+
/// @notice Test disburseFees reverts when sending to a recipient fails
437+
function test_feeSplitterDisburseFees_WhenSendingFails_Reverts() public {
438+
uint256 _sequencerAmount = 1 ether;
439+
_setupStandardFeeVaultMocks(_sequencerAmount, 0, 0, 0);
440+
441+
address revertingRecipient = address(new RevertingRecipient());
442+
ISharesCalculator.ShareInfo[] memory shareInfo = new ISharesCalculator.ShareInfo[](1);
443+
shareInfo[0] = ISharesCalculator.ShareInfo(payable(revertingRecipient), _sequencerAmount);
444+
445+
address actualSharesCalculator = address(feeSplitter.sharesCalculator());
446+
vm.mockCall(
447+
actualSharesCalculator,
448+
abi.encodeCall(ISharesCalculator.getRecipientsAndAmounts, (_sequencerAmount, 0, 0, 0)),
449+
abi.encode(shareInfo)
450+
);
451+
452+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
453+
454+
vm.expectRevert(IFeeSplitter.FeeSplitter_FailedToSendToRevenueShareRecipient.selector);
455+
feeSplitter.disburseFees();
456+
}
457+
458+
/// @notice Test disburseFees reverts when total shares do not match gross revenue
459+
function test_feeSplitterDisburseFees_WhenSharesMalformed_Reverts() public {
460+
uint256 _sequencerAmount = 1 ether;
461+
_setupStandardFeeVaultMocks(_sequencerAmount, 0, 0, 0);
462+
463+
ISharesCalculator.ShareInfo[] memory shareInfo = new ISharesCalculator.ShareInfo[](1);
464+
shareInfo[0] = ISharesCalculator.ShareInfo(payable(_defaultRevenueShareRecipient), _sequencerAmount - 1);
465+
466+
address actualSharesCalculator = address(feeSplitter.sharesCalculator());
467+
vm.mockCall(
468+
actualSharesCalculator,
469+
abi.encodeCall(ISharesCalculator.getRecipientsAndAmounts, (_sequencerAmount, 0, 0, 0)),
470+
abi.encode(shareInfo)
471+
);
472+
473+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
474+
475+
vm.expectRevert(IFeeSplitter.FeeSplitter_SharesCalculatorMalformedOutput.selector);
476+
feeSplitter.disburseFees();
477+
}
394478
}
395479

396480
/// @title FeeSplitter_SetSharesCalculator_Test
@@ -500,7 +584,7 @@ contract FeeSplitter_DisburseFees_TestFail is FeeSplitter_TestInit {
500584
// Override the selected vault with insufficient balance
501585
_setFeeVaultData(vaults[_vaultIndex], insufficientBalance, _minWithdrawalAmount);
502586

503-
vm.warp(block.timestamp + 25 hours);
587+
vm.warp(block.timestamp + feeSplitter.feeDisbursementInterval() + 1);
504588

505589
// The entire disbursement should revert because one vault doesn't meet its minimum
506590
vm.expectRevert("FeeVault: withdrawal amount must be greater than minimum withdrawal amount");
@@ -625,3 +709,10 @@ contract MockFeeVault {
625709
return value;
626710
}
627711
}
712+
713+
/// @notice Helper recipient that always reverts on receiving ETH
714+
contract RevertingRecipient {
715+
receive() external payable {
716+
revert("RevertingRecipient: cannot accept ETH");
717+
}
718+
}

‎packages/contracts-bedrock/test/L2/FeeVault.t.sol‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,7 @@ abstract contract FeeVault_Test is CommonTest {
4747
_name: feeVaultName,
4848
_args: DeployUtils.encodeConstructor(
4949
abi.encodeCall(
50-
IFeeVault.__constructor__,
51-
(
52-
recipient,
53-
minWithdrawalAmount,
54-
Types.WithdrawalNetwork.L2
55-
)
50+
IFeeVault.__constructor__, (recipient, minWithdrawalAmount, Types.WithdrawalNetwork.L2)
5651
)
5752
)
5853
})
@@ -61,7 +56,7 @@ abstract contract FeeVault_Test is CommonTest {
6156
}
6257

6358
/// @notice Tests that the l1 fee wallet is correct.
64-
function test_constructor_succeeds() external view virtual {
59+
function test_constructor_succeeds() external view {
6560
assertEq(feeVault.RECIPIENT(), recipient);
6661
assertEq(feeVault.recipient(), recipient);
6762
assertEq(feeVault.MIN_WITHDRAWAL_AMOUNT(), minWithdrawalAmount);
@@ -136,7 +131,7 @@ abstract contract FeeVault_Test is CommonTest {
136131
}
137132

138133
/// @notice Tests that `withdraw` successfully initiates a withdrawal to L2.
139-
function test_withdraw_toL2_succeeds() external {
134+
function test_withdraw_toL2_succeeds() public {
140135
_setupL2Withdrawal();
141136

142137
uint256 amount = feeVault.MIN_WITHDRAWAL_AMOUNT() + 1;
@@ -148,9 +143,7 @@ abstract contract FeeVault_Test is CommonTest {
148143
vm.expectEmit(address(address(feeVault)));
149144
emit Withdrawal(address(feeVault).balance, feeVault.RECIPIENT(), address(this));
150145
vm.expectEmit(address(address(feeVault)));
151-
emit Withdrawal(
152-
address(feeVault).balance, feeVault.RECIPIENT(), address(this), Types.WithdrawalNetwork.L2
153-
);
146+
emit Withdrawal(address(feeVault).balance, feeVault.RECIPIENT(), address(this), Types.WithdrawalNetwork.L2);
154147

155148
// The entire feeVault's balance is withdrawn
156149
vm.expectCall(recipient, address(feeVault).balance, bytes(""));
@@ -318,9 +311,8 @@ abstract contract FeeVault_Test is CommonTest {
318311
assertEq(uint8(feeVault.withdrawalNetwork()), uint8(immutableValue));
319312

320313
// Set a different value via owner (toggle between L1 and L2)
321-
Types.WithdrawalNetwork newValue = immutableValue == Types.WithdrawalNetwork.L1
322-
? Types.WithdrawalNetwork.L2
323-
: Types.WithdrawalNetwork.L1;
314+
Types.WithdrawalNetwork newValue =
315+
immutableValue == Types.WithdrawalNetwork.L1 ? Types.WithdrawalNetwork.L2 : Types.WithdrawalNetwork.L1;
324316
vm.prank(owner);
325317
IFeeVault(payable(address(feeVault))).setWithdrawalNetwork(newValue);
326318

@@ -329,4 +321,4 @@ abstract contract FeeVault_Test is CommonTest {
329321
assertNotEq(uint8(feeVault.withdrawalNetwork()), uint8(immutableValue));
330322
assertEq(uint8(feeVault.WITHDRAWAL_NETWORK()), uint8(immutableValue)); // immutable unchanged
331323
}
332-
}
324+
}

‎packages/contracts-bedrock/test/vendor/Initializable.t.sol‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@ contract Initializer_Test is CommonTest {
378378
function test_cannotReinitialize_succeeds() public {
379379
// Collect exclusions.
380380
uint256 j;
381-
string[] memory excludes = new string[](11);
381+
string[] memory excludes = new string[](12);
382382
// Contract is currently not being deployed as part of the standard deployment script.
383383
excludes[j++] = "src/L2/OptimismSuperchainERC20.sol";
384384
// Periphery contracts don't get deployed as part of the standard deployment script.
@@ -400,6 +400,7 @@ contract Initializer_Test is CommonTest {
400400
excludes[j++] = "src/L1/OptimismPortalInterop.sol";
401401
// L2 contract initialization is tested in Predeploys.t.sol
402402
excludes[j++] = "src/L2/*";
403+
excludes[j++] = "src/L1/FeesDepositor.sol";
403404

404405
// Get all contract names in the src directory, minus the excluded contracts.
405406
string[] memory contractNames = ForgeArtifacts.getContractNames("src/*", excludes);

0 commit comments

Comments
 (0)