ark-core: implement ArkNote encoding and cross-language validation #98
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds ArkNote feature across core, client, and gRPC: new ArkNote type and VirtualUtxoScript, proof-of-funds preimage path, client methods to create and redeem notes, admin gRPC proto and client wiring, E2E test, Cargo deps, CI workflow tweak, and a new integration-tests Just target. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Client as Ark Client
participant Core as ark-core (PoF/Batch)
participant Server as Ark Server
rect rgba(200,230,255,0.3)
note over App,Server: ArkNote creation via Admin API
App->>Client: create_arknote(amount)
Client->>Server: AdminService.CreateNote(amount, quantity=1)
Server-->>Client: notes: [arknote_str]
Client-->>App: arknote_str
end
rect rgba(200,255,200,0.3)
note over App,Server: ArkNote redemption via batch join
App->>Client: redeem_note(rng, arknote)
Client->>Client: get_offchain_address()
Client->>Core: join_next_batch(arknotes=[ArkNote], outputs=Board{to_address,to_amount})
activate Core
Core->>Core: convert ArkNote -> PoF::Input(preimage=Some)
Core->>Server: submit batch intent/proof
Server-->>Core: batch accepted, txid (optional)
deactivate Core
Client-->>App: Option<Txid>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
✨ 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 (
|
vincenzopalazzo
force-pushed
the
macros/arknotes-on-v7
branch
from
4598b5d to
c2abfb1
Compare
vincenzopalazzo
force-pushed
the
macros/arknotes-on-v7
branch
3 times, most recently
from
8283bd0 to
a79107c
Compare
vincenzopalazzo
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements ArkNote encoding and cross-language validation functionality for the ark-core library. It adds support for creating, encoding, decoding, and redeeming ArkNotes - fake VTXO coins that can be spent by revealing a preimage. The implementation includes comprehensive test vectors for validation and extends the client to support ArkNote operations.
Key changes:
- Add ArkNote struct with encoding/decoding functionality using SHA256 preimage validation
- Implement VirtualUtxoScript for taproot script management
- Add admin service support for ArkNote creation via gRPC
- Extend proof-of-funds system to handle ArkNote inputs
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ark-core/src/arknote.rs | Core ArkNote implementation with encoding/decoding and test vectors |
| ark-core/src/vtxo.rs | VirtualUtxoScript implementation and VTXO extensions for ArkNotes |
| ark-core/src/proof_of_funds.rs | Extended proof system to support ArkNote inputs with preimage validation |
| ark-client/src/lib.rs | Client methods for creating and redeeming ArkNotes |
| ark-client/src/batch.rs | Batch processing support for ArkNote redemption |
| ark-grpc/proto/ark/v1/admin.proto | Admin service protobuf definitions |
| ark-grpc/src/client.rs | gRPC client support for admin operations |
| e2e-tests/tests/e2e_arknote.rs | End-to-end integration tests for ArkNote functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| sig | ||
| } | ||
| // FIXME: this is an horrible hack to handle arknotes. |
There was a problem hiding this comment.
The FIXME comment indicates this is a temporary hack. Consider implementing a proper enum-based solution for different input types as suggested in the comment on line 52.
Copilot uses AI. Check for mistakes.
| vtxo_script: VirtualUtxoScript, | ||
| tap_tree_bytes: Vec<String>, // Cache for tap_tree() method | ||
| status: Status, | ||
| // FIXME: this is necessary? |
There was a problem hiding this comment.
The FIXME comment about the necessity of extra_witness field should be resolved or converted to proper documentation explaining when this field is used.
| // FIXME: this is necessary? | |
| /// Extra witness data required for spending the ArkNote. | |
| /// This field is used to store additional witness elements that may be needed | |
| /// when constructing spending transactions, such as signatures or scripts | |
| /// required by the spending conditions of the note. |
Copilot uses AI. Check for mistakes.
vincenzopalazzo
force-pushed
the
macros/arknotes-on-v7
branch
2 times, most recently
from
31cd7d3 to
7385b39
Compare
This commit is implementing the ark note functionality, allowing for the creation and management of ark notes within the system. Co-Developed-by: @luckysori Signed-off-by: Vincenzo Palazzo <[email protected]>
This include a sequence of functonality that allow to work with notes with arkd. Co-Developed-by: @luckysori Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo
force-pushed
the
macros/arknotes-on-v7
branch
from
7385b39 to
eb97995
Compare
Sometimes it is nice to have a fixup commit and be able to run the tests without having to pass clippy or other repo rules passing before running the e2e tests. Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit introduces a new integration-tests recipe that provides a complete end-to-end testing workflow by: • Cleaning up existing test environment (stopping nigiri, killing arkd processes) • Setting up fresh Bitcoin regtest environment with nigiri • Rebuilding arkd from latest master branch • Configuring and starting arkd services (redis, wallet, main daemon) • Funding the arkd wallet with 20 UTXOs for testing • Running the complete e2e test suite This recipe ensures a clean, reproducible testing environment for integration tests by resetting all components and dependencies before running tests. Co-authored-by: Copilot <[email protected]> Co-authored-by: @luckysori Signed-off-by: Vincenzo Palazzo <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| /// Redeem multiple ArkNotes by settling them into new VTXOs | ||
| /// | ||
| /// This method takes ArkNote objects and creates a batch transaction to convert | ||
| /// them into spendable VTXOs at the user's offchain address. | ||
| pub async fn redeem_notes<R>( | ||
| &self, | ||
| rng: &mut R, | ||
| arknotes: Vec<ArkNote>, | ||
| ) -> Result<Option<Txid>, Error> |
There was a problem hiding this comment.
I guess ark-client should be able to keep track of your Ark notes for you. It's weird to have to pass in the notes from the outside.
Having said that, it might still be useful to expose a method which lets you pick which VTXOs/boarding outputs/notes to settle.
| self.redeem_notes(rng, vec![arknote]).await | ||
| } | ||
|
|
||
| pub async fn create_arknote(&self, amount: Amount) -> Result<ArkNote, Error> { |
There was a problem hiding this comment.
AFAIK, this should not be part of the public API of the Client, because it only makes sense to use this for testing (it calls an admin API on the server).
I'm not against using it in the tests if we have to, but this only needs the GRPC client and doesn't use any of the ark_client::Client's state. I think this should just be a function defined in e2e-tests.
| pub(crate) async fn join_next_batch<R>( | ||
| &self, | ||
| rng: &mut R, | ||
| onchain_inputs: Vec<batch::OnChainInput>, | ||
| vtxo_inputs: Vec<batch::VtxoInput>, | ||
| arknotes: Vec<ArkNote>, |
There was a problem hiding this comment.
Shouldn't we just continue the pattern of using the settle API to settle everything?
I do think that it might also be useful to create another API which lets you choose what you are settling, but I'm not sure we want to be adding redeem_notes/redeem_note. To me it's more natural to settle everything (including Ark notes) with settle.
| let admin_client = AdminServiceClient::connect(self.url.clone()) | ||
| .await | ||
| .map_err(Error::connect)?; |
There was a problem hiding this comment.
Hmmm, I think we should expose a separate admin client. An ark_grpc::Client in production will never use an admin client.
I would just create a separate module admin_client.rs and move all the relevant bits there.
There was a problem hiding this comment.
nice idea thanks!
e2e-tests/Cargo.toml
Outdated
| ark-bdk-wallet = { path = "../ark-bdk-wallet" } | ||
| ark-client = { path = "../ark-client" } | ||
| ark-core = { path = "../ark-core" } | ||
| ark-grpc = { path = "../ark-grpc" } |
There was a problem hiding this comment.
👍 To be able to use the admin client API. But I believe this is not being used directly in the e2e tests yet.
| // this is inside the taproot script path | ||
| let node_script = value.note_script.clone(); | ||
| let Some(control_block) = | ||
| spending_info.control_block(&(node_script.clone(), LeafVersion::TapScript)) | ||
| else { | ||
| // FIXME: probably we need a tryfrom? | ||
| panic!("no control block found"); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I think we need to model things differently so that the conversion from Ark note to proof of funds input is infallible. An Ark note always has the same structure AFAIK, so it should just work.
There was a problem hiding this comment.
Ok to me this just sounds calling .unwrap or expect on the control_block ?
Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
ark-core/src/arknote.rs (5)
55-56: Resolve FIXME on extra_witness or remove the fieldIf not required by spending logic, drop it; otherwise, replace FIXME with a doc comment explaining when/why it’s used.
Apply (if dropping):
- // FIXME: this is necessary? - extra_witness: Vec<Vec<u8>>, + // extra witness not needed for ArkNote spend pathAnd delete the initializer and accessor accordingly.
58-59: Avoid exposing note_script as pub; add a getterKeep invariants internal; provide a read-only accessor.
- pub note_script: ScriptBuf, + note_script: ScriptBuf,Add:
+ pub fn note_script(&self) -> &ScriptBuf { + &self.note_script + }
30-34: Prefer an enum for Status over a bare boolA bool won’t scale (e.g., Pending/Confirmed/Spent). Define an enum and consider Default.
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct Status { - pub confirmed: bool, -} +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum Status { + Pending, + Confirmed, + Spent, +}Update usages accordingly (e.g., Status::Confirmed).
212-231: Offer FromStr impl for DEFAULT_HRP parsingAdd core::str::FromStr so consumers can parse via "str".parse().
Example:
impl core::str::FromStr for ArkNote { type Err = Error; fn from_str(s: &str) -> Result<Self, Self::Err> { Self::from_string(s) } }
161-168: Stop silent truncation: guard u64→u32 cast in encode()Casting sats to u32 will wrap for values > u32::MAX. Add a hard guard to fail loudly at encode-time (at minimum).
Apply:
// Use big-endian to match TypeScript's writeUInt32BE - result.extend_from_slice(&(self.value.to_sat() as u32).to_be_bytes()); + let sats = self.value.to_sat(); + assert!( + sats <= u32::MAX as u64, + "ArkNote value exceeds 32-bit encoding" + ); + result.extend_from_slice(&(sats as u32).to_be_bytes());
🧹 Nitpick comments (9)
ark-core/src/arknote.rs (9)
87-88: Avoid unnecessary expect on Txid constructionUse from_slice is fine but the expect is redundant; the slice length is guaranteed. Prefer a comment or use from_slice without expect where available.
- let txid = Txid::from_slice(preimage_hash.as_byte_array()).expect("valid txid"); + let txid = Txid::from_slice(preimage_hash.as_byte_array()).unwrap();
36-42: Consider Base58Check to catch typos/corruptionYou’re emitting raw base58 without checksum. Switch to bs58 with_check for encode/decode to detect errors early.
- let value = format!("{}{}", self.hrp, bs58::encode(encoded).into_string()); + let value = format!("{}{}", self.hrp, bs58::encode(encoded).with_check().into_string());And in from_string_with_hrp:
- let decoded = bs58::decode(encoded) - .into_vec() + let decoded = bs58::decode(encoded) + .with_check(None) + .into_vec() .map_err(|e| Error::ad_hoc(format!("failed to decode base58: {e}")))?;
134-137: extra_witness() always returns Some — drop Option for a plain sliceReturning Option here adds noise; this is unconditionally Some.
- pub fn extra_witness(&self) -> Option<&[Vec<u8>]> { - Some(&self.extra_witness) - } + pub fn extra_witness(&self) -> &[Vec<u8>] { + &self.extra_witness + }If you remove the field per previous comment, drop this API.
53-54: Naming + allocation: tap_tree_bytes is hex strings and clones on access
- Rename to tap_tree_hex (it’s Vec, not bytes).
- Return a slice to avoid cloning on every call.
- tap_tree_bytes: Vec<String>, // Cache for tap_tree() method + tap_tree_hex: Vec<String>, // Hex-encoded cache for tap_tree() @@ - pub fn tap_tree(&self) -> Vec<String> { - self.tap_tree_bytes.clone() - } + pub fn tap_tree(&self) -> &[String] { + &self.tap_tree_hex + }Also applies to: 139-143
36-42: Delimiter-free “hrp + payload” is ambiguous; consider a separatorOptional: use a separator (e.g., ':') to future-proof HRP parsing and avoid accidental overlaps.
Also applies to: 212-231
83-84: Reuse Secp256k1 contextInstantiate once (e.g., lazy_static/OnceCell) to avoid per-call cost.
static SECP: once_cell::sync::Lazy<Secp256k1<bitcoin::secp256k1::VerifyOnly>> = once_cell::sync::Lazy::new(Secp256k1::verification_only);
145-154: Indexing scripts()[0] assumes at least one scriptAdd an assertion to document invariant.
- &self.vtxo_script.scripts()[0] + { + let scripts = self.vtxo_script.scripts(); + debug_assert!(!scripts.is_empty(), "ArkNote expects one script"); + &scripts[0] + }
170-173: Nit: to_encoded_string() duplicates to_string()Consider removing or documenting why both exist; they’re identical.
598-608: Base58 error message specificityIf you adopt Base58Check, error messages will improve; otherwise consider adding guidance (“invalid alphabet or checksum”).
📜 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 (2)
ark-core/src/arknote.rs(1 hunks)e2e-tests/tests/e2e_arknote.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/tests/e2e_arknote.rs
🔇 Additional comments (3)
ark-core/src/arknote.rs (3)
239-245: TxOut construction LGTM
321-399: Add a negative test for > u32::MAX valuesEnsure encoding rejects values that exceed 32-bit.
Add:
#[test] #[should_panic(expected = "exceeds 32-bit")] fn test_arknote_value_overflow_panics_in_encode() { let note = ArkNote::new([0x11; PREIMAGE_LENGTH], Amount::from_sat(u32::MAX as u64 + 1)); let _ = note.encode(); // should assert or, if using try_new*, construction should fail instead }If you adopt try_new*, switch to expecting an Err on construction.
Also applies to: 434-459, 567-621
200-205: decode_with_hrp constructs via new_with_hrp → consider try_new_with_hrpIf you switch to fallible constructors, decode_with_hrp should call try_new_with_hrp and propagate errors.
| let secp = Secp256k1::new(); | ||
| let vtxo_script = VirtualUtxoScript::new(&secp, vec![note_script.clone()]) | ||
| .expect("failed to create VirtualUtxoScript"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid expect() in library code; propagate VirtualUtxoScript::new error
Don’t panic in constructors; bubble errors instead (see previous diff).
🤖 Prompt for AI Agents
In ark-core/src/arknote.rs around lines 83 to 86, remove the expect() call so
the VirtualUtxoScript::new error is propagated instead of panicking: update the
enclosing function/signature to return Result (or propagate an existing Result)
and replace the expect with the ? operator, making sure to import/return the
appropriate error type and adjust upstream callsites to handle the Result; do
not catch and panic here.
Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo
force-pushed
the
macros/arknotes-on-v7
branch
from
a68fda4 to
8b0ee15
Compare
Signed-off-by: Vincenzo Palazzo <[email protected]>
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 (3)
ark-core/src/vtxo.rs (3)
75-81: Inverted length check causes false error.This check rejects the correct case and lets the incorrect case pass.
- if leaf_distribution.len() == scripts.len() { - return Err(Error::ad_hoc("wrong leaf distribution calculated")); - } + if leaf_distribution.len() != scripts.len() { + return Err(Error::ad_hoc("wrong leaf distribution calculated")); + }However, see next comment: the distribution function itself is incorrect for n ≥ 3 and can produce extra entries.
225-255: Replace buggy leaf-depth calculation with a simple, consistent scheme.
calculate_leaf_depthsover-allocates (e.g., n=3 → 4 depths). Align withVirtualUtxoScript::newand add leaves at depth 1. This also keeps cross-language behavior predictable.- let scripts = [vec![forfeit_script, redeem_script], extra_scripts.clone()].concat(); - - let leaf_distribution = calculate_leaf_depths(scripts.len()); - - if leaf_distribution.len() == scripts.len() { - return Err(Error::ad_hoc("wrong leaf distribution calculated")); - } - - let mut builder = TaprootBuilder::new(); - for (script, depth) in scripts.iter().zip(leaf_distribution.iter()) { - builder = builder - .add_leaf(*depth as u8, script.clone()) - .map_err(Error::ad_hoc)?; - } + let scripts = [vec![forfeit_script, redeem_script], extra_scripts.clone()].concat(); + + let mut builder = TaprootBuilder::new(); + for script in scripts { + builder = builder.add_leaf(1, script).map_err(Error::ad_hoc)?; + }
161-168: Avoid expect() in a fallible API.
get_spend_inforeturnsResultbut still usesexpect(...). Return an error instead.- let control_block = self - .spend_info - .control_block(&(script, LeafVersion::TapScript)) - .expect("forfeit script"); + let control_block = self + .spend_info + .control_block(&(script, LeafVersion::TapScript)) + .ok_or_else(|| Error::ad_hoc("script not found in spend_info"))?;
♻️ Duplicate comments (3)
ark-core/src/proof_of_funds.rs (2)
2-2: Fix broken LeafVersion import (compilation error).Import
LeafVersionfrom the bitcoin crate, not from this module.-use crate::proof_of_funds::taproot::LeafVersion; +use bitcoin::taproot::LeafVersion;
85-90: Don’t panic in conversions; make ArkNote→Input fallible.
panic!("no control block found")in a conversion will crash the process. UseTryFromand propagate an error.-impl From<&ArkNote> for Input { - fn from(value: &ArkNote) -> Self { +impl TryFrom<&ArkNote> for Input { + type Error = Error; + fn try_from(value: &ArkNote) -> Result<Self, Error> { let spending_info = value.vtxo_script().spend_info(); // this is inside the taproot script path let node_script = value.note_script().clone(); - let Some(control_block) = - spending_info.control_block(&(node_script.clone(), LeafVersion::TapScript)) - else { - // FIXME: probably we need a tryfrom? - panic!("no control block found"); - }; + let control_block = spending_info + .control_block(&(node_script.clone(), LeafVersion::TapScript)) + .ok_or_else(|| Error::ad_hoc("no control block found"))?; - Self { + Ok(Self { outpoint: value.outpoint(), sequence: Sequence::MAX, witness_utxo: TxOut { value: value.value(), // This should be unspendable script? script_pubkey: value.vtxo_script().script_pubkey(), }, - // This should be empty? - tapscripts: vec![], + // Provide the tapscripts for intent encoding. + tapscripts: value.vtxo_script().scripts().to_vec(), pk: value.vtxo_script().x_only_public_key(), // This contains the extra info to spend the note, right? spend_info: (node_script, control_block), is_onchain: false, preimage: Some(*value.preimage()), - } + }) } }Follow-up: update call sites to use
Input::try_from(¬e)?or(¬e).try_into()?.ark-core/src/arknote.rs (1)
166-173: Value truncation: 64-bit sats cast to u32 in encode().This will silently wrap for values > u32::MAX. Enforce the 32‑bit invariant at construction and keep encode() safe.
Apply fallible constructors and validate amount; keep existing
new*as thin wrappers if you don’t want to ripple changes yet:@@ - /// Create a new ArkNote with the given preimage and value - pub fn new(preimage: [u8; PREIMAGE_LENGTH], value: Amount) -> Self { - Self::new_with_hrp(preimage, value, DEFAULT_HRP.to_string()) - } + /// Create a new ArkNote with the given preimage and value + pub fn new(preimage: [u8; PREIMAGE_LENGTH], value: Amount) -> Self { + Self::try_new(preimage, value).expect("valid ArkNote") + } @@ - /// Create a new ArkNote with a custom HRP - pub fn new_with_hrp(preimage: [u8; PREIMAGE_LENGTH], value: Amount, hrp: String) -> Self { + /// Create a new ArkNote with a custom HRP + pub fn new_with_hrp(preimage: [u8; PREIMAGE_LENGTH], value: Amount, hrp: String) -> Self { + Self::try_new_with_hrp(preimage, value, &hrp).expect("valid ArkNote") + } + + /// Fallible constructor (preferred): validates amount/HRP and propagates errors. + pub fn try_new(preimage: [u8; PREIMAGE_LENGTH], value: Amount) -> Result<Self, Error> { + Self::try_new_with_hrp(preimage, value, DEFAULT_HRP) + } + + pub fn try_new_with_hrp( + preimage: [u8; PREIMAGE_LENGTH], + value: Amount, + hrp: &str, + ) -> Result<Self, Error> { + let sats = value.to_sat(); + if sats > u32::MAX as u64 { + return Err(Error::ad_hoc("ArkNote value exceeds 32-bit encoding")); + } + if hrp.is_empty() || hrp.len() > 32 || !hrp.chars().all(|c| c.is_ascii_alphanumeric()) { + return Err(Error::ad_hoc("invalid HRP")); + } let preimage_hash = sha256::Hash::hash(&preimage); @@ - let secp = Secp256k1::new(); - let vtxo_script = VirtualUtxoScript::new(&secp, vec![note_script.clone()]) - .expect("failed to create VirtualUtxoScript"); + let secp = Secp256k1::verification_only(); + let vtxo_script = VirtualUtxoScript::new(&secp, vec![note_script.clone()])?; @@ - ArkNote { + Ok(ArkNote { preimage, value, - hrp, + hrp: hrp.to_string(), txid, vtxo_script, tap_tree_bytes: encoded_scripts, status: Status { confirmed: true }, extra_witness: vec![preimage.to_vec()], note_script, - } + }) } @@ - pub fn encode(&self) -> Vec<u8> { + pub fn encode(&self) -> Vec<u8> { let mut result = Vec::with_capacity(ARKNOTE_LENGTH); result.extend_from_slice(&self.preimage); // Use big-endian to match TypeScript's writeUInt32BE - result.extend_from_slice(&(self.value.to_sat() as u32).to_be_bytes()); + let sats = self.value.to_sat(); + debug_assert!(sats <= u32::MAX as u64); + result.extend_from_slice(&(sats as u32).to_be_bytes()); result }
🧹 Nitpick comments (3)
ark-core/src/proof_of_funds.rs (2)
52-53: Model input kind explicitly.The
preimage: Option<...>+is_onchaincombination is a smell. Preferenum InputKind { Onchain { ... }, Vtxo { ... }, ArkNote { preimage: [u8; PREIMAGE_LENGTH] } }to make witnesses/flow explicit.
230-276: Name the witness element, not “sig”.This variable holds either a Schnorr signature or a preimage. Rename to
witness_elem(and reflect in comments) to reduce confusion.ark-core/src/arknote.rs (1)
139-143: Return a slice; the Option is always Some.
extra_witness()always returnsSome. Return a slice directly.- pub fn extra_witness(&self) -> Option<&[Vec<u8>]> { - Some(&self.extra_witness) - } + pub fn extra_witness(&self) -> &[Vec<u8>] { + &self.extra_witness + }
📜 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 (5)
ark-core/src/arknote.rs(1 hunks)ark-core/src/lib.rs(3 hunks)ark-core/src/proof_of_funds.rs(7 hunks)ark-core/src/vtxo.rs(1 hunks)e2e-tests/tests/e2e_arknote.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e-tests/tests/e2e_arknote.rs
- ark-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
ark-core/src/proof_of_funds.rs (2)
ark-core/src/arknote.rs (2)
value(105-107)preimage(110-112)ark-core/src/vtxo.rs (1)
exit_script(220-222)
ark-core/src/vtxo.rs (4)
ark-core/src/proof_of_funds.rs (1)
new(57-76)ark-core/src/send.rs (1)
script_pubkey(243-245)ark-core/src/script.rs (1)
tr_script_pubkey(30-37)ark-core/src/arknote.rs (1)
vtxo_script(162-164)
ark-core/src/arknote.rs (1)
ark-core/src/proof_of_funds.rs (4)
encode(442-447)encode(465-487)new(57-76)decode(490-520)
🔇 Additional comments (1)
ark-core/src/vtxo.rs (1)
324-414: VirtualUtxoScript: solid, minimal API.Creation, encoding, and accessors look good and align with the Taproot builder behavior.
2 participants
replacing #88 and building on the v7 API
Summary by CodeRabbit