Skip to content

chore: low-hanging deviations in integ tests #2135

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

Open
jcsec-security wants to merge 8 commits into
base: draft-v31
Choose a base branch
from
Open

chore: low-hanging deviations in integ tests #2135

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
524b094
Expected reverts and events
jcsec-security Apr 13, 2026
53a5731
additional instance
jcsec-security Apr 13, 2026
3e4ebc6
Tautologie removal and events
jcsec-security Apr 13, 2026
28cfbd0
Tagged pending justifications of commented code
jcsec-security Apr 14, 2026
1159a5b
LogFinder library
jcsec-security Apr 14, 2026
6106bef
Log finder usage
jcsec-security Apr 14, 2026
afddb84
Path fixes and linting
jcsec-security Apr 14, 2026
ec663d9
hashes
jcsec-security Apr 14, 2026
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
3 changes: 3 additions & 0 deletions l1-contracts/test/foundry/l1/integration/AssetRouterTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,9 @@ contract AssetRouterIntegrationTest is L1ContractDeployer, ZKChainDeployer, Toke
abi.encodeWithSignature("L1_CHAIN_ID()"),
abi.encode(block.chainid)
);

vm.expectEmit(true, true, false, true, address(bridgedToken));
emit IERC20.Transfer(address(this), address(0), 100);
vm.broadcast(L2_NATIVE_TOKEN_VAULT_ADDR); // kl todo call ntv, or even assetRouter/bridgehub
bridgedToken.bridgeBurn(address(this), 100);
}
Expand Down
18 changes: 14 additions & 4 deletions l1-contracts/test/foundry/l1/integration/BridgehubNormalTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,16 @@ import {TokenDeployer} from "./_SharedTokenDeployer.t.sol";
import {ZKChainDeployer} from "./_SharedZKChainDeployer.t.sol";
import {L2TxMocker} from "./_SharedL2TxMocker.t.sol";
import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol";
import {IBridgehubBase} from "contracts/core/bridgehub/IBridgehubBase.sol";

import {SLNotWhitelisted} from "contracts/core/bridgehub/L1BridgehubErrors.sol";
import {NotCurrentSettlementLayer, SettlementLayersMustSettleOnL1} from "contracts/common/L1ContractErrors.sol";
import {
CTMNotRegistered,
NotCurrentSettlementLayer,
SettlementLayersMustSettleOnL1,
Unauthorized,
ZeroAddress
} from "contracts/common/L1ContractErrors.sol";

contract BridgehubNormalTest is L1ContractDeployer, ZKChainDeployer, TokenDeployer, L2TxMocker {
using stdStorage for StdStorage;
Expand Down Expand Up @@ -49,7 +56,7 @@ contract BridgehubNormalTest is L1ContractDeployer, ZKChainDeployer, TokenDeploy
// Verify address(0) is an invalid CTM address
assertEq(ctm, address(0), "Testing removal of zero address CTM");

vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(ZeroAddress.selector));
vm.prank(owner);
addresses.bridgehub.removeChainTypeManager(ctm);

Expand All @@ -68,7 +75,7 @@ contract BridgehubNormalTest is L1ContractDeployer, ZKChainDeployer, TokenDeploy
address currentCtm = addresses.bridgehub.chainTypeManager(eraZKChainId);
assertTrue(currentCtm != address(1), "Address(1) should not be the current CTM");

vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(CTMNotRegistered.selector));
vm.prank(owner);
addresses.bridgehub.removeChainTypeManager(ctm);

Expand All @@ -85,6 +92,9 @@ contract BridgehubNormalTest is L1ContractDeployer, ZKChainDeployer, TokenDeploy
assertTrue(owner != address(0), "Owner should not be zero address");
assertTrue(ctm != address(0), "CTM address should not be zero");

// Expect ChainTypeManagerRemoved event
vm.expectEmit(true, true, true, true, address(addresses.bridgehub));
emit IBridgehubBase.ChainTypeManagerRemoved(ctm);
vm.prank(owner);
addresses.bridgehub.removeChainTypeManager(ctm);

Expand Down Expand Up @@ -116,7 +126,7 @@ contract BridgehubNormalTest is L1ContractDeployer, ZKChainDeployer, TokenDeploy
address notAllowed = makeAddr("notAllowed");
assertTrue(notAllowed != owner, "notAllowed should be different from owner");

vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector, notAllowed));
vm.prank(notAllowed);
addresses.bridgehub.setAddressesV31(newChainRegistrationSender);

Expand Down
12 changes: 7 additions & 5 deletions l1-contracts/test/foundry/l1/integration/L1ChainAssetHandlerTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ import {
MigrationNumberMismatch,
MigrationIntervalNotSet,
MigrationIntervalInvalid,
HistoricalSettlementLayerMismatch
HistoricalSettlementLayerMismatch,
NotSystemContext
} from "contracts/core/bridgehub/L1BridgehubErrors.sol";
import {NotAssetRouter, MigrationPaused} from "contracts/common/L1ContractErrors.sol";

import {DataEncoding} from "contracts/common/libraries/DataEncoding.sol";

Expand Down Expand Up @@ -206,15 +208,15 @@ contract L1ChainAssetHandlerTest is L1ContractDeployer, ZKChainDeployer, TokenDe
}

function test_bridgeBurn_Failed() public {
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(NotAssetRouter.selector, address(this), address(0)));
IChainAssetHandlerBase(address(l2ChainAssetHandler)).bridgeBurn(eraZKChainId, 0, 0, address(0), "");

address owner = Ownable2StepUpgradeable(address(ecosystemAddresses.bridgehub.proxies.chainAssetHandler))
.owner();
vm.prank(address(0));
IChainAssetHandlerBase(address(l2ChainAssetHandler)).pauseMigration();

vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(MigrationPaused.selector));
vm.prank(address(0));
IChainAssetHandlerBase(address(l2ChainAssetHandler)).bridgeBurn(eraZKChainId, 0, 0, address(0), "");
}
Expand Down Expand Up @@ -245,7 +247,7 @@ contract L1ChainAssetHandlerTest is L1ContractDeployer, ZKChainDeployer, TokenDe

function test_setSettlementLayerChainId_NotSystemContext() public {
address notSystemContext = makeAddr("notSystemContext");
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(NotSystemContext.selector, notSystemContext));
vm.prank(notSystemContext);
l2ChainAssetHandler.setSettlementLayerChainId(eraZKChainId, eraZKChainId);
}
Expand Down Expand Up @@ -380,7 +382,7 @@ contract L1ChainAssetHandlerTest is L1ContractDeployer, ZKChainDeployer, TokenDe
isActive: false
});

vm.expectRevert();
vm.expectRevert("Ownable: caller is not the owner");
vm.prank(makeAddr("notOwner"));
_l1ChainAssetHandler().setHistoricalMigrationInterval(eraZKChainId, 0, interval);
}
Expand Down
5 changes: 4 additions & 1 deletion l1-contracts/test/foundry/l1/integration/L1GatewayTests.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ contract L1GatewayTests is L1ContractDeployer, ZKChainDeployer, TokenDeployer, L
// Verify gateway is not whitelisted before setup
bool isWhitelistedBefore = addresses.bridgehub.whitelistedSettlementLayers(gatewayChainId);

// Expect SettlementLayerRegistered event from Bridgehub
vm.expectEmit(true, true, true, true, address(addresses.bridgehub));
emit IBridgehubBase.SettlementLayerRegistered(gatewayChainId, true);
_setUpGatewayWithFilterer();

// Verify gateway is whitelisted as a settlement layer
Expand Down Expand Up @@ -486,7 +489,7 @@ contract L1GatewayTests is L1ContractDeployer, ZKChainDeployer, TokenDeployer, L
bytes
memory data = hex"74beea820000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000010f000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010003000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000002e49c884fd1000000000000000000000000000000000000000000000000000000000000010f93d0008af83c021d815bd4e76d7297c69d7f4cc4cf0b8892f7f74f6e33e11829000000000000000000000000c71d126d294a5d2e4002a62d0017b7109f18ade9000000000000000000000000c71d126d294a5d2e4002a62d0017b7109f18ade900000000000000000000000058dc094d71c4c3740bc1ef43d46b58717fa3595a000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000001c101000000000000000000000000000000000000000000000000000000000000000900000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000018000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000457425443000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000045742544300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0101030000000000000000000000000000000000000000000000000000000000e4ed1ec13a28c40715db6399f6f99ce04e5f19d60ad3ff6831f098cb6cf7594400000000000000000000000000000000000000000000000000000000000000079ba301ae10c10e68bffcc2b466aac46d7c7cd6f87eb055e4d43897f303c7a03a21b22cb4099a976636357d5d1f46deeb36f60ec6557eef0da85abaa8222c8c018dba9883941a824d6545029e626b54bd10404b2b8fff432a39ad36d9a36fe3d6000000000000000000000000000000110000000000000000000000000000000500000000000000000000000000000000000000000000000000000000000001fa0103000100000000000000000000000000000000000000000000000000000000f84927dc03d95cc652990ba75874891ccc5a4d79a0e10a2ffdd238a34a39f82823d18b4879c426cf1cb583e1102d9d7f4a5a3a2d01e3f7cc6d042de25409fef1178cf3cbada927540027845a799eab8cf1d788869a9cc11c0f3ebfec198ff347";
address eraAddress = bridgehub.getZKChain(migratingChainId);
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(InvalidProof.selector));
address(addresses.l1Nullifier).call(data);
}

Expand Down
6 changes: 4 additions & 2 deletions l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2AssetTracker.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import {DataEncoding} from "contracts/common/libraries/DataEncoding.sol";
import {L2AssetTrackerData} from "./L2AssetTrackerData.sol";
import {L2UtilsBase} from "../l2-tests-in-l1-context/L2UtilsBase.sol";

import {Unauthorized} from "contracts/common/L1ContractErrors.sol";

abstract contract L2AssetTrackerTest is Test, SharedL2ContractDeployer {
using stdStorage for StdStorage;

Expand Down Expand Up @@ -374,7 +376,7 @@ abstract contract L2AssetTrackerTest is Test, SharedL2ContractDeployer {
/// @notice A random address must not be able to call handleFinalizeBaseTokenBridgingOnL2.
function test_handleFinalizeBaseTokenBridgingOnL2_revertUnauthorized() public {
vm.prank(address(0xDEAD));
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector, address(0xDEAD)));
Copy link
Copy Markdown
Collaborator

@StanislavBreadless StanislavBreadless Apr 13, 2026

Choose a reason for hiding this comment

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

maybe makes sense to make a dedicated constants for 0xdead?

Copy link
Copy Markdown
Collaborator Author

@jcsec-security jcsec-security Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We can just add address constant RAND_ADDRESS = address(0xDEAD); to a new TestConstants.sol, for example at l1-contracts/test/foundry . We can also use foundry labeling through address randomAddress = makeAddr("randomAddress");, it gives clearer traces but we will have to sprinkle it around the test-suite.

L2_ASSET_TRACKER.handleFinalizeBaseTokenBridgingOnL2(1, 100);
}

Expand Down Expand Up @@ -436,7 +438,7 @@ abstract contract L2AssetTrackerTest is Test, SharedL2ContractDeployer {
/// @notice Verifies that only the ComplexUpgrader can call registerBaseTokenDuringUpgrade.
function test_registerBaseTokenDuringUpgrade_revertUnauthorized() public {
vm.prank(address(0xDEAD));
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector, address(0xDEAD)));
L2_ASSET_TRACKER.registerBaseTokenDuringUpgrade();
}

Expand Down
34 changes: 32 additions & 2 deletions l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2Erc20TestAbstract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ pragma solidity ^0.8.20;
// solhint-disable gas-custom-errors

import {StdStorage, Test, stdStorage} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import "forge-std/console.sol";

import {BridgedStandardERC20} from "contracts/bridge/BridgedStandardERC20.sol";
import {BridgedStandardERC20, NonSequentialVersion} from "contracts/bridge/BridgedStandardERC20.sol";
import {L2AssetRouter} from "contracts/bridge/asset-router/L2AssetRouter.sol";
import {IL2NativeTokenVault} from "contracts/bridge/ntv/IL2NativeTokenVault.sol";
import {DataEncoding} from "contracts/common/libraries/DataEncoding.sol";
Expand Down Expand Up @@ -35,7 +36,9 @@ abstract contract L2Erc20TestAbstract is Test, SharedL2ContractDeployer {
address depositor = makeAddr("depositor");
address receiver = makeAddr("receiver");

vm.recordLogs();
performDeposit(depositor, receiver, 100);
Vm.Log[] memory logs = vm.getRecordedLogs();

address l2TokenAddress = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).l2TokenAddress(L1_TOKEN_ADDRESS);

Expand All @@ -44,6 +47,33 @@ abstract contract L2Erc20TestAbstract is Test, SharedL2ContractDeployer {
assertEq(BridgedStandardERC20(l2TokenAddress).name(), TOKEN_DEFAULT_NAME);
assertEq(BridgedStandardERC20(l2TokenAddress).symbol(), TOKEN_DEFAULT_SYMBOL);
assertEq(BridgedStandardERC20(l2TokenAddress).decimals(), TOKEN_DEFAULT_DECIMALS);

// Verify Transfer event (mint: from address(0) to receiver)
Copy link
Copy Markdown
Collaborator

@StanislavBreadless StanislavBreadless Apr 13, 2026

Choose a reason for hiding this comment

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

can we use standard foundry assertions for events?
if not, is it possible to make a standalone function for checking these events? So that it could reused in the future

Copy link
Copy Markdown
Collaborator Author

@jcsec-security jcsec-security Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think not, as these events are not emitted by the fn being called, but by subsequent calls along the line, that is why I was using vm.getRecordedLogs and then iterating over the contents.

I added a library in l1-contracts/test/foundry/l1/integration/utils/, please check and let me know.

bytes32 transferSig = keccak256("Transfer(address,address,uint256)");
bool foundTransfer = false;
for (uint256 i = 0; i < logs.length; i++) {
if (
logs[i].topics.length > 2 &&
logs[i].topics[0] == transferSig &&
logs[i].emitter == l2TokenAddress &&
logs[i].topics[1] == bytes32(uint256(0)) &&
logs[i].topics[2] == bytes32(uint256(uint160(receiver)))
) {
assertEq(abi.decode(logs[i].data, (uint256)), 100, "Transfer amount should be 100");
foundTransfer = true;
}
}
assertTrue(foundTransfer, "Transfer event (mint) should be emitted");

// Verify DepositFinalizedAssetRouter event
bytes32 depositFinalizedSig = keccak256("DepositFinalizedAssetRouter(uint256,bytes32,bytes)");
bool foundDepositFinalized = false;
for (uint256 i = 0; i < logs.length; i++) {
if (logs[i].topics.length > 0 && logs[i].topics[0] == depositFinalizedSig) {
foundDepositFinalized = true;
}
}
assertTrue(foundDepositFinalized, "DepositFinalizedAssetRouter event should be emitted");
}

function test_governanceShouldBeAbleToReinitializeToken() public {
Expand Down Expand Up @@ -72,7 +102,7 @@ abstract contract L2Erc20TestAbstract is Test, SharedL2ContractDeployer {
ignoreDecimals: false
});

vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(NonSequentialVersion.selector));
vm.prank(ownerWallet);
BridgedStandardERC20(l2TokenAddress).reinitializeToken(getters, "TestTokenNewName", "TTN", 20);
}
Expand Down
31 changes: 24 additions & 7 deletions l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2GatewayTestAbstract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ abstract contract L2GatewayTestAbstract is Test, SharedL2ContractDeployer {

function test_gatewayShouldFinalizeDeposit() public {
finalizeDeposit();
require(l2Bridgehub.ctmAssetIdFromAddress(address(chainTypeManager)) == ctmAssetId, "ctmAssetId mismatch");
require(l2Bridgehub.ctmAssetIdFromChainId(mintChainId) == ctmAssetId, "ctmAssetIdFromChainId mismatch");
assertEq(l2Bridgehub.ctmAssetIdFromAddress(address(chainTypeManager)), ctmAssetId, "ctmAssetId mismatch");
assertEq(l2Bridgehub.ctmAssetIdFromChainId(mintChainId), ctmAssetId, "ctmAssetIdFromChainId mismatch");

address diamondProxy = l2Bridgehub.getZKChain(mintChainId);
require(!GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
assertFalse(GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
}

function test_gatewayNonEmptyPriorityQueueMigration() public {
Expand All @@ -69,7 +69,7 @@ abstract contract L2GatewayTestAbstract is Test, SharedL2ContractDeployer {
finalizeDepositWithCustomCommitment(abi.encode(commitment));

address diamondProxy = l2Bridgehub.getZKChain(mintChainId);
require(!GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
assertFalse(GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
}

function test_forwardToL2OnGateway_L2() public {
Expand Down Expand Up @@ -135,9 +135,26 @@ abstract contract L2GatewayTestAbstract is Test, SharedL2ContractDeployer {
Vm.Log[] memory logs = vm.getRecordedLogs();
assertTrue(logs.length > 0, "Withdrawal should emit events when sending message to L1");

// Verify the withdrawal was for the correct chain and asset
assertTrue(data.chainId == mintChainId, "Withdrawal data should reference the correct chain");
assertTrue(ctmAssetId != bytes32(0), "CTM asset ID should be valid");
// Verify WithdrawalInitiatedAssetRouter event was emitted with correct params
// Event sig: WithdrawalInitiatedAssetRouter(uint256 chainId, address indexed l2Sender, bytes32 indexed assetId, bytes assetData)
bytes32 withdrawalSig = keccak256("WithdrawalInitiatedAssetRouter(uint256,address,bytes32,bytes)");
Comment thread
jcsec-security marked this conversation as resolved.
Outdated
bool foundWithdrawal = false;
for (uint256 i = 0; i < logs.length; i++) {
if (logs[i].topics.length > 2 && logs[i].topics[0] == withdrawalSig) {
assertEq(
logs[i].topics[1],
bytes32(uint256(uint160(ownerWallet))),
"WithdrawalInitiatedAssetRouter: l2Sender should be ownerWallet"
);
assertEq(
logs[i].topics[2],
ctmAssetId,
"WithdrawalInitiatedAssetRouter: assetId should match ctmAssetId"
);
foundWithdrawal = true;
}
}
assertTrue(foundWithdrawal, "WithdrawalInitiatedAssetRouter event should be emitted");
}

function test_finalizeDepositWithRealChainData() public {
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2InteropFeesTestAbstract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ abstract contract L2InteropFeesTestAbstract is L2InteropTestUtils {

// SafeERC20 safeTransfer to address(0) will revert
vm.prank(coinbaseAddr);
vm.expectRevert();
vm.expectRevert("ERC20: transfer to the zero address");
l2InteropCenter.claimZKFees(address(0));
}

Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2WethTestAbstract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ abstract contract L2WethTestAbstract is Test, SharedL2ContractDeployer {
}

function test_revertWhenWithdrawingMoreThanBalance() public {
vm.expectRevert();
vm.expectRevert("ERC20: burn amount exceeds balance");
weth.withdraw(1);
}

Expand Down
Loading