lightning: implement the htlc script #117
Conversation
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
WalkthroughAdds a new workspace crate ark-lightning with a VHTLC module, exposing script builders and Taproot construction. Introduces crate manifest, library entry, VHTLC implementation, example, a Node.js test-vector generator, and a fixture-driven Rust test suite with JSON fixtures. Updates root Cargo.toml to include the new member. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant V as VhtlcOptions
participant VS as VhtlcScript
participant B as bitcoin::script & taproot
Dev->>V: Construct with keys, preimage, locktimes, delays
Dev->>VS: new(VhtlcOptions)
activate VS
VS->>V: validate()
V-->>VS: Ok / Error
alt valid
VS->>B: build six scripts (claim/refund/... variants)
VS->>B: assemble Taproot tree (leaves + weights)
B-->>VS: TaprootSpendInfo (tweaked key)
VS-->>Dev: VhtlcScript
else invalid
VS-->>Dev: VhtlcError
end
deactivate VS
sequenceDiagram
autonumber
participant App as App/Test
participant VS as VhtlcScript
participant B as bitcoin::address
App->>VS: claim_script()/refund_script()/...
VS-->>App: ScriptBuf
App->>VS: script_pubkey()
VS-->>App: Option<ScriptBuf>
App->>VS: address(Network, serverXOnly)
VS->>B: derive address from tweaked key + server
B-->>VS: ArkAddress
VS-->>App: Option<ArkAddress>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. 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
🧹 Nitpick comments (17)
ark-lightning/Cargo.toml (1)
1-4: Confirm toolchain supports edition = "2024"If CI uses an older stable, the build will fail. Pin a compatible toolchain via rust-toolchain.toml to avoid surprises.
Example rust-toolchain.toml:
[toolchain] channel = "stable" # or a specific version known to support edition 2024 components = ["clippy", "rustfmt"]ark-lightning/scripts/generate_ts_vectors.js (2)
1-6: Make SDK path configurable + add shebang/strict modeImproves portability when running from different repos/dirs.
+#!/usr/bin/env node +'use strict'; -const { VhtlcScript } = require('../path/to/ts-sdk'); // Adjust path as needed +const sdkPath = process.argv[2] || '../path/to/ts-sdk'; // allow CLI override +const { VhtlcScript } = require(sdkPath); // e.g.: node generate_ts_vectors.js ../ts-sdk/dist
52-58: Return non-zero exit code on failureSo CI/automation can detect failures.
} catch (error) { console.error('Error generating TypeScript vectors:', error); console.log('\nPlease ensure:'); console.log('1. You are running this in the arkade-os/ts-sdk directory'); console.log('2. The path to VhtlcScript is correct'); console.log('3. All dependencies are installed (npm install)'); + process.exitCode = 1; }ark-lightning/tests/fixtures/vhtlc.json (2)
130-172: Invalid-fixture error messages likely don’t match Rust errorsYour fixtures expect messages like “preimage hash must be 20 bytes”, but
VhtlcErrorcurrently formats as “Invalid preimage hash length: expected 20 bytes, got N”. Align either the fixtures or the Rust errors, or assert substrings instead of exact string equality.
31-35: Gitleaks false positives on public test keysDetections on tweaked/internal pubkeys are harmless test vectors. Silence them via an allowlist for
ark-lightning/tests/fixtures/*.jsonto keep CI green.Example .gitleaks.toml snippet:
[allowlist] paths = [ "ark-lightning/tests/fixtures/.*\\.json" ]Also applies to: 74-77, 116-119
ark-lightning/src/vhtlc.rs (6)
151-161: Fix Clippy: remove needless borrows in push_sliceSatisfies CI and avoids extra references.
.push_opcode(OP_HASH160) - .push_slice(&self.options.preimage_hash) + .push_slice(self.options.preimage_hash) .push_opcode(OP_EQUAL) @@ .push_opcode(OP_HASH160) - .push_slice(&self.options.preimage_hash) + .push_slice(self.options.preimage_hash) .push_opcode(OP_EQUAL)Also applies to: 197-209
274-278: Replace unwrap() with expect() to appease Clippy and document invariantsThese unwraps are safe by construction; use expect with brief context.
- let b = lst.pop().unwrap(); - let a = lst.pop().unwrap(); + let b = lst.pop().expect("pop b: lst.len() >= 2 by loop condition"); + let a = lst.pop().expect("pop a: lst.len() >= 2 by loop condition"); @@ - Ok(lst.into_iter().next().unwrap()) + Ok(lst + .into_iter() + .next() + .expect("non-empty: checked at function start"))Also applies to: 296-298
381-383: Checkaddress()return type and remove redundant.into()If
ArkAddress::new(...)returnsOption<ArkAddress>,.into()is redundant. If it returnsArkAddress, wrap withSome(...). Please confirm and simplify accordingly.
74-116: Optional: tighten validation to match fixtures and BIP semantics
- Consider enforcing CLTV domain (height vs timestamp) for
refund_locktimeand rejecting ambiguous values.- If seconds-based CSV is allowed, ensure multiples of 512 and a minimum of 512 seconds when the “seconds” bit is set (BIP68).
240-358: Deterministic tie-break for equal weights in TapTreeAll weights are
1, so ordering is driven by insertion order. To reduce cross-impl drift when order changes, add a secondary sort key (e.g., script bytes or leaf-hash) when weights are equal.
23-35: Remove unused error variant or wire it up
InvalidPublicKeyLengthisn’t used. Either validate incoming keys and use it, or drop the variant.ark-lightning/src/lib.rs (1)
5-6: Re-export primary types from the crate root for ergonomic importsThis lets downstreams do
use ark_lightning::{VhtlcScript, VhtlcOptions, VhtlcError};.pub mod vhtlc; + +// Convenient re-exports +pub use vhtlc::{VhtlcError, VhtlcOptions, VhtlcScript};ark-lightning/examples/vhtlc_example.rs (1)
7-56: Prefer fallible example with?overunwrap/expectand also show address derivationImproves robustness and demonstrates a full flow including address generation.
-use bitcoin::Sequence; +use bitcoin::{Network, Sequence}; use bitcoin::XOnlyPublicKey; use std::str::FromStr; -fn main() { +fn main() -> Result<(), Box<dyn std::error::Error>> { // Create test keys for sender, receiver, and server - let sender = XOnlyPublicKey::from_str( + let sender = XOnlyPublicKey::from_str( "18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166", - ) - .unwrap(); + )?; - let receiver = XOnlyPublicKey::from_str( + let receiver = XOnlyPublicKey::from_str( "28845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166", - ) - .unwrap(); + )?; - let server = XOnlyPublicKey::from_str( + let server = XOnlyPublicKey::from_str( "38845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166", - ) - .unwrap(); + )?; // Create a preimage hash (in a real scenario, this would be the hash of a secret) let preimage_hash = [42u8; 20]; // Configure the VHTLC options let options = VhtlcOptions { sender, receiver, server, preimage_hash, refund_locktime: 100000, // Block height for CLTV - unilateral_claim_delay: Sequence::from_seconds_ceil(3600).unwrap(), // 1 hour - unilateral_refund_delay: Sequence::from_seconds_ceil(7200).unwrap(), // 2 hours - unilateral_refund_without_receiver_delay: Sequence::from_seconds_ceil(10800).unwrap(), /* 3 hours */ + unilateral_claim_delay: Sequence::from_seconds_ceil(3600)?, // 1 hour + unilateral_refund_delay: Sequence::from_seconds_ceil(7200)?, // 2 hours + unilateral_refund_without_receiver_delay: Sequence::from_seconds_ceil(10800)?, /* 3 hours */ }; // Create the VHTLC script - let vhtlc = VhtlcScript::new(options).expect("Failed to create VHTLC"); + let vhtlc = VhtlcScript::new(options)?; // Get the taproot output key and script pubkey if let Some(taproot_info) = vhtlc.taproot_info() { println!("Taproot output key: {}", taproot_info.output_key()); if let Some(script_pubkey) = vhtlc.script_pubkey() { println!("Script pubkey: {}", script_pubkey); } } + // Derive and print a testnet address + let addr = vhtlc.address(Network::Testnet, server)?; + println!("Address (testnet): {}", addr); + + Ok(()) }ark-lightning/tests/vhtlc_fixtures.rs (4)
376-380: Make the CSV encoding assertion robustString-matching
"0111"is brittle. Decode the script and assert the pushed integer equals 17 beforeOP_CSV.Example approach:
use bitcoin::opcodes::all::OP_CHECKSEQUENCEVERIFY as OP_CSV; use bitcoin::script::Instruction; let mut iter = unilateral_claim.instructions(); let (push, csv) = (iter.next().unwrap()?, iter.nth(5).unwrap()?); // adjust index to your layout match push { Instruction::PushBytes(bytes) => assert_eq!(bytes, &[0x11]), Instruction::PushNum(n) => assert_eq!(n, 17.into()), _ => panic!("Expected CSV delay push"), } assert!(matches!(csv, Instruction::Op(op) if op == OP_CSV));
250-254: Useto_string()for address formatting
Address::to_string()is canonical and avoids any ambiguity aroundencode().- let address_str = addr.encode(); + let address_str = addr.to_string();
38-40: Remove unuseddecoded_scriptsfrom fixtures or assert on itCurrently parsed but unused; either drop it from
ValidTestCaseor validate expected disassembly to prevent fixture drift.
135-139: Embed fixtures at compile time to avoid path issuesOptional: use
include_str!to make tests hermetic to the working directory.let fixtures_json = include_str!("fixtures/vhtlc.json"); let fixtures: Fixtures = serde_json::from_str(fixtures_json).expect("Failed to parse fixtures");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Cargo.toml(1 hunks)ark-lightning/Cargo.toml(1 hunks)ark-lightning/examples/vhtlc_example.rs(1 hunks)ark-lightning/scripts/generate_ts_vectors.js(1 hunks)ark-lightning/src/lib.rs(1 hunks)ark-lightning/src/vhtlc.rs(1 hunks)ark-lightning/tests/fixtures/vhtlc.json(1 hunks)ark-lightning/tests/vhtlc_fixtures.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
ark-lightning/examples/vhtlc_example.rs (2)
ark-lightning/scripts/generate_ts_vectors.js (1)
vhtlc(20-20)ark-lightning/src/vhtlc.rs (3)
new(137-145)taproot_info(368-370)script_pubkey(372-379)
ark-lightning/src/lib.rs (1)
ark-lightning/scripts/generate_ts_vectors.js (1)
vhtlc(20-20)
ark-lightning/scripts/generate_ts_vectors.js (1)
ark-lightning/src/vhtlc.rs (1)
address(381-383)
ark-lightning/tests/vhtlc_fixtures.rs (2)
ark-lightning/src/vhtlc.rs (8)
new(137-145)claim_script(150-161)refund_script(166-175)refund_without_receiver_script(180-190)unilateral_claim_script(195-208)unilateral_refund_script(213-224)unilateral_refund_without_receiver_script(229-238)taproot_info(368-370)ark-core/src/boarding_output.rs (1)
output_key(111-113)
ark-lightning/src/vhtlc.rs (1)
ark-lightning/scripts/generate_ts_vectors.js (1)
address(48-48)
🪛 Gitleaks (8.27.2)
ark-lightning/tests/fixtures/vhtlc.json
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 76-76: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: CI
ark-lightning/src/vhtlc.rs
[error] 153-153: Clippy: needlessly_borrows_for_generic_args. Remove the borrow: change .push_slice(&self.options.preimage_hash) to .push_slice(self.options.preimage_hash).
[error] 199-199: Clippy: needlessly_borrows_for_generic_args. Remove the borrow: change .push_slice(&self.options.preimage_hash) to .push_slice(self.options.preimage_hash).
[error] 275-275: Clippy: unwrap_used. Used unwrap() on an Option value; this may panic. Consider using expect() or proper handling of None.
[error] 276-276: Clippy: unwrap_used. Used unwrap() on an Option value; this may panic. Consider using expect() or proper handling of None.
[error] 297-297: Clippy: unwrap_used. Used unwrap() on an Option value; this may panic. Consider using expect() or proper handling of None.
🔇 Additional comments (3)
Cargo.toml (1)
13-14: LGTM: add ark-lightning to workspace membersWorkspace wiring looks correct.
ark-lightning/scripts/generate_ts_vectors.js (1)
47-50: Verify address API parity with RustTS uses
vhtlc.address('testnet'), while Rust’saddressrequires(network, server: XOnlyPublicKey). Ensure both derive the same address for the same inputs, or document the discrepancy.ark-lightning/src/lib.rs (1)
1-5: LGTM: minimal, clear module exposurePublicly exposing
vhtlcfrom the crate root looks good.
| "blocks" => { | ||
| if self.value == 0 { | ||
| return Err("unilateral claim delay must greater than 0".to_string()); | ||
| } | ||
| Ok(Sequence::from_height(self.value as u16)) | ||
| } | ||
| "seconds" => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential u16 wrap when converting block delays
Sequence::from_height(self.value as u16) will silently wrap for values > 65535. Add an upper bound check and improve the error text.
- "blocks" => {
- if self.value == 0 {
- return Err("unilateral claim delay must greater than 0".to_string());
- }
- Ok(Sequence::from_height(self.value as u16))
- }
+ "blocks" => {
+ if self.value == 0 {
+ return Err("blocks timelock must be greater than 0".to_string());
+ }
+ if self.value > u16::MAX as u32 {
+ return Err(format!("blocks timelock must be <= {}", u16::MAX));
+ }
+ Ok(Sequence::from_height(self.value as u16))
+ }📝 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.
| "blocks" => { | |
| if self.value == 0 { | |
| return Err("unilateral claim delay must greater than 0".to_string()); | |
| } | |
| Ok(Sequence::from_height(self.value as u16)) | |
| } | |
| "seconds" => { | |
| "blocks" => { | |
| if self.value == 0 { | |
| return Err("blocks timelock must be greater than 0".to_string()); | |
| } | |
| if self.value > u16::MAX as u32 { | |
| return Err(format!("blocks timelock must be <= {}", u16::MAX)); | |
| } | |
| Ok(Sequence::from_height(self.value as u16)) | |
| } |
🤖 Prompt for AI Agents
In ark-lightning/tests/vhtlc_fixtures.rs around lines 97 to 103, the conversion
Sequence::from_height(self.value as u16) can wrap for values > 65535; add an
explicit upper-bound check (if self.value > u16::MAX) and return a clear Err
like "unilateral claim delay must be between 1 and 65535" (preserving the
existing zero check) otherwise safely cast to u16 and call
Sequence::from_height.
@vincenzopalazzo: Since you mention that this is just informational, let's close it. Anyone can still find it even if it's closed. |
|
Mh @luckysori yeah probably it dependes if you want review a big blob or a sequence of patch but anyway I am fine |
This is built on top of a commit from #98 and introduces the vthlc that will be needed for the lightning swaps
This PR is just FYI for the readers who want to double-check my code!
Summary by CodeRabbit