add the package to broadcast to UnrollStep#222
add the package to broadcast to UnrollStep#222louisinger wants to merge 3 commits intoarkade-os:masterfrom
Conversation
WalkthroughSession.next now computes a pkg tuple (parent, child tx hex) via bumper.bumpP2A(tx) and attaches it to the UnrollStep; doUnroll accepts that pkg and broadcasts using broadcastTransaction(...pkg). A new prepareUnrollTransaction builds/signs the spend tx; completeUnroll delegates to it. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Session
participant Bumper
participant Explorer
Note over Session,Bumper: Session.next constructs tx
Client->>Session: next()
Session->>Bumper: bumpP2A(tx)
Bumper-->>Session: pkg [parentHex, childHex]
Session-->>Client: Step{ type: UNROLL, pkg, do: doUnroll(Explorer,pkg) }
alt Manual broadcast flow
Client->>Explorer: broadcastTransaction(parentHex, childHex)
Explorer-->>Client: txids/result
else Automatic via step.do()
Client->>Session: invoke step.do()
Session->>Explorer: doUnroll -> broadcastTransaction(...pkg)
Explorer-->>Client: txids/result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/unroll.ts (1)
204-217: Iterator unconditionally broadcasts transactions.The async iterator always calls
await step.do()on line 213, which means it always broadcasts transactions. This prevents using the iterator for the backup/inspection use case described in the PR objectives (extract transaction hex without broadcasting).Consider making the
do()call conditional based on the broadcast flag:async *[Symbol.asyncIterator](): AsyncIterator<Step> { let lastStep: StepType | undefined; do { if (lastStep !== undefined) { // wait 1 second before trying the next step in order to give time to the // explorer to update the tx status await sleep(1_000); } const step = await this.next(); - await step.do(); + if (this.broadcast) { + await step.do(); + } yield step; lastStep = step.type; } while (lastStep !== StepType.DONE); }This allows users to iterate and collect transaction hex without broadcasting when
broadcast = false.
🧹 Nitpick comments (1)
src/wallet/unroll.ts (1)
68-86: Update documentation to demonstrate new pkg field usage.The JSDoc example doesn't demonstrate how to access the new
pkgfield, which is a key feature of this PR. Consider adding an example showing the backup/inspection use case:/** * @example * // Example 1: Standard flow with automatic broadcasting * const session = await Unroll.Session.create(vtxoOutpoint, bumper, explorer, indexer); * for await (const step of session) { * // ... existing example * } * * @example * // Example 2: Extract transaction hex without broadcasting (if broadcast=false supported) * const session = await Unroll.Session.create(vtxoOutpoint, bumper, explorer, indexer, false); * const step = await session.next(); * if (step.type === Unroll.StepType.UNROLL) { * console.log(`Parent tx hex: ${step.pkg[0]}`); * console.log(`Child tx hex: ${step.pkg[1]}`); * // Save hex for later broadcast or backup * // Optionally broadcast later: await step.do() * } */Note: This example assumes the broadcast parameter is implemented as specified in the PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wallet/unroll.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-07T17:02:12.316Z
Learnt from: louisinger
Repo: arkade-os/ts-sdk PR: 115
File: src/wallet/onchain.ts:180-187
Timestamp: 2025-07-07T17:02:12.316Z
Learning: In the OnchainWallet.bumpP2A method in src/wallet/onchain.ts, the return statement in the finally block is intentional. The method is designed to always return the transaction hex strings even if broadcasting fails, allowing callers to handle the transactions as needed.
Applied to files:
src/wallet/unroll.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 (2)
src/wallet/unroll.ts (2)
191-191: LGTM!The computation of the P2A package via
bumper.bumpP2A(tx)is correctly placed here, allowing transaction hex to be prepared regardless of whether broadcasting occurs.Based on learnings.
192-197: Remove this review comment—it's based on incorrect assumptions about PR objectives.The PR objective is to expose
pkg(parent and child transaction hex strings) in theUnrollSteptype so callers can access the transaction data. This has been accomplished. The review assumes the PR should implement conditional broadcasting via abroadcastflag, but:
- No
broadcastparameter exists in theUnrollclass constructor- The
doUnrollfunction is designed to always broadcast (intentional per learnings)- The async iterator always calls
step.do()(by design)- The git commit message confirms the PR goal: "add the pkg to broadcast to UnrollStep"
The suggested code change implements functionality that was not part of this PR's scope.
Likely an incorrect or invalid review comment.
|
Can you update the readme on how to use it to get the tx hex without broadcasting? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
⏰ 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 (1)
README.md (1)
212-212: Updated log message aligns with refactored behavior.The message change from "Broadcasting transaction ..." to "Transaction ... unrolled" appropriately reflects the step, though note that whether actual broadcast occurs now depends on the
broadcastflag.
|
I don't think this does what I want The new README says that if you want to control the broadcasting process manually, you should run the command Which sounds like it is still broadcasts the transaction "in the background" I don't want it to broadcast the transaction, I just want it to give me the transaction hex. I also want the wallet to continue working as it worked previously, without thinking I've "unilaterally exited." I didn't unilaterally exit, I just got the transaction hexes for doing so later, if I want to. |
* Prepare unroll transaction separately from broadcast * Revert pgnbxplorer port quotes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/wallet/unroll.ts (2)
251-255:prepareUnrollTransactionis exported outside theUnrollnamespace.Everything else in this module is scoped under
Unroll. This function being a top-level export breaks that convention. Consider either moving it inside the namespace or documenting the intentional divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/unroll.ts` around lines 251 - 255, The function prepareUnrollTransaction is exported at module top-level while the rest of the module is scoped under the Unroll namespace; to fix, move the prepareUnrollTransaction declaration and export inside the existing Unroll namespace (or, if intentional, add a clear comment/doc above prepareUnrollTransaction explaining why it must remain top-level), updating any references to Unroll.prepareUnrollTransaction where used; ensure the function signature (prepareUnrollTransaction(wallet: Wallet, vtxoTxIds: string[], outputAddress: string): Promise<Transaction>) and exports reflect the new placement.
291-298:VtxoScript.decode(vtxo.tapTree)is called twice per VTXO.Lines 291 and 308 both decode the same
tapTree. Extract once to a local variable.Proposed fix
+ const vtxoScript = VtxoScript.decode(vtxo.tapTree); + const spendingLeaf = vtxoScript.findLeaf( hex.encode(exit.script) ); // ... inputs.push({ // ... witnessUtxo: { amount: BigInt(vtxo.value), - script: VtxoScript.decode(vtxo.tapTree).pkScript, + script: vtxoScript.pkScript, }, });Also applies to: 306-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/unroll.ts` around lines 291 - 298, VtxoScript.decode(vtxo.tapTree) is being called twice for the same VTXO; extract the result to a local variable (e.g., const tapTreeScript = VtxoScript.decode(vtxo.tapTree)) and use tapTreeScript.findLeaf(hex.encode(exit.script)) when computing spendingLeaf and in the other occurrence around lines 306-309 so the decoding happens once per VTXO and both uses reference the same decoded instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/unroll.ts`:
- Around line 326-330: feeRate returned by wallet.onchainProvider.getFeeRate()
may be fractional and passing it directly into BigInt(...) will throw; before
calling txWeightEstimator.vsize().fee(BigInt(...)) convert the effective feeRate
to an integer (use Math.ceil like other code paths) so both the value from
getFeeRate() and fallback Wallet.MIN_FEE_RATE are rounded up to an integer prior
to BigInt conversion; update the logic around feeRate and the call to
txWeightEstimator.vsize().fee to use Math.ceil(feeRate) (then BigInt) to avoid
runtime TypeError.
---
Duplicate comments:
In `@src/wallet/unroll.ts`:
- Around line 346-352: The doUnroll function currently always broadcasts the
transaction; update it so callers can opt out and obtain the raw tx hex instead:
change doUnroll (and the Unroll.UnrollStep structure if needed) to accept a
broadcast flag (or detect a pkg.broadcast boolean) and, when broadcast is false,
return a function that resolves with undefined after preserving/returning the tx
hex (without calling onchainProvider.broadcastTransaction); also update the
Symbol.asyncIterator usage (the iterator that calls step.do()) to pass the
broadcast flag through from the session/iteration options so iterating consumers
can choose to only get tx hexs rather than broadcasting. Ensure identifiers to
change include doUnroll, Unroll.UnrollStep.pkg, and the Symbol.asyncIterator
invocation that calls step.do().
---
Nitpick comments:
In `@src/wallet/unroll.ts`:
- Around line 251-255: The function prepareUnrollTransaction is exported at
module top-level while the rest of the module is scoped under the Unroll
namespace; to fix, move the prepareUnrollTransaction declaration and export
inside the existing Unroll namespace (or, if intentional, add a clear
comment/doc above prepareUnrollTransaction explaining why it must remain
top-level), updating any references to Unroll.prepareUnrollTransaction where
used; ensure the function signature (prepareUnrollTransaction(wallet: Wallet,
vtxoTxIds: string[], outputAddress: string): Promise<Transaction>) and exports
reflect the new placement.
- Around line 291-298: VtxoScript.decode(vtxo.tapTree) is being called twice for
the same VTXO; extract the result to a local variable (e.g., const tapTreeScript
= VtxoScript.decode(vtxo.tapTree)) and use
tapTreeScript.findLeaf(hex.encode(exit.script)) when computing spendingLeaf and
in the other occurrence around lines 306-309 so the decoding happens once per
VTXO and both uses reference the same decoded instance.
| let feeRate = await wallet.onchainProvider.getFeeRate(); | ||
| if (!feeRate || feeRate < Wallet.MIN_FEE_RATE) { | ||
| feeRate = Wallet.MIN_FEE_RATE; | ||
| } | ||
| const feeAmount = txWeightEstimator.vsize().fee(BigInt(feeRate)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for getFeeRate definition and return type
rg -n -B2 -A10 'getFeeRate\(' --type tsRepository: arkade-os/ts-sdk
Length of output: 5641
🏁 Script executed:
#!/bin/bash
# Check BigInt usage with feeRate
rg -n 'BigInt.*feeRate|BigInt.*fee' --type tsRepository: arkade-os/ts-sdk
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check the type of feeRate in context
rg -n 'feeRate.*=' --type ts | head -20Repository: arkade-os/ts-sdk
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Look for Wallet.MIN_FEE_RATE definition and its type
rg -n 'MIN_FEE_RATE' --type ts -B2 -A2Repository: arkade-os/ts-sdk
Length of output: 1668
Convert fractional feeRate to integer before passing to BigInt().
Line 330 converts feeRate directly to BigInt, but getFeeRate() returns a number type that can be fractional. JavaScript's BigInt() constructor throws a TypeError on non-integer values (e.g., BigInt(1.5) fails). This will crash at runtime if feeRate is fractional.
Other code in the repository handles this correctly using Math.ceil() (see onchain.ts lines 113 and 180).
Proposed fix
- const feeAmount = txWeightEstimator.vsize().fee(BigInt(feeRate));
+ const feeAmount = txWeightEstimator.vsize().fee(BigInt(Math.ceil(feeRate)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let feeRate = await wallet.onchainProvider.getFeeRate(); | |
| if (!feeRate || feeRate < Wallet.MIN_FEE_RATE) { | |
| feeRate = Wallet.MIN_FEE_RATE; | |
| } | |
| const feeAmount = txWeightEstimator.vsize().fee(BigInt(feeRate)); | |
| let feeRate = await wallet.onchainProvider.getFeeRate(); | |
| if (!feeRate || feeRate < Wallet.MIN_FEE_RATE) { | |
| feeRate = Wallet.MIN_FEE_RATE; | |
| } | |
| const feeAmount = txWeightEstimator.vsize().fee(BigInt(Math.ceil(feeRate))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/unroll.ts` around lines 326 - 330, feeRate returned by
wallet.onchainProvider.getFeeRate() may be fractional and passing it directly
into BigInt(...) will throw; before calling
txWeightEstimator.vsize().fee(BigInt(...)) convert the effective feeRate to an
integer (use Math.ceil like other code paths) so both the value from
getFeeRate() and fallback Wallet.MIN_FEE_RATE are rounded up to an integer prior
to BigInt conversion; update the logic around feeRate and the call to
txWeightEstimator.vsize().fee to use Math.ceil(feeRate) (then BigInt) to avoid
runtime TypeError.
4 participants
add the 1C1P package to broadcast to
Unroll.UnrollStep@tiero @supertestnet please review
it closes #219
it closes #218
Summary by CodeRabbit