adds mergeCheckpoints and mergeTxs utils#217
Conversation
|
Warning Rate limit exceeded@bordalix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdded a new utility function Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ArkUtils as arkTransaction
Note right of ArkUtils #D6EFFF: combineTapscriptSigs
Caller->>ArkUtils: combineTapscriptSigs(signedTx, originalTx)
ArkUtils->>ArkUtils: for each input i
ArkUtils->>ArkUtils: if originalTx.inputs[i].tapScriptSig missing -> throw
ArkUtils->>ArkUtils: originalTx.inputs[i].tapScriptSig += signedTx.inputs[i].tapScriptSig
ArkUtils-->>Caller: return modified originalTx
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(2 hunks)src/utils/arkTransaction.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/arkTransaction.ts (1)
src/utils/transaction.ts (1)
Transaction(9-27)
⏰ 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)
src/index.ts (1)
91-92: LGTM! Public API exports are correct.The new utility functions are properly imported and exported, making them available as part of the public API. However, ensure the critical issues identified in
src/utils/arkTransaction.tsare addressed before merging.Also applies to: 191-192
src/utils/arkTransaction.ts
Outdated
| for (let i = 0; i < originalCheckpointTxs.length; i++) { | ||
| const myCheckpointTx = originalCheckpointTxs[i]; | ||
| const signedCheckpointTx = signedCheckpointTxs.find( | ||
| (_) => _.id === myCheckpointTx.id | ||
| ); | ||
| if (!signedCheckpointTx) { | ||
| throw new Error("Signed checkpoint not found"); | ||
| } | ||
| // for every input, concatenate its signatures with the signature from the server | ||
| for (let j = 0; j < myCheckpointTx.inputsLength; j++) { | ||
| const input = myCheckpointTx.getInput(j); | ||
| const inputFromServer = signedCheckpointTx.getInput(j); | ||
| if (!inputFromServer.tapScriptSig) | ||
| throw new Error("No tapScriptSig"); | ||
| myCheckpointTx.updateInput(i, { | ||
| tapScriptSig: input.tapScriptSig?.concat( | ||
| inputFromServer.tapScriptSig | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Wrong index used in updateInput call.
Line 356 uses the outer loop variable i (checkpoint index) instead of the inner loop variable j (input index) when calling updateInput. This will update the wrong input within the checkpoint transaction.
Apply this diff to fix the index bug:
- myCheckpointTx.updateInput(i, {
+ myCheckpointTx.updateInput(j, {
tapScriptSig: input.tapScriptSig?.concat(
inputFromServer.tapScriptSig
),
});Additionally, the optional chaining on line 357 (input.tapScriptSig?.concat(...)) can silently result in undefined if input.tapScriptSig is missing. Consider adding an explicit check:
for (let j = 0; j < myCheckpointTx.inputsLength; j++) {
const input = myCheckpointTx.getInput(j);
const inputFromServer = signedCheckpointTx.getInput(j);
+ if (!input.tapScriptSig)
+ throw new Error(`Input ${j} in checkpoint ${i} missing tapScriptSig`);
if (!inputFromServer.tapScriptSig)
- throw new Error("No tapScriptSig");
+ throw new Error(`Input ${j} in signed checkpoint ${i} missing tapScriptSig`);
- myCheckpointTx.updateInput(j, {
- tapScriptSig: input.tapScriptSig?.concat(
+ myCheckpointTx.updateInput(j, {
+ tapScriptSig: input.tapScriptSig.concat(
inputFromServer.tapScriptSig
),
});📝 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.
| for (let i = 0; i < originalCheckpointTxs.length; i++) { | |
| const myCheckpointTx = originalCheckpointTxs[i]; | |
| const signedCheckpointTx = signedCheckpointTxs.find( | |
| (_) => _.id === myCheckpointTx.id | |
| ); | |
| if (!signedCheckpointTx) { | |
| throw new Error("Signed checkpoint not found"); | |
| } | |
| // for every input, concatenate its signatures with the signature from the server | |
| for (let j = 0; j < myCheckpointTx.inputsLength; j++) { | |
| const input = myCheckpointTx.getInput(j); | |
| const inputFromServer = signedCheckpointTx.getInput(j); | |
| if (!inputFromServer.tapScriptSig) | |
| throw new Error("No tapScriptSig"); | |
| myCheckpointTx.updateInput(i, { | |
| tapScriptSig: input.tapScriptSig?.concat( | |
| inputFromServer.tapScriptSig | |
| ), | |
| }); | |
| } | |
| for (let i = 0; i < originalCheckpointTxs.length; i++) { | |
| const myCheckpointTx = originalCheckpointTxs[i]; | |
| const signedCheckpointTx = signedCheckpointTxs.find( | |
| (_) => _.id === myCheckpointTx.id | |
| ); | |
| if (!signedCheckpointTx) { | |
| throw new Error("Signed checkpoint not found"); | |
| } | |
| // for every input, concatenate its signatures with the signature from the server | |
| for (let j = 0; j < myCheckpointTx.inputsLength; j++) { | |
| const input = myCheckpointTx.getInput(j); | |
| const inputFromServer = signedCheckpointTx.getInput(j); | |
| if (!input.tapScriptSig) | |
| throw new Error(`Input ${j} in checkpoint ${i} missing tapScriptSig`); | |
| if (!inputFromServer.tapScriptSig) | |
| throw new Error(`Input ${j} in signed checkpoint ${i} missing tapScriptSig`); | |
| myCheckpointTx.updateInput(j, { | |
| tapScriptSig: input.tapScriptSig.concat( | |
| inputFromServer.tapScriptSig | |
| ), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/utils/arkTransaction.ts around lines 342 to 361, the inner-loop updates
use the outer loop index `i` instead of the input index `j` which writes the
wrong input; change the call to updateInput to use `j` as the input index, and
guard against a missing local tapScriptSig by explicitly checking that
input.tapScriptSig exists (or treating it as an empty array) before
concatenation — if missing, either throw a clear error or initialize to an empty
signature array, then concatenate with inputFromServer.tapScriptSig and pass the
result to updateInput.
src/utils/arkTransaction.ts
Outdated
| export function mergeTxs(signedTx: Transaction, originalTx: Transaction) { | ||
| for (let i = 0; i < signedTx.inputsLength; i++) { | ||
| const input = originalTx.getInput(i); | ||
| const inputFromServer = signedTx.getInput(i); | ||
| if (!input.tapScriptSig) throw new Error("No tapScriptSig"); | ||
| originalTx.updateInput(i, { | ||
| tapScriptSig: input.tapScriptSig?.concat( | ||
| inputFromServer.tapScriptSig! | ||
| ), | ||
| }); | ||
| } | ||
| return originalTx; | ||
| } |
There was a problem hiding this comment.
Add validation for input count and missing tapScriptSig from server.
The function has several issues:
- No validation that
originalTxandsignedTxhave the same number of inputs. If they differ, this could cause out-of-bounds access or miss inputs. - Line 378 uses non-null assertion on
inputFromServer.tapScriptSigwithout checking if it exists. - Line 377 uses optional chaining after already validating
input.tapScriptSigexists on line 375, which is redundant.
Apply this diff to add proper validation:
export function mergeTxs(signedTx: Transaction, originalTx: Transaction) {
+ if (signedTx.inputsLength !== originalTx.inputsLength) {
+ throw new Error(
+ `Input count mismatch: signed tx has ${signedTx.inputsLength} inputs, original tx has ${originalTx.inputsLength} inputs`
+ );
+ }
for (let i = 0; i < signedTx.inputsLength; i++) {
const input = originalTx.getInput(i);
const inputFromServer = signedTx.getInput(i);
- if (!input.tapScriptSig) throw new Error("No tapScriptSig");
+ if (!input.tapScriptSig)
+ throw new Error(`Input ${i} in original tx missing tapScriptSig`);
+ if (!inputFromServer.tapScriptSig)
+ throw new Error(`Input ${i} in signed tx missing tapScriptSig`);
originalTx.updateInput(i, {
- tapScriptSig: input.tapScriptSig?.concat(
- inputFromServer.tapScriptSig!
+ tapScriptSig: input.tapScriptSig.concat(
+ inputFromServer.tapScriptSig
),
});
}📝 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.
| export function mergeTxs(signedTx: Transaction, originalTx: Transaction) { | |
| for (let i = 0; i < signedTx.inputsLength; i++) { | |
| const input = originalTx.getInput(i); | |
| const inputFromServer = signedTx.getInput(i); | |
| if (!input.tapScriptSig) throw new Error("No tapScriptSig"); | |
| originalTx.updateInput(i, { | |
| tapScriptSig: input.tapScriptSig?.concat( | |
| inputFromServer.tapScriptSig! | |
| ), | |
| }); | |
| } | |
| return originalTx; | |
| } | |
| export function mergeTxs(signedTx: Transaction, originalTx: Transaction) { | |
| if (signedTx.inputsLength !== originalTx.inputsLength) { | |
| throw new Error( | |
| `Input count mismatch: signed tx has ${signedTx.inputsLength} inputs, original tx has ${originalTx.inputsLength} inputs` | |
| ); | |
| } | |
| for (let i = 0; i < signedTx.inputsLength; i++) { | |
| const input = originalTx.getInput(i); | |
| const inputFromServer = signedTx.getInput(i); | |
| if (!input.tapScriptSig) | |
| throw new Error(`Input ${i} in original tx missing tapScriptSig`); | |
| if (!inputFromServer.tapScriptSig) | |
| throw new Error(`Input ${i} in signed tx missing tapScriptSig`); | |
| originalTx.updateInput(i, { | |
| tapScriptSig: input.tapScriptSig.concat( | |
| inputFromServer.tapScriptSig | |
| ), | |
| }); | |
| } | |
| return originalTx; | |
| } |
🤖 Prompt for AI Agents
In src/utils/arkTransaction.ts around lines 371 to 383, validate that
signedTx.inputsLength === originalTx.inputsLength before the loop and throw a
descriptive error if they differ; inside the loop, remove the redundant optional
chaining on input.tapScriptSig (use the already-checked value) and explicitly
check that inputFromServer.tapScriptSig is present before concatenating,
throwing a clear error if the server is missing tapScriptSig for that index;
ensure error messages include the input index for easier debugging.
src/utils/arkTransaction.ts
Outdated
| * @param signedTx signed transaction | ||
| * @param originalTx original transaction | ||
| */ | ||
| export function mergeTxs(signedTx: Transaction, originalTx: Transaction) { |
There was a problem hiding this comment.
| export function mergeTxs(signedTx: Transaction, originalTx: Transaction) { | |
| export function combineTapscriptSigs(signedTx: Transaction, originalTx: Transaction) { |
"combine" is the term used in PSBT BIP so maybe better to reuse it ?
also it doesn't merge all fields but only tapscript sigs ?
src/utils/arkTransaction.ts
Outdated
| * @param signedCheckpointTxs signed checkpoint transactions | ||
| * @param originalCheckpointTxs original checkpoint transactions | ||
| */ | ||
| export function mergeCheckpoints( |
There was a problem hiding this comment.
| export function mergeCheckpoints( | |
| export function mergeCheckpoints( |
do we really need this function ? in the end it's applying the mergeTx one on an array
2 participants
@louisinger please review
Summary by CodeRabbit