feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738
feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738brbrr wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3738 +/- ##
==========================================
+ Coverage 75.13% 75.24% +0.11%
==========================================
Files 425 426 +1
Lines 38331 38489 +158
==========================================
+ Hits 28799 28961 +162
+ Misses 7538 7522 -16
- Partials 1994 2006 +12 ☔ 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 |
2 participants
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.