Conversation
WalkthroughAdds a delegator subsystem (provider, tapscript, manager), threads delegator support through wallet and service-worker flows, updates intent/forfeit and script utilities, adds a Fulmine service and Dockerfile, and expands E2E tests and test setup to exercise delegation scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Wallet (Client)
participant SW as ServiceWorker
participant Delegator as RestDelegatorProvider
participant Ark as Ark Server
participant Chain as Blockchain
Client->>SW: InitWallet(delegatorUrl)
SW->>Delegator: GET /api/v1/delegate/info
Delegator-->>SW: { pubkey, fee, delegatorAddress }
SW-->>Client: provide delegatorProvider
Client->>Client: buildSignedDelegateIntent(...)
Client->>Delegator: POST /api/v1/delegate { intent, forfeits }
Delegator->>Ark: forward delegate request
Ark->>Chain: broadcast forfeit/renewal txs
Chain-->>Ark: confirmations
Ark-->>Delegator: success
Delegator-->>Client: success / error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/providers/delegator.ts`:
- Around line 64-69: After parsing response.json() in
src/providers/delegator.ts, stop defaulting to empty strings for pubkey, fee,
and delegatorAddress; instead validate that data.pubkey, data.fee, and
data.delegatorAddress are present and non-empty and throw a clear Error if any
are missing. Locate the block around const data = await response.json() and
replace the fallback object with an explicit validation step that throws
(including which field is missing) before returning the object with pubkey, fee,
and delegatorAddress.
In `@src/wallet/wallet.ts`:
- Around line 217-221: The current delegatePubKey computation always slices off
the first byte via hex.decode(info.pubkey).slice(1), which corrupts 32‑byte
x‑only keys; instead: decode info.pubkey, inspect the resulting byte length and
prefix byte (from config.delegatorProvider.getDelegateInfo()), and only drop the
first byte when it's a 33‑byte compressed key (leading 0x02/0x03); if it's
already 32 bytes (x‑only) use it as‑is; update the delegatePubKey assignment to
perform this length/prefix check before slicing so tapscript keys remain
correct.
- Around line 1592-1606: The current defaulting logic in the block that computes
delegateAt uses vtxos.reduce with coin.virtualStatus.batchExpiry ?? 0, which
causes any missing expiry to be treated as 0 (epoch); instead, update the logic
in the same function (the code that computes expireAt from vtxos and sets
delegateAt) to ignore vtxos whose virtualStatus.batchExpiry is null/undefined:
first collect or map only defined expiries (e.g., filter coins where
coin.virtualStatus?.batchExpiry != null), then compute the minimum of those
expiries (or throw the existing "unable to delegate" error if none exist) and
set delegateAt to new Date((expireAt - 12 * 60 * 60) * 1000) as before.
- Around line 1624-1672: The amount accumulation loop over vtxos currently sums
candidate input contributions but later computes the delegator fee using
vtxos.length (including inputs we skipped) and doesn't subtract the delegator
fee from the main output before the dust check; fix by tracking only the
included inputs (e.g., increment an includedInputsCount when you accept a coin
in the loop), compute delegatorFeeAmount = BigInt(Number(fee)) *
BigInt(includedInputsCount), push the delegator output using that
delegatorFeeAmount, then compute outputFee based on the actual outputs array and
subtract both delegatorFeeAmount and outputFee from amount before performing the
dust check and returning amount; use the existing symbols
estimator.evalOffchainInput, estimator.evalOffchainOutput, vtxos, amount,
outputs, delegatorProvider.getDelegateInfo, and dust to locate and update the
logic.
In `@test/setup.mjs`:
- Around line 219-231: The curl calls that create/unlock the Fulmine wallet
embed a hardcoded private key and password; remove these secrets and fetch them
from environment variables or generate a keypair at test runtime instead.
Replace the literal JSON in the execCommand calls (the POST to
/api/v1/wallet/create and /api/v1/wallet/unlock) so they interpolate
process.env.FULMINE_PRIVATE_KEY and process.env.FULMINE_PASSWORD (or values
returned from a runtime key generator called before execCommand), and ensure
execCommand and any test teardown use those variables consistently; also add a
runtime check that fails the test if the required env vars are missing.
🧹 Nitpick comments (4)
docker-compose.yml (1)
98-119: Inconsistent YAML indentation style.The
fulmineservice uses 2-space indentation fordepends_on(lines 100-101) andenvironmentlist items (lines 107-113), while other services in this file use 4-space indentation. While technically valid YAML, this breaks consistency with the rest of the file.🛠️ Suggested fix for consistent indentation
fulmine: container_name: fulmine depends_on: - - arkd + - arkd build: context: . dockerfile: fulmine.Dockerfile restart: unless-stopped environment: - - FULMINE_ARK_SERVER=arkd:7070 - - FULMINE_ESPLORA_URL=http://chopsticks:3000 - - FULMINE_NO_MACAROONS=true - - FULMINE_LOG_LEVEL=5 - - FULMINE_DISABLE_TELEMETRY=true - - FULMINE_SWAP_TIMEOUT=120 - - FULMINE_SCHEDULER_POLL_INTERVAL=10 + - FULMINE_ARK_SERVER=arkd:7070 + - FULMINE_ESPLORA_URL=http://chopsticks:3000 + - FULMINE_NO_MACAROONS=true + - FULMINE_LOG_LEVEL=5 + - FULMINE_DISABLE_TELEMETRY=true + - FULMINE_SWAP_TIMEOUT=120 + - FULMINE_SCHEDULER_POLL_INTERVAL=10 ports: - - "7000:7000" - - "7001:7001" + - "7000:7000" + - "7001:7001" volumes: - type: tmpfs target: /app/datatest/setup.mjs (1)
51-80: Avoid nested retry loops for wallet readiness.
checkWalletStatusalready retries up to 30 times, andwaitForWalletReadyrepeats it up to 30 times more. Worst‑case wait is ~30 minutes. Consider makingcheckWalletStatusa single attempt and letwaitForWalletReadymanage retries.♻️ Suggested refactor (single-attempt status check)
-async function checkWalletStatus(maxRetries = 30, retryDelay = 2000) { - const cmd = `${arkdExec} arkd wallet status`; - for (let i = 0; i < maxRetries; i++) { - try { - const statusOutput = execSync(cmd, { stdio: "pipe" }).toString(); - const initialized = statusOutput.includes("initialized: true"); - const unlocked = statusOutput.includes("unlocked: true"); - const synced = statusOutput.includes("synced: true"); - return { initialized, unlocked, synced }; - } catch { - await sleep(retryDelay); - } - } -} +async function checkWalletStatus() { + const cmd = `${arkdExec} arkd wallet status`; + const statusOutput = execSync(cmd, { stdio: "pipe" }).toString(); + const initialized = statusOutput.includes("initialized: true"); + const unlocked = statusOutput.includes("unlocked: true"); + const synced = statusOutput.includes("synced: true"); + return { initialized, unlocked, synced }; +}test/e2e/utils.ts (1)
12-81: Consider factoring shared wallet config to avoid drift.
createTestArkWalletandcreateTestArkWalletWithDelegateduplicate the same config. A small helper keeps changes in one place.♻️ Example refactor
+function buildArkWalletConfig( + identity: SingleKey, + delegatorProvider?: RestDelegatorProvider +) { + return { + identity, + arkServerUrl: "http://localhost:7070", + onchainProvider: new EsploraProvider("http://localhost:3000", { + forcePolling: true, + pollingInterval: 2000, + }), + ...(delegatorProvider ? { delegatorProvider } : {}), + }; +} + export async function createTestArkWallet(): Promise<TestArkWallet> { const identity = createTestIdentity(); const wallet = await Wallet.create({ - identity, - arkServerUrl: "http://localhost:7070", - onchainProvider: new EsploraProvider("http://localhost:3000", { - forcePolling: true, - pollingInterval: 2000, - }), + ...buildArkWalletConfig(identity), }); @@ export async function createTestArkWalletWithDelegate(): Promise<TestArkWallet> { const identity = createTestIdentity(); const wallet = await Wallet.create({ - identity, - arkServerUrl: "http://localhost:7070", - onchainProvider: new EsploraProvider("http://localhost:3000", { - forcePolling: true, - pollingInterval: 2000, - }), - delegatorProvider: new RestDelegatorProvider("http://localhost:7001"), + ...buildArkWalletConfig( + identity, + new RestDelegatorProvider("http://localhost:7001") + ), });test/e2e/ark.test.ts (1)
78-80: Reduce flakiness by replacing fixed sleeps with polling/notifications.There are many hard-coded delays after funding, settlement, and delegation. Prefer
waitFor/indexer-driven checks to avoid intermittent failures on slower CI.Also applies to: 120-121, 190-191, 214-215, 250-251, 292-294, 349-351, 383-386, 431-432, 486-487, 522-523, 548-549, 564-566, 648-655, 690-697, 789-790, 797-798, 819-820, 930-931, 976-977, 1051-1067
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docker-compose.ymlfulmine.Dockerfilepackage.jsonsrc/forfeit.tssrc/providers/delegator.tssrc/script/default.tssrc/wallet/index.tssrc/wallet/wallet.tstest/e2e/ark.test.tstest/e2e/utils.tstest/setup.jstest/setup.mjs
💤 Files with no reviewable changes (1)
- test/setup.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T14:50:47.093Z
Learnt from: louisinger
Repo: arkade-os/ts-sdk PR: 209
File: src/wallet/wallet.ts:587-625
Timestamp: 2025-10-23T14:50:47.093Z
Learning: In the Wallet.sendBitcoin method in src/wallet/wallet.ts (around lines 587-625), the return statement in the finally block is intentional. The method is designed to always return the arkTxid even if the repository operations (saving VTXOs and transactions) fail, because the transaction has already been finalized with the Ark provider. This allows callers to receive the transaction ID regardless of local storage failures.
Applied to files:
test/e2e/ark.test.ts
🧬 Code graph analysis (7)
test/e2e/utils.ts (2)
src/providers/onchain.ts (1)
EsploraProvider(59-345)src/providers/delegator.ts (1)
RestDelegatorProvider(27-71)
src/providers/delegator.ts (3)
src/index.ts (1)
Intent(232-232)src/intent/index.ts (1)
RegisterMessage(94-100)src/wallet/serviceWorker/response.ts (1)
Error(53-57)
src/wallet/index.ts (1)
src/providers/delegator.ts (1)
DelegatorProvider(10-16)
test/setup.mjs (1)
test/e2e/utils.ts (2)
arkdExec(14-15)execCommand(27-31)
src/forfeit.ts (1)
src/utils/transaction.ts (1)
Transaction(9-27)
src/wallet/wallet.ts (5)
src/providers/delegator.ts (1)
DelegatorProvider(10-16)src/arkfee/estimator.ts (1)
Estimator(23-142)src/script/base.ts (1)
scriptFromTapLeafScript(31-33)src/script/tapscript.ts (1)
decodeTapscript(50-72)src/forfeit.ts (1)
buildForfeitTxWithOutput(31-46)
src/script/default.ts (1)
src/script/base.ts (1)
TapLeafScript(19-26)
🪛 Gitleaks (8.30.0)
test/setup.mjs
[high] 221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (12)
fulmine.Dockerfile (3)
5-5: Dockerfile clones from personal fork - update before merge.Both
git clonecommands referencelouisinger/fulmine.git(personal fork) with thedelegatorbranch. The PR description mentions the official repo isArkLabsHQ/fulmine. This setup is fragile because:
- Once the upstream PR is merged, the
delegatorbranch may be deleted- Production builds shouldn't depend on personal forks
Consider updating to clone from the official repository once the upstream PR is merged, or use a tagged release.
-RUN git clone -b delegator --single-branch https://github.com/louisinger/fulmine.git +RUN git clone -b master --single-branch https://github.com/ArkLabsHQ/fulmine.gitAlso applies to: 21-21
24-41: LGTM - Build structure is well-designed.The multi-stage build is properly structured:
- Static Go binary with
CGO_ENABLED=0for Alpine compatibility- Minimal final image reduces attack surface
- Web assets correctly copied from builder stage
10-10: No changes needed —golang:1.25.5is valid.Go 1.25.5 is the latest stable patch release in the Go 1.25 series, released December 2, 2025. The Dockerfile uses a legitimate, current version of Go.
Likely an incorrect or invalid review comment.
src/script/default.ts (1)
25-105: Delegate path wiring looks consistent.
Conditional tapscript inclusion and the newhasDelegate()/delegate()accessors are clear and match the existing forfeit/exit pattern.package.json (1)
66-68: Script updates align with the new setup.mjs flow.
No concerns.src/wallet/index.ts (1)
10-37: Config extension is clean and backward‑compatible.
Adding an optionaldelegatorProviderintegrates neatly without breaking existing configs.src/forfeit.ts (1)
21-45: Nice extraction; shared forfeit construction stays consistent.Keeps P2A output handling centralized and avoids duplicated tx assembly.
test/e2e/ark.test.ts (1)
36-41: Good coverage: parameterized wallet factories.The looped
describestructure cleanly validates both wallet variants without duplicating test logic.src/wallet/wallet.ts (3)
130-132: Delegator provider is threaded consistently through wallet creation.Constructor and factory paths pass
delegatorProviderin a coherent way.Also applies to: 253-254, 275-277, 663-678, 729-731
1442-1451: Valid‑at propagation for register intents looks good.The optional
validAthook cleanly threads through to the intent message.
1695-1745: Delegator forfeit construction looks solid.Finding the delegate tap leaf and using
ALL_ANYONECANPAYfits the intended “add connector input later” flow.src/providers/delegator.ts (1)
40-45: > Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
we should not only generate static addresses with delegate Rather make it more like a consolidation procedure Move all you VTXO to VTXO+delegate with one click (on tx that pays to many) Ideally yes we may generate delegate scripts on default from that point on |
The problem of consolidation is that it will always result in a smaller expiry. If I have 1 coin expiring in 1 month and another coin in 1 day. Consolidation will make the delegator to refresh everything in 1 day. |
|
What I meant is Wallet has 3 VTXO bare Delegators are introduced Now wallet chose a delegate and sweep all the funds to new script Expiry is orthogonal even without the feature to move to new script is something the Wallet should have dealt anyway |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/serviceWorker/request.ts (1)
33-50: ValidatedelegatorUrlwhen present.Line 41–49:
isInitWalletdoesn’t validatedelegatorUrl, so a non-string value can slip through and later become an invalid URL. Add a type guard similar toarkServerPublicKey.Suggested fix
export function isInitWallet(message: Base): message is InitWallet { return ( message.type === "INIT_WALLET" && "arkServerUrl" in message && typeof message.arkServerUrl === "string" && ("arkServerPublicKey" in message ? message.arkServerPublicKey === undefined || typeof message.arkServerPublicKey === "string" - : true) + : true) && + ("delegatorUrl" in message + ? message.delegatorUrl === undefined || + typeof message.delegatorUrl === "string" + : true) ); }
🤖 Fix all issues with AI agents
In `@src/wallet/serviceWorker/worker.ts`:
- Around line 120-128: The handleDelegate method calls this.wallet.delegate(...)
even when no delegator provider is configured, causing runtime errors; modify
handleDelegate (in worker.ts) to first check whether delegation is configured
(e.g., a delegator provider/this.wallet.delegator or delegatorUrl flag) and
return early if not configured, and/or wrap the await
this.wallet.delegate(spendableVtxos) call in a try/catch that only swallows the
specific "no delegator configured" error while rethrowing other errors so normal
updates for non-delegation wallets don’t crash the flow. Ensure you reference
handleDelegate and this.wallet.delegate when making the guard or catch.
🧹 Nitpick comments (1)
docker-compose.yml (1)
98-120: Inconsistent YAML indentation style.The
fulmineservice uses different indentation levels compared to other services in this file. Specifically,depends_on,environment, andportsuse 8-space indentation whilevolumesuses 12-space indentation. Other services consistently use 12-space indentation for these sections.While YAML will parse this correctly, aligning with the existing style improves readability and maintainability.
🔧 Suggested indentation fix
fulmine: container_name: fulmine depends_on: - - arkd + - arkd build: context: . dockerfile: fulmine.Dockerfile restart: unless-stopped environment: - - FULMINE_ARK_SERVER=arkd:7070 - - FULMINE_ESPLORA_URL=http://chopsticks:3000 - - FULMINE_NO_MACAROONS=true - - FULMINE_LOG_LEVEL=5 - - FULMINE_DISABLE_TELEMETRY=true - - FULMINE_SWAP_TIMEOUT=120 - - FULMINE_SCHEDULER_POLL_INTERVAL=10 - - FULMINE_DELEGATE_ENABLED=true + - FULMINE_ARK_SERVER=arkd:7070 + - FULMINE_ESPLORA_URL=http://chopsticks:3000 + - FULMINE_NO_MACAROONS=true + - FULMINE_LOG_LEVEL=5 + - FULMINE_DISABLE_TELEMETRY=true + - FULMINE_SWAP_TIMEOUT=120 + - FULMINE_SCHEDULER_POLL_INTERVAL=10 + - FULMINE_DELEGATE_ENABLED=true ports: - - "7000:7000" - - "7001:7001" + - "7000:7000" + - "7001:7001" volumes: - type: tmpfs target: /app/data
There was a problem hiding this comment.
My idea is to treat delegates as custom script, rather than a default script with optionality, also because creates an hard dependency to an unknown/spontaneous third party we can't control.
We can generalize then that a custom script becomes the "main" script template via an opt-in config of the Wallet instance so that getAdddress (and in the future change address) could be encoded from such a script template (and filled of derived keys when needed).
That's would differ from one-off contracts (HTLC,Escrow...) that aren't spenadable / outgoing
We assume already a persistent and durable storage for script template / derivation mappings so restore should be already solved.
nit: I would not run a e2e with real fulmine, rather than mocking these behaviors as in the end are side effects. The SDK test should focus on the restore and encoding, let's mock server-side interaction around registration of scripts and actual delegated intents.
| delegate( | ||
| intent: SignedIntent<Intent.RegisterMessage>, | ||
| forfeits: string[] | ||
| ): Promise<void>; |
There was a problem hiding this comment.
It's not really helpful for the user if it has to come up with all the parameters at their final state already computed as parameters.
I would expect to initialize a class over-time with a Signer/Identity, a VTXO set, a mapping script template / script paramenters
It's in the end we need to accomodate all kinds of variants, our class serve as sugar syntax for well known script collections and tx workflows
There was a problem hiding this comment.
I agree but it misses some of WIP components like the contractRepository reworked : #251
There was a problem hiding this comment.
this is meant to be just light api service AFAICT
Kukks
left a comment
Kukks
left a comment
There was a problem hiding this comment.
As @tiero said let's separate into a new contract in itself. The new contract manager will be able to track the both efficiently.
I would also do a DelegatorManager service, which takes care of keeping an eye on vtxos and handling the business logic for delegate interactions. We need to keep the Wallet code lean
| delegate( | ||
| intent: SignedIntent<Intent.RegisterMessage>, | ||
| forfeits: string[] | ||
| ): Promise<void>; |
There was a problem hiding this comment.
this is meant to be just light api service AFAICT
src/script/default.ts
Outdated
| pubKey: Bytes; | ||
| serverPubKey: Bytes; | ||
| csvTimelock?: RelativeTimelock; | ||
| delegatePubKey?: Bytes; |
There was a problem hiding this comment.
Separate contract;
maybe introduce IHasDelegate interface? (we can do this another time)
There was a problem hiding this comment.
I did a DelegateVtxo extending DefaultVtxo (see last commit)
hard to do IHasDelegate because it depends where you want to set the delegate key (can be on any forfeit path, or several ones, or including extra spending condition like preimage reveal for vhtlc).
"hasDelegate" is already "agnostic" (does not depend on any classes) because it iterates over the tapscripts and search for the one with delegator public key. The signing step is the tricky part but as far as I understand it is abstracted in the ContractManager svc ?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/providers/delegator.ts`:
- Around line 55-72: The getDelegateInfo method currently rejects a valid fee
value of "0" due to the falsy check on data.fee; update validation in
getDelegateInfo to explicitly check types/emptiness (e.g., ensure data.pubkey
and data.delegatorAddress are non-empty strings and that data.fee is a string —
allowing "0" — or at minimum data.fee !== undefined/null and typeof data.fee ===
"string"), so DelegateInfo is returned with fee "0" accepted; reference
getDelegateInfo and the DelegateInfo contract and keep compatibility with the
usage in wallet.ts where code checks if (fee !== "0").
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/wallet/delegator.ts`:
- Around line 92-100: The grouped-path in delegator.ts currently ignores the
caller-supplied schedule by not forwarding the delegateAt argument into the
batched delegate calls; update the entries.map inside the Promise.allSettled so
each invocation of delegate(...) receives the delegateAt (explicit schedule)
parameter (i.e., pass delegateAt along with this.identity,
this.delegatorProvider, this.arkInfoProvider, vtxosGroup, destinationScript) so
the delegate function is called with the same schedule whether invoked per-item
or in grouped mode.
- Around line 132-143: The calculation for delegateAt uses vtxos.reduce with
(coin.virtualStatus.batchExpiry ?? 0), which treats missing batchExpiry as 0 and
can produce a past timestamp; change the logic in the block that computes
expireAt (around the delegateAt assignment) to first filter vtxos to only those
with a defined virtualStatus.batchExpiry (e.g., vtxos.filter(c =>
c.virtualStatus?.batchExpiry != null)), then compute the minimum expiry from
that filtered list and throw an error if the filtered list is empty; update the
reference to virtualStatus.batchExpiry and the error message to reflect missing
expiries so delegateAt is never set from a zero/default value.
- Around line 80-102: The current code captures entries from groupByExpiry into
the local variable entries before merging recoverableVtxos into the earliest
group, so the subsequent Promise.allSettled maps over the stale entries and
never sends recoverableVtxos to delegate; fix by moving or recomputing the
entries after you call groupByExpiry.set(earliestGroup, [...,
...recoverableVtxos]) (i.e., re-create entries =
Array.from(groupByExpiry.entries()) or perform the Promise.allSettled over
groupByExpiry.entries() directly) so delegate(this.identity,
this.delegatorProvider, this.arkInfoProvider, vtxosGroup, destinationScript) is
invoked with the updated groups including recoverableVtxos.
- Around line 316-319: The getDayTimestamp function currently normalizes to
local midnight using date.setHours(0,0,0,0), which creates timezone-dependent
day buckets; change it to use UTC by calling date.setUTCHours(0,0,0,0) instead
so server-provided expiry timestamps are grouped consistently across
environments (keep the existing multiplication by 1000 and return
date.getTime()).
- Around line 182-208: The fee handling currently uses Number(fee) and string
comparison which breaks on decimals and variations like "0.0" and also only
accounts for the delegator fee output when computing outputFee; update the logic
to parse fee as an integer BigInt (use BigInt(fee)) and compare numerically to
0n (e.g., feeBig !== 0n) when deciding whether to push the delegator fee output
(the output that uses ArkAddress.decode(delegatorAddress).pkScript and amount =
BigInt(fee) * BigInt(vtxos.length)); separately compute the
estimator.evalOffchainOutput cost for the destination output (using
destinationScript and the final destination amount) and include that in the
total outputFee before validating against dust and subtracting from amount,
ensuring amount -= totalOutputFee happens before pushing the destination output
(which uses destinationScript and amount).
In `@src/wallet/serviceWorker/worker.ts`:
- Around line 324-326: The subscription callback is currently awaiting
this.handler?.handleDelegate(), so any exception will bubble up and terminate
the update loop; wrap the call to handleDelegate() in a try/catch inside the
subscription callback (the block containing "delegate vtxos" / await
this.handler?.handleDelegate()), log the error (without rethrowing) and continue
so failures are best-effort and do not abort vtxo subscriptions.
🧹 Nitpick comments (4)
src/script/base.ts (1)
150-158: Prefer reusingscriptFromTapLeafScriptto strip the leaf version.Reduces duplication and keeps leaf parsing consistent with the existing helper.
♻️ Suggested refactor
- const scriptWithLeafVersion = tapLeafScript[1]; - const script = scriptWithLeafVersion.subarray( - 0, - scriptWithLeafVersion.length - 1 - ); + const script = scriptFromTapLeafScript(tapLeafScript);src/intent/index.ts (1)
289-295: Prefer"vout"over"tapTree"as the discriminator for distinguishingExtendedCoinfromTransactionInput.Both properties work for discrimination (since
TransactionInputhas neither), butvoutis more semantically appropriate. It directly identifiesCoin-based types (vout: numberinOutpoint), whereasTransactionInputusesindex. Usingvoutmakes the type checking clearer and more future-proof.Suggested fix
- if (!("tapTree" in coin)) { + if (!("vout" in coin)) { return coin; }test/e2e/ark.test.ts (2)
78-79: Replace fixed sleeps with condition-based waits to reduce flakiness.Several tests still rely on hard-coded delays after funding or block generation. Using the existing
waitForhelper (as you already do elsewhere) should make these tests more resilient under variable CI timing.Also applies to: 190-191, 250-251, 431-432, 522-523, 548-549, 1051-1052, 1066-1067
1060-1064: Make delegate manager presence explicit in the delegate test.Optional chaining can turn a missing delegatorManager into a silent no-op; a direct assertion will make failures more actionable.
✅ Suggested tweak
- await alice.wallet.delegatorManager?.delegate( + expect(alice.wallet.delegatorManager).toBeDefined(); + await alice.wallet.delegatorManager!.delegate( [vtxoBeforeDelegate], await alice.wallet.getAddress(), new Date(Date.now() + 1000) );
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/wallet/serviceWorker/worker.ts`:
- Around line 121-129: In handleDelegate, the code currently filters vtxos only
by !v.isSpent which still allows expired/recoverable/subdust inputs; change the
filter to use the vtxo.isSpendable predicate (or reuse
this.wallet.getSpendableVtxos if present) before calling
this.wallet.delegatorManager.delegate so only truly spendable VTXOs are passed
to DelegatorManager.delegate; update the call site in handleDelegate and remove
the old !v.isSpent filtering so delegation failures from non-spendable vtxos are
avoided.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/providers/utils.ts`:
- Around line 18-21: The current errorHandler creates an Error from
JSON.stringify(event) which yields "{}" and loses context; change it to build a
descriptive message from the EventSource context instead: detect the EventSource
via event.target (or fallback to event.currentTarget), read its readyState and
url (or provide "unknown" defaults), and if event is an ErrorEvent include its
.message; then construct the Error with that message and keep error.name =
"EventSourceError" (refer to the errorHandler function and use
EventSource.readyState / EventSource.url and ErrorEvent.message to populate the
error).
| FROM node:22 AS web-builder | ||
|
|
||
| WORKDIR /app | ||
| RUN git clone -b delegator --single-branch https://github.com/louisinger/fulmine.git |
There was a problem hiding this comment.
I think dockerfiles should reference the main repositories, not the personal ones.
There was a problem hiding this comment.
yep 100% but it's a workaround until ArkLabsHQ/fulmine#361 is merged
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wallet/delegator.ts`:
- Around line 67-75: The loop in delegator.ts silently skips non-recoverable
VTXOs when vtxo.virtualStatus.batchExpiry is missing, causing dropped VTXOs;
update the logic in the loop that processes vtxos (the block referencing
isRecoverable(vtxo), recoverableVtxos, and vtxo.virtualStatus.batchExpiry) to
fail fast or explicitly collect these cases instead of continue: either throw a
descriptive error when a non-recoverable VTXO lacks batchExpiry, or add them to
a new array (e.g., nonRecoverableMissingExpiry) and handle/report them after the
loop so they aren’t silently dropped, and adjust callers to handle the new error
or reporting path.
- Around line 153-166: The current computation multiplies the epoch (expireAt)
by 0.9 which scales the timestamp instead of choosing a time 10% before expiry;
update the block that sets delegateAt (use the variables expireAt, vtxos,
virtualStatus.batchExpiry) to compute the remaining time and subtract 10% of
that remaining interval from expiry. Concretely: treat expireAt as seconds,
compute nowSec = Math.floor(Date.now()/1000), remaining = Math.max(0, expireAt -
nowSec), delta = Math.ceil(remaining * 0.1), then set delegateAt = new
Date((expireAt - delta) * 1000) (ensure you handle the case remaining === 0
appropriately so delegateAt is set to now or a minimal future time).
♻️ Duplicate comments (1)
src/wallet/delegator.ts (1)
203-234: Fix fee parsing and include destination output fee.
BigInt(Number(fee))risks precision loss and breaks on large values; additionally, the destination output’s fee is never accounted for. This can overspend the destination output.🛠️ Suggested fix
- const outputs = []; - - if (fee !== "0") { - outputs.push({ - script: ArkAddress.decode(delegatorAddress).pkScript, - amount: BigInt(Number(fee)) * BigInt(vtxos.length), - }); - } - - const outputFee = outputs.reduce((fee, output) => { - if (!output.amount || !output.script) return fee; - return ( - fee + - estimator.evalOffchainOutput({ - amount: output.amount, - script: hex.encode(output.script), - }).satoshis - ); - }, 0); - - if (amount - BigInt(outputFee) <= dust) { + const outputs: TransactionOutput[] = []; + + const feeAmount = BigInt(fee); + if (feeAmount !== 0n) { + outputs.push({ + script: ArkAddress.decode(delegatorAddress).pkScript, + amount: feeAmount * BigInt(vtxos.length), + }); + } + + let outputFee = 0n; + for (const output of outputs) { + outputFee += BigInt( + estimator.evalOffchainOutput({ + amount: output.amount, + script: hex.encode(output.script), + }).satoshis + ); + } + + amount -= outputFee; + const destinationFee = BigInt( + estimator.evalOffchainOutput({ + amount, + script: hex.encode(destinationScript), + }).satoshis + ); + + if (amount - destinationFee <= dust) { throw new Error("Amount is below dust limit, cannot delegate"); } - amount -= BigInt(outputFee); + amount -= destinationFee;
| for (const vtxo of vtxos) { | ||
| if (isRecoverable(vtxo)) { | ||
| recoverableVtxos.push(vtxo); | ||
| continue; | ||
| } | ||
|
|
||
| const expiry = vtxo.virtualStatus.batchExpiry; | ||
| if (!expiry) continue; | ||
|
|
There was a problem hiding this comment.
Don’t silently skip non‑recoverable VTXOs without expiry.
A missing batchExpiry currently drops the VTXO from delegation entirely, which can lead to un-delegated funds or an empty call. Prefer failing fast (or explicitly grouping them) instead of continue.
🛠️ Suggested guard
- const expiry = vtxo.virtualStatus.batchExpiry;
- if (!expiry) continue;
+ const expiry = vtxo.virtualStatus.batchExpiry;
+ if (expiry == null) {
+ throw new Error("unable to delegate: missing batchExpiry");
+ }🤖 Prompt for AI Agents
In `@src/wallet/delegator.ts` around lines 67 - 75, The loop in delegator.ts
silently skips non-recoverable VTXOs when vtxo.virtualStatus.batchExpiry is
missing, causing dropped VTXOs; update the logic in the loop that processes
vtxos (the block referencing isRecoverable(vtxo), recoverableVtxos, and
vtxo.virtualStatus.batchExpiry) to fail fast or explicitly collect these cases
instead of continue: either throw a descriptive error when a non-recoverable
VTXO lacks batchExpiry, or add them to a new array (e.g.,
nonRecoverableMissingExpiry) and handle/report them after the loop so they
aren’t silently dropped, and adjust callers to handle the new error or reporting
path.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wallet/delegator.ts`:
- Around line 127-134: The JSDoc for the delegation function incorrectly states
the default timing as "12 hours before expiry" while the implementation uses
~10% of remaining time (or a +1 minute fallback); update the JSDoc for the
function that takes parameters vtxos and delegateAt to accurately describe the
actual default behavior (i.e., if delegateAt is omitted it defaults to ~10% of
the remaining time until the earliest vtxo expiry, with a minimum fallback of 1
minute), and make the same doc change for the second documentation block
referenced around lines 153-172 so both comments match the implementation.
- Around line 107-123: The current Promise.allSettled call in the delegation
loop swallows partial failures by only logging rejected items, so update the
handling in the block that calls Promise.allSettled (the mapping over
groupByExpiry and calls to delegate(this.identity, this.delegatorProvider,
this.arkInfoProvider, vtxosGroup, destinationScript)) to collect any rejected
results and after logging throw an AggregateError (or a new Error containing all
rejection reasons) so callers can react to partial delegation failures; preserve
successful results if you need to return them, but ensure the method throws when
any resultItem.status === "rejected" and include the resultItem.reason(s) in the
error message/aggregate.
♻️ Duplicate comments (2)
src/wallet/delegator.ts (2)
67-75: Don’t silently drop non‑recoverable VTXOs without expiry.Missing
batchExpirycurrently skips non‑recoverable inputs, which can leave funds undelegated without any signal to the caller. Prefer failing fast (or explicitly collecting/reporting these cases).🛠️ Suggested guard
- const expiry = vtxo.virtualStatus.batchExpiry; - if (!expiry) continue; + const expiry = vtxo.virtualStatus.batchExpiry; + if (expiry == null) { + throw new Error("unable to delegate: missing batchExpiry"); + }
210-241: Fix delegator fee parsing and include destination output fee.
fee !== "0"+BigInt(Number(fee))can mis-handle"0.0"and loses precision/throws for non‑integer strings, andoutputFeecurrently excludes the destination output fee. Both can miscompute amounts and dust checks.🛠️ One possible fix
- const outputs = []; - - if (fee !== "0") { + const outputs: TransactionOutput[] = []; + const feeStr = fee.trim(); + if (!/^\d+$/.test(feeStr)) { + throw new Error("delegator fee must be integer satoshis"); + } + const feeSat = BigInt(feeStr); + if (feeSat > 0n) { outputs.push({ script: ArkAddress.decode(delegatorAddress).pkScript, - amount: BigInt(Number(fee)) * BigInt(vtxos.length), + amount: feeSat * BigInt(vtxos.length), }); } - const outputFee = outputs.reduce((fee, output) => { + const outputFee = outputs.reduce((fee, output) => { if (!output.amount || !output.script) return fee; return ( fee + estimator.evalOffchainOutput({ amount: output.amount, script: hex.encode(output.script), }).satoshis ); }, 0); + const amountAfterBaseFee = amount - BigInt(outputFee); + const destinationOutputFee = estimator.evalOffchainOutput({ + amount: amountAfterBaseFee, + script: hex.encode(destinationScript), + }).satoshis; + const totalOutputFee = outputFee + destinationOutputFee; - if (amount - BigInt(outputFee) <= dust) { + if (amount - BigInt(totalOutputFee) <= dust) { throw new Error("Amount is below dust limit, cannot delegate"); } - amount -= BigInt(outputFee); + amount -= BigInt(totalOutputFee);#!/bin/bash # Verify the delegator fee type/format across the codebase rg -n "interface DelegateInfo|type DelegateInfo|fee" src/providers/delegator.ts rg -n "getDelegateInfo" src -g'*.ts' rg -n "delegat.*fee" test -g'*.ts'
| const result = await Promise.allSettled( | ||
| Array.from(groupByExpiry.entries()).map(async ([, vtxosGroup]) => | ||
| delegate( | ||
| this.identity, | ||
| this.delegatorProvider, | ||
| this.arkInfoProvider, | ||
| vtxosGroup, | ||
| destinationScript | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| for (const resultItem of result) { | ||
| if (resultItem.status === "rejected") { | ||
| console.error("delegate error", resultItem.reason); | ||
| } | ||
| } |
There was a problem hiding this comment.
Surface grouped delegation failures to the caller.
Promise.allSettled logs and returns success even when some groups fail, so callers can’t react to partial delegation. Consider throwing an aggregate error after logging.
🧭 Possible fix
- for (const resultItem of result) {
- if (resultItem.status === "rejected") {
- console.error("delegate error", resultItem.reason);
- }
- }
+ const failures = result.filter(
+ (r): r is PromiseRejectedResult => r.status === "rejected"
+ );
+ if (failures.length) {
+ failures.forEach((f) => console.error("delegate error", f.reason));
+ throw new AggregateError(
+ failures.map((f) => f.reason),
+ "delegation failed for one or more expiry groups"
+ );
+ }🤖 Prompt for AI Agents
In `@src/wallet/delegator.ts` around lines 107 - 123, The current
Promise.allSettled call in the delegation loop swallows partial failures by only
logging rejected items, so update the handling in the block that calls
Promise.allSettled (the mapping over groupByExpiry and calls to
delegate(this.identity, this.delegatorProvider, this.arkInfoProvider,
vtxosGroup, destinationScript)) to collect any rejected results and after
logging throw an AggregateError (or a new Error containing all rejection
reasons) so callers can react to partial delegation failures; preserve
successful results if you need to return them, but ensure the method throws when
any resultItem.status === "rejected" and include the resultItem.reason(s) in the
error message/aggregate.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/wallet/delegator.ts`:
- Line 178: Rename the misspelled variable deletageAtSeconds to
delegateAtSeconds in the delegator code: update the declaration (const
deletageAtSeconds = delegateAt.getTime() / 1000) and all usages that reference
it (the subsequent places noted in the review where deletageAtSeconds is read)
so they use delegateAtSeconds consistently; ensure any exported or
function-scoped references (e.g., inside the functions handling delegation
timing) are updated to the corrected identifier to prevent runtime/reference
errors.
♻️ Duplicate comments (5)
src/wallet/delegator.ts (5)
67-74: Non-recoverable VTXOs without expiry are silently dropped.The
continuestatement at line 74 silently skips VTXOs that lackbatchExpiry, potentially leaving funds undelegated without any indication to the caller.
107-123: Grouped delegation failures are not surfaced to the caller.
Promise.allSettledlogs rejected results but the method still returns successfully, preventing callers from detecting partial delegation failures.
127-134: JSDoc default timing doesn't match implementation.The documentation states "defaults to 12 hours before the earliest expiry" but the implementation at lines 166-172 uses 10% of remaining time.
212-219: Fee parsing can throw on decimal strings and mishandles "0.0".
BigInt(Number(fee))throwsRangeErroriffeeis a decimal string (e.g.,"0.5"). Additionally,fee !== "0"fails for"0.0"(string inequality), causing a zero-amount output to be created.
221-245: Destination output fee is not included inoutputFeecalculation.The
outputFeereduce at lines 221-230 only accounts for the delegator fee output. The destination output (pushed at line 242-245) also incurs an offchain fee that isn't calculated or deducted, causing the actual destination amount to overspend by that fee.
| const { fees, dust, forfeitAddress, network } = | ||
| await arkInfoProvider.getInfo(); | ||
|
|
||
| const deletageAtSeconds = delegateAt.getTime() / 1000; |
There was a problem hiding this comment.
Typo in variable name.
deletageAtSeconds should be delegateAtSeconds.
🔧 Suggested fix
- const deletageAtSeconds = delegateAt.getTime() / 1000;
+ const delegateAtSeconds = delegateAt.getTime() / 1000;Also update the references at lines 184, 188, and 253.
🤖 Prompt for AI Agents
In `@src/wallet/delegator.ts` at line 178, Rename the misspelled variable
deletageAtSeconds to delegateAtSeconds in the delegator code: update the
declaration (const deletageAtSeconds = delegateAt.getTime() / 1000) and all
usages that reference it (the subsequent places noted in the review where
deletageAtSeconds is read) so they use delegateAtSeconds consistently; ensure
any exported or function-scoped references (e.g., inside the functions handling
delegation timing) are updated to the corrected identifier to prevent
runtime/reference errors.
| const errorHandler = () => { | ||
| const error = new Error("EventSource error"); | ||
| const errorHandler: EventListenerOrEventListenerObject = (event: Event) => { | ||
| const error = new Error(JSON.stringify(event)); |
There was a problem hiding this comment.
event could be syntetized byt a 3rd party polyfill with unpredictable results. For example, a circular structure will make JSON.stringify throw.
It also leads to the creation of an error with a potentially very long string.
If we are interested in preserving the exact origin of the error, a safer approach is new Error("whatever", { cause: event }) but requires an upgrade of our compile target (> 2025 is think).
Otherwise you may want to extract event.type and use it in the error message.
| let earliestGroup = entries[0][0]; | ||
| for (const [dayKey] of entries) { | ||
| earliestGroup = Math.min(earliestGroup, dayKey); | ||
| } |
There was a problem hiding this comment.
These four lines can make use of more idiomatic JS:
const earliestGroup = Math.min(...groupByExpiry.keys());
|
|
||
| for (const resultItem of result) { | ||
| if (resultItem.status === "rejected") { | ||
| console.error("delegate error", resultItem.reason); |
There was a problem hiding this comment.
The caller could be interested in knowing that some VTXOs couldn't be delegated.
Without throwing, we can return Promise<{ delegated: Outpoint[], failed: Outpoint[] }> or something along those lines.
Then the caller can implement its own strategy (ignore, retry, UI comms, ...).
4 participants
This PR implements the delegator client for the new delegate service implements by fulmine ArkLabsHQ/fulmine#361
The
Walletclass accepts an optionalDelegatorProvider. If set, delegator service will be included as extra forfeit tapscript in the offchain ark address.Wallet.delegatemethod can be used to delegate the renewal of a given set of vtxos. e2e test provide a full exemple of a wallet using delegator service.@tiero @pietro909 @bordalix @Kukks please review
Summary by CodeRabbit
New Features
Infrastructure
Tests
✏️ Tip: You can customize this high-level summary in your review settings.