feat: add ArkCash bearer instruments for address-free transfers#337
feat: add ArkCash bearer instruments for address-free transfers#337
Conversation
WalkthroughAdds a new ArkCash bearer-instrument class with bech32m encode/decode, Schnorr key derivation, and timelock handling; exposes it in the public API; integrates ArkCash creation/claiming into Wallet; and adds unit and end-to-end tests covering encoding, derivation, and claim flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet as Wallet
participant ArkCash as ArkCash
participant Server as Server
participant Blockchain as Blockchain
Wallet->>ArkCash: generate(serverPubKey, csvTimelock)
ArkCash->>ArkCash: create random privateKey\nderive publicKey (Schnorr)
ArkCash-->>Wallet: ArkCash instance
Wallet->>ArkCash: address(hrp)
ArkCash-->>Wallet: ArkAddress
Wallet->>Server: send amount to ArkAddress
Server->>Blockchain: broadcast tx
Wallet->>ArkCash: toString()
ArkCash-->>Wallet: encoded bech32m string
sequenceDiagram
participant Wallet as Wallet
participant ArkCash as ArkCash
participant Server as Server
participant Contracts as Contracts
Wallet->>ArkCash: fromString(encoded)
ArkCash->>ArkCash: validate version & payload length\nextract keys and timelock
ArkCash-->>Wallet: ArkCash instance
Wallet->>Server: query spendable VTXOs for ArkCash contract
Server-->>Wallet: VTXO list
Wallet->>Wallet: sweep eligible VTXOs to wallet address
Wallet->>Contracts: import remaining VTXOs as contract
Contracts-->>Wallet: contract refs
Wallet-->>Wallet: return swept & imported amounts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/ark.test.ts (1)
2329-2411: Please add one test for theimportedbranch.These cases only verify the direct sweep path. A subdust or recoverable ArkCash token should also assert
result.imported > 0so the fallback contract-import flow is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/ark.test.ts` around lines 2329 - 2411, Add a new e2e test that triggers the fallback contract-import path and asserts result.imported > 0: createTestArkWallet for Alice and Bob, fund Alice via faucetOffchain, have Alice create a subdust/recoverable cash token (e.g., very small amount likely to be considered dust), then call bob.wallet.claimCash(cashStr) and assert the returned result.imported > 0 (and optionally result.swept === 0). Use waitFor with bob.wallet.getBalance() or bob.wallet.getVtxos() to wait for visibility; reference functions createTestArkWallet, faucetOffchain, createCash, claimCash, getBalance, getVtxos in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/arkcash/index.ts`:
- Around line 60-63: The call to bech32m.decodeUnsafe in src/arkcash/index.ts
currently lowercases the input (encoded.trim().toLowerCase()), which disables
strict mixed-case rejection; change it to pass the trimmed string as-is
(encoded.trim()) into bech32m.decodeUnsafe so the library enforces
BIP-173/BIP-350 validation, update any surrounding validation logic that assumes
lowercasing (e.g., where decoded is used), and add a regression test that
supplies a mixed-case bech32m string to verify it is rejected.
In `@src/wallet/wallet.ts`:
- Around line 2037-2057: The temporary Wallet.create(...) used for sweeping
ArkCash should not be used because it rebuilds the contract and eagerly starts a
VtxoManager; instead perform the sweep against the existing cash.vtxoScript
(i.e., use the cash contract’s vtxoScript as the UTXO selector/witness for the
send/sweep operation) OR if you must construct a temporary wallet, create it
with settlement disabled (pass settlementConfig: false / or the equivalent
option that prevents new VtxoManager creation) and ensure you call
wallet.dispose() in a finally block; update the try/catch around the sweep to
still surface the original error message but guarantee disposal when a temp
wallet was created.
In `@test/e2e/ark.test.ts`:
- Around line 2376-2379: Change the brittle exact-error assertion on
charlie.wallet.claimCash(cashStr) to accept both failure modes by matching
either "No VTXOs found" or the sweep/race failure message; update the
expect(...).rejects.toThrow to use a regex (e.g. /No VTXOs found|sweep/i or
similar) so the test passes whether Charlie fails due to missing VTXOs or due to
the wrapped sweep race.
---
Nitpick comments:
In `@test/e2e/ark.test.ts`:
- Around line 2329-2411: Add a new e2e test that triggers the fallback
contract-import path and asserts result.imported > 0: createTestArkWallet for
Alice and Bob, fund Alice via faucetOffchain, have Alice create a
subdust/recoverable cash token (e.g., very small amount likely to be considered
dust), then call bob.wallet.claimCash(cashStr) and assert the returned
result.imported > 0 (and optionally result.swept === 0). Use waitFor with
bob.wallet.getBalance() or bob.wallet.getVtxos() to wait for visibility;
reference functions createTestArkWallet, faucetOffchain, createCash, claimCash,
getBalance, getVtxos in the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf8a57d3-8ccc-4d36-93f3-5c6d21877b93
📒 Files selected for processing (5)
src/arkcash/index.tssrc/index.tssrc/wallet/wallet.tstest/arkcash.test.tstest/e2e/ark.test.ts
| const decoded = bech32m.decodeUnsafe( | ||
| encoded.trim().toLowerCase(), | ||
| 1023 | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does BIP-173/BIP-350 require decoders to reject mixed-case bech32/bech32m strings, and does @scure/base bech32m.decodeUnsafe preserve that check when the caller does not lowercase the input first?
💡 Result:
Yes.
- BIP-173 explicitly requires that decoders MUST NOT accept mixed-case strings (strings containing both uppercase and lowercase characters). It allows all-lowercase or all-uppercase inputs, but not a mix. [1]
- BIP-350 (bech32m) changes only the checksum constant and says all other aspects of Bech32 remain unchanged, and it includes a “Mixed case” example among invalid test vectors—so the same mixed-case rejection rule applies to bech32m too. [2]
And @scure/base’s bech32m.decodeUnsafe preserves that rule even if the caller does not lowercase first:
- In
@scure/basev2.0.0, the internaldecode()does:lowered = str.toLowerCase()- rejects if
stris neither exactlylowered(all-lowercase) nor exactlystr.toUpperCase()(all-uppercase):if (str !== lowered && str !== str.toUpperCase()) throw ...(i.e., mixed case is rejected) - then proceeds using
loweredfor parsing/checksum
decodeUnsafeis just anunsafeWrapper(decode)(returnsundefinedinstead of throwing), so the mixed-case check remains enforced. [3]
Sources:
[1] BIP-173 “Uppercase/lowercase” rule (mixed-case rejection) https://en.bitcoin.it/wiki/BIP_0173
[2] BIP-350 “All other aspects … unchanged” + invalid vector labeled “Mixed case” https://en.bitcoin.it/wiki/BIP_0350
[3] @scure/base v2.0.0 genBech32().decode() and decodeUnsafe implementation https://app.unpkg.com/@scure/base@2.0.0/files/index.ts
Remove .toLowerCase() to enforce strict bech32m format validation.
BIP-173 and BIP-350 require decoders to reject mixed-case bech32/bech32m strings, accepting only all-lowercase or all-uppercase inputs. By lowercasing the input before decoding, the code bypasses this validation check built into @scure/base's bech32m.decodeUnsafe. For a bearer token, enforce strict format compliance by passing the trimmed string as-is and add a regression test for mixed-case rejection.
🧩 Suggested change
- const decoded = bech32m.decodeUnsafe(
- encoded.trim().toLowerCase(),
- 1023
- );
+ const decoded = bech32m.decodeUnsafe(encoded.trim(), 1023);📝 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.
| const decoded = bech32m.decodeUnsafe( | |
| encoded.trim().toLowerCase(), | |
| 1023 | |
| ); | |
| const decoded = bech32m.decodeUnsafe(encoded.trim(), 1023); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/arkcash/index.ts` around lines 60 - 63, The call to bech32m.decodeUnsafe
in src/arkcash/index.ts currently lowercases the input
(encoded.trim().toLowerCase()), which disables strict mixed-case rejection;
change it to pass the trimmed string as-is (encoded.trim()) into
bech32m.decodeUnsafe so the library enforces BIP-173/BIP-350 validation, update
any surrounding validation logic that assumes lowercasing (e.g., where decoded
is used), and add a regression test that supplies a mixed-case bech32m string to
verify it is rejected.
| const cashWallet = await Wallet.create({ | ||
| identity: cashIdentity, | ||
| arkProvider: this.arkProvider, | ||
| indexerProvider: this.indexerProvider, | ||
| onchainProvider: this.onchainProvider, | ||
| storage: { | ||
| walletRepository: this.walletRepository, | ||
| contractRepository: this.contractRepository, | ||
| }, | ||
| }); | ||
|
|
||
| try { | ||
| await cashWallet.send({ | ||
| address: myAddress, | ||
| amount: sweptAmount, | ||
| }); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed to sweep arkcash VTXOs — they may have been claimed by another party: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid Wallet.create() for the ArkCash sweep.
This rebuilds the contract from current getInfo() values, so a token minted before a signer/timelock rotation can stop matching the temporary wallet, and the temp wallet is never disposed even though Wallet.create() eagerly starts a VtxoManager. Sweep against cash.vtxoScript directly, or at least disable settlement and dispose the temp wallet in finally.
🩹 Minimum mitigation
const cashWallet = await Wallet.create({
identity: cashIdentity,
arkProvider: this.arkProvider,
indexerProvider: this.indexerProvider,
onchainProvider: this.onchainProvider,
+ settlementConfig: false,
storage: {
walletRepository: this.walletRepository,
contractRepository: this.contractRepository,
},
});
try {
await cashWallet.send({
address: myAddress,
amount: sweptAmount,
});
} catch (error) {
throw new Error(
`Failed to sweep arkcash VTXOs — they may have been claimed by another party: ${error instanceof Error ? error.message : String(error)}`
);
+ } finally {
+ await cashWallet.dispose();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.ts` around lines 2037 - 2057, The temporary
Wallet.create(...) used for sweeping ArkCash should not be used because it
rebuilds the contract and eagerly starts a VtxoManager; instead perform the
sweep against the existing cash.vtxoScript (i.e., use the cash contract’s
vtxoScript as the UTXO selector/witness for the send/sweep operation) OR if you
must construct a temporary wallet, create it with settlement disabled (pass
settlementConfig: false / or the equivalent option that prevents new VtxoManager
creation) and ensure you call wallet.dispose() in a finally block; update the
try/catch around the sweep to still surface the original error message but
guarantee disposal when a temp wallet was created.
| // Charlie tries to claim the same cash — should fail (no VTXOs left) | ||
| await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow( | ||
| "No VTXOs found" | ||
| ); |
There was a problem hiding this comment.
Allow both double-claim failure modes.
After Bob claims, Charlie can lose either before the indexer drops the original VTXOs or during the wrapped sweep race. Asserting only "No VTXOs found" makes this test flaky.
🧪 Suggested assertion
- await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow(
- "No VTXOs found"
- );
+ await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow(
+ /(No VTXOs found|Failed to sweep arkcash VTXOs)/
+ );📝 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.
| // Charlie tries to claim the same cash — should fail (no VTXOs left) | |
| await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow( | |
| "No VTXOs found" | |
| ); | |
| // Charlie tries to claim the same cash — should fail (no VTXOs left) | |
| await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow( | |
| /(No VTXOs found|Failed to sweep arkcash VTXOs)/ | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/ark.test.ts` around lines 2376 - 2379, Change the brittle
exact-error assertion on charlie.wallet.claimCash(cashStr) to accept both
failure modes by matching either "No VTXOs found" or the sweep/race failure
message; update the expect(...).rejects.toThrow to use a regex (e.g. /No VTXOs
found|sweep/i or similar) so the test passes whether Charlie fails due to
missing VTXOs or due to the wrapped sweep race.
Bearer instrument for Ark protocol - encodes a private key + server pubkey + CSV timelock as a bech32m string (arkcash1...), enabling wallet-to-wallet transfers without address exchange. Includes 13 unit tests covering encode/decode roundtrips, key derivation, vtxo script generation, and validation.
- createCash(amount): generates a fresh keypair, sends VTXOs to a DefaultVtxo controlled by the new key, returns arkcash1... string - claimCash(str): parses arkcash, queries VTXOs, sweeps spendable ones offchain or imports as contract for subdust/unswept VTXOs - Exports ArkCash from package index
Tests cover: - Happy path: Alice creates cash, Bob claims without address exchange - Double-claim protection: second claim fails with "No VTXOs found" - Multiple tokens: create and claim multiple arkcash in sequence - Encoding roundtrip: encode/decode preserves all fields
- Use sendBitcoin with selectedVtxos to drain exact coins, avoiding dust change stranded in the ephemeral cash wallet - Wrap sweep errors with descriptive message for race conditions - Add JSDoc note about imported contract private key retention - Replace require() with ESM imports in e2e tests
sendBitcoin with selectedVtxos requires ExtendedVirtualCoin objects with tap tree data, but the indexer returns raw Vtxo objects. Using send() lets the cash wallet do its own coin selection via getVtxos() which properly hydrates the tap tree data.
Use spendableOnly: true so already-spent VTXOs (from a prior claim) are excluded. This ensures claimCash correctly throws "No VTXOs found" when the arkcash has already been swept by another party.
🔍 Arkana Review — ts-sdk#337 (updated)ArkCash bearer instruments for address-free transfers Re-review after new commits (rebased with sweep fixes). SummaryIntroduces Security Review
✅ Race condition handling:
✅ No key material leakage paths:
Protocol Correctness✅ Encoding format (69 bytes):
✅ HRP derivation:
✅ VTXO script construction:
Observations
Test Coverage ✅
Clean implementation. The bearer instrument model is sound and the sweep-or-import strategy handles edge cases well. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/arkcash/index.ts (1)
60-63:⚠️ Potential issue | 🟡 MinorPreserve strict bech32m case validation by removing lowercasing.
At Line [61], forcing lowercase before
decodeUnsafecan let mixed-case inputs slip past strict format checks.🛠️ Suggested fix
- const decoded = bech32m.decodeUnsafe( - encoded.trim().toLowerCase(), - 1023 - ); + const decoded = bech32m.decodeUnsafe(encoded.trim(), 1023);Does `@scure/base` bech32m.decodeUnsafe reject mixed-case bech32m strings when the original (non-lowercased) input is passed directly?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/arkcash/index.ts` around lines 60 - 63, The code currently calls bech32m.decodeUnsafe with encoded.trim().toLowerCase(), which defeats the library's strict mixed-case validation; change the call to pass the trimmed original input (use encoded.trim() without .toLowerCase()) so bech32m.decodeUnsafe can properly reject mixed-case bech32m strings (adjust references: decoded = bech32m.decodeUnsafe(...), input variable encoded).src/wallet/wallet.ts (1)
2037-2057:⚠️ Potential issue | 🟠 MajorDispose the temporary cash wallet and disable settlement for it.
At Line [2037], the temporary wallet created for sweep is never disposed, and it uses default settlement behavior. This can leave unnecessary background manager state alive and make the claim path brittle.
🩹 Minimum mitigation
const cashWallet = await Wallet.create({ identity: cashIdentity, arkProvider: this.arkProvider, indexerProvider: this.indexerProvider, onchainProvider: this.onchainProvider, + settlementConfig: false, storage: { walletRepository: this.walletRepository, contractRepository: this.contractRepository, }, }); try { await cashWallet.send({ address: myAddress, amount: sweptAmount, }); } catch (error) { throw new Error( `Failed to sweep arkcash VTXOs — they may have been claimed by another party: ${error instanceof Error ? error.message : String(error)}` ); + } finally { + await cashWallet.dispose(); }Based on learnings,
new VtxoManager(wallet)is intentionally default-enabled unlesssettlementConfig: falseis passed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 2037 - 2057, The temporary cash wallet created via Wallet.create (cashWallet) is left running with default settlement enabled; update the create call to disable automatic settlement by passing settlementConfig: false in the options, and ensure the wallet is always torn down by wrapping the send in a try/finally and calling await cashWallet.dispose() (or the wallet’s proper teardown method) so the temporary VtxoManager/background state is cleaned up even on errors; adjust around the send() call and catch block to use try { await cashWallet.send(...) } finally { await cashWallet.dispose() } to guarantee disposal.test/e2e/ark.test.ts (1)
2376-2379:⚠️ Potential issue | 🟡 MinorMake the double-claim assertion robust to both valid failure modes.
At Line [2377], asserting only
"No VTXOs found"is flaky; race timing can also surface the wrapped sweep error.🧪 Suggested assertion
- await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow( - "No VTXOs found" - ); + await expect(charlie.wallet.claimCash(cashStr)).rejects.toThrow( + /(No VTXOs found|Failed to sweep arkcash VTXOs)/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/ark.test.ts` around lines 2376 - 2379, The test's single-string assertion on charlie.wallet.claimCash is flaky because the failure can be either "No VTXOs found" or the wrapped sweep error; update the expectation to accept both valid failure modes by asserting the thrown error message matches either "No VTXOs found" or the sweep/failure message (e.g., via a regex or checking error.message includes one of the two strings) when calling charlie.wallet.claimCash in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/arkcash/index.ts`:
- Around line 60-63: The code currently calls bech32m.decodeUnsafe with
encoded.trim().toLowerCase(), which defeats the library's strict mixed-case
validation; change the call to pass the trimmed original input (use
encoded.trim() without .toLowerCase()) so bech32m.decodeUnsafe can properly
reject mixed-case bech32m strings (adjust references: decoded =
bech32m.decodeUnsafe(...), input variable encoded).
In `@src/wallet/wallet.ts`:
- Around line 2037-2057: The temporary cash wallet created via Wallet.create
(cashWallet) is left running with default settlement enabled; update the create
call to disable automatic settlement by passing settlementConfig: false in the
options, and ensure the wallet is always torn down by wrapping the send in a
try/finally and calling await cashWallet.dispose() (or the wallet’s proper
teardown method) so the temporary VtxoManager/background state is cleaned up
even on errors; adjust around the send() call and catch block to use try { await
cashWallet.send(...) } finally { await cashWallet.dispose() } to guarantee
disposal.
In `@test/e2e/ark.test.ts`:
- Around line 2376-2379: The test's single-string assertion on
charlie.wallet.claimCash is flaky because the failure can be either "No VTXOs
found" or the wrapped sweep error; update the expectation to accept both valid
failure modes by asserting the thrown error message matches either "No VTXOs
found" or the sweep/failure message (e.g., via a regex or checking error.message
includes one of the two strings) when calling charlie.wallet.claimCash in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5a867d3-a57a-46fe-a998-641d53fff5c4
📒 Files selected for processing (5)
src/arkcash/index.tssrc/index.tssrc/wallet/wallet.tstest/arkcash.test.tstest/e2e/ark.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
1 participant
Summary
Replaces the server-issued ArkNote model with user-generated bearer instruments. Users can now send value to each other without the receiver sharing an address first.
ArkCashclass (src/arkcash/index.ts): Standalone utility that encodes a private key + server pubkey + CSV timelock as a bech32m string (arkcash1...). Follows the same patterns asArkAddress(bech32m) andArkNote(encoding class).wallet.createCash(amount): Generates a fresh keypair, sends VTXOs to a standard DefaultVtxo controlled by the new key, returns the encoded arkcash string.wallet.claimCash(str): Parses the arkcash string, queries VTXOs, and uses a tiered claim strategy:Encoding format
Example usage
Test plan
Summary by CodeRabbit
New Features
Tests