Feat/arkcash#76
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Feat/arkcash — #76
Reviewer: Arkana (automated, aggressive review)
Verdict: Request Changes
⚠️ PROTOCOL-CRITICAL — This PR introduces a bearer instrument that embeds raw private keys. Any bug in serialization, parsing, or key handling means lost funds. Requires human sign-off before merge.
🔴 HIGH — Must fix
1. ArkCash holds ECPrivKey but does not implement IDisposable
NArk.Abstractions/ArkCash.cs:22 — ECPrivKey implements IDisposable (confirmed: your own test calls TestPrivKey.Dispose() in OneTimeTearDown). ArkCash holds a private key but never zeros it from memory. For a bearer instrument that IS a private key, this is a real concern.
Fix: Implement IDisposable, call PrivKey.Dispose() in Dispose().
2. Private key exposed as public field, not property
NArk.Abstractions/ArkCash.cs:22 — public readonly ECPrivKey PrivKey is a field, not a property. The rest of the class uses properties (Pubkey, ServerPubkey, LockTime). This is inconsistent and makes it harder to add access control later (e.g., logging access to private key material).
Fix: Change to public ECPrivKey PrivKey { get; } for consistency and future-proofing.
3. HRP hardcoded to mainnet/testnet only — breaks signet/mutinynet/other networks
NArk.Abstractions/ArkCash.cs:40-43 — Constructor rejects any HRP that isn't "arkcash" or "tarkcash". The ts-sdk reference implementation (PR arkade-os/ts-sdk#337) derives HRP dynamically from the network prefix (ark→arkcash, tark→tarkcash, nark→narkcash). This hardcoding will break on signet or any future network.
Fix: Either accept arbitrary HRPs (validate format, not exact match), or add all known variants and create/cache encoders dynamically.
4. Static encoder fields are not readonly
NArk.Abstractions/ArkCash.cs:18-19 — MainnetEncoder and TestnetEncoder are private static but not readonly. They're only assigned in the static constructor. Mark them static readonly to prevent accidental reassignment.
5. PayloadLength should be const
NArk.Abstractions/ArkCash.cs:12 — private static readonly int PayloadLength = 1 + 32 + 32 + 4 — this is a compile-time constant. Use const int PayloadLength = 69; for clarity and minor perf (inlined by compiler).
🟡 MEDIUM — Should fix
6. CannotDoubleSpendArkCash test doesn't actually attempt a double spend
NArk.Tests.End2End/ArkCashTests.cs:96-181 — Despite the name, this test only verifies VTXOs are marked spent after the first claim. It never creates a second wallet that tries to claim the same ArkCash. A proper double-spend test should:
- First wallet claims → succeeds
- Second wallet imports same ArkCash contract → attempts claim → verify it fails or finds no unspent VTXOs to sweep
The current assertions (lines 172-180) prove post-conditions of a single claim, not double-spend resistance.
7. Generate throws misleading ArgumentNullException
NArk.Abstractions/ArkCash.cs:55 — throw new ArgumentNullException(nameof(key)) when ECPrivKey.TryCreate fails. This isn't a null-argument problem — it's a key generation failure. Use InvalidOperationException("Failed to generate private key") or CryptographicException.
8. Unused import Google.Api
NArk.Tests/ArkCashTests.cs:1 — using Google.Api; is unused. Remove it.
9. No explicit key-length validation in constructor
NArk.Abstractions/ArkCash.cs:38-48 — The ts-sdk reference implementation validates privateKey.length === 32 and serverPubKey.length === 32 with clear error messages. The dotnet version relies on NBitcoin's internal validation, which may produce cryptic errors. Add explicit checks for defense-in-depth, especially since this is a parsing boundary for untrusted input.
🔵 LOW / Nits
10. Missing newline at end of all 4 files — All new files lack a trailing newline. Most .NET style guides and git diff prefer a final newline.
11. Extra blank lines — ArkCash.cs:24 (blank line after LockTime), ArkCash.cs:103 (blank line before return in Parse). Minor, but clean them up.
✅ What looks good
- Payload format matches ts-sdk spec exactly: version(1) + privkey(32) + server_pubkey(32) + csv(4) = 69 bytes, big-endian uint32 for locktime, bech32m encoding. ✅
TryParse/Parsepattern is idiomatic C#. ✅- Unit tests cover roundtrip, identity, address generation, invalid input rejection. Good coverage for the core type. ✅
- E2E roundtrip test exercises the full flow from generation → funding → import → batch claim. ✅
ToContractextension correctly bridges to existingArkPaymentContractviaParseOutputDescriptor. ✅stackallocfor payload inToString()— good, avoids heap allocation for sensitive key material. ✅
Summary
The core serialization logic is correct and matches the ts-sdk reference spec. The main concerns are:
- Missing
IDisposable— private key material stays in memory indefinitely - Hardcoded HRPs — will break on non-mainnet/testnet networks
- Double-spend test is incomplete — doesn't actually test the double-spend scenario
Fix items 1-5, strengthen the E2E test, and this is ready for human sign-off.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Code Review: Feat/arkcash — #76
Reviewer: Arkana (automated, aggressive review)
Verdict: Request Changes
⚠️ PROTOCOL-CRITICAL — This PR introduces a bearer instrument that embeds raw private keys. Any bug in serialization, parsing, or key handling means lost funds. Requires human sign-off before merge.
🔴 HIGH — Must fix
1. ArkCash holds ECPrivKey but does not implement IDisposable
NArk.Abstractions/ArkCash.cs:22 — ECPrivKey implements IDisposable (confirmed: your own test calls TestPrivKey.Dispose() in OneTimeTearDown). ArkCash holds a private key but never zeros it from memory. For a bearer instrument that IS a private key, this is a real concern.
Fix: Implement IDisposable, call PrivKey.Dispose() in Dispose().
2. Private key exposed as public field, not property
NArk.Abstractions/ArkCash.cs:22 — public readonly ECPrivKey PrivKey is a field, not a property. The rest of the class uses properties (Pubkey, ServerPubkey, LockTime). This is inconsistent and makes it harder to add access control later (e.g., logging access to private key material).
Fix: Change to public ECPrivKey PrivKey { get; } for consistency and future-proofing.
3. HRP hardcoded to mainnet/testnet only — breaks signet/mutinynet/other networks
NArk.Abstractions/ArkCash.cs:40-43 — Constructor rejects any HRP that isn't "arkcash" or "tarkcash". The ts-sdk reference implementation (PR arkade-os/ts-sdk#337) derives HRP dynamically from the network prefix (ark→arkcash, tark→tarkcash, nark→narkcash). This hardcoding will break on signet or any future network.
Fix: Either accept arbitrary HRPs (validate format, not exact match), or add all known variants and create/cache encoders dynamically.
4. Static encoder fields are not readonly
NArk.Abstractions/ArkCash.cs:18-19 — MainnetEncoder and TestnetEncoder are private static but not readonly. They're only assigned in the static constructor. Mark them static readonly to prevent accidental reassignment.
5. PayloadLength should be const
NArk.Abstractions/ArkCash.cs:12 — private static readonly int PayloadLength = 1 + 32 + 32 + 4 — this is a compile-time constant. Use const int PayloadLength = 69; for clarity and minor perf (inlined by compiler).
🟡 MEDIUM — Should fix
6. CannotDoubleSpendArkCash test doesn't actually attempt a double spend
NArk.Tests.End2End/ArkCashTests.cs:96-181 — Despite the name, this test only verifies VTXOs are marked spent after the first claim. It never creates a second wallet that tries to claim the same ArkCash. A proper double-spend test should:
- First wallet claims → succeeds
- Second wallet imports same ArkCash contract → attempts claim → verify it fails or finds no unspent VTXOs to sweep
The current assertions (lines 172-180) prove post-conditions of a single claim, not double-spend resistance.
7. Generate throws misleading ArgumentNullException
NArk.Abstractions/ArkCash.cs:55 — throw new ArgumentNullException(nameof(key)) when ECPrivKey.TryCreate fails. This isn't a null-argument problem — it's a key generation failure. Use InvalidOperationException("Failed to generate private key") or CryptographicException.
8. Unused import Google.Api
NArk.Tests/ArkCashTests.cs:1 — using Google.Api; is unused. Remove it.
9. No explicit key-length validation in constructor
NArk.Abstractions/ArkCash.cs:38-48 — The ts-sdk reference implementation validates privateKey.length === 32 and serverPubKey.length === 32 with clear error messages. The dotnet version relies on NBitcoin's internal validation, which may produce cryptic errors. Add explicit checks for defense-in-depth, especially since this is a parsing boundary for untrusted input.
🔵 LOW / Nits
10. Missing newline at end of all 4 files — All new files lack a trailing newline. Most .NET style guides and git diff prefer a final newline.
11. Extra blank lines — ArkCash.cs:24 (blank line after LockTime), ArkCash.cs:103 (blank line before return in Parse). Minor, but clean them up.
✅ What looks good
- Payload format matches ts-sdk spec exactly: version(1) + privkey(32) + server_pubkey(32) + csv(4) = 69 bytes, big-endian uint32 for locktime, bech32m encoding. ✅
TryParse/Parsepattern is idiomatic C#. ✅- Unit tests cover roundtrip, identity, address generation, invalid input rejection. Good coverage for the core type. ✅
- E2E roundtrip test exercises the full flow from generation → funding → import → batch claim. ✅
ToContractextension correctly bridges to existingArkPaymentContractviaParseOutputDescriptor. ✅stackallocfor payload inToString()— good, avoids heap allocation for sensitive key material. ✅
Summary
The core serialization logic is correct and matches the ts-sdk reference spec. The main concerns are:
- Missing
IDisposable— private key material stays in memory indefinitely - Hardcoded HRPs — will break on non-mainnet/testnet networks
- Double-spend test is incomplete — doesn't actually test the double-spend scenario
Fix items 1-5, strengthen the E2E test, and this is ready for human sign-off.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Re-review after rebase — #76
Reviewer: Arkana (iterative review)
The branch was rebased (a8c8d97 → 4ab7e6c4) but the diff is unchanged. None of the 11 findings from my previous review were addressed.
Still outstanding (unchanged):
🔴 HIGH — Must fix before merge:
- Missing
IDisposable—ArkCashholdsECPrivKey(disposable) but never disposes it. Private key material lingers in memory. (ArkCash.cs:22) PrivKeyis a field, not a property — inconsistent withPubkey,ServerPubkey,LockTime. (ArkCash.cs:22)- Hardcoded HRPs — only
arkcash/tarkcashaccepted; breaks signet/mutinynet. ts-sdk handles this dynamically. (ArkCash.cs:40-43) - Static encoders not
readonly(ArkCash.cs:18-19) PayloadLengthshould beconst(ArkCash.cs:12)
🟡 MEDIUM:
6. Double-spend test doesn't double-spend — only checks post-conditions of first claim (ArkCashTests.cs:96-181)
7. Misleading ArgumentNullException in Generate (ArkCash.cs:55)
8. Unused using Google.Api (NArk.Tests/ArkCashTests.cs:1)
9. No explicit key-length validation in constructor (ArkCash.cs:38-48)
🔵 LOW: Missing trailing newlines (all 4 files), extra blank lines.
⚠️ PROTOCOL-CRITICAL — bearer instrument with embedded private keys. Requires human sign-off even after fixes.
Please address findings 1-5 at minimum before next review round.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Follow-up Review (post 5d4e43c) — #76
Thanks for addressing the majority of the first review. IDisposable, properties, const/readonly, unused import, and the E2E race fix all look good. ✅
Remaining issues from the first round + one new finding:
🔴 HIGH — Security
1. Generate leaks private key bytes on the managed heap
ArkCash.cs:55 — var pk = RandomUtils.GetBytes(32); allocates 32 bytes of raw private key material on the managed heap. After ECPrivKey.TryCreate(pk, ...), pk is never zeroed. The GC may relocate or leave copies in memory indefinitely. For a bearer instrument where the private key IS the money, this matters.
Fix: Zero the buffer after use:
var pk = RandomUtils.GetBytes(32);
try
{
if (!ECPrivKey.TryCreate(pk, out var key))
throw new InvalidOperationException("Failed to generate private key");
// ...
}
finally
{
CryptographicOperations.ZeroMemory(pk);
}2. Generate throws ArgumentNullException — wrong exception type
ArkCash.cs:58 — throw new ArgumentNullException(nameof(key)) when TryCreate fails. This isn't a null-argument scenario. Use InvalidOperationException("Failed to generate private key") or CryptographicException.
🟡 MEDIUM — Still open
3. HRP hardcoded to mainnet/testnet — breaks signet
ArkCash.cs:41 — if (hrp is not ("tarkcash" or "arkcash")) still rejects signet (narkcash) and any future network. The ts-sdk reference (arkade-os/ts-sdk#337) supports dynamic HRPs. At minimum, add narkcash for signet parity, or create encoders dynamically for unknown HRPs.
4. Version should be const
ArkCash.cs:11 — private static readonly byte Version = 0x00; — this is a compile-time constant, same as PayloadLength. Use private const byte Version = 0x00;.
5. Double-spend test name is misleading
ArkCashTests.cs (E2E):96 — CannotDoubleSpendArkCash still only verifies VTXOs are marked spent after a single claim. It never creates a second wallet that attempts to claim the same ArkCash token. The test proves post-conditions, not double-spend resistance. Either rename to VtxosAreMarkedSpentAfterClaim or add an actual second-wallet claim attempt.
🔵 LOW
6. ArkCashExtensions.cs still missing trailing newline — The other files were fixed, this one was missed.
✅ Fixed since last review
- IDisposable ✅
- PrivKey/Hrp as properties ✅
- PayloadLength as const ✅
- Static encoders as readonly ✅
- Unused Google.Api import ✅
- E2E race condition (VtxosChanged → PollScriptsForVtxos) ✅
- Trailing newlines on most files ✅
Items 1-3 should be addressed before merge. 4-6 are nice-to-haves.
🤖 Reviewed by Arkana
1 participant
No description provided.