Rework Ark PSBT fields #726
Conversation
WalkthroughReplaces legacy PSBT Unknowns helpers with a typed Ark PSBT field API (SetArkPsbtField / GetArkPsbtFields). Updates many call sites to read/write Ark-specific PSBT input fields (taptree, cosigners, expiry, condition witness), removes legacy psbt.go, updates builders, musig/tree logic, tests, and module pins. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Builder as Tx Builder
participant PSBT as PSBT Packet
participant Fields as Ark PSBT Fields
participant Consumer as Consumer (core/wallet/script)
rect rgb(240,245,255)
note over Builder,PSBT: Build phase (new)
Builder->>PSBT: create inputs
Builder->>Fields: SetArkPsbtField(VtxoTaprootTreeField, inputIndex, TapTree)
Builder->>Fields: SetArkPsbtField(VtxoTreeExpiryField, inputIndex, expiry)
Builder->>Fields: SetArkPsbtField(CosignerPublicKeyField, inputIndex, {index, pubkey})*
end
rect rgb(245,255,245)
note over Consumer,PSBT: Consume / validate / sign
Consumer->>Fields: GetArkPsbtFields(VtxoTaprootTreeField, inputIndex)
Consumer->>Fields: GetArkPsbtFields(CosignerPublicKeyField, inputIndex)
Consumer->>Fields: GetArkPsbtFields(VtxoTreeExpiryField, inputIndex)
alt missing required field
Consumer-->>Consumer: return error ("missing taptree/fields")
else present
Consumer->>Consumer: derive scripts, aggregate keys, verify/construct witness
end
end
sequenceDiagram
autonumber
participant Wallet as Wallet Service
participant Fields as Ark PSBT Fields
participant PSBT as PSBT Packet
Wallet->>Fields: GetArkPsbtFields(ConditionWitnessField, inputIndex)
alt witness exists
Wallet->>PSBT: psbt.WriteTxWitness(witness = fields[0])
Wallet-->>Wallet: finalize input
else no witness
Wallet-->>Wallet: proceed standard signing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/interface/grpc/permissions/permissions.go (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). (3)
🔇 Additional comments (1)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/ark-lib/note/note.go (1)
122-130: Use Ark PSBT field coders; avoid raw Unknowns and ensure key-type 255Manually building Unknowns bypasses the new field coders and misses PSBT key-type 0xFF encoding. Encode with txutils coders instead.
Apply this diff:
- var witnessBytes bytes.Buffer - if err := psbt.WriteTxWitness(&witnessBytes, wire.TxWitness{n.Preimage[:]}); err != nil { - return nil, nil, fmt.Errorf("failed to write witness: %w", err) - } + cwField, err := txutils.ConditionWitnessField.Encode(wire.TxWitness{n.Preimage[:]}) + if err != nil { + return nil, nil, fmt.Errorf("failed to encode condition witness: %w", err) + } @@ - taptree, err := txutils.TapTree(scripts).Encode() - if err != nil { - return nil, nil, fmt.Errorf("failed to encode taptree: %w", err) - } + ttField, err := txutils.VtxoTaprootTreeField.Encode(txutils.TapTree(scripts)) + if err != nil { + return nil, nil, fmt.Errorf("failed to encode taptree: %w", err) + }Also applies to: 132-135
internal/core/application/service.go (4)
613-619: Fix type mismatch: RevealedTapscripts field is []stringoffchain.VtxoInput.RevealedTapscripts expects []string. Cast the TapTree.
Apply this diff:
- RevealedTapscripts: taptree, + RevealedTapscripts: []string(taptree),
922-926: Cast TapTree to []string when building ports.Inputports.Input.Tapscripts is []string.
Apply this diff:
- input := ports.Input{ - Outpoint: vtxoOutpoint, - Tapscripts: tapscripts, - } + input := ports.Input{ + Outpoint: vtxoOutpoint, + Tapscripts: []string(tapscripts), + }
2548-2551: Fix ParseVtxoScript param type in validateBoardingInputCast TapTree to []string before parsing.
Apply this diff:
- vtxoScript, err := script.ParseVtxoScript(tapscripts) + vtxoScript, err := script.ParseVtxoScript([]string(tapscripts))
2603-2606: Fix ParseVtxoScript param type in validateVtxoInputCast TapTree to []string before parsing.
Apply this diff:
- vtxoScript, err := script.ParseVtxoScript(tapscripts) + vtxoScript, err := script.ParseVtxoScript([]string(tapscripts))
🧹 Nitpick comments (2)
pkg/ark-lib/txutils/psbt_fields.go (1)
116-121: TreeExpiry encoding: remove non‑minimal extra-byte ruleAdding an extra 0x00 when MSB is set breaks the “minimal LE bytes” claim and is unnecessary outside script number encoding.
- // if the most significant bit of the last byte is set, - // we need one more byte to avoid sign ambiguity - if sequenceLE[numBytes-1]&0x80 != 0 { - numBytes++ - }This preserves minimal LE by trimming trailing zeros only. Confirm cross‑lang parity with ts‑sdk before changing.
pkg/ark-lib/txutils/indexed_pubkey.go (1)
16-26: Consider adding index validation.The sorting and reconstruction logic is correct. However,
MakeCosignerPublicKeyListcurrently performs no validation of the indices. Consider adding checks for:
- Duplicate indices
- Non-contiguous indices (gaps in the sequence)
This would catch encoding errors early and make use of the error return.
If validation is not needed at this layer (e.g., it's guaranteed by encoding), the implementation is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpkg/ark-cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.mod(1 hunks)internal/core/application/service.go(4 hunks)internal/core/application/utils.go(1 hunks)internal/infrastructure/tx-builder/covenantless/builder.go(6 hunks)pkg/ark-cli/go.mod(1 hunks)pkg/ark-lib/intent/proof.go(1 hunks)pkg/ark-lib/note/note.go(1 hunks)pkg/ark-lib/offchain/tx.go(1 hunks)pkg/ark-lib/script/finalizer.go(1 hunks)pkg/ark-lib/tree/builder.go(1 hunks)pkg/ark-lib/tree/musig2.go(6 hunks)pkg/ark-lib/tree/validation.go(1 hunks)pkg/ark-lib/txutils/indexed_pubkey.go(1 hunks)pkg/ark-lib/txutils/psbt.go(0 hunks)pkg/ark-lib/txutils/psbt_fields.go(1 hunks)pkg/ark-lib/txutils/psbt_fields_test.go(4 hunks)pkg/ark-lib/txutils/utils.go(1 hunks)pkg/arkd-wallet/core/application/wallet/service.go(1 hunks)test/e2e/e2e_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/ark-lib/txutils/psbt.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
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:
pkg/ark-lib/offchain/tx.gopkg/ark-lib/txutils/psbt_fields_test.gointernal/core/application/service.go
🧬 Code graph analysis (14)
pkg/ark-lib/note/note.go (1)
pkg/ark-lib/txutils/psbt_fields.go (2)
ArkFieldConditionWitness(23-23)ArkFieldTaprootTree(17-17)
pkg/ark-lib/tree/validation.go (2)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)CosignerPublicKeyField(29-29)pkg/ark-lib/txutils/indexed_pubkey.go (1)
MakeCosignerPublicKeyList(16-26)
pkg/ark-lib/tree/musig2.go (2)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)CosignerPublicKeyField(29-29)pkg/ark-lib/txutils/indexed_pubkey.go (1)
MakeCosignerPublicKeyList(16-26)
pkg/arkd-wallet/core/application/wallet/service.go (1)
pkg/ark-lib/txutils/psbt_fields.go (3)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)ArkFieldConditionWitness(23-23)
pkg/ark-lib/offchain/tx.go (1)
pkg/ark-lib/txutils/psbt_fields.go (2)
SetArkPsbtField(38-45)VtxoTaprootTreeField(27-27)
pkg/ark-lib/tree/builder.go (2)
pkg/ark-lib/txutils/psbt_fields.go (3)
SetArkPsbtField(38-45)CosignerPublicKeyField(29-29)VtxoTreeExpiryField(28-28)pkg/ark-lib/txutils/indexed_pubkey.go (1)
IndexedCosignerPublicKey(11-14)
pkg/ark-lib/intent/proof.go (1)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)
internal/infrastructure/tx-builder/covenantless/builder.go (4)
pkg/ark-lib/txutils/psbt_fields.go (4)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)VtxoTreeExpiryField(28-28)CosignerPublicKeyField(29-29)pkg/ark-lib/script/closure.go (1)
CSVMultisigClosure(290-293)pkg/ark-lib/txutils/indexed_pubkey.go (1)
MakeCosignerPublicKeyList(16-26)pkg/ark-lib/tree/musig2.go (1)
AggregateKeys(126-164)
internal/core/application/utils.go (1)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)CosignerPublicKeyField(29-29)
pkg/ark-lib/txutils/psbt_fields_test.go (2)
pkg/ark-lib/txutils/psbt_fields.go (6)
SetArkPsbtField(38-45)ConditionWitnessField(30-30)GetArkPsbtFields(48-63)VtxoTreeExpiryField(28-28)CosignerPublicKeyField(29-29)VtxoTaprootTreeField(27-27)pkg/ark-lib/txutils/indexed_pubkey.go (1)
IndexedCosignerPublicKey(11-14)
test/e2e/e2e_test.go (2)
pkg/ark-lib/txutils/psbt_fields.go (3)
SetArkPsbtField(38-45)ConditionWitnessField(30-30)VtxoTaprootTreeField(27-27)pkg/ark-lib/txutils/taptree.go (1)
TapTree(14-14)
pkg/ark-lib/script/finalizer.go (1)
pkg/ark-lib/txutils/psbt_fields.go (3)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)ArkFieldConditionWitness(23-23)
internal/core/application/service.go (2)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)VtxoTaprootTreeField(27-27)pkg/ark-lib/script/vtxo_script.go (1)
ParseVtxoScript(37-53)
pkg/ark-lib/txutils/psbt_fields.go (3)
pkg/ark-lib/txutils/taptree.go (2)
TapTree(14-14)DecodeTapTree(47-79)pkg/ark-lib/txutils/indexed_pubkey.go (1)
IndexedCosignerPublicKey(11-14)pkg/ark-lib/txutils/utils.go (1)
ReadTxWitness(10-29)
⏰ 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). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (21)
pkg/ark-lib/script/finalizer.go (1)
25-39: Condition witness retrieval/serialization looks goodUsing GetArkPsbtFields + WriteTxWitness and passing under ArkFieldConditionWitness aligns with the new API.
Please confirm closure.Witness expects the "condition" arg key (string(ArkFieldConditionWitness)) for these scripts.
pkg/ark-lib/txutils/utils.go (1)
10-29: Witness deserialization mirrors psbt.WriteTxWitness; LGTMReads varint count and varbytes with MaxScriptSize; symmetric to encoding.
pkg/ark-cli/go.mod (1)
10-11: Sync versions with root and verify btcec replace override
- These bumps match root. Ensure the CLI compiles against the updated ark-lib/go-sdk.
- replace pins btcec/v2 v2.3.3 while an indirect require lists v2.3.4; confirm the override is intentional and consistent with root. Based on learnings
Use the script shared in the root go.mod comment to audit all go.mod files.
internal/core/application/utils.go (1)
319-327: LGTM: switch to Ark PSBT field API for cosignersCorrect use of GetArkPsbtFields with CosignerPublicKeyField and topic derivation.
test/e2e/e2e_test.go (2)
1959-1963: LGTM: set TapTree via Ark PSBT fieldUsing SetArkPsbtField with VtxoTaprootTreeField and a txutils.TapTree value is correct.
1750-1751: LGTM: set condition witness via Ark PSBT fieldSetting ConditionWitnessField with wire.TxWitness is correct and aligns with the new API.
pkg/arkd-wallet/core/application/wallet/service.go (1)
560-572: LGTM: retrieve and write condition witness via Ark PSBT fieldsGetArkPsbtFields for ConditionWitnessField and passing it to args using ArkFieldConditionWitness key is correct.
pkg/ark-lib/tree/musig2.go (5)
209-221: LGTM! Clean migration to Ark PSBT field API.The cosigner key retrieval logic correctly migrates from direct extraction to field-based retrieval. Error handling is appropriate, and the indexed key reconstruction via
MakeCosignerPublicKeyListpreserves ordering.
505-517: LGTM! Consistent field-based retrieval.The changes mirror the pattern established in
ValidateTreeSigs. Cosigner data is correctly retrieved via Ark PSBT fields with appropriate error handling.
651-662: LGTM! Field-based lookup preserves matching logic.The function correctly adapts to Ark PSBT fields. The signer lookup iterates over
cosignerFieldsand checksfield.PublicKey, maintaining the original matching semantics.
680-710: LGTM! Nonce aggregation correctly adapted.The function properly retrieves cosigner fields and accesses
field.PublicKeyfor serialization and error reporting. The logic flow for nonce aggregation is preserved.Note: The variable name
keysat line 680 now holdsIndexedCosignerPublicKeyfields rather than raw keys, but the usage is consistent throughout the function.
818-830: LGTM! Signature combination correctly migrated.The cosigner retrieval follows the established pattern with appropriate error handling. The reconstructed keys list is correctly used for signature combination.
pkg/ark-lib/tree/validation.go (1)
103-111: LGTM! Validation correctly migrated to field-based retrieval.The cosigner extraction for validation follows the same clean pattern as the musig2 changes. Error handling is appropriate, and the subsequent deduplication logic is preserved.
pkg/ark-lib/tree/builder.go (1)
232-243: LGTM! PSBT construction migrated to field-based API.The function correctly uses
SetArkPsbtFieldwith indexed cosigner keys to preserve ordering. The expiry field is also properly encoded using the new API. Error handling is appropriate for both operations.pkg/ark-lib/txutils/psbt_fields_test.go (4)
34-46: LGTM! Condition witness test correctly validates API.The test properly exercises
SetArkPsbtFieldandGetArkPsbtFieldsfor the witness field, verifying round-trip encoding and content integrity.
66-76: LGTM! Expiry field test validates structure correctly.The test properly verifies the
RelativeLocktimefield encoding and decoding, accessing the struct fields (Type,Value) throughfields[0].
92-113: LGTM! Cosigner batch encoding test is thorough.The test validates encoding of 40 indexed cosigner keys, ensuring both the indexing and public key data are correctly round-tripped. The verification via
fields[i].PublicKeyconfirms proper field access.
141-153: LGTM! Tapscript array test covers multiple cases.The test properly validates tapscript array encoding with empty, single, and multiple script cases. The verification via
fields[0][i]correctly accesses individual scripts within the array field.internal/infrastructure/tx-builder/covenantless/builder.go (3)
114-121: LGTM! Witness retrieval correctly migrated.The condition witness is now retrieved via Ark PSBT fields with appropriate fallback to an empty witness if the field is absent. The logic flow for condition evaluation is preserved.
1197-1270: LGTM! Sweep leaf extraction comprehensively migrated.The function signature and implementation are correctly updated to use Ark PSBT fields:
- Bounds checking added for
inputIndex- Expiry and cosigner data retrieved via
GetArkPsbtFields- Cosigner list constructed with
MakeCosignerPublicKeyList- Return values expanded to include
internalKeyandvtxoTreeExpiryError handling is appropriate at each step. The signature change requires callers to pass
ptxandinputIndexinstead of the input directly (verified at line 695).
695-695: LGTM! Call site correctly updated.The call to
extractSweepLeafis properly updated to passvtxoTree.Rootand0instead of the input, matching the new signature. The additional return values (internalKey,vtxoTreeExpiry) are captured and used in the subsequent sweep input construction.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/core/application/service.go (2)
499-506: Fix type mismatch: Pass []string to ParseVtxoScripttaptreeFields[0] is txutils.TapTree (named type). script.ParseVtxoScript expects []string. Convert explicitly.
Apply this diff:
- taptree := taptreeFields[0] - - vtxoScript, err := script.ParseVtxoScript(taptree) + taptree := taptreeFields[0] + + vtxoScript, err := script.ParseVtxoScript([]string(taptree))Additionally, ensure other ParseVtxoScript call sites convert txutils.TapTree to []string (see suggestions below).
881-888: Declare tapscripts as TapTree to avoid assignment mismatchtaptreeFields[0] is txutils.TapTree; current []string declaration risks a type error.
Apply this diff:
- tapscripts := make([]string, 0) - if len(taptreeFields) > 0 { - tapscripts = taptreeFields[0] - } + var tapscripts txutils.TapTree + if len(taptreeFields) > 0 { + tapscripts = taptreeFields[0] + }Note: Where tapscripts is passed to functions expecting []string, convert with []string(tapscripts).
🧹 Nitpick comments (1)
internal/core/application/service.go (1)
452-452: Nit: error text doesn’t match conditionIf spendingTapscript is nil, “no matching taptree found” is misleading. Consider “missing taproot leaf script”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/core/application/service.go(4 hunks)internal/infrastructure/tx-builder/covenantless/builder.go(6 hunks)pkg/arkd-wallet-btcwallet/core/wallet_service.go(1 hunks)test/e2e/e2e_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
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:
internal/core/application/service.go
🧬 Code graph analysis (4)
pkg/arkd-wallet-btcwallet/core/wallet_service.go (1)
pkg/ark-lib/txutils/psbt_fields.go (3)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)ArkFieldConditionWitness(23-23)
internal/core/application/service.go (2)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(48-63)VtxoTaprootTreeField(27-27)pkg/ark-lib/script/vtxo_script.go (1)
ParseVtxoScript(37-53)
internal/infrastructure/tx-builder/covenantless/builder.go (4)
pkg/ark-lib/txutils/psbt_fields.go (4)
GetArkPsbtFields(48-63)ConditionWitnessField(30-30)VtxoTreeExpiryField(28-28)CosignerPublicKeyField(29-29)pkg/ark-lib/script/closure.go (1)
CSVMultisigClosure(290-293)pkg/ark-lib/txutils/indexed_pubkey.go (1)
MakeCosignerPublicKeyList(16-26)pkg/ark-lib/tree/musig2.go (1)
AggregateKeys(126-164)
test/e2e/e2e_test.go (2)
pkg/ark-lib/txutils/psbt_fields.go (3)
SetArkPsbtField(38-45)ConditionWitnessField(30-30)VtxoTaprootTreeField(27-27)pkg/ark-lib/txutils/taptree.go (1)
TapTree(14-14)
⏰ 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). (2)
- GitHub Check: integration tests
- GitHub Check: unit tests
🔇 Additional comments (7)
pkg/arkd-wallet-btcwallet/core/wallet_service.go (1)
489-501: Switch to Ark PSBT condition witness looks correctDecoding wire.TxWitness via Ark PSBT fields and passing it under the "condition" arg matches the new field API. LGTM.
test/e2e/e2e_test.go (2)
1750-1755: Correctly sets ConditionWitness via Ark PSBT fieldProviding the preimage as wire.TxWitness matches the condition script evaluation flow. LGTM.
1964-1967: Stores TapTree via Ark PSBT field (correct type)Casting []string to txutils.TapTree and writing it to input 1 is consistent with the new API. LGTM.
internal/core/application/service.go (1)
486-494: Reading taptree from the checkpoint input is correctExtracting the Vtxo taproot tree from the checkpoint PSBT input aligns with the intended data flow.
Based on learnings
internal/infrastructure/tx-builder/covenantless/builder.go (3)
114-121: Correct Ark PSBT condition witness retrievalFetching wire.TxWitness from Ark fields and evaluating the condition is sound. Empty/default witness correctly fails the condition.
695-695: Updated extractSweepLeaf call is appropriateUsing input index 0 for the single-input root PSBT is correct. LGTM.
1197-1279: extractSweepLeaf refactor is solid; validates required fields and derives control block correctly
- Bounds check for inputIndex.
- Requires VtxoTreeExpiry and CosignerPublicKey fields.
- Aggregates cosigner keys with sweep tweak; uses PreTweakedKey as internal key. Correct.
| cosignerFields, err := txutils.GetArkPsbtFields(ptx, inputIndex, txutils.CosignerPublicKeyField) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| if len(cosignerFields) == 0 { | ||
| return nil, nil, nil, fmt.Errorf("no cosigner pubkeys found") | ||
| } | ||
|
|
||
| cosignerPubKeys, err := txutils.MakeCosignerPublicKeyList(cosignerFields) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } |
There was a problem hiding this comment.
shall we abstract this code into a single function in txutils.GetCosignersFromArkPsbt(pts, inputIndex)?
There was a problem hiding this comment.
IMO better to "split" so the sorting is explicitly used only when useful (sometimes we don't need the keys to be ordered). wdyt ?
There was a problem hiding this comment.
if one doesn't want the func that combines txutils.GetArkPsbtFields + txutils.MakeCosignerPublicKeyList, he would still be free to use these functions... i'm not saying to replace 2 funcs with 1, but to have 1 that calls both
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/infrastructure/tx-builder/covenantless/builder.go (1)
1224-1226: Validate single VtxoTreeExpiry field if multiple are not allowed.Similar to the ConditionWitness extraction, this code retrieves all
VtxoTreeExpiryFieldentries and uses the first one. If the Ark PSBT field specification prohibits multiple expiry fields per input, add a validation check to catch unexpected data.If only a single
VtxoTreeExpiryFieldis expected per input, consider:vtxoTreeExpiryFields, err := txutils.GetArkPsbtFields( ptx, inputIndex, txutils.VtxoTreeExpiryField, ) if err != nil { return nil, nil, nil, err } if len(vtxoTreeExpiryFields) == 0 { return nil, nil, nil, fmt.Errorf("no vtxo tree expiry found") } + if len(vtxoTreeExpiryFields) > 1 { + return nil, nil, nil, fmt.Errorf("multiple vtxo tree expiry fields found") + } vtxoTreeExpiry := vtxoTreeExpiryFields[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/core/application/service.go(6 hunks)internal/infrastructure/tx-builder/covenantless/builder.go(6 hunks)pkg/ark-lib/intent/proof.go(2 hunks)pkg/ark-lib/tree/musig2.go(7 hunks)pkg/ark-lib/tree/validation.go(1 hunks)pkg/ark-lib/txutils/indexed_pubkey.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/ark-lib/intent/proof.go
- internal/core/application/service.go
- pkg/ark-lib/tree/validation.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/ark-lib/tree/musig2.go (2)
pkg/ark-lib/txutils/indexed_pubkey.go (2)
ParseCosignerKeysFromArkPsbt(29-35)ParseCosignersToECPubKeys(17-27)pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(53-72)CosignerPublicKeyField(30-30)
internal/infrastructure/tx-builder/covenantless/builder.go (4)
pkg/ark-lib/txutils/psbt_fields.go (3)
GetArkPsbtFields(53-72)ConditionWitnessField(31-31)VtxoTreeExpiryField(29-29)pkg/ark-lib/script/closure.go (1)
CSVMultisigClosure(290-293)pkg/ark-lib/txutils/indexed_pubkey.go (1)
ParseCosignerKeysFromArkPsbt(29-35)pkg/ark-lib/tree/musig2.go (1)
AggregateKeys(126-164)
pkg/ark-lib/txutils/indexed_pubkey.go (1)
pkg/ark-lib/txutils/psbt_fields.go (2)
GetArkPsbtFields(53-72)CosignerPublicKeyField(30-30)
⏰ 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). (3)
- GitHub Check: integration tests
- GitHub Check: Build and Scan
- GitHub Check: unit tests
🔇 Additional comments (2)
internal/infrastructure/tx-builder/covenantless/builder.go (2)
114-121: Verify behavior when multiple ConditionWitness fields are present.The code retrieves all ConditionWitness fields via
GetArkPsbtFieldsbut only uses the first one (witnessFields[0]). If the Ark PSBT field specification allows multiple ConditionWitness fields per input, using only the first might silently ignore additional witnesses, potentially missing required validation logic.Confirm whether:
- The Ark PSBT field specification permits multiple ConditionWitness fields per input, or
- Only a single ConditionWitness field is expected (in which case, add a validation check).
If multiple fields are valid, clarify the selection logic (first vs. last vs. specific match). If only one is expected, consider adding:
witnessFields, err := txutils.GetArkPsbtFields(ptx, index, txutils.ConditionWitnessField) if err != nil { return false, txid, err } + if len(witnessFields) > 1 { + return false, txid, fmt.Errorf("multiple condition witnesses found for input %d", index) + } witness := make(wire.TxWitness, 0) if len(witnessFields) > 0 { witness = witnessFields[0] }
1245-1255: No action needed: cosigner keys are sorted by index before aggregation.
ParseCosignersToECPubKeysappliessort.Slice(fields, … fields[i].Index < fields[j].Index)to enforce a consistent key order for MuSig2.
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 (2)
Makefile(1 hunks)internal/interface/grpc/permissions/permissions.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/interface/grpc/permissions/permissions.go (1)
api-spec/protobuf/gen/ark/v1/admin_grpc.pb.go (1)
AdminService_ServiceDesc(504-567)
🪛 GitHub Actions: ci_unit
internal/interface/grpc/permissions/permissions.go
[error] 287-287: File is not properly formatted (golines)
🪛 GitHub Check: unit tests
internal/interface/grpc/permissions/permissions.go
[failure] 287-287:
File is not properly formatted (golines)
⏰ 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). (2)
- GitHub Check: Build and Scan
- GitHub Check: integration tests
🔇 Additional comments (2)
internal/interface/grpc/permissions/permissions.go (1)
275-279: LGTM! Method names correctly updated.The RPC method name updates for Conviction management endpoints are consistent with the gRPC service descriptor.
Makefile (1)
71-75: Critical:find -execdirhides failing tests;failedmay stay 0
|| failed=1here checksfind’s exit code, not the exit codes of thego testcommands it runs. If any pkg module tests fail,findstill exits 0 and the target will exit 0 incorrectly.Fix by iterating modules and capturing each
go teststatus:@@ - @failed=0; \ - go test -v -count=1 -race ./internal/... || failed=1; \ - find ./pkg -name go.mod -execdir go test -v ./... \; || failed=1; \ - $(MAKE) droppgtest && $(MAKE) redis-down; \ - exit $$failed + @failed=0; \ + go test -v -count=1 -race ./internal/... || failed=1; \ + modules=$$(find ./pkg -name go.mod -exec dirname {} \;); \ + for d in $$modules; do \ + (cd $$d && go test -v ./...) || failed=1; \ + done; \ + $(MAKE) droppgtest && $(MAKE) redis-down; \ + exit $$failedLikely an incorrect or invalid review comment.
This PR reworks the custom PSBT input fields and fix its encoding. Including a constant PSBT key type (
255).The abstraction is inspired by ts-sdk one.
It also properly adds the PSBT key type
255when serializing the field Key.@altafan @Kukks please review
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores