feat(client): support external signers in OfflineClient - #261
Conversation
WalkthroughChangesThe SDK introduces a public Signer abstraction and client integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TransactionFlow
participant OfflineClient
participant Signer
TransactionFlow->>OfflineClient: request signature for public key
OfflineClient->>Signer: sign_schnorr(pk, msg)
Signer-->>OfflineClient: Schnorr signature
OfflineClient-->>TransactionFlow: signature and public key
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (3)
ark-client/src/boltz.rs (1)
641-650: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated single-pk sign-and-wrap boilerplate (9 occurrences in this file).
The
sign_for_pk(&pk, &msg).map_err(|e| ark_core::Error::ad_hoc(e.to_string()))?; Ok(vec![(sig, pk)])pattern is duplicated at lines 641-650, 667-676, 921-930, 1014-1023, ~1555-1560, ~1802-1807, ~2219-2224, 2553-2561, and 2578-2586. Consider extracting a small helper (e.g.fn schnorr_sign_single(&self, pk: XOnlyPublicKey, msg: &secp256k1::Message) -> Result<Vec<(schnorr::Signature, XOnlyPublicKey)>, ark_core::Error>) and having each closure call it.♻️ Example helper
fn schnorr_sign_single( &self, pk: XOnlyPublicKey, msg: &secp256k1::Message, ) -> Result<Vec<(schnorr::Signature, XOnlyPublicKey)>, ark_core::Error> { let sig = self .sign_for_pk(&pk, msg) .map_err(|e| ark_core::Error::ad_hoc(e.to_string()))?; Ok(vec![(sig, pk)]) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-client/src/boltz.rs` around lines 641 - 650, Extract the repeated single-key signing and result-wrapping logic into a helper on the enclosing type, such as schnorr_sign_single, preserving the existing error mapping and return type. Update all nine affected closures to delegate to this helper, passing their respective public key and message references, without changing signing behavior.ark-client/src/signer.rs (1)
27-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new
ExternalSignercontract.Consider adding a minimal mock
ExternalSignerand unit tests (inlib.rsor here) coveringnext_signing_pk/can_sign_for_pk/sign_for_pkdispatch between local key provider and external signer, given this is security-sensitive signing-delegation logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-client/src/signer.rs` around lines 27 - 37, Add minimal mock ExternalSigner coverage for next_signing_pk, can_sign_for_pk, and sign_for_pk, verifying dispatch between the local key provider and external signer, including successful external signing and unsupported-key behavior. Place the unit tests in the relevant existing test module and keep the mock focused on the ExternalSigner contract.ark-client/src/send_vtxo.rs (1)
235-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "sign every signable checksig pubkey" loop across three files.
Five sites implement the identical loop: extract checksig pubkeys from a witness script, skip any the client can't sign for (
can_sign_for_pk), otherwise sign viasign_for_pkand collect(sig, pk), mapping errors intoark_core::Error::ad_hoc. Sincesign_for_pk/can_sign_for_pkare already centralized onClient(inlib.rs), this loop is a natural candidate for one more shared helper instead of five copies.
ark-client/src/send_vtxo.rs#L235-L259: promotemake_sign_fn's loop body into a sharedClienthelper (e.g.fn sign_checksig_pubkeys(&self, script: &Script, msg: &secp256k1::Message) -> Result<Vec<(schnorr::Signature, XOnlyPublicKey)>, ark_core::Error>) and havemake_sign_fncall it.ark-client/src/send_vtxo.rs#L757-L781: replacesign_for_vtxo_fn's inline loop with a call to the same shared helper.ark-client/src/boltz.rs#L3096-L3120: replacesign_for_vtxo_fn's inline loop with a call to the same shared helper.ark-client/src/boltz.rs#L3678-L3697: replacesign_checkpoint_with_own_keys's inline loop with a call to the same shared helper.ark-client/src/unilateral_exit.rs#L261-L282: replace the inline loop in thesignclosure with a call to the same shared helper.♻️ Example shared helper
fn sign_checksig_pubkeys( &self, script: &bitcoin::Script, msg: &secp256k1::Message, ) -> Result<Vec<(schnorr::Signature, XOnlyPublicKey)>, ark_core::Error> { let pks = extract_checksig_pubkeys(script); let mut res = vec![]; for pk in pks { if self.can_sign_for_pk(&pk) { let sig = self .sign_for_pk(&pk, msg) .map_err(|e| ark_core::Error::ad_hoc(e.to_string()))?; res.push((sig, pk)); } } Ok(res) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-client/src/send_vtxo.rs` around lines 235 - 259, Extract the duplicated checksig signing loop into a shared Client helper, such as sign_checksig_pubkeys, reusing extract_checksig_pubkeys, can_sign_for_pk, sign_for_pk, and the existing ark_core::Error mapping. Update ark-client/src/send_vtxo.rs lines 235-259 and 757-781, ark-client/src/boltz.rs lines 3096-3120 and 3678-3697, and ark-client/src/unilateral_exit.rs lines 261-282 so each call uses the helper while preserving each closure’s existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ark-client/src/boltz.rs`:
- Around line 641-650: Extract the repeated single-key signing and
result-wrapping logic into a helper on the enclosing type, such as
schnorr_sign_single, preserving the existing error mapping and return type.
Update all nine affected closures to delegate to this helper, passing their
respective public key and message references, without changing signing behavior.
In `@ark-client/src/send_vtxo.rs`:
- Around line 235-259: Extract the duplicated checksig signing loop into a
shared Client helper, such as sign_checksig_pubkeys, reusing
extract_checksig_pubkeys, can_sign_for_pk, sign_for_pk, and the existing
ark_core::Error mapping. Update ark-client/src/send_vtxo.rs lines 235-259 and
757-781, ark-client/src/boltz.rs lines 3096-3120 and 3678-3697, and
ark-client/src/unilateral_exit.rs lines 261-282 so each call uses the helper
while preserving each closure’s existing behavior.
In `@ark-client/src/signer.rs`:
- Around line 27-37: Add minimal mock ExternalSigner coverage for
next_signing_pk, can_sign_for_pk, and sign_for_pk, verifying dispatch between
the local key provider and external signer, including successful external
signing and unsupported-key behavior. Place the unit tests in the relevant
existing test module and keep the mock focused on the ExternalSigner contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abd18a89-07cb-4ac8-8cb2-cea092312100
📒 Files selected for processing (5)
ark-client/src/boltz.rsark-client/src/lib.rsark-client/src/send_vtxo.rsark-client/src/signer.rsark-client/src/unilateral_exit.rs
muchai254
left a comment
muchai254
left a comment
There was a problem hiding this comment.
Logic looks pretty solid.
Maybe we could consider adding the following tests:
- Test asserting secret-key operations error for a signer-only client
- An integration test driving a supported flow, offchain send or VHTLC claim, end-to-end through a mock signer
- Unit tests for
UnavailableKeyProvider,sign_for_pk routing,can_sign_for_pk
luckysori
left a comment
luckysori
left a comment
There was a problem hiding this comment.
Whilst obviously a useful feature, I am not sold on the design.
It's not a good sign that we have to introduce the clunky UnavailableKeyProvider for this to work. I think it should be possible to redesign KeyProvider to allow the signing key to be external to this library.
2ac292d to
973b2e0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ark-client/src/lib.rs (1)
654-669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDouble
Arcwrap here is subtle — worth a one-line comment.
Arc::new(key_provider)wraps the already type-erasedArc<dyn KeyProvider>in a secondArc, producingArc<Arc<dyn KeyProvider>>before it coerces toArc<dyn Signer>. This is required because Rust has no directdyn KeyProvider→dyn Signerupcasting (they're unrelated traits, only linked by the blanket impl), so the extraArclayer supplies a concrete,Sizedtype that the blanket impl can attach to. It's correct, but looks like an accidental double-wrap to anyone unfamiliar with this trick and could get "cleaned up" into a compile error later.💬 Suggested clarifying comment
pub fn with_key_provider( config: OfflineClientConfig, key_provider: Arc<dyn KeyProvider>, blockchain: Arc<B>, wallet: Arc<W>, swap_storage: Arc<S>, ) -> Self { + // Wrap the already-erased `Arc<dyn KeyProvider>` in another `Arc` so the blanket + // `Signer` impl (over a *concrete* `KeyProvider` type) has something `Sized` to attach + // to; `dyn KeyProvider` cannot upcast directly to `dyn Signer` (unrelated traits). Self::with_signer_parts( config, Arc::new(key_provider), None, blockchain, wallet, swap_storage, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-client/src/lib.rs` around lines 654 - 669, Add a concise explanatory comment immediately before Arc::new(key_provider) in with_key_provider, documenting that the extra Arc layer is intentional: it enables the blanket KeyProvider-to-Signer implementation because direct trait-object upcasting is unavailable. Leave the existing wrapping and with_signer_parts behavior unchanged.ark-client/src/signer.rs (1)
72-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a unit test for the blanket
Signerimpl.This blanket implementation is the load-bearing bridge that keeps every existing
KeyProvider(static, BIP32) signing correctly through the newSignerabstraction. A small test (e.g. usingStaticKeyProvider) asserting thatSigner::sign_schnorrproduces a signature that verifies againstsigning_pks()/get_cached_pks()would guard this critical path against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-client/src/signer.rs` around lines 72 - 97, Add a focused unit test for the blanket Signer implementation using StaticKeyProvider, asserting that Signer::sign_schnorr produces a signature that verifies with the public key returned by signing_pks (and get_cached_pks). Keep the test scoped to validating the blanket impl’s signing bridge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ark-client/src/lib.rs`:
- Around line 654-669: Add a concise explanatory comment immediately before
Arc::new(key_provider) in with_key_provider, documenting that the extra Arc
layer is intentional: it enables the blanket KeyProvider-to-Signer
implementation because direct trait-object upcasting is unavailable. Leave the
existing wrapping and with_signer_parts behavior unchanged.
In `@ark-client/src/signer.rs`:
- Around line 72-97: Add a focused unit test for the blanket Signer
implementation using StaticKeyProvider, asserting that Signer::sign_schnorr
produces a signature that verifies with the public key returned by signing_pks
(and get_cached_pks). Keep the test scoped to validating the blanket impl’s
signing bridge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 261521e4-7192-4124-bef7-f448308d86bc
📒 Files selected for processing (3)
ark-client/src/key_provider.rsark-client/src/lib.rsark-client/src/signer.rs
Add a Signer trait as the client's single signing abstraction, so all Schnorr signing can be delegated to an external component (threshold schemes like FROST, hardware wallets, remote signers) instead of an in-process key provider. - Signer requires only signing_pks + sign_schnorr; raw keypair access (needed for musig2 flows: settlement, boarding, chain swaps) is an optional capability defaulting to none. - A blanket impl makes every KeyProvider a full Signer, so existing constructors and local key management keep working unchanged. - New OfflineClient::with_signer constructor; such a client holds no secret key material and cannot settle, board or chain-swap. - Route send_vtxo, boltz swaps and unilateral exit signing through the signer.
973b2e0 to
df67511
Compare
CI installs the latest stable toolchain (clippy 1.97), which added for_kv_map coverage and useless_borrows_in_formatting. Pre-existing on master; fixed here so the pipeline is green.
286a462 to
f01e4fe
Compare
Settlement, boarding and Boltz chain swaps needed raw keypairs only as an implementation artifact, not by protocol necessity: - The musig2 cosigner key for the VTXO tree is ephemeral and generated by the client per settlement; it never comes from the wallet key. - Intent, forfeit and delegate PSBT signing are plain BIP340 script-path signatures; route them through Signer::sign_schnorr instead of raw keypair lookups. - Chain swap claim/refund spends are script-path too (the musig2 aggregate with Boltz is only the taproot internal key of the lockup and is never signed for); derive the per-swap keys via the signer and sign via Signer::sign_schnorr. The client no longer requires raw keypairs anywhere. The optional keypair accessors on Signer remain only to preserve pubkey parity for local wallets; external signer keys lift to even parity.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Reviewed at head a41227e. This is a signing-abstraction refactor touching every place the client produces a BIP340 signature (settlement/forfeit/delegate/intent in batch.rs, all Boltz flows in boltz.rs, offchain sends in send_vtxo.rs, unilateral exit in unilateral_exit.rs). Requesting a human protocol-review sign-off before merge — the surface area is wide and the failure mode of a wrong signature is silent loss of funds.
Blocking
1. Zero test coverage for the new Signer abstraction — ark-client/src/signer.rs
Danger already flagged this. No test in the repo exercises an external Signer — the blanket impl<T: KeyProvider> Signer for T path is the only one existing tests cover, so the trait's default methods and the next_signing_public_key parity-lift branch in lib.rs:2029-2043 are entirely untested. For a trait that will be plugged into settlement/forfeit signing, that is not acceptable. Please add at least:
- a mock
Signerreturning a fixedsigning_pks/sign_schnorr(nokeypair_for_pk/next_keypairoverrides), exercised throughClient::sign_for_pk,Client::can_sign_for_pk, andClient::next_signing_public_key; - a regtest run of
settle_vtxos(or an equivalent batch flow) with such a signer, asserting the resulting BIP340 sigs validate under the x-only key returned bysigning_pks.
2. next_signing_pk default silently ignores KeypairIndex::New — ark-client/src/signer.rs:50-56 + ark-client/src/lib.rs:2029-2043
The default Signer::next_signing_pk always returns signing_pks()?.first() regardless of the KeypairIndex argument. For a single-key signer that's fine (and documented). But this trait is the escape hatch for FROST / HSM / remote signers, which are usually multi-key or at least have a receive-index notion, and there's nothing forcing them to override it.
Concrete consequence in create_chain_swap (boltz.rs:1871-1879): the flow calls next_signing_public_key(KeypairIndex::New) twice — once for claim, once for refund. With the default impl on a multi-key external signer, both calls return the same key, so claim_public_key == refund_public_key and the resulting VHTLC has the same key on both leaves. Same story for any future multi-role flow. Please either:
- remove the default and force implementors to think about it, or
- add a very loud doc-comment warning on
next_signing_pkthat any signer with more than one key MUST override it, and document the chain-swap dual-role case specifically.
Same reasoning applies to next_keypair's Ok(None) default combined with next_signing_public_key's fallback lift — the caller can't tell "no raw keys exposed" from "you forgot to advance the index."
3. can_sign_for_pk swallows signer errors — ark-client/src/lib.rs:2050-2057
fn can_sign_for_pk(&self, pk: &XOnlyPublicKey) -> bool {
if matches!(self.inner.signer.keypair_for_pk(pk), Ok(Some(_))) { return true; }
self.inner.signer.signing_pks().is_ok_and(|pks| pks.contains(pk))
}Both keypair_for_pk and signing_pks return Result. Errors from either — transient HSM/network failure on a remote signer, poisoned mutex on Bip32KeyProvider's RwLock — are silently coerced to false. Downstream that means the signing closure just doesn't push a signature for that pk (see e.g. batch.rs:642-648, send_vtxo.rs:246-256), the input goes out under-signed, and the batch/forfeit/checkpoint later fails with an unrelated "signature validation failed" or "missing witness" from the server. That's the exact class of bug that eats hours during incident response.
Please at least log at warn when keypair_for_pk or signing_pks returns Err inside can_sign_for_pk. Better: make can_sign_for_pk return Result<bool, Error> and propagate.
Non-blocking observations
- Parity-even lift is fine here but worth documenting.
next_signing_public_key(lib.rs:2035-2042) lifts external-signer x-only keys viapk.public_key(Parity::Even)and stores that fullPublicKeyinSubmarineSwapData::refund_public_keyetc. (boltz.rs:3904). All downstream construction goes back through.x_only_public_key().0before script-building/signing (boltz.rs:3424-3425,boltz.rs:2401,boltz.rs:2735), so parity is thrown away and BIP340 correctness is preserved. Worth a one-line comment onnext_signing_public_keystating "the parity byte is not observed by any downstream code path" so a future refactor doesn't accidentally start honoring it. sign_schnorr_no_aux_randin the blanket impl (signer.rs:80) matches the pre-refactor behavior — noted, not a regression.- Duplicated
for pk in pks { if can_sign_for_pk … sign_for_pk … }loop appears inbatch.rs(x4),boltz.rs(x2 in signing closures + refund/claim helpers),send_vtxo.rs(x2),unilateral_exit.rs(x1). CodeRabbit already suggested extracting asign_checksig_pubkeyshelper onClient; I'd second that — it also gives you one place to fix the error-swallowing above. ark-core/src/tx_graph.rs:151— pure clippyiter().values()cleanup, no behavior change.derivation_index_for_pkwill always returnNonefor external signers (they don't implementDiscoverableKeyProvider), so every new Boltz swap under an external signer will persistkey_derivation_index: None.ensure_swap_key_cached(boltz.rs:3455-3461) currently treatsNoneas "legacy, skip recovery" and warns — which means pending-VHTLC recovery after a restart is a silent no-op for external-signer wallets. That may be intentional (external signer is expected to still have the key without derivation-index help), but the current warn-and-skip path is misleading. Consider skipping the warn when the signer has no discoverable provider, or short-circuiting oncan_sign_for_pk(pk)before the index check.
Cross-repo
Signer / with_signer are additive; existing with_keypair / with_bip32 / with_key_provider constructors are preserved, and the removed Client helpers (keypair_by_pk, next_keypair) were never pub. No downstream Rust consumer, ts-sdk, go-sdk, or dotnet-sdk breakage from this change.
4 participants
Add a
Signertrait as the client's single signing abstraction, so all Schnorr signing can be delegated to an external component (threshold schemes like FROST, hardware wallets, remote signers) instead of an in-process key provider.Signerrequires onlysigning_pks+sign_schnorr; a blanket impl makes everyKeyProvidera fullSigner, so existing constructors and local key management keep working unchanged.OfflineClient::with_signerconstructor; such a client holds no secret key material.Signerremain only to preserve pubkey parity for local wallets.