Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Nov 24, 2025

test(contracts): add AddressAliasHelper coverage and fix SystemConfig flaky test

Summary

This PR includes two test improvements:

1. AddressAliasHelper test coverage (original scope):
Adds individual test coverage for applyL1ToL2Alias and undoL1ToL2Alias functions in the AddressAliasHelper library. Previously, only an integration test existed that verified the round-trip property (apply → undo = identity). This PR adds function-specific fuzz tests that verify each function's arithmetic behavior independently.

Changes:

  • Added AddressAliasHelper_ApplyL1ToL2Alias_Test contract with fuzz test verifying offset addition
  • Added AddressAliasHelper_UndoL1ToL2Alias_Test contract with fuzz test verifying offset subtraction
  • Both tests use unchecked arithmetic blocks to match the source contract's intentional overflow/underflow behavior

2. SystemConfig flaky test fix (requested by maintainer):
Fixes the flaky testFuzz_isFeatureEnabled_unsetFeature_succeeds test that was failing in the CUSTOM_GAS_TOKEN CI environment.

Changes:

  • Removed view modifier to allow state changes
  • Normalizes CUSTOM_GAS_TOKEN to disabled state before running the assertion
  • The test now always runs assertFalse(systemConfig.isFeatureEnabled(_feature)) for all fuzz inputs

Implementation rationale: The test normalizes CUSTOM_GAS_TOKEN (which can be enabled in some deploy configs) to ensure consistent state, then asserts that isFeatureEnabled returns false for the fuzzed feature value. This fixes the original flake while keeping the test straightforward.

Updates Since Initial Fix

The SystemConfig fix went through multiple iterations:

  1. Initial attempt tried to disable all known features (CUSTOM_GAS_TOKEN and ETH_LOCKBOX), but this caused SystemConfig_InvalidFeatureState reverts in upgrade environments where ETH_LOCKBOX cannot be disabled
  2. Second iteration added a runtime guard to skip features that are enabled, but this made the test a no-op for some fuzz cases
  3. Final version (per maintainer feedback) removes the runtime guard and only normalizes CUSTOM_GAS_TOKEN, letting the assertion run for all fuzz inputs

Updates Since Last Revision

  • Rebased on top of latest develop branch (commit 4b21038)
  • All tests verified passing locally after rebase

Review & Testing Checklist for Human

  • Critical: Verify contracts-bedrock-tests CUSTOM_GAS_TOKEN CI job passes (original flake)
  • Critical: Verify upgrade tests pass (contracts-bedrock-tests-upgrade jobs)
  • Important: Check if test fails in environments where ETH_LOCKBOX or other features are enabled in deploy configs - if so, this may need further iteration
  • Run both test files locally to ensure no regressions:
    • forge test --match-path test/vendor/AddressAliasHelper.t.sol -vv
    • forge test --match-path test/L1/SystemConfig.t.sol -vv

Notes

Last update: 2025-11-24 15:42 ET

@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner November 24, 2025 07:09
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.01%. Comparing base (f2ab75e) to head (4b21038).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #18360      +/-   ##
===========================================
+ Coverage    75.39%   80.01%   +4.62%     
===========================================
  Files          185      130      -55     
  Lines        11001     6970    -4031     
===========================================
- Hits          8294     5577    -2717     
+ Misses        2561     1393    -1168     
+ Partials       146        0     -146     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 80.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@smartcontracts
Copy link
Contributor

smartcontracts commented Nov 24, 2025

devinai there is an unrelated flake in your PR but can you please fix it?

  • testFuzz_isFeatureEnabled_unsetFeature_succeeds

fix by setting all features disabled first, then running the check

@devin-ai-integration
Copy link
Contributor Author

@smartcontracts Fixed in 7999b5b

  • Removed view modifier from test to allow state changes
  • Added logic to disable CUSTOM_GAS_TOKEN and ETH_LOCKBOX features before fuzzing
  • Only disables features if currently enabled to avoid revert from sanity check

@devin-ai-integration devin-ai-integration bot changed the title test(contracts): add comprehensive coverage for AddressAliasHelper functions test(contracts): add AddressAliasHelper coverage and fix SystemConfig flaky test Nov 24, 2025
@devin-ai-integration
Copy link
Contributor Author

@smartcontracts Fixed in 6bf83d6

  • Removed ETH_LOCKBOX disabling code that caused SystemConfig_InvalidFeatureState in upgrade envs
  • Added runtime guard to skip features that are currently enabled
  • Only normalizes CUSTOM_GAS_TOKEN to fix the original flake

@smartcontracts
Copy link
Contributor

devinai please rebase on top of develop

…nctions

- Add ApplyL1ToL2Alias_Test contract with fuzz test for applyL1ToL2Alias
- Add UndoL1ToL2Alias_Test contract with fuzz test for undoL1ToL2Alias
- Both tests verify correct offset arithmetic with unchecked overflow handling
- Ensures all public functions have individual test coverage
Fixes flaky test testFuzz_isFeatureEnabled_unsetFeature_succeeds by
disabling all known features (CUSTOM_GAS_TOKEN, ETH_LOCKBOX) before
checking that arbitrary features are disabled. This prevents
environment-dependent state from causing test failures in CI.
Improves the fix for testFuzz_isFeatureEnabled_unsetFeature_succeeds by:
- Removing ETH_LOCKBOX disabling code (causes revert in upgrade envs)
- Adding runtime guard to skip features that are currently enabled
- Only normalizing CUSTOM_GAS_TOKEN to fix the original flake

This approach avoids SystemConfig_InvalidFeatureState reverts when
ETH_LOCKBOX is enabled with a configured lockbox address in mainnet
upgrade test environments.
Per maintainer feedback, removes the early exit that was skipping some
fuzz cases. The test now always runs the assertion for all fuzz inputs.

The CUSTOM_GAS_TOKEN normalization is kept to fix the original flake
in the CUSTOM_GAS_TOKEN CI environment.
@devin-ai-integration devin-ai-integration bot force-pushed the ai/improve-address-alias-helper-coverage branch from af8b93b to 4b21038 Compare November 24, 2025 20:42
@devin-ai-integration
Copy link
Contributor Author

@smartcontracts Fixed in 4b21038

  • Rebased ai/improve-address-alias-helper-coverage on top of latest develop
  • All tests passing locally (AddressAliasHelper and SystemConfig)

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