Skip to content

chore: low-hanging deviations in integ tests#2135

Open
jcsec-security wants to merge 8 commits intodraft-v31from
jcr/integ-test-low-hanging
Open

chore: low-hanging deviations in integ tests#2135
jcsec-security wants to merge 8 commits intodraft-v31from
jcr/integ-test-low-hanging

Conversation

@jcsec-security
Copy link
Copy Markdown
Collaborator

@jcsec-security jcsec-security commented Apr 13, 2026

What ❔

Part of the hardening efforts to adhere to the new testing guidelines. Limited to our current integration tests.

  • Empty vm.expectRevert() instaces.
  • Missing validation of emitted events.
  • Use assert* statements instead of require for outcome validation for consistency.
  • L2GatewayTestAbstract.t.sol 139-140 did not validate anything in reality, as the values of those variables were manually set in ln 117 and SharedL2ContractDeployer.sol:101
  • Tagged unjustified commented code

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.

@jcsec-security
Copy link
Copy Markdown
Collaborator Author

Open question:

  • In the ChainRegistrationSender contract (here) the registerChain() function does not emit any event although it modifies its storage. Is there a reason for this?

@jcsec-security jcsec-security marked this pull request as ready for review April 13, 2026 14:44
function test_handleFinalizeBaseTokenBridgingOnL2_revertUnauthorized() public {
vm.prank(address(0xDEAD));
vm.expectRevert();
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector, address(0xDEAD)));
Copy link
Copy Markdown
Collaborator

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

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.

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

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

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.

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.

2 participants