refactor: make wallet events atomic#116
Open
xdustinface wants to merge 23 commits into
Open
Conversation
* chore: add Manki AI code review configuration * fix: pass `claude_code_oauth_token` to Manki action
* feat: add session outcome fields to `PeerReputation` Adds `last_success`, `last_tried`, and `consecutive_failures` to `PeerReputation`, plus a `record_connection_failure` method on `PeerReputationManager`. All new fields use `#[serde(default)]` so existing `reputations.json` files load without migration. `record_connection_attempt` now sets `last_tried` and `record_successful_connection` sets `last_success` and resets `consecutive_failures`. * test: cover `PeerReputation` session outcome transitions Adds unit tests for default values, `last_tried` on attempt, `last_success` plus `consecutive_failures` reset on success, failure streak increment preserving `last_success`, and legacy `reputations.json` decoding with missing fields. * feat: bump `AddrV2.time` on successful handshake Adds `AddrV2Handler::mark_seen` to refresh the stored timestamp for a directly observed peer, preserving existing services for known entries and inserting a fresh entry otherwise. `connect_to_peer` now calls `mark_seen` after a successful handshake so the `peers.dat` time reflects first-hand observation instead of gossip. * feat: track peer connection outcomes in network manager Calls `record_connection_failure` on both the TCP connect failure and the handshake failure paths in `PeerNetworkManager::connect_to_peer`, so the `consecutive_failures` streak reflects every unsuccessful attempt. * refactor: drop backward-compat shims from `PeerReputation` Removes `#[serde(default)]` on the new session outcome fields and the legacy-JSON load test. Backward compatibility of `reputations.json` across versions is no longer a requirement, so the shims and test are dead weight. * refactor: thread peer-advertised services into `mark_seen` * docs: clarify `last_connection` vs `last_tried` on `PeerReputation` * refactor: extract `make_addr_message` helper in `addrv2` * refactor: defensively set `last_tried` in `record_connection_failure` * refactor: add atomic `record_failure_with_penalty` and use at failure sites * chore: apply `cargo fmt` * refactor: clamp `consecutive_failures` on deserialization * refactor: extract `apply_score_change` and always update `last_tried` on failure * test: cover `record_failure_with_penalty` directly * refactor: clamp \`consecutive_failures\` at runtime and consolidate failure-field updates Extract private `record_failure_fields` that applies `last_tried = now` and `consecutive_failures.saturating_add(1).min(MAX_CONSECUTIVE_FAILURES)`. Both `record_connection_failure` and `record_failure_with_penalty` now delegate to it, eliminating duplicated mutations and capping the in-memory streak at the same 1000 limit enforced by the deserializer. * test: cover \`consecutive_failures\` clamp, \`last_success\` preservation, \`mark_seen\` eviction - Assert `last_success` is unchanged after `record_failure_with_penalty` - Deserialize a `PeerReputation` with `consecutive_failures: 99999` and assert it clamps to `MAX_CONSECUTIVE_FAILURES` - Fill `AddrV2Handler` to capacity and assert `mark_seen` stays bounded and includes the new entry * refactor: remove unused `record_connection_failure` * test: stabilize eviction test and cover runtime `consecutive_failures` saturation * refactor: document and assert non-negative contract on `record_failure_with_penalty` * test: cover `last_tried` preservation on success and update on failure * fix: clamp negative `score_change` in `record_failure_with_penalty` * test: cover happy-path attempt to success lifecycle * refactor: tighten `clamp_future_system_time` bounds and enforce load invariants Add a 30-day lower bound to `clamp_future_system_time` so stale or corrupted timestamps (including epoch 0) are discarded on load, in addition to future ones. Add `PeerReputation::normalize_after_load` and call it from the storage load path. It resets `consecutive_failures` to 0 whenever `last_tried` is `None`, preventing the inconsistent state where a non-zero failure streak has no temporal anchor. * test: cover `clamp_future_system_time` edge cases Add three tests: future timestamp rejected, epoch-zero rejected (exercising the new lower bound), and recent-past timestamp preserved. * docs: clarify zero-`score_change` contract on `record_failure_with_penalty` A value of 0 is a deliberate no-op for the reputation score but still records the failure counter and timestamp, which is useful for failures that should be tracked without contributing toward a ban. * fix: use `checked_sub` in `clamp_future_system_time` to avoid panic on broken clocks * test: cover `normalize_after_load` via storage round-trip * fix: use `checked_add` in `clamp_future_system_time` to avoid panic on far-future clocks * test: also cover stale-timestamp path in `normalize_after_load` round-trip * refactor: drop 30-day stale-timestamp floor from `clamp_future_system_time` Remove `TIMESTAMP_MAX_AGE` and the `floor` computation that rejected timestamps older than 30 days. The future-timestamp guard (10-second tolerance) is the only meaningful constraint. Update the `normalize_after_load` doc comment to drop the "stale" reference. * test: remove obsolete stale-timestamp tests and consolidate `clamp_future_system_time` coverage Delete `test_normalize_after_load_via_storage_round_trip_stale` (tested the removed stale-floor path). Merge the three `test_clamp_future_system_time_*` tests into a single `test_clamp_future_system_time` covering future rejection and recent-past acceptance. * fix: \`mark_seen\` now overwrites \`services\` on existing entries Since round 1 the caller passes the actual handshake-negotiated services, so preserving the gossip-sourced value was inverted. The handshake-observed value is authoritative and is now written on both new and existing entries. Rename \`test_mark_seen_bumps_time_and_preserves_services\` to \`test_mark_seen_bumps_time_and_updates_services\` and update its assertion to expect the handshake services, not the original gossip services. * test: cover positive \`normalize_after_load\` branch Add \`test_normalize_after_load_preserves_failures_when_last_tried_valid\` to assert that a valid (non-future) \`last_tried\` and a non-zero \`consecutive_failures\` are both preserved through the load round-trip, complementing the existing reset-path test. * refactor: move `record_failure_fields` to `impl PeerReputation` * refactor: move `apply_score_change` to `impl PeerReputation`
|
Manki — Review complete Planner (17s) Review — 12 findings Judge — 12 kept · 0 dropped (30s) Review metadataConfig:
Judge decisions:
Timing:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Atomic wallet event refactor is solid, but the FFI surface has gaps: a missing null-check on a callback txid pointer, silent CString/u32 truncation on derivation paths and update counts, and test coverage that logs but doesn't assert key boundaries (mempool=0 during sync, SyncedHeightUpdated firing).
📊 12 findings (2 warning, 3 suggestion, 7 nitpick) · 1923 lines · 611s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 611210,
"diffLines": 1923,
"diffAdditions": 1018,
"diffDeletions": 905,
"filesReviewed": 16,
"agents": [
"Correctness & Logic",
"Security & Safety",
"Testing & Coverage"
],
"findingsRaw": 12,
"findingsKept": 12,
"findingsDropped": 0,
"severity": {
"blocker": 0,
"warning": 2,
"suggestion": 3,
"nitpick": 7
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "1f20837af475544fe18f1df92aa136724496cbe3",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"responseLength": 1388
},
{
"name": "Security & Safety",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 4387
},
{
"name": "Testing & Coverage",
"findingsRaw": 7,
"findingsKept": 7,
"responseLength": 7085
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 8,
"medium": 4,
"low": 0
},
"severityChanges": 12,
"mergedDuplicates": 0,
"defensiveHardeningCount": 3,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/src/bin/ffi_cli.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 4,
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 3,
"key-wallet-manager/src/event_tests.rs": 2,
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 2
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}* refactor(key-wallet-manager): track wallet heights per wallet Move committed sync height into wallet metadata and expose it through `WalletInfoInterface` so `WalletManager` no longer relies on global cached heights. Compute manager-level synced_height and tip_height as the minimum across wallet infos, propagate committed sync updates per wallet, and keep tip_height as per-wallet applied chain state. Update tests and example messaging to match the new aggregation semantics. * enforce `Send + Sync + 'static` on generic type of the wallet manager
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
`test_callbacks_post_sync_transactions_and_disconnect` mined a block immediately after `send_to_address`, so the SPV node could process the transaction via the block path before observing it in the mempool. The later `wait_for_callback` for `mempool_transaction_received_count` would then time out and the assertion that the mempool callback fired would fail. Wait for the mempool callback before calling `generate_blocks` so the mempool path is exercised deterministically. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
`mempool_received_count` was loaded and logged in `test_all_callbacks_during_sync` but never asserted. During initial block-chain replay, `MempoolTransactionReceived` must not fire for historical transactions (they arrive via `BlockProcessChange`). Without the assertion, a regression that fired the mempool callback for every confirmed transaction would pass undetected. Add the explicit zero assertion to enforce the cross-path boundary. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
`synced_height_updated_count` and `last_synced_height` were tracked but never asserted in `test_all_callbacks_during_sync`. `SyncedHeightUpdated` is the atomicity boundary consumers use to safely flush their persistence layer after a filter batch — a regression that silenced the callback would let consumers stop checkpointing and the test would still pass. Assert the counter advances and the reported height matches `dashd.initial_height` so the callback contract is enforced. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
Reshape `WalletEvent` so each variant carries the records and post-change balance needed to persist a wallet update atomically off a single event. Replaces `TransactionReceived` / `TransactionStatusChanged` / `BalanceUpdated` with `MempoolTransactionReceived`, `TransactionInstantSendLocked`, and `BlockProcessChange`, adds `RecordAction` and `BlockRecordUpdate` for per-record context, and identifies accounts by `DerivationPath` instead of `account_index: u32`.
`test_callbacks_post_sync_transactions_and_disconnect` mined a block immediately after `send_to_address`, so the SPV node could process the transaction via the block path before observing it in the mempool. The later `wait_for_callback` for `mempool_transaction_received_count` would then time out and the assertion that the mempool callback fired would fail. Wait for the mempool callback before calling `generate_blocks` so the mempool path is exercised deterministically. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
`mempool_received_count` was loaded and logged in `test_all_callbacks_during_sync` but never asserted. During initial block-chain replay, `MempoolTransactionReceived` must not fire for historical transactions (they arrive via `BlockProcessChange`). Without the assertion, a regression that fired the mempool callback for every confirmed transaction would pass undetected. Add the explicit zero assertion to enforce the cross-path boundary. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
`synced_height_updated_count` and `last_synced_height` were tracked but never asserted in `test_all_callbacks_during_sync`. `SyncedHeightUpdated` is the atomicity boundary consumers use to safely flush their persistence layer after a filter batch — a regression that silenced the callback would let consumers stop checkpointing and the test would still pass. Assert the counter advances and the reported height matches `dashd.initial_height` so the callback contract is enforced. Addresses manki-review review comment on PR [#116](#116): #116 (comment)
xdustinface
force-pushed
the
refactor/wallet-events-atomic
branch
from
cf668fd to
ebf8d81
Compare
|
Manki — Review complete Planner (17s) Review — 26 findings Judge — 19 kept · 0 dropped (108s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Atomicity refactor lands cleanly but most reviewer noise is duplicates around the same three FFI dispatch lines (redundant drops, u32-truncation cast, CString::new fallbacks). Real concerns: removed lifecycle unit tests have no deterministic replacement, and the spendable→confirmed semantic shift in FFIBalance can silently inflate displayed balances for downstream consumers.
📊 19 findings (6 warning, 6 suggestion, 7 nitpick) · 1959 lines · 480s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 479677,
"diffLines": 1959,
"diffAdditions": 1048,
"diffDeletions": 911,
"filesReviewed": 16,
"agents": [
"Correctness & Logic",
"Security & Safety",
"Architecture & Design",
"Testing & Coverage",
"Dependencies & Integration"
],
"findingsRaw": 26,
"findingsKept": 19,
"findingsDropped": 7,
"severity": {
"blocker": 0,
"warning": 6,
"suggestion": 6,
"nitpick": 7
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "ebf8d81464456177490a614e32928f7c50fc8d3c",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4069
},
{
"name": "Security & Safety",
"findingsRaw": 5,
"findingsKept": 3,
"responseLength": 6458
},
{
"name": "Architecture & Design",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 5564
},
{
"name": "Testing & Coverage",
"findingsRaw": 6,
"findingsKept": 6,
"responseLength": 6559
},
{
"name": "Dependencies & Integration",
"findingsRaw": 6,
"findingsKept": 3,
"responseLength": 5621
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 8,
"medium": 9,
"low": 2
},
"severityChanges": 19,
"mergedDuplicates": 7,
"defensiveHardeningCount": 3,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/src/bin/ffi_cli.rs": 1,
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 3,
"dash-spv-ffi/src/callbacks.rs": 9,
"key-wallet-manager/src/event_tests.rs": 2,
"key-wallet-manager/src/events.rs": 1,
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 3
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}The per-wallet semantics from #117 keep `synced_height` and `last_processed_height` independent. The rebase reintroduced a manager-level bump that mirrored the trait default, contradicting the test and per-wallet model.
|
Manki — Review complete Planner (24s) Review — 34 findings Judge — 18 kept · 0 dropped (102s) Review metadataConfig:
Judge decisions:
Timing:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #116 +/- ##
=============================================
- Coverage 70.50% 70.26% -0.25%
=============================================
Files 319 319
Lines 66758 66832 +74
=============================================
- Hits 47071 46960 -111
- Misses 19687 19872 +185
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Major FFI refactor for atomic wallet events — new event shape solves the cross-event ordering hazard but introduces an ABI break, several FFI pointer-lifetime concerns, and significant test-coverage regressions. Lifecycle/duplicate-suppression unit tests were deleted without deterministic replacements (4 reviewers flagged related issues).
📊 18 findings (7 warning, 6 suggestion, 5 nitpick) · 1955 lines · 473s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 472776,
"diffLines": 1955,
"diffAdditions": 1044,
"diffDeletions": 911,
"filesReviewed": 16,
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design"
],
"findingsRaw": 34,
"findingsKept": 18,
"findingsDropped": 16,
"severity": {
"blocker": 0,
"warning": 7,
"suggestion": 6,
"nitpick": 5
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "3747aa8da50bb7ec9d429603a11d8b3636f038f9",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 14,
"findingsKept": 12,
"responseLength": 10896
},
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4996
},
{
"name": "Testing & Coverage",
"findingsRaw": 6,
"findingsKept": 5,
"responseLength": 6448
},
{
"name": "Architecture & Design",
"findingsRaw": 10,
"findingsKept": 8,
"responseLength": 10288
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 12,
"medium": 6,
"low": 0
},
"severityChanges": 18,
"mergedDuplicates": 16,
"defensiveHardeningCount": 1,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 2,
"dash-spv-ffi/src/callbacks.rs": 8,
"key-wallet-manager/src/event_tests.rs": 2,
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 3,
"dash-spv-ffi/src/bin/ffi_cli.rs": 2,
"dash-spv/tests/dashd_sync/helpers.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Empty `Vec::as_ptr()` returns a non-null, unspecified pointer. The C/Swift callback should receive `NULL` paired with `update_count = 0` to follow standard C-API conventions. Addresses manki review on PR #116 #116 (comment)
Splits the shared `received_transactions` bucket into per-callback buckets so a broken mempool callback can't be masked by the block path's success (or vice versa). Adds tracker fields and assertions for `FFIRecordAction` discriminants and BIP-32 account path strings, both of which were delivered by the new atomic events but never inspected in tests. Increments `block_process_change_count` inside the same lock as `block_process_change_record_count` so a test waiting on either counter is guaranteed to see a consistent view of the other. Adds an explicit `wait_for_callback` for `synced_height_updated` before reading `last_synced_height`, since that callback is dispatched separately from `BlockProcessChange` on the same broadcast channel. Addresses manki review on PR #116 #116 (comment) #116 (comment) #116 (comment) #116 (comment) #116 (comment)
The implementation currently downgrades a confirmed record's context from `InBlock(_)` back to `InstantSend(_)` when a late IS-lock arrives for an already-confirmed transaction, which re-emits `TransactionInstantSendLocked`. Capture that observable behavior so any future change to the record-context downgrade or event-suppression rules shows up as a deterministic test failure rather than a silent semantic drift. Addresses manki review on PR #116 #116 (comment)
If a test waits on `block_process_change_record_count` and then immediately reads `block_process_change_count`, the latter could read zero because it was incremented after the per-record loop. Reordering the increments guarantees that observing any record implies the per-callback counter is already bumped.
|
Manki — Review complete Planner (25s) Review — 23 findings Judge — 20 kept · 2 dropped (88s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Most prior threads remain open — the FFI ABI break, multi-account silent drop, and missing IS-lock positive test are still unresolved. New finding worth attention: the synced_height_updated store/fetch_add ordering is reversed, which can cause spurious test failures.
📊 20 findings (8 warning, 7 suggestion, 5 nitpick) · 2125 lines · 463s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 463119,
"diffLines": 2125,
"diffAdditions": 1198,
"diffDeletions": 927,
"filesReviewed": 16,
"agents": [
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"findingsRaw": 23,
"findingsKept": 20,
"findingsDropped": 3,
"severity": {
"blocker": 0,
"warning": 8,
"suggestion": 7,
"nitpick": 5
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "a3bfe37685387f00be28496d9f3eacf8ce622d85",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 4905
},
{
"name": "Testing & Coverage",
"findingsRaw": 5,
"findingsKept": 5,
"responseLength": 5982
},
{
"name": "Architecture & Design",
"findingsRaw": 7,
"findingsKept": 7,
"responseLength": 6649
},
{
"name": "Dependencies & Integration",
"findingsRaw": 6,
"findingsKept": 3,
"responseLength": 5094
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 17,
"medium": 5,
"low": 0
},
"severityChanges": 22,
"mergedDuplicates": 1,
"defensiveHardeningCount": 2,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 4,
"dash-spv-ffi/src/callbacks.rs": 7,
"dash-spv-ffi/src/bin/ffi_cli.rs": 1,
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 4,
"dash-spv-ffi/tests/dashd_sync/tests_transaction.rs": 1,
"dash-spv/tests/dashd_sync/helpers.rs": 1,
"key-wallet-manager/src/lib.rs": 1,
"key-wallet-manager/src/event_tests.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Store `last_synced_height` before bumping `synced_height_updated_count` and bump `block_process_change_count` after all per-record state is written so tests waiting on the per-callback counter observe consistent record state. Update test waits to use the per-callback counter. Capture IS lock bytes in `last_islock_bytes` so future tests can verify the payload round-trips through `InstantLock`. Slice `block_record_actions` and `block_account_paths` by the pre-sync index so post-sync assertions only check the new delivery window. Assert `last_unconfirmed > 0` after mempool receipt before the block-process callback overwrites it. Add `FFIRecordAction` and BIP-32 path assertions to `test_ffi_sync_then_generate_blocks`. Replace strict equality on `last_synced_height` with `>=` to tolerate concurrent block production.
Add `dropped_records_missing_path: u32` to `CheckTransactionsResult` and increment it whenever `AccountType::derivation_path` fails inside `check_transaction_in_all_wallets`. Callers and tests can assert this stays at zero to detect silent record loss for any account type whose `derivation_path` implementation is missing or buggy.
|
Manki — Review complete Planner (26s) Review — 17 findings Judge — 9 kept · 0 dropped (89s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Most prior thread concerns remain open — the breaking ABI change, MempoolTransactionReceived single-account-path design, and missing IS-lock positive-path test still stand. New findings center on FFI safety hardening in the CLI binary (non-ASCII slice panic, unguarded txid deref) and an architectural inconsistency where TransactionInstantSendLocked breaks the atomic-event contract by omitting the updated TransactionRecord.
📊 9 findings (1 warning, 1 suggestion, 7 nitpick) · 2204 lines · 443s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 442902,
"diffLines": 2204,
"diffAdditions": 1274,
"diffDeletions": 930,
"filesReviewed": 16,
"agents": [
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Security & Safety"
],
"findingsRaw": 17,
"findingsKept": 9,
"findingsDropped": 8,
"severity": {
"blocker": 0,
"warning": 1,
"suggestion": 1,
"nitpick": 7
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "51bf6e73014170cb5156eb61e6adc4ad3914a251",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 1,
"responseLength": 3338
},
{
"name": "Testing & Coverage",
"findingsRaw": 5,
"findingsKept": 1,
"responseLength": 5038
},
{
"name": "Architecture & Design",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 3548
},
{
"name": "Security & Safety",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 3667
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 8,
"medium": 1,
"low": 0
},
"severityChanges": 9,
"mergedDuplicates": 0,
"defensiveHardeningCount": 5,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/src/callbacks.rs": 4,
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 3,
"dash-spv-ffi/src/bin/ffi_cli.rs": 2
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}
|
Manki — Review complete Planner (29s) Review — 15 findings Judge — 7 kept · 0 dropped (98s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
The IS-lock callback still omits the updated TransactionRecord — three of four reviewers independently flagged this as breaking the PR's stated atomic-persistence goal, and the prior-review thread on the same issue is still unaddressed. Two new ordering races between counter increments and per-record state writes in the test tracker are real but narrow; the rest are minor.
📊 7 findings (2 warning, 2 suggestion, 3 nitpick) · 2199 lines · 564s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 564261,
"diffLines": 2199,
"diffAdditions": 1269,
"diffDeletions": 930,
"filesReviewed": 16,
"agents": [
"Correctness & Logic",
"Security & Safety",
"Testing & Coverage",
"Architecture & Design"
],
"findingsRaw": 15,
"findingsKept": 7,
"findingsDropped": 8,
"severity": {
"blocker": 0,
"warning": 2,
"suggestion": 2,
"nitpick": 3
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "a9b17e12ea76fe9bc4f32be38f7434543441a8bc",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 2,
"responseLength": 3209
},
{
"name": "Security & Safety",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 4096
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 2,
"responseLength": 5036
},
{
"name": "Architecture & Design",
"findingsRaw": 4,
"findingsKept": 3,
"responseLength": 5181
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 6,
"medium": 1,
"low": 0
},
"severityChanges": 7,
"mergedDuplicates": 4,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 3,
"dash-spv-ffi/src/bin/ffi_cli.rs": 2,
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Order `record_balance` before `fetch_add` in `on_mempool_transaction_received` so the balance is always stored before the counter increments, matching the pattern in `on_synced_height_updated`. Add an explicit length `assert!` before slicing `block_record_actions` in `tests_callback.rs` so a counter/vector mismatch produces a clear diagnostic instead of an index-out-of-bounds panic. Log a `tracing::warn!` in `read_balance` of `ffi_cli` when the balance pointer is null, so the demo CLI no longer silently treats a null as zero.
|
Manki — Review complete Planner Review — 27 findings Judge — 10 kept · 0 dropped (123s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Four reviewers independently flagged the same unguarded slice in tests_transaction.rs that tests_callback.rs already guards — the inconsistency is the most actionable finding here. Most other items are stylistic (redundant drops, short_wallet allocations) and the multi-mutex pattern is debatable since the test's atomicity gate is the counter, not the locks.
📊 10 findings (1 warning, 3 suggestion, 6 nitpick) · 2208 lines · 406s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 405861,
"diffLines": 2208,
"diffAdditions": 1278,
"diffDeletions": 930,
"filesReviewed": 16,
"agents": [
"Security & Safety",
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage",
"Dependencies & Integration",
"Performance & Efficiency",
"Maintainability & Readability"
],
"findingsRaw": 27,
"findingsKept": 10,
"findingsDropped": 17,
"severity": {
"blocker": 0,
"warning": 1,
"suggestion": 3,
"nitpick": 6
},
"verdict": "REQUEST_CHANGES",
"prNumber": 116,
"commitSha": "3d2275458f41f2f3e313a096e2d99276ad32210f",
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 3,
"findingsKept": 1,
"responseLength": 2861
},
{
"name": "Architecture & Design",
"findingsRaw": 4,
"findingsKept": 2,
"responseLength": 4700
},
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 2,
"responseLength": 4120
},
{
"name": "Testing & Coverage",
"findingsRaw": 3,
"findingsKept": 2,
"responseLength": 3389
},
{
"name": "Dependencies & Integration",
"findingsRaw": 3,
"findingsKept": 3,
"responseLength": 3205
},
{
"name": "Performance & Efficiency",
"findingsRaw": 5,
"findingsKept": 4,
"responseLength": 4955
},
{
"name": "Maintainability & Readability",
"findingsRaw": 5,
"findingsKept": 1,
"responseLength": 5552
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 6,
"medium": 4,
"low": 0
},
"severityChanges": 10,
"mergedDuplicates": 11,
"defensiveHardeningCount": 2,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 15,
".toml": 1
},
"findingsPerFile": {
"dash-spv-ffi/src/bin/ffi_cli.rs": 3,
"dash-spv-ffi/tests/dashd_sync/callbacks.rs": 1,
"dash-spv-ffi/src/callbacks.rs": 4,
"dash-spv-ffi/tests/dashd_sync/tests_transaction.rs": 1,
"dash-spv-ffi/tests/dashd_sync/tests_callback.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}Add explicit bounds `assert!` before slicing `block_record_actions` and `block_account_paths` by `block_records_before` in `tests_transaction.rs`, matching the diagnostic style used in `tests_callback.rs`. Change the mempool BIP-32 path check from `any` to `all` so it matches the strictness of the post-sync block path assertion below it.
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
…t ids Rename and reshape the atomic wallet events surfaced in #116 based on user feedback: - `MempoolTransactionReceived` → `TransactionReceived` to drop the misleading channel-specific name (the variant also fires for first-seen IS-locked transactions, where the record's `context` is `InstantSend(_)` and the balance reports as `confirmed`). - `BlockProcessChange` → `BlockProcessed` for a cleaner reading. - Replace `BlockRecordUpdate` with a single `RecordChange { account_type, action, record }` reused by both `TransactionReceived` (single value) and `BlockProcessed` (vec). On the receive path `action` is always `Inserted`, but the symmetric type lets consumers share one record-handling code path. Carry `AccountType` directly through `CheckTransactionsResult` and the events instead of the previously-derived `DerivationPath`. The path was deterministic from `(AccountType, Network)` anyway, so the consumer can reconstruct it on demand. This eliminates the conversion failure mode and removes the `dropped_records_missing_path` counter and its tracking. On the FFI side, replace the borrowed BIP-32 path C-string in `FFIBlockRecordUpdate` with a typed `FFIAccountType` discriminant plus `account_index: i32` (`-1` for variants without an index) on the new `FFIRecordChange`. This drops a `CString` allocation per record, gives consumers a `switch`-able enum that mirrors `AccountType` exactly, and keeps the path recoverable from `(account_type, account_index, network)` when it is needed. Tests updated to match: `event_tests.rs` asserts on `account_type` discriminants, the FFI integration tracker captures `FFIAccountType`/`account_index` per callback and asserts the BIP-44 account 0 reaches the boundary intact.
Records now carry their owning `AccountType` directly via a new `account_type` field on `TransactionRecord`, eliminating the external `(AccountType, TransactionRecord)` pairs previously used in `TransactionCheckResult.new_records`/`updated_records` and `CheckTransactionsResult.per_wallet_new_records`/`per_wallet_updated_records`. Producers in `wallet_checker.rs` and `record_transaction` populate the field at creation time using `ManagedAccountType::to_account_type()`. Consumers in `key-wallet-manager/src/process_block.rs` read it back when constructing `RecordChange`. The on-disk format is unchanged because the field is transient: it is `#[serde(skip)]` with a sentinel default, and the parent `ManagedCoreAccount`'s `Deserialize` impl walks `transactions.values_mut()` after decoding to overwrite each record's `account_type` with its own. `TransactionRecord` has no bincode derive, so no further bincode handling is required.
Renames the wrapper struct emitted by `WalletEvent::TransactionReceived` and `WalletEvent::BlockProcessed` from `RecordChange` to `TransactionRecordUpdate`, and renames the corresponding event fields from `change` to `update` and `changes` to `updates` so the type and the field name both convey "a record paired with the action that just happened to it".
Drops the wrapper's `account_type` field since `TransactionRecord` now carries its own `account_type` (from the prior commit), so the wrapper is just `{ action, record }`. Consumers that previously read `change.account_type` now read `update.record.account_type`.
The FFI side in `dash-spv-ffi/src/callbacks.rs` keeps the existing `FFIRecordChange` struct name for now (the larger FFI consolidation lands in a follow-up), and renames its constructor to `FFIRecordChange::from_record_update` to take `&TransactionRecordUpdate`. The dispatch arms and FFI-facing doc comments are updated to match the new field names (`update` / `updates`, `update_count`).
Delete the duplicate `FFIAccountType` introduced in `dash-spv-ffi/src/callbacks.rs` and reuse the canonical `key_wallet_ffi::types::FFIAccountType`. The two enums had different discriminants for seven variants and shipping both in the public C API was a real ABI hazard.
Move `FFIRecordAction` and the renamed `FFITransactionRecordUpdate` (formerly `FFIRecordChange`) from `dash-spv-ffi/src/callbacks.rs` into `key-wallet-ffi/src/types.rs`, alongside `FFIBalance` and `FFIAccountType`. The wallet-event callback function pointers (`OnTransactionReceivedCallback`, `OnWalletBlockProcessedCallback`) stay in `dash-spv-ffi` and reference the moved types via `key_wallet_ffi::types::*`. `dash-spv-ffi/src/lib.rs` re-exports them so consumers can keep writing `dash_spv_ffi::FFIAccountType`.
Extend `FFITransactionRecord` to carry the owning account on each record. New fields: `account_type: FFIAccountType`, `account_index: u32`, `account_index_secondary: i32` (`-1` sentinel mirroring the existing `FFIAccountType::from_account_type` tuple contract), plus `account_identity_user`/`account_identity_friend: *const [u8; 32]` and `account_key_class: i32` to preserve the Dashpay identity ids and `PlatformPayment` `key_class` that the previous lossy mapping silently dropped at the FFI boundary. The identity-id pointers are heap-allocated on conversion and freed in `Drop`, so they have stable lifetimes both for borrowed-during-callback dispatch and for the owned-by-caller `managed_core_account_get_transactions` path.
Drop the now-redundant `account_type`/`account_index` fields on `FFITransactionRecordUpdate`. The wrapper degenerates to `{ action, record }`, matching the Rust-side `TransactionRecordUpdate { action, record }`. Callback signatures lose their per-update account params and read them from `update.record` instead.
xdustinface
force-pushed
the
v0.42-dev
branch
2 times, most recently
from
66e5614 to
05b14a8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reshape
WalletEventso each variant carries the records and post-change balance needed to persist a wallet update atomically off a single event. ReplacesTransactionReceived/TransactionStatusChanged/BalanceUpdatedwithMempoolTransactionReceived,TransactionInstantSendLocked, andBlockProcessChange, addsRecordActionandBlockRecordUpdatefor per-record context, and identifies accounts byDerivationPathinstead ofaccount_index: u32.