Conversation
WalkthroughUpdated dependency layout to @scure/btc-signer 2.0.1 and switched many imports to explicit ESM entry points (*.js); replaced manual txid double‑SHA256 computations with Transaction.id usage; adjusted one method parameter type to use typeof NETWORK; Docker build ARG set to a specific ref. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Code using tx
participant txObj as Transaction object
rect rgb(220,240,255)
Note over Caller,txObj: Previous flow (before changes)
Caller->>txObj: toBytes(true)
txObj-->>Caller: bytes
Caller->>Caller: sha256x2(bytes)
Caller->>Caller: reverse() + hex.encode -> txid
end
rect rgb(235,255,220)
Note over Caller,txObj: New flow (after changes)
Caller->>txObj: id
txObj-->>Caller: id (hex txid)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (3)
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: 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/wallet.ts (1)
1348-1365: Race: possible watcher leak if event fires before stopFunc is set.If coins arrive before notifyIncomingFunds resolves, stopFunc is undefined and never called. Await the registration first.
-export async function waitForIncomingFunds( - wallet: Wallet -): Promise<IncomingFunds> { - let stopFunc: (() => void) | undefined; - const promise = new Promise<IncomingFunds>((resolve) => { - wallet - .notifyIncomingFunds((coins: IncomingFunds) => { - resolve(coins); - if (stopFunc) stopFunc(); - }) - .then((stop) => { - stopFunc = stop; - }); - }); - return promise; -} +export async function waitForIncomingFunds( + wallet: Wallet +): Promise<IncomingFunds> { + const stop = await wallet.notifyIncomingFunds((coins) => { + // resolved in same tick; stop is always available + resolveFn(coins); + stop(); + }); + let resolveFn!: (c: IncomingFunds) => void; + return new Promise<IncomingFunds>((resolve) => (resolveFn = resolve)); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
examples/vhtlc.js(1 hunks)package.json(1 hunks)server.Dockerfile(1 hunks)src/arknote/index.ts(1 hunks)src/bip322/index.ts(1 hunks)src/forfeit.ts(1 hunks)src/identity/singleKey.ts(1 hunks)src/musig2/keys.ts(1 hunks)src/musig2/nonces.ts(1 hunks)src/musig2/sign.ts(1 hunks)src/script/base.ts(2 hunks)src/script/default.ts(1 hunks)src/script/tapscript.ts(1 hunks)src/script/vhtlc.ts(1 hunks)src/tree/signingSession.ts(3 hunks)src/tree/txTree.ts(3 hunks)src/tree/validation.ts(2 hunks)src/utils/anchor.ts(1 hunks)src/utils/arkTransaction.ts(2 hunks)src/utils/unknownFields.ts(1 hunks)src/wallet/index.ts(1 hunks)src/wallet/onchain.ts(1 hunks)src/wallet/unroll.ts(1 hunks)src/wallet/wallet.ts(2 hunks)test/e2e/indexer.test.ts(1 hunks)test/e2e/vhtlc.test.ts(1 hunks)test/tapscript.test.ts(1 hunks)test/verifySignatures.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T11:00:23.721Z
Learnt from: louisinger
PR: arkade-os/ts-sdk#121
File: src/identity/singleKey.ts:61-65
Timestamp: 2025-07-10T11:00:23.721Z
Learning: The `signIdx` method from the `scure/btc-signer` library already performs bounds checking internally for input indexes, making additional bounds checking in wrapper methods redundant.
Applied to files:
src/identity/singleKey.ts
⏰ 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 (40)
src/tree/validation.ts (2)
44-49: LGTM: Simplified txid computation using Transaction.id property.Replacing the manual
sha256x2hash computation withsettlementTx.idis cleaner and more maintainable. Ensure thatTransaction.idreturns a hex-encoded string (notUint8Array) to match the comparison at line 48.
83-91: LGTM: Consistent txid simplification.Same clean refactor as the settlement transaction: using
roundTransaction.idinstead of manual hash computation.src/tree/txTree.ts (3)
43-44: LGTM: Refactored to use Transaction.id property.Replacing the manual txid computation with
decodedChunk.tx.idsimplifies the code and ensures consistency across the codebase.
138-149: LGTM: Simplified parent txid lookup.Using
this.root.idfor validation is cleaner and consistent with the broader refactor. The parent-child relationship validation logic remains sound.
185-187: LGTM: Cleaner txid getter implementation.Returning
this.root.iddirectly is more maintainable than recomputing the hash. IfTransaction.idis a cached property, this also improves performance by avoiding redundant hash computations.src/tree/signingSession.ts (3)
5-5: LGTM: Explicit ESM module path.Adding the
.jsextension aligns with ES module resolution standards and is consistent with the broader module path updates in this PR.
225-230: LGTM: Simplified root input detection.Using
tx.id === graph.txidis cleaner and more readable than the previous hash-based computation. This aligns with the consistent refactor pattern across the PR.
235-235: LGTM: Removed redundant Uint8Array wrapper.Simplifying
hex.encode(new Uint8Array(parentInput.txid))tohex.encode(parentInput.txid)removes an unnecessary allocation, asparentInput.txidis already aUint8Array.src/wallet/unroll.ts (1)
1-3: Imports and version verified
@scure/btc-signer 2.0.1 is published and its exports include./psbt.js, so the explicit ESM imports (with.jsextension) are correct.src/musig2/sign.ts (1)
1-1: LGTM: ESM import path updated correctly.The explicit
.jsextension aligns with ES module resolution requirements for @scure/btc-signer 2.x.src/utils/unknownFields.ts (1)
3-3: LGTM: PSBT import path updated for ESM compatibility.The
.jsextension is correctly added to the psbt module import.src/identity/singleKey.ts (1)
1-1: LGTM: Utils import path updated for ESM.The explicit
.jsextension correctly updates the utils module import path.src/script/vhtlc.ts (1)
2-2: LGTM: Utils import updated consistently.The
.jsextension is correctly added for the Bytes type import.test/tapscript.test.ts (1)
9-9: LGTM: Test import path updated for ESM.The script module import correctly uses the
.jsextension to match the source code updates.src/script/default.ts (1)
1-1: LGTM! ESM import path update is correct.The explicit
.jsextension aligns with ES module resolution standards and is consistent with the project-wide migration pattern.test/e2e/indexer.test.ts (1)
17-17: LGTM! Import path correctly updated for ESM compliance.The
.jsextension addition is consistent with the project-wide ESM migration.server.Dockerfile (1)
8-8: Temporary commit pin acknowledged.As noted in the PR description, this pins the Docker build to arkd commit
74f4793while waiting for PR #153 to merge. This is an acceptable temporary measure for stabilizing CI tests.Consider creating a tracking issue or TODO comment to ensure this is reverted to
masteronce PR #153 is merged, preventing this temporary pin from becoming permanent.examples/vhtlc.js (1)
24-24: LGTM! Import path update is correct.The explicit
.jsextension is required for proper ES module resolution and aligns with the project-wide migration.package.json (1)
61-61: Confirm @scure/btc-signer@2.0.1 is valid and latest. NPM shows version 2.0.1 is published and is the current latest release.src/wallet/index.ts (1)
1-1: LGTM: Import path updated for ESM compatibility.The explicit
.jsextension aligns with ES module resolution requirements.src/musig2/keys.ts (1)
1-1: LGTM: Import path updated for ESM compatibility.The explicit
.jsextension aligns with ES module resolution requirements.src/musig2/nonces.ts (1)
1-1: LGTM: Import path updated for ESM compatibility.The explicit
.jsextension aligns with ES module resolution requirements.src/utils/anchor.ts (1)
3-3: LGTM: Import path updated for ESM compatibility.The explicit
.jsextension aligns with ES module resolution requirements.test/verifySignatures.test.ts (3)
4-4: LGTM: Import path updated for ESM compatibility.The explicit
.jsextension aligns with ES module resolution requirements.
8-8: LGTM: Import source updated.The import remains functionally identical; only the source location changed.
1-247: @scure/btc-signer v2.0.1 confirmed publishednpm shows version 2.0.1 is available and set as the latest release.
src/forfeit.ts (1)
2-2: LGTM! ESM import path updated correctly.The addition of the
.jsextension aligns with ES module resolution standards.test/e2e/vhtlc.test.ts (1)
5-5: LGTM! Test import updated for ESM compatibility.The
.jsextension ensures proper module resolution.src/script/tapscript.ts (1)
2-3: LGTM! Import consolidation and ESM path updates.The consolidated import of
p2tr_mswith other types from the main package improves organization, and the.jsextension on the utils import ensures ESM compatibility.src/arknote/index.ts (1)
2-4: LGTM! ESM path update and import ordering.The
.jsextension ensures proper module resolution, and the reordered imports improve consistency.src/bip322/index.ts (2)
2-5: LGTM! Import reorganization with ESM paths.The imports are correctly updated to use
.jsextensions for ESM compatibility. All imported symbols (schnorr,Bytes,base64,TransactionInput,TransactionOutput) are used within the file.
1-5: Dependency @scure/btc-signer@2.0.1 verified
Version 2.0.1 is published on npm; no further action required.src/wallet/wallet.ts (3)
3-12: ESM subpath imports look correct; verify resolver settings.Using @scure/btc-signer/*.js requires NodeNext/Bundler moduleResolution and ESM-aware tooling. Please confirm tsconfig and bundler resolve these paths in CI.
1058-1060: Switch to connectorLeaf.id is fine; confirm BE txid semantics.scure uses txid (big‑endian) consistently; id should match explorer-format txid expected by downstream. Please double-check any consumers comparing/reversing txids.
For reference, scure docs state “We have txid (BE) instead of hash (LE).” (github.com)
1-12: Version availability heads‑up (npm vs JSR).As of Oct 1, 2025: npm lists @scure/btc-signer latest as 2.0.0, while JSR/jsDelivr show 2.0.1. If your install pulls from npm, a 2.0.1 pin will fail. Align package source or version. (npmjs.com)
src/utils/arkTransaction.ts (2)
1-7: Imports align with scure 2.x module layout.Looks good; no issues spotted.
153-154: Using checkpointTx.id is preferable; confirm id format at call sites.id should already be BE txid hex. Verify any code that previously reversed bytes no longer does so. (github.com)
src/script/base.ts (2)
2-12: Import surface updates LGTM; confirm TAP_LEAF_VERSION source.payment.js export for TAP_LEAF_VERSION/tapLeafHash varies across versions; ensure it’s present in your pinned version to avoid runtime import errors.
89-94: Public API type changed: onchainAddress(network: typeof NETWORK).
Detected call at src/wallet/wallet.ts:197; confirm that the passedthis.networkis the scureNETWORK(or a fully compatible alias) and not a custom project-specific shape.src/wallet/onchain.ts (1)
1-2: Validate @scure/btc-signer@2.0.1 availability andpayment.jsexport
- Package.json pins
@scure/btc-signer@2.0.1but npm registry still shows 2.0.0 as the latest—ensure 2.0.1 is published and resolvable or revert to 2.0.0.- Verify that
node_modules/@scure/btc-signer/payment.jsexists and correctly exportsP2TRfor your ESM imports.
1 participant
Update to btc-signer 2.0.1
This PR freezes the CI tests to arkd commit : 74f4793, waiting for #153 to be merged to revert to master.
@tiero @bordalix @Kukks please review
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Potentially Breaking