Feat/delegate contract#156
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces delegate contract support to the Go SDK by adding a ChangesDelegate Contract Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@arkanaai @coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Arkana Code Review —
|
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — feat/delegate-contract
Reviewed the full diff, all changed files in context, the underlying arkd script package, and cross-repo consumers. This is protocol-critical code (VTXO contract construction and spending paths). Requesting changes + human review.
🔴 P0 — Forfeit Closure Classification Is Ordering-Dependent
delegate_handler.go:47-49 — The delegate leaf MultisigClosure{PubKeys: [owner, delegate, server]} will be classified as a forfeit closure by ForfeitClosures() in arkd/pkg/ark-lib/script/vtxo_script.go:204-213, because ForfeitClosures() matches ALL *MultisigClosure types regardless of key count.
The client-lib (batch_session_handler.go:653,857, send.go:397, utils.go:425) consistently uses forfeitClosures[0], which works only because the 2-of-2 forfeit at leaf [1] appears before the 3-of-3 delegate at leaf [2] in the closures slice.
This ordering is load-bearing and undocumented. If anyone reorders the closures (forfeit at [2], delegate at [1]), the client-lib will pick the 3-of-3 delegate leaf as the forfeit path. The round will fail silently because only 2 of 3 signatures are available — or worse, succeed partially and create an unspendable forfeit transaction.
Recommendations:
- Add a prominent doc comment on the closure ordering requirement in
DelegateHandler.DeriveContract, explaining WHY the order matters. - Consider adding a dedicated closure type (e.g.,
DelegateMultisigClosure) in arkd soForfeitClosures()can distinguish delegate paths from actual forfeit paths. File as a follow-up issue at minimum. - Add a test that verifies
ForfeitClosures()on the produced script returns exactly 1 forfeit (not 2).
🔴 P1 — Nil delegateKey Panics
delegate_handler.go:34 — DeriveContract does not validate that delegateKey is non-nil. A nil pointer passed here will panic at schnorr.SerializePubKey(delegateKey) (line 49, inside the MultisigClosure PubKeys) or at the Params serialization (line 87).
// Add at the top of DeriveContract:
if delegateKey == nil {
return nil, fmt.Errorf("delegate key must not be nil")
}Same for manager.go:271 — NewDelegate passes delegateKey straight through without validation.
🟡 P2 — No Way to Select the Delegate Path
delegate_handler.go:92-107 — SelectPath returns forfeit (collaborative) or exit (non-collaborative). The delegate 3-of-3 path (leaf [2]) is never selectable through SelectPath. It only appears in GetSpendablePaths.
How does the delegate actually initiate a spend? If the intent is that delegate spending goes through a different code path, document it. If SelectPath should support it, PathContext needs a field (e.g., UseDelegatePath bool) to select leaf [2].
🟡 P3 — No Key Identity Validation
delegate_handler.go:42-49 — No validation that delegateKey differs from key.PubKey (owner) or cfg.SignerPubKey (server).
- If
delegateKey == ownerKey: the 3-of-3 becomes effectively 2-of-3 since one party signs twice. The security guarantee of requiring a distinct third party is silently bypassed. - If
delegateKey == signerKey: same issue, server controls 2-of-3.
Add explicit checks:
if delegateKey.IsEqual(key.PubKey) {
return nil, fmt.Errorf("delegate key must differ from owner key")
}
if delegateKey.IsEqual(cfg.SignerPubKey) {
return nil, fmt.Errorf("delegate key must differ from signer key")
}🟢 P4 — Minor / Style
-
manager.go:296-324— TheSelectPath/GetSpendablePathstype switches will need manual updates for every new contract type. Consider a handler registry (map[string]Handler). Not blocking. -
Test coverage is solid.
delegate_handler_test.gocovers derive, determinism, different-delegate-different-script, path selection (both directions), spendable paths, and error cases. Good. -
Mock updates in
watcher_test.go:131-146andspendable_vtxos_test.go:113-128— Correct and necessary for the interface expansion. No issues.
Cross-Repo Impact
- No downstream breakage. The
contractpackage is not imported by any consumer repo (bancod, demos, asset-demos). They depend on higher-levelarksdk.ArkClient. - No related open PRs found on consumer repos.
- arkd server compatibility:
ParseVtxoScripthandles arbitrary leaf counts.Validate()passes because the delegate closure contains the signer key. TheforfeitClosures[0]usage happens to pick the correct leaf due to ordering (see P0 above).
Verdict
The implementation is clean, well-tested, and follows existing patterns. However, the forfeit closure classification issue (P0) is a latent correctness risk that needs documentation at minimum and ideally a structural fix in arkd. The nil panic (P1) is a straightforward fix. P2 and P3 should be addressed before this ships.
There was a problem hiding this comment.
🔒 Arkana Protocol Review — Delegate Contract Handler
Verdict: Request changes (protocol-critical — requires human sign-off)
This PR adds a delegate spending path (3-of-3: owner + delegate + server) to Ark VTXOs. Any change to VTXO script structure is protocol-critical. The implementation is clean and well-tested, but I have one concrete issue and protocol-level flags that need human eyes.
🚨 Issue: Missing UseDelegatePath in PathContext (current base)
The diff adds UseDelegatePath bool to PathContext (types.go) and DelegateHandler.SelectPath uses it to choose between the 2-of-2 forfeit leaf and the 3-of-3 delegate leaf. However:
delegate_handler.go:106-108 — SelectPath returns the delegate leaf (tapscripts[2]) when pctx.UseDelegatePath is true, but there is no validation that UseDelegatePath is only set for delegate-type contracts. If a caller sets UseDelegatePath: true on a TypeDefault contract, DefaultHandler.SelectPath silently ignores it and returns the forfeit leaf. This is technically safe but semantically confusing — consider returning an error from manager.go:SelectPath if UseDelegatePath is set on a non-delegate contract type, or document the no-op behavior explicitly.
⚠️ Protocol-Critical Flags (require human review)
1. Tapscript leaf ordering is load-bearing — delegate_handler.go:18-24
The comment correctly warns that closure order matters: ForfeitClosures() in arkd matches all *MultisigClosure and the 2-of-2 forfeit at index [1] must precede the 3-of-3 delegate at index [2]. The forfeit path test (delegate_handler_test.go:152-161) verifies SelectPath(Collaborative: true) returns tapscripts[1]. Good, but fragile. Consider adding an explicit test that decodes the tapscript at index [1] and asserts it contains exactly 2 pubkeys, not 3 — this would catch an accidental reorder at the script level.
2. Forfeit transaction compatibility — delegate_handler.go:53-57
The 2-of-2 forfeit closure MultisigClosure{[owner, server]} matches the default contract's forfeit path. Confirm with human reviewers that arkd's round lifecycle and forfeit-building code handles delegate VTXOs identically to default VTXOs when taking the forfeit path. If arkd's forfeit builder uses ForfeitClosures()[0] as stated in the comment, this is correct.
3. Exit path — delegate_handler.go:49-52
The unilateral exit path is CSVMultisigClosure{[owner], UnilateralExitDelay} — only the owner can exit after the CSV timelock. The delegate has no unilateral exit path, meaning a delegate cannot independently recover funds. This is the correct security property (owner retains full custody), but human reviewers should confirm this matches the intended delegation model.
✅ What looks correct
-
manager.go:280-340—NewDelegatewithdelegateCreateMu: Properly serializes the check-create sequence. The dedup check (GetContracts→ scan for matching delegate key hex) happens BEFOREm.ks.NewKey(ctx), avoiding wasted key derivation. Good. -
manager.go:110-120— Load skip for delegate-only keys: Correctly preventsLoad()from creating default contracts (offchain/boarding/onchain) for keys that were created byNewDelegate. The logic handles the edge case of a key with both delegate and partial default contracts (proceeds to fill in missing defaults). Sound. -
manager.go:296-320— SelectPath/GetSpendablePaths routing: AddingTypeDefaultOnchainto the switch gives a better error path ("requires at least 2 tapscripts") instead of "unsupported type". The test atmanager_test.go:309-322explicitly verifies this. Good UX improvement. -
delegate_handler.go:36-46— Input validation: Nil check, owner-key check, signer-key check. All correct. Defense in depth with the manager's own nil check. -
Test coverage: Comprehensive — derivation determinism, different-delegate-different-script, path selection for all 3 modes (collaborative, delegate, unilateral), validation errors, tapscript count boundary. The
TestManager_NewDelegatetests cover dedup and isolation.
📝 Nits (non-blocking)
-
delegate_handler.go:55—cfg.SignerPubKeyis used directly as the server key in the forfeit and delegate closures. If the signer key rotates between rounds, delegate contracts derived with the old key become un-forfeitable. This is the same as default contracts, but worth a comment since delegate contracts may be longer-lived. -
manager.go:270-272— The nil check ondelegateKeyis duplicated betweenmanagerImpl.NewDelegateandDelegateHandler.DeriveContract. Defense in depth is fine; just noting it's intentional.
Bottom line: Code quality is high, patterns are consistent with DefaultHandler, tests are thorough. The one actionable issue is the UseDelegatePath semantic gap. The protocol-critical flags above need a human to confirm the delegation model matches arkd's round/forfeit behavior. Requesting changes to gate on human sign-off.
There was a problem hiding this comment.
Arkana Code Review — feat/delegate-contract
Verdict: Request changes (protocol-critical — requires human sign-off)
This PR adds a delegate VTXO contract type with a 3-of-3 (owner, delegate, server) spending path alongside the standard exit and forfeit paths. The code is well-structured, mirrors DefaultHandler correctly, and has strong test coverage including concurrency. Three items need attention before merge.
🔴 P0 — Tapscript ordering is load-bearing and fragile
contract/delegate_handler.go:36-39 — The comment correctly warns:
The 2-of-2 forfeit MUST remain at index [1] so it is picked ahead of the 3-of-3 delegate at index [2]. Do not reorder.
I verified this against arkd source (pkg/ark-lib/script/vtxo_script.go:204-213): ForfeitClosures() matches all *MultisigClosure regardless of key count and returns them in closure-list order. Downstream consumers (e.g. bancod/pkg/contract/taker.go:253) use forfeitClosures[0]. If the 3-of-3 delegate closure were to appear before the 2-of-2 forfeit, the server would attempt to build forfeit transactions requiring a delegate signature it cannot obtain — effectively making the VTXO unforfeitable.
The ordering is correct today, but this is a footgun. Recommendation: add a unit test in delegate_handler_test.go that explicitly asserts the closure type and key count at each index after TapTree() construction, so any reordering breaks the build, not production.
🟡 P1 — NewDelegate dedup check uses schnorr.SerializePubKey for key comparison
contract/manager.go (diff hunk at +280-356) — The deduplication logic serializes delegate keys with schnorr.SerializePubKey (x-only, 32 bytes) and compares hex strings:
delegateKeyHex := hex.EncodeToString(schnorr.SerializePubKey(delegateKey))
// ...
if existing[i].Params[ParamDelegateKey] == delegateKeyHex {Two distinct secp256k1 public keys with the same x-coordinate but different y-parity will collide here, causing NewDelegate to return the wrong contract. While btcec.PublicKey typically normalises to even-y (so this is unlikely in practice), the comparison should use the full 33-byte compressed encoding to be safe, or at minimum document the assumption that only even-y keys are accepted.
This same pattern exists in DeriveContract (delegate_handler.go:108) where the param is stored. Fix in both places if changing.
🟢 P2 — Delegate key ordering in 3-of-3 MultisigClosure
contract/delegate_handler.go:72-74 — The delegate closure is constructed as:
PubKeys: []*btcec.PublicKey{key.PubKey, delegateKey, cfg.SignerPubKey},This key ordering (owner, delegate, server) determines the signing order in the MuSig/multisig script. Please confirm this matches the server-side expectation in arkd. If arkd expects (owner, server, delegate) or any other ordering, signature verification will fail silently — the tap leaf hash won't match and the spend path will be unusable.
✅ What looks good
- Concurrency:
delegateCreateMucorrectly serializes the check-then-create sequence; lock ordering (delegateCreateMu→mu.RLock) is consistent withdefaultCreateMuusage. Concurrent test (TestManager_NewDelegate_Concurrent) with 20 goroutines validates this. - Load() skip logic (
manager.godiff hunk at +110-125): Correctly preventsLoad()from creating default contracts for delegate-only keys, while still handling crash recovery for partialNewDefaultwrites. SelectPathdispatch (manager.go:296-306): AddingTypeDefaultOnchainto the default handler case is a deliberate choice — the handler returns a descriptive "tapscripts" error rather than "unsupported type". Tested.- Validation: nil check, owner-key collision, signer-key collision all covered.
- Test coverage: Comprehensive — derivation, determinism, different-delegate-different-script, path selection (all 4 combinations of collaborative×delegate), spendable paths, event emission/non-emission on reuse, concurrent access, keystore errors.
- No cross-repo breakage: No downstream Go consumers directly implement the
Managerinterface outside this repo. Mock updates inwatcher_test.goandspendable_vtxos_test.goare correct.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Arkana Code Review — #156
Verdict: Request changes (minor) + request human review (protocol-critical)
This PR adds a DelegateHandler that derives offchain VTXO contracts with a 3-of-3 delegate spending path (owner + delegate + server) alongside the standard 2-of-2 forfeit and CSV exit paths. It also extends Manager with NewDelegate, SelectPath, and GetSpendablePaths, and hardens the Watcher with new tests.
🔴 Protocol-Critical — Requires Human Review
This PR modifies VTXO tapscript structure, which is protocol-critical. Even though the code looks correct after thorough analysis, a human must sign off before merge.
Closure ordering is load-bearing and correct. I verified against the upstream ForfeitClosures() implementation in arkd/pkg/ark-lib/script/vtxo_script.go:204-213:
ForfeitClosures()matches*MultisigClosure,*CLTVMultisigClosure,*ConditionMultisigClosure- It does NOT match
*CSVMultisigClosure(exit closure) - With the delegate layout
[0]=CSV exit, [1]=2-of-2 forfeit, [2]=3-of-3 delegate:ForfeitClosures()[0]= 2-of-2 (owner+server) ✓ForfeitClosures()[1]= 3-of-3 (owner+delegate+server) ✓
- Downstream consumer
bancod/pkg/contract/taker.go:253usesforfeitClosures[0]for forfeit signing — this correctly picks the 2-of-2 path even for delegate VTXOs.
Security property confirmed: The server can forfeit a delegate VTXO via the 2-of-2 path without the delegate's participation. This is correct — delegates must not have veto power over forfeit (anti-double-spend) operations.
The TestDelegateHandler_ClosureOrdering test at delegate_handler_test.go:515-575 is excellent — it independently reconstructs the closure layout and verifies ForfeitClosures() returns the expected order. This is the kind of test that prevents future reordering regressions.
🟡 Bug: Doc/code mismatch on ParamDelegateKey serialization
contract/types.go — The ParamDelegateKey constant comment says:
ParamDelegateKey = "delegateKey" // hex-encoded schnorr delegate public keyBut the actual serialization in delegate_handler.go:108 and manager.go (NewDelegate) uses SerializeCompressed() (33-byte with 02/03 parity prefix), not schnorr.SerializePubKey() (32-byte x-only):
ParamDelegateKey: hex.EncodeToString(delegateKey.SerializeCompressed()),This is internally consistent (storage and lookup both use compressed), so it's not a functional bug. But the comment will mislead future developers who try to deserialize with schnorr.DeserializePubKey().
Fix: Change the comment to // hex-encoded compressed delegate public key.
✅ What looks good
-
Validation (
delegate_handler.go:46-54): nil check, delegate≠owner, delegate≠signer — prevents degenerate contracts. -
Concurrency (
manager.go):delegateCreateMuserializes the check-then-create inNewDelegate, preventing duplicate contracts for the same delegate key under concurrent calls. Verified byTestManager_NewDelegate_Concurrentwith 20 goroutines. -
Load() skip logic (
manager.go:109-118): Correctly skips default contract derivation for keys that only have delegate contracts, preventing unwanted side effects during wallet reload. -
SelectPath routing (
manager.go): AddingTypeDefaultOnchainto the switch intentionally routes it throughDefaultHandlerwhich fails with "requires at least 2 tapscripts" — a more informative error than "unsupported type". Test atmanager_test.goconfirms this. -
Watcher tests: Backoff retry, context cancellation, address dedup, unsubscribe-on-stop, concurrent stop, malformed address handling — all solid additions that harden the watcher against real-world failure modes.
-
Test coverage: Comprehensive across all new code paths — derivation, path selection, manager integration, concurrency, error propagation, and the critical closure ordering invariant.
-
Cross-repo impact: Minimal. No external repos directly import
go-sdk/contract. TheManagerinterface expansion is SDK-internal.PathContext.UseDelegatePathis additive.
Summary
The protocol logic is sound and well-tested. One doc fix needed on ParamDelegateKey. Human review required on the VTXO tapscript structure change per protocol-critical policy.
🤖 Reviewed by Arkana
Resolves conflicts from feat/contract-watcher rebase. The contractManager architecture (types.Contract, handlers.Handler interface, GetHandler/NewContract/ ScanContracts API) replaces the old managerImpl. DelegateHandler is ported to the new interface: DeriveContract now returns *types.Contract using DelegateConfig instead of *clientTypes.Config, and the handler implements handlers.Handler with pre-computed tapscripts and "block:N"/"second:N" exit delay format stored in params. Manager.NewDelegate is added to support delegate contract creation with a delegate key.
|
@coderabbitai @arkanaai review |
|
✅ Actions performedReview triggered.
|
Arkana Code Review —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/manager.go (1)
51-54:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDelegate contracts are still invisible to manager-wide discovery.
Because
ContractTypeDelegateis not registered inm.handlers,GetSupportedContractTypes()will omit it andScanContracts()will never consider it. The laterGetHandlerspecial-case only fixes direct lookup, not the rest of the manager API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/manager.go` around lines 51 - 54, The handlers map initialization omits types.ContractTypeDelegate so manager methods like GetSupportedContractTypes() and ScanContracts() never see delegate contracts; add an entry to m.handlers mapping types.ContractTypeDelegate to the appropriate handler (e.g., defaultHandler.NewHandler(cachedClient, args.Network, true) if delegates should be treated like boarding) when building the handlers map so GetSupportedContractTypes(), ScanContracts(), and other manager APIs include delegates (this complements the existing GetHandler special-case).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/delegate_handler.go`:
- Around line 69-83: DeriveContract currently only checks delegateKey but later
dereferences key.PubKey and cfg.SignerKey; add precondition checks at the start
of DeriveContract to validate the owner and signer keys exist (e.g., ensure key
!= nil and key.PubKey != nil, and cfg.SignerKey != nil/zero) and return
descriptive errors if they are missing, so subsequent comparisons like
delegateKey.IsEqual(key.PubKey) and delegateKey.IsEqual(cfg.SignerKey) cannot
panic.
In `@contract/manager.go`:
- Around line 250-304: NewDelegate currently always advances the wallet key and
stores a new contract even when a contract for the same delegateKey already
exists; before calling m.keyProvider.NextKeyId or deriving/storing a contract,
check the store for an existing delegate contract keyed by the provided
delegateKey (e.g. call a new or existing method like
m.store.GetDelegateContract(ctx, delegateKey) or
m.store.FindContractByDelegateKey), if found unlock and return that existing
contract (and do not call m.keyProvider.NextKeyId, m.keyProvider.GetKey,
dh.DeriveContract, m.store.AddContract, or m.emit); only if no existing contract
is found proceed with the current flow (NextKeyId -> GetKey -> dh.DeriveContract
-> AddContract -> m.emit).
In `@contract/types.go`:
- Around line 38-41: The change adds NewDelegate to the exported Manager
interface which breaks downstream implementers (tests had to update
fixContractManager and watcherMockManager); instead revert Manager to its
previous shape and expose delegate creation via a new, separate interface or
helper: define a DelegateCreator interface (or a package-level function) that
declares NewDelegate and have concrete manager implementations optionally
implement it; update tests/mocks to implement DelegateCreator (not Manager)
where needed and remove NewDelegate from Manager to preserve source
compatibility.
---
Outside diff comments:
In `@contract/manager.go`:
- Around line 51-54: The handlers map initialization omits
types.ContractTypeDelegate so manager methods like GetSupportedContractTypes()
and ScanContracts() never see delegate contracts; add an entry to m.handlers
mapping types.ContractTypeDelegate to the appropriate handler (e.g.,
defaultHandler.NewHandler(cachedClient, args.Network, true) if delegates should
be treated like boarding) when building the handlers map so
GetSupportedContractTypes(), ScanContracts(), and other manager APIs include
delegates (this complements the existing GetHandler special-case).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a7e027c-c324-4139-9621-042f21888c6f
📒 Files selected for processing (7)
contract/delegate_handler.gocontract/delegate_handler_test.gocontract/manager.gocontract/types.gocontract/watcher_test.gospendable_vtxos_test.gotypes/types.go
| func (h *DelegateHandler) DeriveContract( | ||
| _ context.Context, | ||
| key wallet.KeyRef, | ||
| cfg DelegateConfig, | ||
| delegateKey *btcec.PublicKey, | ||
| ) (*types.Contract, error) { | ||
| if delegateKey == nil { | ||
| return nil, fmt.Errorf("delegate key must not be nil") | ||
| } | ||
| if delegateKey.IsEqual(key.PubKey) { | ||
| return nil, fmt.Errorf("delegate key must differ from owner key") | ||
| } | ||
| if delegateKey.IsEqual(cfg.SignerKey) { | ||
| return nil, fmt.Errorf("delegate key must differ from signer key") | ||
| } |
There was a problem hiding this comment.
Validate owner and signer keys before using them.
delegateKey is checked, but key.PubKey and cfg.SignerKey are dereferenced later without guards. Calling DeriveContract with an incomplete wallet.KeyRef or config will panic instead of returning an error.
Suggested guard
func (h *DelegateHandler) DeriveContract(
_ context.Context,
key wallet.KeyRef,
cfg DelegateConfig,
delegateKey *btcec.PublicKey,
) (*types.Contract, error) {
if delegateKey == nil {
return nil, fmt.Errorf("delegate key must not be nil")
}
+ if key.PubKey == nil {
+ return nil, fmt.Errorf("owner key must not be nil")
+ }
+ if cfg.SignerKey == nil {
+ return nil, fmt.Errorf("signer key must not be nil")
+ }
if delegateKey.IsEqual(key.PubKey) {
return nil, fmt.Errorf("delegate key must differ from owner key")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/delegate_handler.go` around lines 69 - 83, DeriveContract currently
only checks delegateKey but later dereferences key.PubKey and cfg.SignerKey; add
precondition checks at the start of DeriveContract to validate the owner and
signer keys exist (e.g., ensure key != nil and key.PubKey != nil, and
cfg.SignerKey != nil/zero) and return descriptive errors if they are missing, so
subsequent comparisons like delegateKey.IsEqual(key.PubKey) and
delegateKey.IsEqual(cfg.SignerKey) cannot panic.
| m.mu.Lock() | ||
|
|
||
| latestContract, err := m.store.GetLatestContract(ctx, types.ContractTypeDelegate) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, err | ||
| } | ||
|
|
||
| var keyId string | ||
| if latestContract != nil { | ||
| dh := &DelegateHandler{} | ||
| keyRef, err := dh.GetKeyRef(*latestContract) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, fmt.Errorf("failed to get key ref for latest delegate contract: %w", err) | ||
| } | ||
| keyId = keyRef.Id | ||
| } | ||
|
|
||
| nextKeyId, err := m.keyProvider.NextKeyId(ctx, keyId) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, fmt.Errorf("failed to compute next key index: %w", err) | ||
| } | ||
|
|
||
| keyRef, err := m.keyProvider.GetKey(ctx, nextKeyId) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, fmt.Errorf("failed to derive key for contract: %w", err) | ||
| } | ||
|
|
||
| dh := &DelegateHandler{} | ||
| contract, err := dh.DeriveContract(ctx, *keyRef, cfg, delegateKey) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, err | ||
| } | ||
|
|
||
| keyIndex, err := m.keyProvider.GetKeyIndex(ctx, keyRef.Id) | ||
| if err != nil { | ||
| m.mu.Unlock() | ||
| return nil, fmt.Errorf("failed to get key index: %w", err) | ||
| } | ||
|
|
||
| if err := m.store.AddContract(ctx, *contract, keyIndex); err != nil { | ||
| m.mu.Unlock() | ||
| return nil, fmt.Errorf("failed to store delegate contract: %w", err) | ||
| } | ||
|
|
||
| log.Debugf("%s added new delegate contract %s", logPrefix, contract.Script) | ||
| m.mu.Unlock() | ||
|
|
||
| m.emit(*contract) | ||
|
|
||
| return contract, nil |
There was a problem hiding this comment.
NewDelegate is missing the delegate-key dedup check.
This path always advances to the next wallet key and stores a new contract. A second call with the same delegateKey will therefore create duplicates and emit another creation event instead of returning the existing delegate contract.
One localized way to enforce dedup before key allocation
m.mu.Lock()
+ delegateKeyHex := hex.EncodeToString(delegateKey.SerializeCompressed())
+
+ existingContracts, err := m.store.GetContractsByType(ctx, types.ContractTypeDelegate)
+ if err != nil {
+ m.mu.Unlock()
+ return nil, fmt.Errorf("failed to load delegate contracts: %w", err)
+ }
+ for i := range existingContracts {
+ if existingContracts[i].Params[ParamDelegateKey] == delegateKeyHex {
+ m.mu.Unlock()
+ return &existingContracts[i], nil
+ }
+ }
latestContract, err := m.store.GetLatestContract(ctx, types.ContractTypeDelegate)
if err != nil {
m.mu.Unlock()
return nil, err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/manager.go` around lines 250 - 304, NewDelegate currently always
advances the wallet key and stores a new contract even when a contract for the
same delegateKey already exists; before calling m.keyProvider.NextKeyId or
deriving/storing a contract, check the store for an existing delegate contract
keyed by the provided delegateKey (e.g. call a new or existing method like
m.store.GetDelegateContract(ctx, delegateKey) or
m.store.FindContractByDelegateKey), if found unlock and return that existing
contract (and do not call m.keyProvider.NextKeyId, m.keyProvider.GetKey,
dh.DeriveContract, m.store.AddContract, or m.emit); only if no existing contract
is found proceed with the current flow (NextKeyId -> GetKey -> dh.DeriveContract
-> AddContract -> m.emit).
| // NewDelegate creates and stores a new delegate contract for the given delegate | ||
| // public key. The owner key is derived from the key provider. Returns an error if | ||
| // delegateKey is nil or matches the owner or signer key. | ||
| NewDelegate(ctx context.Context, delegateKey *btcec.PublicKey) (*types.Contract, error) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the contract/types.go file to understand the Manager interface context
cat -n contract/types.go | head -80Repository: arkade-os/go-sdk
Length of output: 3947
🏁 Script executed:
# Search for the Manager interface definition and any existing methods
rg "type Manager interface" -A 20 contract/Repository: arkade-os/go-sdk
Length of output: 1845
🏁 Script executed:
# Find all implementations of the Manager interface in the codebase
rg "Manager interface|func.*Manager.*\{" --type go -B 2 -A 10Repository: arkade-os/go-sdk
Length of output: 17444
🏁 Script executed:
# Check if there are any existing implementations by searching for types that implement Manager
rg "implement|implements" contract/types.go --type goRepository: arkade-os/go-sdk
Length of output: 42
🏁 Script executed:
# Look for test doubles and mock implementations that would need updating
fd -e "test.go" -o -name "*mock*" -o -name "*double*" | head -20Repository: arkade-os/go-sdk
Length of output: 291
🏁 Script executed:
# Search for NewDelegate implementations
rg "NewDelegate" --type go -B 2 -A 10Repository: arkade-os/go-sdk
Length of output: 4390
🏁 Script executed:
# Check if test doubles have the NewDelegate method
rg "func.*NewDelegate" --type go -B 2 -A 5Repository: arkade-os/go-sdk
Length of output: 1337
🏁 Script executed:
# Search in specific test files for NewDelegate
rg "fixContractManager\|watcherMockManager" -A 50 --type go | rg -A 2 "NewDelegate"Repository: arkade-os/go-sdk
Length of output: 42
🏁 Script executed:
# Look at contract/manager.go to see if it implements NewDelegate
cat -n contract/manager.go | grep -A 10 "NewDelegate"Repository: arkade-os/go-sdk
Length of output: 529
Manager interface expansion is a breaking API change for downstream implementers.
Adding NewDelegate to the exported Manager interface will require all external implementations to add this method. This PR's own test doubles (fixContractManager, watcherMockManager) had to be updated to satisfy the widened interface. If source compatibility must be maintained across releases, consider exposing delegate creation through a separate interface or concrete helper instead of widening Manager.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/types.go` around lines 38 - 41, The change adds NewDelegate to the
exported Manager interface which breaks downstream implementers (tests had to
update fixContractManager and watcherMockManager); instead revert Manager to its
previous shape and expose delegate creation via a new, separate interface or
helper: define a DelegateCreator interface (or a package-level function) that
declares NewDelegate and have concrete manager implementations optionally
implement it; update tests/mocks to implement DelegateCreator (not Manager)
where needed and remove NewDelegate from Manager to preserve source
compatibility.
…ateCreator from Manager
|
done in 8c407d3
done in 8c407d3 |
1 participant
closes #149
Summary
DelegateHandlerderiving an offchain VTXO contract with a 3-of-3(owner, delegate, server)spending path alongside the standard forfeit and unilateral-exit pathsManager.NewDelegatewith dedup, concurrent-safe check-then-create, andcontract_createdevent emission>= 0readiness check inTestWatcher_StopClosesEventsReview feedback addressed
TestDelegateHandler_ClosureOrdering, which asserts viaForfeitClosures()that the 2-of-2 forfeit is at index 0 and the 3-of-3 delegate is at index 1 — swapping them breaks both the address assertion and the new closure-order assertionParamDelegateKeystorage and theNewDelegatededup lookup from x-only schnorr (32 bytes) toSerializeCompressed(33 bytes), preventing a theoretical collision between keys with the same x-coordinate but opposite y-paritySummary by CodeRabbit
New Features
Tests