Skip to content

feat: add L2NetworkConfig with optional InitialLER override#1558

Merged
joanestebanr merged 2 commits intodevelopfrom
feat/initial_ler_configurable-pm290
Mar 27, 2026
Merged

feat: add L2NetworkConfig with optional InitialLER override#1558
joanestebanr merged 2 commits intodevelopfrom
feat/initial_ler_configurable-pm290

Conversation

@joanestebanr
Copy link
Copy Markdown
Collaborator

@joanestebanr joanestebanr commented Mar 27, 2026

🚀 What's New

The call to get the initial Local Exit Root from the contract requires an archive node. Using the configurable InitialLER, we can skip this call.

Introduces a new [L2NetworkConfig] section in the top-level config to hold L2-specific settings. The first field, InitialLER, allows operators to override the initial Local Exit Root without querying the RollupManager contract on L1, and so, let to use an archive-node.

  • New L2NetworkConfig struct in etherman/config/network.go
  • New [L2NetworkConfig] section in the main Config and default.go
  • GetInitialLER in cmd/run.go accepts an optional override; if non-nil, the contract call is skipped
  • Logs the resolved initial LER on startup

🐛 Bug Fixes

None.

📋 Config Updates

New optional field under [L2NetworkConfig]:

[L2NetworkConfig]
# InitialLER: optional override for the initial Local Exit Root (0x000...000 is a valid value).
# If not set, the value is queried from the RollupManager contract on L1.
# InitialLER = "0xaabbccdd..."

Default: not set (field absent → contract is queried as before).

⚠️ Breaking Changes

None.

🤖 Generated with Claude Code

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @joanestebanr's task in 1m 33s —— View job


PR Review Complete ✅

Reviewed PR #1558: feat: add L2NetworkConfig with optional InitialLER override


🚀 What's New

Introduces a new [L2NetworkConfig] section in the top-level config to hold L2-specific settings. The first field, InitialLER, allows operators to override the initial Local Exit Root without querying the RollupManager contract on L1. This is useful for testing scenarios, disaster recovery, and specialized deployments where the LER value is known in advance.

🐛 Bug Fixes

None.

📋 Config Updates

New optional field under [L2NetworkConfig]:

[L2NetworkConfig]
# InitialLER: optional override for the initial Local Exit Root (0x000...000 is a valid value).
# If not set, the value is queried from the RollupManager contract on L1.
# InitialLER = "0xaabbccdd..."

Default behavior: Field is omitted (nil) → contract query occurs as before. No migration needed.

⚠️ Breaking Changes

None. This is a backward-compatible addition. Existing configs without [L2NetworkConfig] will continue to query the contract.


📝 Code Review Feedback

✅ Strengths

  1. Well-tested: Comprehensive test coverage with three scenarios (specific hash, zero hash, and omitted field)
  2. Clean implementation: Minimal changes with clear separation of concerns
  3. Backward compatible: Existing configs continue to work without modification
  4. Proper logging: Initial LER is logged on startup for observability
  5. Good documentation: Config comments clearly explain the field and valid values

🟢 Optional Improvements

1. Function Signature Documentation (cmd/run.go:795-799)

The GetInitialLER function has a new parameter but lacks a doc comment. Consider adding:

// GetInitialLER returns the initial Local Exit Root for the L2 network.
// If initialLEROverride is non-nil, it is returned directly without querying L1.
// Otherwise, the value is queried from the RollupManager contract on L1.
func GetInitialLER(
	initialLEROverride *common.Hash,
	rollupCreationBlockL1 uint64,
	rollupDataQuerier *ethermanquierier.RollupDataQuerier) (*common.Hash, error) {

Why: Go best practices require exported functions to have doc comments starting with the function name (per CLAUDE.md guidelines).

2. Test Assertion Enhancement (config/config_test.go:84)

The test in TestLoadDefaultConfig checks require.Nil(t, cfg.L2NetworkConfig.InitialLER), but doesn't verify that the entire L2NetworkConfig struct is properly initialized. Consider:

require.NotNil(t, cfg.L2NetworkConfig, "L2NetworkConfig should be initialized")
require.Nil(t, cfg.L2NetworkConfig.InitialLER, "InitialLER should be nil by default")

Why: Ensures the config section is properly initialized, not accidentally nil.

3. Validate Method Consideration (etherman/config/network.go:29-35)

The L2NetworkConfig struct doesn't have a Validate() method like L1NetworkConfig does. While not strictly necessary now (since InitialLER can be any value including nil), consider adding:

// Validate checks if the L2NetworkConfig is valid
func (c *L2NetworkConfig) Validate() error {
	// Currently no validation needed as InitialLER is optional and any hash value is valid
	return nil
}

Why: Establishes the pattern for future L2-specific validations and maintains consistency with L1NetworkConfig.

4. Log Level Consideration (cmd/run.go:148)

The initial LER is logged at Info level. For production deployments, this is good for observability. However, consider if this should be a Debug log when using the default (contract-queried) value, and Info/Warn only when an override is used:

if cfg.L2NetworkConfig.InitialLER != nil {
	log.Infof("Initial Local Exit Root (LER) using OVERRIDE: %s", initialLER.Hex())
} else {
	log.Debugf("Initial Local Exit Root (LER) from contract: %s", initialLER.Hex())
}

Why: Makes it more obvious in production logs when an override is being used versus the standard contract query.

🔍 Code Quality Analysis

Go Best Practices: ✅ All code follows Go conventions

  • Proper use of pointer for optional field (*gethcommon.Hash)
  • Correct struct tags (mapstructure:"InitialLER")
  • Follows naming conventions (exported types, clear names)

Error Handling: ✅ Appropriate

  • Error wrapping with context in cmd/run.go:146
  • Early return pattern in GetInitialLER

Test Coverage: ✅ Excellent

  • Tests cover all three scenarios (set to specific hash, set to zero hash, omitted)
  • Uses table-driven tests pattern
  • Validates both nil and non-nil cases

Configuration Design: ✅ Sound

  • Uses pointer to distinguish "not set" from "set to zero hash"
  • Clear comments in default.go explaining the field
  • Commented-out default in config template (good UX)

🔒 Security Analysis

No security concerns identified. The implementation:

  • ✅ Properly validates hash format through go-ethereum's common.Hash type
  • ✅ Doesn't expose sensitive information in logs (only hash values)
  • ✅ Doesn't create injection vulnerabilities
  • ✅ Maintains backward compatibility without introducing security regressions

⚡ Performance Considerations

Performance is improved:

  • ✅ Skips unnecessary L1 contract query when override is set
  • ✅ No additional overhead when override is not used
  • ✅ Early return in GetInitialLER minimizes work

📚 Documentation Quality

Documentation is clear:

  • ✅ Config comments explain the field purpose
  • ✅ Notes that 0x000...000 is a valid value (important!)
  • ✅ Explains fallback behavior (query contract if not set)

Missing: Function-level documentation for GetInitialLER (see optional improvement #1)


🎯 Summary

This is a well-implemented, clean feature that adds useful flexibility without breaking existing functionality. The code quality is high, test coverage is comprehensive, and the design is sound.

Recommendation: ✅ Approve with optional improvements

The optional improvements listed above would enhance the code but are not blockers. The PR is ready to merge as-is if desired.

Priority of suggested improvements:

  • 🟢 All suggestions are optional enhancements, not required changes
  • Adding the function doc comment would align with Go/project standards
  • Other suggestions are minor quality-of-life improvements

joanestebanr and others added 2 commits March 27, 2026 12:08
Introduces a new [L2NetworkConfig] section in the top-level config to hold
L2-specific settings. The first field, InitialLER, allows operators to
override the initial Local Exit Root without querying the RollupManager
contract on L1. A nil value (field absent) preserves the existing behaviour.
Note: 0x000...000 is treated as a valid non-nil override.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@joanestebanr joanestebanr force-pushed the feat/initial_ler_configurable-pm290 branch from 016a5fe to 71ae7bf Compare March 27, 2026 11:12
@sonarqubecloud
Copy link
Copy Markdown

@joanestebanr joanestebanr merged commit 8ff18c4 into develop Mar 27, 2026
24 of 25 checks passed
@joanestebanr joanestebanr deleted the feat/initial_ler_configurable-pm290 branch March 27, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants