feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738
feat(l1): introduce SettlementLayer and EthMessagingClient abstractions#3738brbrr wants to merge 17 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3738 +/- ##
==========================================
- Coverage 75.40% 75.32% -0.08%
==========================================
Files 438 438
Lines 39501 39448 -53
==========================================
- Hits 29786 29715 -71
- Misses 7646 7651 +5
- Partials 2069 2082 +13 ☔ 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}
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 |
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 |
| // L2BlockHash is the Starknet block hash for that block. | ||
| L2BlockHash *felt.Felt | ||
| // StateRoot is the Starknet global state root after the block | ||
| // (the "globalRoot" field in the on-chain event). | ||
| StateRoot *felt.Felt |
There was a problem hiding this comment.
It was a mechanical change, driven by the old usage, and the fact that L1Head uses pointers. But also it seems consistent with Felt usage across the codebase 🤷. Do you think they should be values?
There was a problem hiding this comment.
If the codebase use **felt.Felt you would write it here just like that because of what the codebase had?
I don't know if they should be values or pointers, I am asking why did you chose to define them as pointers.
So, I repeat the question, why did you choose *felt.Felt over felt.Felt?
There was a problem hiding this comment.
You want pointers when the value is shared among many instances, or you want to stablish it as optional (meaning it can be defined or undefined (i.e. nil)).
You want values when this doesn't apply.
There is no performance relevant update, is about stablishing how do you think this values represent as part of the state. I don't think StateRoot for example to something that could be nil in a state update 🤔
There was a problem hiding this comment.
Thanks for the explanation. that a helpful insight.
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
First review, overall looks good and makes sense, will perform additional rounds
RafaelGranza
left a comment
RafaelGranza
left a comment
There was a problem hiding this comment.
LGTM. Just the one nit on the unused fields.
Signed-off-by: Yaroslav Kukharuk <i.kukharuk@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude finished @brbrr's task in 3m 34s —— View job PR Review: L1StateProvider abstraction — re-review after latest push
Re-reviewed the current tip on ✅ Resolved since last review
Verified (no action needed)
Nits (non-blocking, cosmetic — carried over, no inline comments)
Verdict: No blockers — the previous compile/format breakage is fixed, the package builds and is gofmt-clean, and all cross-round feedback is addressed. Good to merge from a correctness standpoint (note: I can't approve PRs). I couldn't run |
| // GethL1StateProvider must satisfy both interfaces it serves. | ||
| var ( | ||
| _ L1StateProvider = (*GethL1StateProvider)(nil) | ||
| _ rpccore.L1Client = (*GethL1StateProvider)(nil) | ||
| ) |
There was a problem hiding this comment.
Define on top of the file please
| func gethReceiptToEth(r *types.Receipt) *eth.Receipt { | ||
| logs := make([]eth.Log, len(r.Logs)) | ||
| for i, l := range r.Logs { | ||
| logs[i] = gethLogToEth(l) | ||
| } | ||
| return ð.Receipt{Logs: logs} | ||
| } |
| l1 Subscriber, | ||
| provider L1StateProvider, | ||
| chain *blockchain.Blockchain, | ||
| logger log.StructuredLogger, |
There was a problem hiding this comment.
Please add an issue that refactors the constructor into making the logger input part of the options
|
|
||
| type recordedCall struct { | ||
| method string | ||
| dur time.Duration |
There was a problem hiding this comment.
full name for this field, duration
| require.NoError(t, err) | ||
| t.Cleanup(s.Close) | ||
| _, err = s.ChainID(ctx) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Additionally, all the test files check that there was a returned error, but they are not checking which error is returned. Please update for every test case, the expected error by using require.ErrorContains(...)
| sink chan<- *StateUpdate, | ||
| ) (Subscription, error) { | ||
| raw := make(chan *contract.StarknetLogStateUpdate, watchForwarderBuffer) | ||
| inner, err := s.filterer.WatchLogStateUpdate(&bind.WatchOpts{Context: ctx}, raw) |
There was a problem hiding this comment.
raw, sink, inner? Could we please use meaningful names.
Suggestions:
raw -> rawEventsCh or gethEventsCh
inner -> sub or gethSub
sink is optional, since it is geth nomenclature, but I would prefer updatesSink or updatesCh (since we are moving away from geth).
| head, err := s.ethClient.HeaderByNumber(ctx, gethFinalizedBlockNumber) | ||
| if err != nil { | ||
| if errors.Is(err, ethereum.NotFound) { | ||
| return 0, fmt.Errorf("finalised block not found: %w", eth.ErrNotFound) | ||
| } | ||
| return 0, fmt.Errorf("getting finalised Ethereum block: %w", err) | ||
| } |
There was a problem hiding this comment.
Do we need to get the entire head just to get the block number, can't just we ask for the block number
There was a problem hiding this comment.
unfortunately there is no easy way to get finalized block number otherwise. There is a getBlockNumber, which returns only latest block, not what we need
| func forwardStateUpdates( | ||
| inner event.Subscription, | ||
| raw <-chan *contract.StarknetLogStateUpdate, | ||
| sink chan<- *StateUpdate, |
There was a problem hiding this comment.
Similar renames as the previous comment, applying the suggestions there mentioned
|
Claude finished @brbrr's task in 3m 34s —— View job PR Review: L1StateProvider abstraction — re-review after latest push (
|
|
Claude finished @brbrr's task in 3m 20s —— View job PR Review: L1StateProvider abstraction — re-review after latest push (
|
brbrr
force-pushed
the
feat/l1-settlement-geth
branch
from
572a12a to
c4674c2
Compare
|
Claude finished @brbrr's task in 3m 11s —— View job PR Review: L1StateProvider abstraction — re-review after latest push (
|
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.