Skip to content

fix: remove tech debt in solidity tests #739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shaspitz merged 6 commits into from
Jul 24, 2025
Merged

fix: remove tech debt in solidity tests #739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa38a6e
wip
shaspitz Jul 24, 2025
d9b82dc
Update ci.yml
shaspitz Jul 24, 2025
3008961
Update ci.yml
shaspitz Jul 24, 2025
3fbf84e
Update ProviderRegistryTest.sol
shaspitz Jul 24, 2025
5579dc0
Update ProviderRegistryTest.sol
shaspitz Jul 24, 2025
363a4d7
fixed remaining tests
shaspitz Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ jobs:
- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1
with:
version: nightly-e649e62f125244a3ef116be25dfdc81a2afbaf2a
version: v1.2.3

- name: Print Versions
run: |
Expand Down
72 changes: 30 additions & 42 deletions contracts/test/core/BidderRegistryTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,25 +100,23 @@ contract BidderRegistryTest is Test {
assertEq(bidderStakeStored2, 2 ether);
}

function testFail_BidderStakeAndRegisterAlreadyRegistered() public {
function test_TwoDeposits() public {
vm.prank(bidder);
bidderRegistry.depositForWindow{value: 2e18 wei}(2);
vm.expectRevert(bytes(""));
bidderRegistry.depositForWindow{value: 1 wei}(2);
}

function testFail_Receive() public {
function test_BidderRegistryReceive() public {
vm.prank(bidder);
vm.expectRevert(bytes(""));
(bool success, ) = address(bidderRegistry).call{value: 1 wei}("");
require(success, "couldn't transfer to bidder");
vm.expectRevert();
payable(address(bidderRegistry)).transfer(1 wei);
}

function testFail_Fallback() public {
function test_BidderRegistryFallback() public {
vm.prank(bidder);
vm.expectRevert(bytes(""));
(bool success, ) = address(bidderRegistry).call{value: 1 wei}("");
require(success, "couldn't transfer to bidder");
bytes memory data = abi.encode(1, 2);
(bool success, ) = address(bidderRegistry).call{value: 1 wei}(data);
require(!success, "should revert");
}

function test_SetNewProtocolFeeRecipient() public {
Expand All @@ -131,9 +129,10 @@ contract BidderRegistryTest is Test {
assertEq(recipient, newRecipient);
}

function testFail_SetNewProtocolFeeRecipient() public {
function test_RevertWhen_SetNewProtocolFeeRecipient() public {
address newRecipient = vm.addr(2);
vm.expectRevert(bytes(""));
vm.prank(vm.addr(1));
vm.expectRevert();
bidderRegistry.setNewProtocolFeeRecipient(newRecipient);
}

Expand All @@ -146,8 +145,9 @@ contract BidderRegistryTest is Test {
assertEq(payoutPeriodBlocks, 890);
}

function testFail_SetNewFeePayoutPeriodBlocks() public {
vm.expectRevert(bytes(""));
function test_RevertWhen_SetNewFeePayoutPeriodBlocks() public {
vm.prank(vm.addr(1));
vm.expectRevert();
bidderRegistry.setNewFeePayoutPeriod(83424);
}

Expand All @@ -157,8 +157,9 @@ contract BidderRegistryTest is Test {
assertEq(bidderRegistry.feePercent(), uint16(25));
}

function testFail_SetNewFeePercent() public {
vm.expectRevert(bytes(""));
function test_RevertWhen_SetNewFeePercent() public {
vm.prank(vm.addr(1));
vm.expectRevert();
bidderRegistry.setNewFeePercent(uint16(25));
}

Expand All @@ -169,10 +170,11 @@ contract BidderRegistryTest is Test {
assertEq(bidderRegistry.preconfManager(), newPreConfContract);
}

function testFail_SetPreconfManager() public {
vm.prank(address(this));
vm.expectRevert(bytes(""));
bidderRegistry.setPreconfManager(address(0));
function test_RevertWhen_SetPreconfManager() public {
vm.prank(vm.addr(1));
address newPreConfContract = vm.addr(3);
vm.expectRevert();
bidderRegistry.setPreconfManager(newPreConfContract);
}

function test_shouldRetrieveFunds() public {
Expand Down Expand Up @@ -255,35 +257,20 @@ contract BidderRegistryTest is Test {
assertEq(bidderRegistry.lockedFunds(bidder, nextWindow), 63 ether);
}

function testFail_shouldRetrieveFundsNotPreConf() public {
function test_RevertWhen_RetrieveFundsNotPreConf() public {
vm.prank(bidder);
uint256 currentWindow = blockTracker.getCurrentWindow();
uint256 nextWindow = currentWindow + 1;
uint64 blockNumber = 66;
bidderRegistry.depositForWindow{value: 2 ether}(nextWindow);
address provider = vm.addr(4);
vm.expectRevert(bytes(""));
bytes32 commitmentDigest = keccak256("1234");
vm.prank(bidderRegistry.preconfManager());
bidderRegistry.openBid(commitmentDigest, 1 ether, bidder, blockNumber);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), bidderRegistry.ONE_HUNDRED_PERCENT());
}

function testFail_shouldRetrieveFundsGreaterThanStake() public {
Copy link
Copy Markdown
Contributor Author

@shaspitz shaspitz Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This test was outdated, it's testing for a revert that no longer happens. See

mev-commit/contracts/contracts/core/BidderRegistry.sol

Line 266 in 2d71b14

bidAmt = availableAmount;

vm.prank(address(this));
bidderRegistry.setPreconfManager(address(this));

vm.prank(bidder);
uint256 currentWindow = blockTracker.getCurrentWindow();
uint256 nextWindow = currentWindow + 1;
uint64 blockNumber = 66;
bidderRegistry.depositForWindow{value: 2 ether}(nextWindow);

address provider = vm.addr(4);
vm.expectRevert(bytes(""));
vm.prank(address(this));
bytes32 commitmentDigest = keccak256("1234");
bidderRegistry.openBid(commitmentDigest, 3 ether, bidder, blockNumber);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), bidderRegistry.ONE_HUNDRED_PERCENT());
vm.prank(vm.addr(1));
uint256 residualBidAfterDecay = bidderRegistry.ONE_HUNDRED_PERCENT();
vm.expectRevert();
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), residualBidAfterDecay);
}

function test_withdrawProviderAmount() public {
Expand All @@ -308,13 +295,14 @@ contract BidderRegistryTest is Test {
assertEq(bidderRegistry.providerAmount(provider), 0);
}

function testFail_withdrawProviderAmount() public {
function test_RevertWhen_WithdrawProviderAmount() public {
bidderRegistry.setPreconfManager(address(this));
uint256 currentWindow = blockTracker.getCurrentWindow();
uint256 nextWindow = currentWindow + 1;
vm.prank(bidder);
bidderRegistry.depositForWindow{value: 5 ether}(nextWindow);
address provider = vm.addr(4);
vm.expectRevert();
bidderRegistry.withdrawProviderAmount(payable(provider));
}

Expand Down
81 changes: 42 additions & 39 deletions contracts/test/core/ProviderRegistryTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ contract ProviderRegistryTest is Test {
hex"bbbbbbbbb1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2";
bytes[] public validBLSPubkeys = [validBLSPubkey];
uint256 public penaltyFeePayoutPeriodMs;
mapping(address => uint256) public commitmentsCount; // For use in test_RevertWhen_WithdrawStakedAmountWithoutCommitments
event ProviderRegistered(address indexed provider, uint256 stakedAmount);
event WithdrawalRequested(address indexed provider, uint256 timestamp);
event WithdrawalCompleted(address indexed provider, uint256 amount);
Expand Down Expand Up @@ -134,19 +135,10 @@ contract ProviderRegistryTest is Test {
assertEq(accumulatedAmount, 0);
}

function testFail_ProviderStakeAndRegisterMinStake() public {
function test_RevertWhen_ProviderStakeAndRegisterMinStake() public {
vm.deal(provider, 3 ether);
vm.prank(provider);
vm.expectRevert(bytes(""));
providerRegistry.registerAndStake{value: 1 wei}();
}

function testFail_ProviderStakeAndRegisterInvalidBLSKey() public {
Comment thread
kant777 marked this conversation as resolved.
vm.deal(provider, 3 ether);
vm.prank(provider);
vm.expectRevert("Invalid BLS public key length");
bytes[] memory invalidBLSPubkeys = new bytes[](1);
invalidBLSPubkeys[0] = abi.encodePacked(uint256(134));
vm.expectRevert();
providerRegistry.registerAndStake{value: 1 wei}();
}

Expand Down Expand Up @@ -196,29 +188,27 @@ contract ProviderRegistryTest is Test {
}
}

function testFail_ProviderStakeAndRegisterAlreadyRegistered() public {
function test_RevertWhen_ProviderStakeAndRegisterAlreadyRegistered() public {
vm.deal(provider, 3 ether);
vm.prank(provider);
providerRegistry.registerAndStake{value: 2e18 wei}();

vm.expectRevert(bytes(""));
vm.expectRevert();
vm.prank(provider);
providerRegistry.registerAndStake{value: 1 wei}();
}

function testFail_Receive() public {
vm.deal(provider, 3 ether);
function test_RevertWhen_Receive() public {
vm.prank(provider);
vm.expectRevert(bytes(""));
(bool success, ) = address(providerRegistry).call{value: 1 wei}("");
require(success, "Couldn't transfer to provider");
vm.expectRevert();
payable(address(providerRegistry)).transfer(1 wei);
}

function testFail_Fallback() public {
vm.deal(provider, 3 ether);
function test_RevertWhen_Fallback() public {
vm.prank(provider);
vm.expectRevert(bytes(""));
(bool success, ) = address(providerRegistry).call{value: 1 wei}("");
require(success, "Couldn't transfer to provider");
bytes memory data = abi.encode(1, 2);
(bool success, ) = address(providerRegistry).call{value: 1 wei}(data);
require(!success, "should revert");
}

function test_SetNewPenaltyFeeRecipient() public {
Expand All @@ -231,9 +221,10 @@ contract ProviderRegistryTest is Test {
assertEq(recipient, newRecipient);
}

function testFail_SetNewPenaltyFeeRecipient() public {
function test_RevertWhen_SetNewPenaltyFeeRecipient() public {
address newRecipient = vm.addr(2);
vm.expectRevert(bytes(""));
vm.expectRevert();
vm.prank(vm.addr(1));
providerRegistry.setNewPenaltyFeeRecipient(newRecipient);
}

Expand All @@ -247,8 +238,9 @@ contract ProviderRegistryTest is Test {
assertEq(payoutPeriodMs, 890);
}

function testFail_SetNewFeePayoutPeriod() public {
vm.expectRevert(bytes(""));
function test_RevertWhen_SetNewFeePayoutPeriod() public {
vm.expectRevert();
vm.prank(vm.addr(1));
providerRegistry.setFeePayoutPeriod(83424);
}

Expand All @@ -259,8 +251,9 @@ contract ProviderRegistryTest is Test {
assertEq(providerRegistry.feePercent(), 25);
}

function testFail_SetNewFeePercent() public {
vm.expectRevert(bytes(""));
function test_RevertWhen_SetNewFeePercent() public {
vm.expectRevert();
vm.prank(vm.addr(1));
providerRegistry.setNewFeePercent(25);
}

Expand All @@ -272,9 +265,9 @@ contract ProviderRegistryTest is Test {
assertEq(providerRegistry.preconfManager(), newPreConfContract);
}

function testFail_SetPreConfContract() public {
vm.prank(address(this));
vm.expectRevert(bytes(""));
function test_RevertWhen_SetPreConfContract() public {
vm.prank(vm.addr(1));
vm.expectRevert();
providerRegistry.setPreconfManager(address(0));
}

Expand Down Expand Up @@ -321,19 +314,20 @@ contract ProviderRegistryTest is Test {
assertEq(providerRegistry.providerStakes(provider), 0.9 ether);
}

function testFail_ShouldRetrieveFundsNotPreConf() public {
function test_RevertWhen_ShouldRetrieveFundsNotPreConf() public {
vm.deal(provider, 3 ether);
vm.prank(provider);
providerRegistry.registerAndStake{value: 2 ether}();

address bidder = vm.addr(4);
vm.expectRevert(bytes(""));
uint256 residualBidPercentAfterDecay = providerRegistry.ONE_HUNDRED_PERCENT();
vm.expectRevert();
providerRegistry.slash(
1 ether,
0,
provider,
payable(bidder),
providerRegistry.ONE_HUNDRED_PERCENT()
residualBidPercentAfterDecay
);
}

Expand Down Expand Up @@ -509,15 +503,19 @@ contract ProviderRegistryTest is Test {
);
}

function testFail_WithdrawStakedAmountUnauthorized() public {
function test_RevertWhen_WithdrawStakedAmountUnauthorized() public {
address newProvider = vm.addr(8);
vm.deal(newProvider, 3 ether);
vm.prank(newProvider);
providerRegistry.registerAndStake{value: 2e18 wei}();

vm.expectRevert(bytes(""));
address wrongNewProvider = vm.addr(12);
vm.prank(wrongNewProvider);
vm.expectRevert();
providerRegistry.unstake();

vm.prank(wrongNewProvider);
vm.expectRevert();
providerRegistry.withdraw();
}

Expand All @@ -539,15 +537,20 @@ contract ProviderRegistryTest is Test {
);
}

function testFail_WithdrawStakedAmountWithoutCommitments() public {
function test_RevertWhen_WithdrawStakedAmountWithoutCommitments() public {
providerRegistry.setPreconfManager(address(this));
commitmentsCount[vm.addr(8)] = 2;

address newProvider = vm.addr(8);
vm.deal(newProvider, 3 ether);
vm.prank(newProvider);
providerRegistry.registerAndStake{value: 2e18 wei}();

vm.prank(newProvider);
providerRegistry.unstake();
vm.warp(block.timestamp + 24 hours); // Move forward in time
vm.expectRevert("Provider Commitments still pending");
vm.expectRevert(abi.encodeWithSelector(IProviderRegistry.ProviderCommitmentsPending.selector, newProvider, 2));
vm.prank(newProvider);
providerRegistry.withdraw();
}

Expand Down
14 changes: 8 additions & 6 deletions contracts/test/standard-bridge/L1GatewayTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,15 @@ contract L1GatewayTest is Test {
assertEq(l1Gateway.transferInitiatedIdx(), 0);
assertEq(l1Gateway.transferFinalizedIdx(), 1);

vm.expectRevert();
vm.prank(bridgeUser);
l1Gateway.initiateTransfer{value: 0.9 ether}(bridgeUser, 0.9 ether);

assertEq(address(bridgeUser).balance, 0.01 ether);
assertEq(l1Gateway.transferInitiatedIdx(), 0);
assertEq(l1Gateway.transferFinalizedIdx(), 1);
// Foundry 1.2.x onward doesn't support vm.expectRevert() for catching EvmError: OutOfFunds. We'll use try/catch instead.
try l1Gateway.initiateTransfer{value: 0.9 ether}(bridgeUser, 0.9 ether) {
fail(); // Call should not succeed
} catch {
assertEq(address(bridgeUser).balance, 0.01 ether);
assertEq(l1Gateway.transferInitiatedIdx(), 0);
assertEq(l1Gateway.transferFinalizedIdx(), 1);
}
}

function test_InitiateTransferValueMismatch() public {
Expand Down
14 changes: 8 additions & 6 deletions contracts/test/standard-bridge/SettlementGatewayTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,15 @@ contract SettlementGatewayTest is Test {
assertEq(settlementGateway.transferInitiatedIdx(), 0);
assertEq(settlementGateway.transferFinalizedIdx(), 1);

vm.expectRevert();
vm.prank(bridgeUser);
settlementGateway.initiateTransfer{value: 0.9 ether}(bridgeUser, 0.9 ether);

assertEq(address(bridgeUser).balance, 0.01 ether);
assertEq(settlementGateway.transferInitiatedIdx(), 0);
assertEq(settlementGateway.transferFinalizedIdx(), 1);
// Foundry 1.2.x onward doesn't support vm.expectRevert() for catching EvmError: OutOfFunds. We'll use try/catch instead.
try settlementGateway.initiateTransfer{value: 0.9 ether}(bridgeUser, 0.9 ether) {
fail(); // Call should not succeed
} catch {
assertEq(address(bridgeUser).balance, 0.01 ether);
assertEq(settlementGateway.transferInitiatedIdx(), 0);
assertEq(settlementGateway.transferFinalizedIdx(), 1);
}
}

function test_InitiateTransferValueMismatch() public {
Expand Down
Loading
Loading