feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738
feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738brbrr wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3738 +/- ##
==========================================
+ Coverage 75.39% 75.46% +0.07%
==========================================
Files 438 437 -1
Lines 39463 39509 +46
==========================================
+ Hits 29753 29817 +64
+ Misses 7637 7618 -19
- Partials 2073 2074 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @brbrr's task in 5m 10s —— View job PR Review: L1 SettlementLayer abstractionI reviewed the diff against Reviewed:
This is a clean, well-structured refactor. Logic is correct; I found no blockers — one test-coverage gap worth addressing and two nits. Important
Nits
Verified (no action needed)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // gethStateUpdateForwarder decodes contract.StarknetLogStateUpdate events | ||
| // into l1.StateUpdate as they arrive from the underlying log subscription. | ||
| type gethStateUpdateForwarder struct { | ||
| inner event.Subscription | ||
| sink chan<- *StateUpdate | ||
| raw chan *contract.StarknetLogStateUpdate | ||
| errCh chan error | ||
| closed chan struct{} | ||
| closeOnce sync.Once | ||
| } | ||
|
|
||
| func (w *gethStateUpdateForwarder) Err() <-chan error { return w.errCh } | ||
|
|
||
| func (w *gethStateUpdateForwarder) Unsubscribe() { | ||
| w.shutdown(nil) | ||
| w.inner.Unsubscribe() | ||
| } | ||
|
|
||
| func (w *gethStateUpdateForwarder) shutdown(cause error) { | ||
| w.closeOnce.Do(func() { | ||
| close(w.closed) | ||
| if cause != nil { | ||
| select { | ||
| case w.errCh <- cause: | ||
| default: | ||
| } | ||
| } | ||
| close(w.errCh) | ||
| }) | ||
| } | ||
|
|
||
| func (w *gethStateUpdateForwarder) run() { | ||
| defer w.shutdown(nil) | ||
| for { | ||
| select { | ||
| case <-w.closed: | ||
| return | ||
| case err, ok := <-w.inner.Err(): | ||
| if !ok { | ||
| return | ||
| } | ||
| w.shutdown(err) | ||
| return | ||
| case ev := <-w.raw: | ||
| su := stateUpdateFromGethContract(ev) | ||
| select { | ||
| case w.sink <- su: | ||
| case <-w.closed: | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Important (test gap): gethStateUpdateForwarder and GethSettlement.WatchStateUpdate are the most concurrency-sensitive code in this PR — goroutine lifecycle, closeOnce shutdown, non-blocking error delivery, sink back-pressure — yet they have no direct test. The new HTTP-based gethJSONRPCServer can only exercise the unary calls; WatchStateUpdate needs a WS endpoint, so nothing here is covered (Codecov flags 54 missing lines, mostly this block).
The l1_*_test.go suites mock WatchStateUpdate at the SettlementLayer boundary, so the real forwarder logic is never run.
A focused unit test driving a fake event.Subscription through the four paths would lock this down:
- normal: event on
raw→ decodedStateUpdateonsink - inner errors →
Err()delivers the cause exactly once, then closes Unsubscribe()→rungoroutine exits andinner.Unsubscribe()is called (assert no leak viagoleak)- sink blocked +
Unsubscribe()concurrently →rununblocks via<-w.closedand returns
brbrr
changed the title
|
Claude finished @brbrr's task in 3m 41s —— View job PR Review: L1 SettlementLayer abstraction — follow-up
Re-reviewed after the Resolved
Nits (non-blocking)
VerdictNo blockers. The previously-flagged test gap is closed and both nits are addressed — this is good to merge from my side (note: I cannot approve PRs). I was unable to run |
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
This PR looks more complicated than it should, let's discuss it more on Monday
|
Claude finished @brbrr's task in 4m 21s —— View job PR Review: SettlementLayer & EthMessagingClient abstractions
I reviewed this against the branch base ( Verified (no action needed)
Nits (non-blocking)
Note: I could not run Re @rodrodros's "more complicated than it should be" — that's a design-altitude question (worth the Monday discussion) rather than a correctness one; from a bug/race/leak standpoint the implementation is sound. |
|
Claude finished @brbrr's task in 3m 47s —— View job PR Review: SettlementLayer & EthMessagingClient abstractions
Re-reviewed after the latest The simplification is a real improvementThe hand-rolled
Verified (no action needed)
Nits (non-blocking)
Note: build is clean, but I couldn't run |
| if err != nil { | ||
| return fmt.Errorf("creating a new L1 client: %w", err) | ||
| } |
There was a problem hiding this comment.
Thoughts: I guess the message is ok, since the err will explain it is a "dial" problem. Also, it is clearer to the user using the term L1 client.
| // WithSettlementLogger attaches a logger forwarded to the settlement. | ||
| // Surfaces transport-level warnings at debug level. | ||
| func WithSettlementLogger(l log.StructuredLogger) GethSettlementOption { | ||
| return func(o *gethSettlementOptions) { o.logger = l } | ||
| } |
There was a problem hiding this comment.
Is this planning to be used in the next PR?
Because the log is never used, and the comment Surfaces transport-level warnings at debug level. is not correct at the moment.
| // SetListener swaps the event listener after construction. The metrics | ||
| // listener captures the settlement in a closure (so it can read gauges | ||
| // off it), which means the listener can only be built AFTER the | ||
| // settlement exists — hence a post-construction setter rather than a | ||
| // constructor option. | ||
| // | ||
| // Concurrency contract: SetListener MUST be called before the | ||
| // settlement is handed off to any goroutine (i.e. before l1.NewClient | ||
| // in node.go). The field is read unlocked from every RPC method; a | ||
| // concurrent SetListener would be a data race. The single-shot wiring | ||
| // in node.go satisfies this — don't introduce a second caller. | ||
| func (s *GethSettlement) SetListener(l EventListener) { s.listener = l } |
There was a problem hiding this comment.
I know you documented why this exists, but this is a VERY weird pattern IMO.
I'm not sure how to solve this, but is this really necessary?
There was a problem hiding this comment.
Pull request overview
This PR introduces a new L1 abstraction boundary by splitting the “settlement layer” (watch/filter state update events, heights, chain ID, receipts) from the higher-level l1.Client sync loop, and wires the new adapter into node startup/migrations and RPC.
Changes:
- Add
l1.SettlementLayer+l1.StateUpdateas chain-neutral types consumed byl1.Client, and implement a go-ethereum-backed adapter (l1.GethSettlement). - Remove the old geth-specific
EthSubscriberand the RPCEthReceiptAdapter, wiringGethSettlementdirectly into RPC handlers forstarknet_getMessageStatus. - Refactor L1 metrics so the listener is shared across
l1.Clientand the settlement adapter, and split settlement polling gauges into a separate registrar.
Reviewed changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/rpccore/rpccore.go | Adds/clarifies the L1Client interface documentation used by RPC for message status. |
| rpc/rpccore/eth_receipt_adapter.go | Removes the receipt adapter now that the settlement client returns Juno eth types directly. |
| rpc/rpccore/eth_receipt_adapter_test.go | Removes adapter tests (adapter deleted). |
| node/node.go | Constructs GethSettlement, attaches metrics listener/options, passes settlement to both l1.Client and RPC. |
| node/migration.go | Uses GethSettlement + l1.Client to fetch L1 head during migration without metrics listener. |
| node/metrics.go | Splits L1 metrics into listener vs settlement polling gauges; hooks into SettlementLayer. |
| node/metrics_test.go | Updates tests to use MockSettlementLayer and the split metric registration flow. |
| mocks/mock_subscriber.go | Deletes mock for the removed Subscriber interface. |
| mocks/mock_settlement_layer.go | Adds mock for the new SettlementLayer interface. |
| l1/settlement.go | Introduces SettlementLayer and chain-neutral StateUpdate. |
| l1/l1.go | Refactors l1.Client to depend on SettlementLayer and StateUpdate instead of geth types. |
| l1/l1_test.go | Updates internal tests to use MockSettlementLayer and StateUpdate. |
| l1/l1_pkg_test.go | Moves tests to l1_test and uses exported test helpers to avoid import cycles. |
| l1/export_test.go | Exposes package-private hooks to external tests (SetSettlement, cache accessors, retry loops). |
| l1/geth/contract/starknet.go | Adds generated contract binding used by GethSettlement. |
| l1/geth/contract/starknet_filter.go | Adds helper to fully drain filtered logs from abigen’s FilterLogs. |
| l1/geth_settlement.go | Adds go-ethereum-backed GethSettlement implementing SettlementLayer (+ receipt lookup). |
| l1/geth_settlement_test.go | Adds unit tests for GethSettlement via a minimal JSON-RPC test server. |
| l1/geth_forwarder_test.go | Adds tests for forwarding/teardown behavior of the state update forwarder. |
| l1/eth/subscription.go | Removes the old eth.Subscription interface (now expected to live at the l1 boundary). |
| l1/eth_subscriber.go | Deletes the old go-ethereum-backed subscriber implementation (replaced by GethSettlement). |
Files not reviewed (1)
- mocks/mock_settlement_layer.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brbrr
force-pushed
the
feat/l1-settlement-geth
branch
from
c30556e to
1c464ae
Compare
|
Claude finished @brbrr's task in 3m 48s —— View job PR Review: SettlementLayer abstraction
Re-reviewed the current tip ( Verified (no action needed)
Nits (non-blocking, all cosmetic — no inline comments posted)
Re @rodrodros's "more complicated than it should be" — the Verdict: No blockers. Good to merge from a correctness standpoint (note: I can't approve PRs). I couldn't run |
This is the first part of #3727 (deprecated) that adds an abstraction layer between the Geth L1 client and Juno, separating the settlement layer and the actual RPC client.