Repository navigation
fix: remove remains of merged old hacks - #217
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe hierarchical deterministic address provider recycle test now uses the default dictionary comparer for ChangesRecycle test setup
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to These test setup edits preserve the relevant equality behavior and align the provider call with its constructor; no actionable merge risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — PR #217
Test-only cleanup; no production code changes. No issues found.
Change 1 — ECXOnlyPubKeyComparer.Instance → bare new Dictionary<ECXOnlyPubKey, long>()
HierarchicalDeterministicAddressProviderRecycleTests.cs:48
Correct. The custom comparer was a workaround for NBitcoin.Secp256k1 < 4.0.1 where ECXOnlyPubKey used reference equality. NArk.Core.csproj now pins NBitcoin.Secp256k1 at exactly 4.0.1, and ECXOnlyPubKeyValueEqualityTests.cs documents and tests the value-equality contract this drop depends on — including a Dictionary_without_a_comparer_looks_up_the_same_key case shaped exactly like DeprecatedSigners. The drop in the test mirrors what was already done in production code on an earlier commit. The version pin is the load-bearing guard; it's present and correct.
Change 2 — sweepDestination: null removed
HierarchicalDeterministicAddressProviderRecycleTests.cs:109
Correct. The HierarchicalDeterministicAddressProvider constructor at this head takes exactly 6 parameters (transport, safetyService, walletStorage, contractStorage, wallet, network) with no sweepDestination. The named argument was a stale residue from a previous interface that was merged via a different PR.
Danger warning — "Source changed with no test changes"
False positive. The only file changed in this PR is a test file. The protocol code these tests exercise (TryGetRecyclableDescriptor, the recycling logic for SendToSelf) was landed in #174 with its own tests. No new production paths are introduced here.
Test coverage
The three tests in scope (Renewal_DoesNotDropPublishedReceiveAddress_FromActiveScripts, Renewal_StillRecyclesSpentChangeAddress, Renewal_DoesNotRecycleInvoiceScript) continue to cover the critical recycling branches — Active-guarded skip, invoice-carve-out, and change-recycling. No regression in coverage.
LGTM on this cleanup. No correctness, security, or protocol concerns.
…t tests arkade-os#214 made LockupFateReader call a lockup Returned only when the spend carried a witness to inspect; a spend with none reads as Unknown. The SpendOf helpers built a bare PSBT, so five tests stopped seeing a refund.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — PR #217
One new commit (88b07f6), test-only. Zero prior issues to close (last pass raised none). Nothing blocking.
Prior issues — closure accounting
No findings were raised in the previous pass. Nothing to close.
New commit — 88b07f6: fix helpers to carry a readable witness on refund-path spends
Files: ,
Change (identical in both files):
else psbt.Inputs[0].FinalScriptWitness = new WitScript(Op.GetPushOp(new byte[32]).ToBytes());Why this is correct. (the engine under ) sets its internal looked flag only when Candidates(input) yields at least one push. Candidates reads from GetArkFieldConditionWitness() first, then FinalScriptWitness. A bare PSBT (no witness fields at all) yields nothing → looked stays false → evidence = Unreadable → cannot count the spend → final result is Unknown instead of Returned. Adding the 32-byte zero push satisfies the witness-present contract and correctly routes the helper into the Absent branch, which earns a Returned verdict.
No false-positive risk. Matches only accepts a candidate whose SHA-256 equals the test's payment hash. In both test files the hash is SHA256(0x11 × 32); SHA256(0x00 × 32) is a distinct value. The zero-push cannot accidentally look like a preimage.
Asymmetry between claim and refund helpers is intentional. Claim calls SetArkFieldConditionWitness (the Ark-specific PSBT field, checked first in Candidates). Refund uses FinalScriptWitness (the standard witness stack, checked second). Both paths are covered by Candidates; using either is correct and this mirrors how externally-built refund transactions would arrive in production.
Danger warning note. The "source changed with no test changes" warning does not apply to this commit — 88b07f6 is test-only. The warning was already present from earlier PR commits; it does not represent a regression introduced here.
No issues with this change. Clean test fix.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — PR #217
One new commit (88b07f6), test-only. Zero prior issues to close (last pass raised none). Nothing blocking.
Prior issues — closure accounting
No findings were raised in the previous pass. Nothing to close.
New commit — 88b07f6: fix SpendOf helpers to carry a readable witness on refund-path spends
Files: NArk.Tests/ArkadeIntents/ArkadeIntentsReconciliationTests.cs:442, NArk.Tests/ArkadeIntents/ArkadeSwapIntentMonitoringServiceTests.cs:172
Change (identical in both files):
else psbt.Inputs[0].FinalScriptWitness = new WitScript(Op.GetPushOp(new byte[32]).ToBytes());Why this is correct. SwapPreimageReader.LookupAsync (the engine under LockupFateReader) sets its internal looked flag only when Candidates(input) yields at least one push. Candidates reads from GetArkFieldConditionWitness() first, then FinalScriptWitness. A bare PSBT (no witness fields at all) yields nothing → looked stays false → evidence = Unreadable → LockupFateReader cannot count the spend → final result is Unknown instead of Returned. Adding the 32-byte zero push satisfies the witness-present contract and correctly routes the helper into the Absent branch, which earns a Returned verdict.
No false-positive risk. Matches only accepts a candidate whose SHA-256 equals the test's payment hash. In both test files the hash is SHA256(0x11 × 32); SHA256(0x00 × 32) is a distinct value. The zero-push cannot accidentally look like a preimage.
Asymmetry between claim and refund helpers is intentional. Claim calls SetArkFieldConditionWitness (the Ark-specific PSBT field, checked first in Candidates). Refund uses FinalScriptWitness (the standard witness stack, checked second). Both paths are covered by Candidates; using either is correct and this mirrors how externally-built refund transactions would arrive in production.
Danger warning note. The "source changed with no test changes" warning does not apply to this commit — 88b07f6 is test-only. The warning was already present from earlier PR commits; it does not represent a regression introduced here.
No issues with this change. Clean test fix.
2 participants
Summary by CodeRabbit