feat: add rollupdata sequencer URL watcher

[arnaubennassar]

🔄 Changes Summary

• Add a new rollupdata package that fetches the current sequencer URL for each rollup attached to the rollup manager and subscribes to future on-chain updates.
• Subscribe to existing rollup URL updates and newly attached rollups from the initial snapshot block.
• Add generated test contracts and simulated backend tests for initial fetches, URL changes, and new rollup attachments.

⚠️ Breaking Changes

• 🛠️ Config: None.
• 🔌 API/CLI: New package API only.
• 🗑️ Deprecated Features: None.

📋 Config Updates

• 🧾 Diff/Config snippet: New rollupdata.Config with RollupManagerAddr and optional UpdateBufferSize.

✅ Testing

• 🤖 Automatic: go test ./rollupdata
• 🤖 Automatic: go test -race ./rollupdata
• 🤖 Automatic: go test ./test/contracts/rollupmanagermock ./test/contracts/sequencerurlrollupmock
• 🖱️ Manual: Not applicable.

🐞 Issues

• None.

🔗 Related PRs

• None.

📝 Notes

• Runtime subscription failures log the error and close the update channel.
• Test contracts live under test/contracts and are consumed through generated Go bindings.

[claude]

Claude finished @arnaubennassar's task in 1m 59s —— View job

---

✅ PR Review Complete

---

🚀 What's New

This PR adds a new rollupdata package that watches the rollup manager contract for sequencer URL updates. It fetches initial sequencer URLs for all attached rollups and subscribes to on-chain events for URL changes and new rollup attachments (via AddExistingRollup, CreateNewRollup, and CreateNewAggchain events).

🐛 Bug Fixes

None.

📋 Config Updates

New rollupdata.Config struct with:

• RollupManagerAddr (required): Address of the rollup manager contract
• UpdateBufferSize (optional, default: 16): Buffer size for the update channel

⚠️ Breaking Changes

None. This is a new package with no impact on existing functionality.

---

Review Findings

🔴 Required Issues

1. Potential Race Condition in addSubscription

Location: rollupdata/rollupdata.go:253-272

The double-check pattern in addSubscription has a subtle race condition. Between the first context check (line 255) and acquiring the lock (line 261), the context could be canceled, but you still add the subscription.

func (w *sequencerURLWatcher) addSubscription(subscription event.Subscription) bool {
	select {
	case <-w.ctx.Done():
		subscription.Unsubscribe()
		return false
	default:
	}

	w.mu.Lock()
	defer w.mu.Unlock()

	select {
	case <-w.ctx.Done():
		subscription.Unsubscribe()
		return false
	default:
		w.subscriptions = append(w.subscriptions, subscription)
		return true
	}
}

While you do have a second check after locking, there's a window where:

1. Thread A passes first check, doesn't hold lock yet
2. Thread B calls cancel() and unsubscribeAll(), gets lock, unsubscribes everything
3. Thread A gets lock, adds new subscription that will never be cleaned up

Suggested Fix: Check if subscriptions slice is nil (set it to nil in unsubscribeAll and shutdown) to detect if cleanup has already happened.

2. Goroutine Leak Risk in Error Paths

Location: rollupdata/rollupdata.go:193-224

In the start() method, if any of the later watchers fail to start (e.g., startCreateNewRollupWatcher fails), the previously started watchers' goroutines are cleaned up via shutdown(), but there's a timing issue. The w.wg.Add(1) happens in startRollupWatcher (line 329), but if startCreateNewRollupWatcher fails immediately, the goroutine might not have reached the defer w.wg.Done() yet, causing w.wg.Wait() in shutdown() to potentially hang temporarily.

Impact: While this is unlikely to cause issues in practice (the goroutines should exit quickly), it's not ideal for error handling.

Suggested Approach: Consider adding all goroutines to the WaitGroup before starting any of them, or ensure all goroutines are started before returning from start().

3. Missing Documentation on Channel Closure Semantics

Location: rollupdata/rollupdata.go:72-103

The GetSequencerURLsAndSubscribe function returns a channel that gets closed in specific circumstances, but this isn't documented. Users need to know:

• The channel closes when the context is canceled
• The channel closes on any subscription error
• After channel closure, no further updates will be received

Suggested Fix: Add documentation to the function explaining these semantics.

🟡 Suggested Improvements

4. Missing Context Validation in New()

Location: rollupdata/rollupdata.go:51-68

The New() function doesn't validate that the Ethereum client is actually functional (e.g., can make calls). This could lead to late failure when GetSequencerURLsAndSubscribe is called.

Suggestion: Consider adding a validation call in New() or documenting that the client must be functional before calling GetSequencerURLsAndSubscribe.

5. Inconsistent Error Handling in Subscription Loops

Location: Multiple locations (lines 335-337, 423-425, 460-462, 497-499)

All subscription error handlers call w.signalError(err) which logs and cancels. However, the logging only happens if the error is not context.Canceled. This means genuine subscription errors during normal operation get logged, but errors during shutdown are silently ignored.

Suggestion: This is reasonable, but consider if you want to distinguish between errors that caused the shutdown vs errors that occurred because of shutdown.

6. Potential Event Reprocessing Issue

Location: rollupdata/rollupdata.go:342-344

In startRollupWatcher, events at or before ignoreThroughBlock are skipped:

if eventData.Raw.BlockNumber <= ignoreThroughBlock {
    continue
}

However, for newly attached rollups (in handleNewRollup), the ignoreThroughBlock is set to eventBlock (line 398). This means if a rollup is attached and its URL is updated in the same block, the update would be ignored.

Impact: Low - it's unlikely a rollup would be attached and have its URL updated in the same block, and the initial URL is already sent in the NewRollup update.

7. Test Coverage: Missing Race Condition Tests

Location: rollupdata/rollupdata_test.go

The tests don't include race condition scenarios such as:

• Canceling context while updates are being sent
• Multiple rapid URL changes
• Rollup attachment during shutdown

Suggestion: Add tests with go test -race to verify concurrent access patterns. (Though I see you mention running with -race flag in the PR description, explicit race-condition tests would strengthen confidence.)

8. Missing Test: Zero Rollups Scenario

Location: rollupdata/rollupdata_test.go

No test covers the case where the rollup manager has zero rollups attached initially.

Suggestion: Add a test for this edge case to ensure the subscription still works correctly.

🟢 Optional Observations

9. Magic Number in Event Channel Buffers

Location: rollupdata/rollupdata.go:317, 404, 441, 478

Event channels are created with buffer size 1. While this is fine, it would be clearer to extract this as a constant with a comment explaining why buffer size 1 is sufficient (events are processed quickly).

10. Exported Type Could Have More Documentation

Location: rollupdata/rollupdata.go:20-27

The SequencerURLUpdate struct is exported but lacks field-level documentation. Consider adding godoc comments for each field to explain their purpose.

11. Contract Mock Simplicity

Location: test/contracts/rollupmanagermock/RollupManagerMock.sol

The RollupManagerMock only implements AddExistingRollup but not CreateNewRollup or CreateNewAggchain. While the tests only use AddExistingRollup, having all three would make the mock more complete for future tests.

Note: This is truly optional as the current mock serves its purpose.

✅ Positive Observations

1. Excellent concurrency design: The use of sync.Mutex, sync.WaitGroup, and context cancellation is well-structured
2. Comprehensive subscription cleanup: The shutdown() and closeOnDone() patterns ensure proper resource cleanup
3. Good test coverage: Tests cover the main scenarios (initial fetch, URL updates, new rollups)
4. Proper error wrapping: All errors are wrapped with context using fmt.Errorf with %w
5. Configuration validation: The Config.Validate() method properly checks for invalid inputs
6. Simulated backend tests: Using simulated.Backend for tests is excellent for testing contract interactions without external dependencies

Code Quality Alignment with CLAUDE.md

✅ Formatting: Code appears properly formatted
✅ Error handling: Proper use of fmt.Errorf with %w
✅ Naming: Good CamelCase usage, no stuttering
✅ Comments: Exported functions have doc comments
⚠️ Imports: Need to verify import grouping (stdlib, external, internal) - appears correct
✅ Line length: No obviously long lines

Summary

This is a well-designed concurrent system with good test coverage. The main concerns are around edge cases in concurrent shutdown scenarios and some missing documentation. The code follows Go best practices and the project's conventions well.

Recommendation: Address the race condition in addSubscription (🔴 #1) and improve documentation on channel semantics (🔴 #3) before merging. The other issues are suggestions for improvement but not blockers.

---