Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Comments

feat(supervisor/e2e-test): added L2 reorg acceptance tests#2724

Merged
sadiq1971 merged 5 commits intomainfrom
sa/test/l2-reorg
Aug 19, 2025
Merged

feat(supervisor/e2e-test): added L2 reorg acceptance tests#2724
sadiq1971 merged 5 commits intomainfrom
sa/test/l2-reorg

Conversation

@sadiq1971
Copy link
Collaborator

Closes #2663
Closes #2497

@claude
Copy link
Contributor

claude bot commented Aug 18, 2025

Claude finished @sadiq1971's task —— View job


PR Review: L2 Reorg Acceptance Tests

Reviewing the L2 reorg acceptance tests implementation. Here's my comprehensive feedback:

Review Checklist

  • Read and analyze test file structure and organization
  • Review init_exec_msg_test.go - execution message initialization tests
  • Review invalid_exec_msgs_test.go - invalid execution message handling tests
  • Review unsafe_head_test.go - unsafe head reorg tests
  • Review init_test.go - test initialization and setup
  • Check Go module dependencies (go.mod/go.sum changes)
  • Assess code quality, best practices, and potential issues
  • Provide comprehensive feedback and recommendations

Overall Assessment

Strong test coverage - The PR adds comprehensive tests for L2 reorg scenarios with good edge case coverage
Clear test structure - Well-organized test files with descriptive names and logical separation
Proper dependency management - Clean dependency updates with appropriate version pinning


Code Quality & Best Practices

Strengths

  1. Excellent Test Organization: Clear separation of concerns with distinct test files for different reorg scenarios
  2. Comprehensive Coverage: Tests cover critical scenarios - init message reorgs, invalid exec messages, and unsafe head reorgs
  3. Good Error Handling: Proper use of require.NoError() and descriptive error messages
  4. Detailed Logging: Extensive use of structured logging helps with debugging and understanding test flow
  5. Proper Cleanup: Tests properly stop/start sequencers and batchers to simulate real conditions

⚠️ Areas for Improvement

  1. Hardcoded Values: Several magic numbers could be extracted as constants:

    // init_exec_msg_test.go:65-67
    nTopics := 3
    lenData := 10

    Recommendation: Extract as package-level constants or test configuration

  2. Repetitive Setup Code: The three test files have similar setup patterns that could be abstracted:

    // Similar across all tests
    alice := sys.FunderA.NewFundedEOA(eth.OneHundredthEther)
    bob := sys.FunderB.NewFundedEOA(eth.OneHundredthEther)

    Recommendation: Consider creating helper functions for common setup

  3. Variable Naming: Some variable names could be more descriptive:

    // init_exec_msg_test.go:37
    cathrine := sys.FunderA.NewFundedEOA(eth.OneHundredthEther)  // typo: should be "catherine"

Security Considerations

Good Security Practices

  1. Controlled Test Environment: Tests use isolated test environments preventing interference
  2. Proper Resource Management: Sequencers and batchers are properly stopped/started
  3. Validation of State Changes: Tests verify expected state transitions and reorgs occur

⚠️ Minor Security Notes

  1. Randomness Source: Using time-based seed is fine for tests but predictable:
    // init_exec_msg_test.go:65
    rng := rand.New(rand.NewSource(time.Now().UnixNano()))
    Note: This is acceptable for tests but documents the predictable nature

Performance Considerations

Good Performance Practices

  1. Efficient Waiting: Proper use of Eventually() and wait.For() with reasonable timeouts
  2. Controlled Block Production: Tests efficiently control when blocks are produced

⚠️ Performance Notes

  1. Sleep Statements: Several time.Sleep() calls could potentially be replaced with more deterministic waiting:
    // invalid_exec_msgs_test.go:188, 206
    time.Sleep(2 * time.Second)
    Recommendation: Consider if these can be replaced with condition-based waiting

Test Coverage & Completeness

Excellent Coverage

  1. Multiple Reorg Scenarios:

    • Init message reorgs during cross-chain communication
    • Invalid execution message handling
    • Unsafe head reorgs
  2. Edge Case Testing: The invalid message tests cover multiple failure modes:

    • Invalid log index (line 31)
    • Invalid block number (line 36)
    • Invalid chain ID (line 42)
  3. State Validation: Tests properly verify that supervisor and sequencer states converge after reorgs


Potential Issues

🔍 Minor Issues Found

  1. Typo in Variable Name:

    // init_exec_msg_test.go:37
    cathrine := sys.FunderA.NewFundedEOA(eth.OneHundredthEther)

    Fix: Should be catherine

  2. Timeout Hardcoding: Some timeouts are hardcoded and could benefit from configuration:

    // Multiple files
    60*time.Second, 3*time.Second  // in Eventually()
    5*time.Second                   // in wait.For()
  3. Error Message Consistency: Some error messages could be more descriptive:

    // invalid_exec_msgs_test.go:202
    if err == nil || err.Error() != "not found" { // want to get "not found" error

    Recommendation: Use more specific error checking patterns


Dependencies Review

Clean Dependency Management

The go.mod changes are minimal and appropriate:

  • Single line change to update the optimism dependency to include latest devstack changes
  • Corresponding go.sum updates are clean
  • No unnecessary or concerning dependencies added

Final Recommendations

  1. Extract Constants: Move magic numbers to package-level constants
  2. Fix Typo: Correct "cathrine" → "catherine"
  3. Consider Helper Functions: Extract common setup patterns to reduce duplication
  4. Add Comments: Consider adding more detailed comments explaining the test scenarios
  5. Timeout Configuration: Consider making timeouts configurable or documenting why specific values are chosen

Verdict

This is a high-quality test implementation that significantly improves the test coverage for L2 reorg scenarios. The tests are well-structured, comprehensive, and follow Go testing best practices. The identified issues are minor and don't affect the core functionality or safety of the tests.

Recommendation: ✅ Approve with minor suggestions - This PR adds valuable test coverage with only minor improvements needed.


@codecov
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.2%. Comparing base (ed54ce2) to head (d750c4a).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@dhyaniarun1993 dhyaniarun1993 left a comment

Choose a reason for hiding this comment

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

Awesome.

@sadiq1971 sadiq1971 enabled auto-merge August 19, 2025 11:43
@sadiq1971 sadiq1971 added this pull request to the merge queue Aug 19, 2025
Merged via the queue into main with commit ebd8181 Aug 19, 2025
33 checks passed
@sadiq1971 sadiq1971 deleted the sa/test/l2-reorg branch August 19, 2025 12:07
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(supervisor): missing init message test(supervisor): block invalidation flow

2 participants