offchain/BuildTxs: remove checkpoint tapscript field #691
Conversation
WalkthroughRemoved the custom CheckpointTapscript and its per-input override. VtxoInput now carries only Outpoint, Tapscript, RevealedTapscripts, and Amount; checkpoint construction uses the main Tapscript.RevealedScript. SubmitOffchainTx and E2E tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Service as SubmitOffchainTx
participant Offchain as Tx Builder
participant Signer as Sign/Encode
note right of Offchain: VtxoInput contains Tapscript + RevealedTapscripts only
User->>Service: SubmitOffchainTx(request)
Service->>Offchain: Build VtxoInputs(Outpoint, Tapscript, RevealedTapscripts, Amount)
Offchain->>Offchain: buildCheckpointTx using Tapscript.RevealedScript
Offchain->>Signer: return arkPtx + checkpoint PSBTs
Signer->>Signer: AddConditionWitness(arkPtx / checkpoints)
Signer-->>Service: encoded/signed transactions
Service-->>User: Submission result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
pkg/ark-lib/offchain/tx.go (1)
25-27: Docs: Clarify what tree RevealedTapscripts representsNit: This comment is good; consider clarifying that these are the tap tree encodings for the input being spent by the Ark tx (i.e., the checkpoint output’s tree), not the original vtxo’s tree. This helps prevent future mix-ups in callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/core/application/service.go(1 hunks)pkg/ark-lib/offchain/tx.go(3 hunks)test/e2e/e2e_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/ark-lib/offchain/tx.go (1)
pkg/ark-lib/script/closure.go (1)
DecodeClosure(31-69)
🔇 Additional comments (3)
test/e2e/e2e_test.go (1)
1659-1661: LGTM: struct literal updated to the new VtxoInput shapeThe test now constructs offchain.VtxoInput without CheckpointTapscript and supplies Amount, Tapscript, RevealedTapscripts. This aligns with the library changes. No issues spotted.
pkg/ark-lib/offchain/tx.go (2)
168-176: LGTM: Collaborative closure now derived from the main tapscriptDecoding the collaborative closure from vtxo.Tapscript.RevealedScript removes the need for a separate CheckpointTapscript and ensures the checkpoint’s collaborative path matches the Ark path. This meets the PR objective of equality between checkpoint and ark tapscripts.
197-205: LGTM: Checkpoint leaf constructed from the same revealed scriptUsing the same revealed script to derive the tapleaf hash for the collaborative proof keeps the control block and script consistent with the checkpoint tree. Matches the new design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
1688-1695: Rename variable for clarity: it’s the ark PSBT, not a “virtual” tx
encodedVirtualTxhere is the encoded ark PSBT. Rename for readability.-encodedVirtualTx, err := arkPtx.B64Encode() +encodedArkTx, err := arkPtx.B64Encode() require.NoError(t, err) signedTx, err := bobWallet.SignTransaction( ctx, explorer, - encodedVirtualTx, + encodedArkTx, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/e2e/e2e_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.013Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
📚 Learning: 2025-08-19T10:58:41.013Z
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.013Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
test/e2e/e2e_test.go
🧬 Code Graph Analysis (1)
test/e2e/e2e_test.go (1)
pkg/ark-lib/offchain/tx.go (2)
BuildTxs(31-64)VtxoInput(20-28)
🔇 Additional comments (3)
test/e2e/e2e_test.go (3)
1652-1662: BuildTxs call correctly reflects removal of CheckpointTapscript; VtxoInput fields alignGood update: you pass Outpoint, Amount, Tapscript, and RevealedTapscripts only, consistent with the new VtxoInput definition. This enforces the same tapscript for both ark tx and checkpoint, aligning with the PR objective and our prior learning about using the checkpoint PSBT input’s tapscript for VtxoInput.Tapscript.
1713-1716: Condition witness on checkpoint PSBTs: good placementAdding the preimage to the checkpoint PSBT inputs after the server returns its signatures is acceptable for tapscript (signing digest doesn’t depend on non-sig witness elements). This mirrors the ark PSBT path and enforces parity between the two spends as intended.
1684-1687: Minor nit: remove redundant slice on preimage
- Adding the preimage witness before signing is correct.
- Since
preimageis already a[]byte, usingpreimage[:]is redundant. You can simplify towire.TxWitness{preimage}.Suggested diff in test/e2e/e2e_test.go:
--- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -1684,7 +1684,7 @@ func TestE2E(t *testing.T) { // add condition witness to the ark ptx - err = txutils.AddConditionWitness(0, arkPtx, wire.TxWitness{preimage[:]}) + err = txutils.AddConditionWitness(0, arkPtx, wire.TxWitness{preimage}) require.NoError(t, err) @@ -1711,7 +1711,7 @@ func TestE2E(t *testing.T) { // add condition witness to the checkpoint ptx - err = txutils.AddConditionWitness(0, ptx, wire.TxWitness{preimage[:]}) + err = txutils.AddConditionWitness(0, ptx, wire.TxWitness{preimage}) require.NoError(t, err) encoded, err := ptx.B64Encode()
it closes #690
@Kukks please review
Summary by CodeRabbit
Refactor
Tests