domain: extract ChainDepthAndParentMarkers helper (part of #1159) - #1160
domain: extract ChainDepthAndParentMarkers helper (part of #1159)#1160bitcoin-coder-bob wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change adds a shared domain helper for VTXO chain depth and parent markers. Offchain transaction finalization and submission use the helper. Tests cover depth calculation, marker filtering, deduplication, and sorting. ChangesVTXO chain metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change centralizes offchain chain-depth and parent-marker calculation while preserving the authoritative empty-spent-set depth behavior. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 7cd9a54 — LGTM.
Verified
ChainDepthAndParentMarkersininternal/core/domain/marker.go:39-63preserves the existing semantics exactly: depth ismax(parent depths) + 1when spent is non-empty,0otherwise; parents are deduped, empty strings dropped, output sorted for determinism.- Both call sites now go through the helper:
service.go(event handler at ~L313, SubmitOffchainTx at ~L1077). Diffed the behaviour —sort.StringsatSubmitOffchainTxmoves inside the helper (same result), and the event-handler path previously did no marker inheritance (dropped) — this PR now inherits parent markers in that path too. That's a behavioural change worth calling out in the PR body: on the OffchainTxAccepted / settlement event path, new vtxos will now carry inheritedMarkerIDs. If that's intentional (which reads that way from the docstring), please confirm; if not, the event-handler branch should still drop them. sortimport removed fromservice.gosince the last use moved to the helper. Clean.- Tests cover: empty spent → depth 0 / no markers; single parent inheritance; multi-parent dedup + sort + empty-drop; parents-with-no-markers → depth-only. Boundary cases are all pinned.
Nit
- Docstring says "single source of truth for the computation recorded on the OffchainTxAccepted event." Since after this PR the same helper is also used from the event handler path that assigns marker IDs to new vtxos (not just the accepted event), the phrasing understates its role. Minor.
Part of the #1159 series — merge order will matter for the domain type additions that follow (VtxoKind, ChainDepthAndParentMarkers, single-spend). No cross-repo impact: ChainDepthAndParentMarkers is a Go domain helper, no wire surface.
Pull the chain-depth and parent-marker computation out of SubmitOffchainTx and the offchain update handler into one pure domain helper, so the two call sites cannot drift and on-chain paths (issue #1159) can reuse the same computation. Behaviour-preserving on the reachable paths. The update handler previously did an unconditional max+1, yielding depth 1 for an empty spent set, while the authoritative accept path yields 0; the helper uses the accept-path (0) semantics. That edge is unreachable, a finalized offchain tx always spends at least one vtxo, and 0 is the correct value. Part of #1159.
740b0cb to
5c818e9
Compare
|
This PR has been open for 3+ days without review. @bitcoin-coder-bob is anyone looking at this? |
|
This PR has been open for 14 days without review. @bitcoin-coder-bob is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — sha 5c818e9
Looks ready to merge.
ChainDepthAndParentMarkers is a clean extraction of duplicated logic that existed in both service.go::registerEventHandlers and service.go::SubmitOffchainTx. The implementations were almost identical but the event-handler path previously omitted the parent-marker accumulation (it only tracked depth). The new shared helper is the full version; the event handler intentionally discards the markers return value (depth, _ := domain.ChainDepthAndParentMarkers(spentVtxos)), which matches the old behavior.
Tests are comprehensive: nil input, single parent, max-depth selection, dedup/sort of marker IDs, and dropped empty strings. The sorting guarantees a deterministic wire format regardless of input order.
No concerns.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — domain: extract ChainDepthAndParentMarkers helper
What changed:
Extracts inline depth+marker computation from two call sites in into . Reduces duplication and makes the rule a single source of truth.
Subtle behaviour change in registerEventHandlers: The old inline code did newVtxos[i].Depth = maxDepth + 1 without guarding on len(spentVtxos) > 0, so an event with no spent vtxos (e.g. boarding) would set depth=1. The new helper correctly returns depth=0 when len(spent)==0. This is the right behaviour — a vtxo with no parents should be at depth 0. The SubmitOffchainTx path already had the if len(spentVtxos) > 0 guard, so it was consistent; registerEventHandlers is now fixed to match.
Tests: TestChainDepthAndParentMarkers covers empty input (depth 0), single parent, multiple parents with dedup+sort, and empty marker strings dropped. Complete.
Looks ready to merge.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — ChainDepthAndParentMarkers helper
Verdict: looks ready to merge.
Clean extraction of the repeated depth + parent-marker computation that existed in two places (service.go:SubmitOffchainTx and service.go:registerEventHandlers) into a single, tested domain function.
- The semantics are identical to what both call sites did inline:
max(parent depths) + 1, deduplicated sorted parent marker IDs with empty strings dropped. - The zero-input case (depth 0, no markers) is explicit and correct.
- Test coverage is thorough: nil input, single parent, multiple-parent dedup+sort+empty-drop, no-marker parents.
- Removing the inline
sort.Stringsimport fromservice.gois a nice hygiene win.
No concerns.
|
This PR has been open for 5+ days without a review. @bitcoin-coder-bob is anyone looking at this? (ChainDepthAndParentMarkers helper (part of #1159); 5 days without review.) |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — domain: extract ChainDepthAndParentMarkers helper
Not protocol-critical. No VTXO signing, forfeit, round lifecycle, or unilateral exit paths are touched.
Summary
Clean, well-scoped refactoring. The new domain.ChainDepthAndParentMarkers correctly captures the logic that previously lived inline in SubmitOffchainTx, and the two call sites now share a single source of truth. Test coverage is adequate for the helper.
Findings
1. Behavior delta in the event handler is reachable (though safe) — internal/core/application/service.go
The PR correctly discloses the semantic difference: old handler did maxDepth + 1 unconditionally (yielding depth 1 for an empty spent set); the new helper returns 0 for empty input.
The claim that the empty path is unreachable is almost right, but the guard at line 320 (len(spentVtxos) != len(spentVtxoKeys)) only fires when the counts differ. If decodeTx returns an empty spentVtxoKeys, both slices are length-zero, the guard passes, and execution continues with depth = 0. The protocol invariant that a finalized offchain tx always spends ≥ 1 VTXO is real, but it's enforced upstream (at submission time), not here. A future code path that re-uses RegisterOffchainTxUpdateHandler with an edge-case tx could hit this. 0 is the correct value either way, so this is not a bug, just worth documenting. An inline comment on the depth, _ := line explaining why 0 is safe (depth-from-empty is unreachable because Accept rejects zero-input txs) would serve future readers.
2. Silent _ discard of parent markers in the event handler — internal/core/application/service.go
depth, _ := domain.ChainDepthAndParentMarkers(spentVtxos)Correct: MarkerIDs are already set on the VTXOs during offchainTx.Accept() in SubmitOffchainTx; the finalization event handler is a post-projection callback that only needs to propagate depth to the decoded newVtxos. The discard is intentional and sound. A brief comment (// MarkerIDs already set during Accept; only depth propagated here) would prevent someone from incorrectly "fixing" this discard later.
3. Test coverage — internal/core/domain/marker_test.go
The four cases cover the key behaviors (zero input, single parent, deduplication/sorting/empty-string dropping, depth-only parents). One gap: no test asserts uint32 overflow behavior when maxDepth == math.MaxUint32. Since Depth is uint32 and the increment is unchecked, a chain long enough to wrap would silently emit depth 0. The MarkerInterval of 100 means a wrapping chain would need ~42 million hops, which is implausibly deep for off-chain operation, but a comment in the function or a //nolint with rationale would make the deliberate non-handling explicit. Not a blocker at current scale.
4. Cross-repo impact
ChainDepthAndParentMarkers is new and unexported outside the arkd module boundary. No SDK repos reference it. No interface or proto changes. No cross-repo breakage.
5. Conflict note
The PR body flags a potential merge conflict with #1162 (both edit SubmitOffchainTx). That's a merge coordination concern, not a correctness one. The logic here is encapsulated in the helper so the conflict, when it arises, is mechanical.
Verdict
No blocking issues. The two comments above (explaining the _ discard and the zero-input semantics) are minor polish suggestions, not change requests. Mechanically correct refactoring; safe to merge once the #1162 merge order is resolved.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass @ 207eb99 — one commit since last review (comments only); 1 prior open item fixed, 0 remain open.
Prior-issue close-out
Behavioural change: marker inheritance in the event-handler path (raised in review at 7cd9a543) — Fixed in 207eb99.
The commit adds an explicit comment block at service.go:334-342 that documents the design decision: marker IDs are intentionally dropped in the event-handler path because the projection of the OffchainTxAccepted event already assigns MarkerIDs to new vtxos from ParentMarkerIDs recorded at Accept. Only depth needs to be carried forward here. The comment also documents why the empty-spent-set depth=0 case is unreachable in practice. Both concerns I raised are now addressed in code.
Nit (docstring scope in marker.go) — Still open, still minor. The ChainDepthAndParentMarkers docstring still says "single source of truth for the computation recorded on the OffchainTxAccepted event," which understates its role now that it serves both the accept path and the settlement event path. The new service.go comment provides the context, so this is readable, just slightly imprecise at the definition site.
Incremental findings (207eb99)
None. The commit is documentation only — no logic, no interface, no protocol surface changed.
Status: No blocking issues. The one open nit (marker.go docstring wording) is cosmetic and can be cleaned up in a follow-on if desired. Code is otherwise clean.
|
This PR has been open for 41+ days without a review. @bitcoin-coder-bob — ChainDepthAndParentMarkers helper: stacked — is it waiting on #1161 or #1159? |
2 participants
First slice of #1159 (on-chain Arkade execution).
Extracts the chain-depth + parent-marker computation into one pure domain helper,
domain.ChainDepthAndParentMarkers, and routes the two existing off-chain call sites through it (SubmitOffchainTxand the offchain update handler). Behaviour-preserving cleanup: one computation, one place, so the two call sites cannot drift.Deliberately the smallest decision-independent piece of the epic, no fee-strategy, discriminator or schema decision is involved.
Update after the #1159 design call: the on-chain scope no longer extends Depth and the marker DAG to on-chain txs (the mempool already provides that ancestry), so this stands purely on its off-chain merit rather than as groundwork for on-chain chaining. Unaffected either way, it only touches existing off-chain paths.
Behaviour note (not strictly byte-identical)
The update handler previously computed depth as an unconditional
max(parent depths) + 1, which yields1for an empty spent set, while the authoritative accept path yields0. The helper uses the accept-path (0) semantics. That edge is unreachable, a finalized offchain tx always spends at least one vtxo, and0is the correct value, but calling it out rather than claiming no behaviour change.Test plan
go build ./...go test ./internal/core/domain/...(newTestChainDepthAndParentMarkers, 4 cases)go test ./internal/core/application/...(offchain flow stays green, proving behaviour-preserving)make lint(0 issues), gofmt cleanStatus
Independent of the rest of the #1159 stack and of every open decision there; merge whenever. If it lands after #1162, that PR needs a master merge, since both edit
SubmitOffchainTx.Summary by CodeRabbit
Bug Fixes
Tests