refactor: heights per wallet#117
Merged
Merged
Conversation
|
Manki — Review complete Planner (28s) Review — 11 findings Judge — 10 kept · 1 dropped (25s) Review metadataConfig:
Judge decisions:
Timing:
|
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.
xdustinface
force-pushed
the
refactor/heights-per-wallet
branch
from
dd9596d to
5b7c419
Compare
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Per-wallet height refactor introduces semantic risks: max() aggregation for last_processed_height misreports manager state, first-wallet imports get birth_height=0 erasing the pre-seeding capability, and the new synced_height field lacks serde(default) for backward-compat with persisted wallets.
📊 10 findings (4 warning, 3 suggestion, 3 nitpick) · 108 lines · 262s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 262305,
"diffLines": 108,
"diffAdditions": 62,
"diffDeletions": 46,
"filesReviewed": 8,
"agents": [
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"findingsRaw": 11,
"findingsKept": 10,
"findingsDropped": 1,
"severity": {
"blocker": 0,
"warning": 4,
"suggestion": 3,
"nitpick": 3
},
"verdict": "REQUEST_CHANGES",
"prNumber": 117,
"commitSha": "dd9596dde4029d5e865a43519a6de0e3db272b65",
"agentMetrics": [
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"responseLength": 4892
},
{
"name": "Architecture & Design",
"findingsRaw": 6,
"findingsKept": 6,
"responseLength": 7744
},
{
"name": "Testing & Coverage",
"findingsRaw": 5,
"findingsKept": 4,
"responseLength": 7235
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 5,
"medium": 6,
"low": 0
},
"severityChanges": 11,
"mergedDuplicates": 0,
"defensiveHardeningCount": 1,
"verdictReason": "novel_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 8
},
"findingsPerFile": {
"key-wallet-manager/src/process_block.rs": 4,
"key-wallet-manager/src/lib.rs": 1,
"key-wallet/src/wallet/metadata.rs": 1,
"key-wallet-manager/src/accessors.rs": 1,
"key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs": 1,
"key-wallet-manager/tests/integration_test.rs": 1,
"key-wallet-ffi/src/wallet_manager_tests.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}
|
Manki — Review complete Planner (22s) Review — 7 findings Judge — 6 kept · 0 dropped (20s) Review metadataConfig:
Judge decisions:
Timing:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #117 +/- ##
==========================================
Coverage 70.51% 70.52%
==========================================
Files 319 319
Lines 66756 66758 +2
==========================================
+ Hits 47074 47080 +6
+ Misses 19682 19678 -4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Previous open threads remain unaddressed in this round. New findings are mostly about the struct-level Send + Sync + 'static bound spreading through impls (a Rust API anti-pattern) and thinned test coverage for the per-wallet height refactor.
📊 6 findings (2 suggestion, 4 nitpick) · 108 lines · 382s
Review stats
{
"model": "claude-sonnet-4-6",
"reviewTimeMs": 382090,
"diffLines": 108,
"diffAdditions": 62,
"diffDeletions": 46,
"filesReviewed": 8,
"agents": [
"Architecture & Design",
"Correctness & Logic",
"Testing & Coverage"
],
"findingsRaw": 7,
"findingsKept": 6,
"findingsDropped": 1,
"severity": {
"blocker": 0,
"warning": 0,
"suggestion": 2,
"nitpick": 4
},
"verdict": "APPROVE",
"prNumber": 117,
"commitSha": "5b7c4199983ab4dbf415cec2818f9abf0ad88c00",
"agentMetrics": [
{
"name": "Architecture & Design",
"findingsRaw": 2,
"findingsKept": 2,
"responseLength": 2329
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 0,
"responseLength": 1365
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"responseLength": 4356
}
],
"judgeMetrics": {
"confidenceDistribution": {
"high": 4,
"medium": 2,
"low": 0
},
"severityChanges": 6,
"mergedDuplicates": 1,
"defensiveHardeningCount": 2,
"verdictReason": "only_nit_or_suggestion"
},
"fileMetrics": {
"fileTypes": {
".rs": 8
},
"findingsPerFile": {
"key-wallet-manager/src/lib.rs": 1,
"key-wallet-manager/src/accessors.rs": 1,
"key-wallet-manager/src/process_block.rs": 1,
"key-wallet-manager/tests/integration_test.rs": 1,
"key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs": 1,
"key-wallet-ffi/src/wallet_manager_tests.rs": 1
}
},
"reviewerModel": "claude-sonnet-4-6",
"judgeModel": "claude-opus-4-7"
}| @@ -220,15 +217,14 @@ mod tests { | |||
| let mut manager: WalletManager<ManagedWalletInfo> = WalletManager::new(Network::Testnet); | |||
There was a problem hiding this comment.
🔴 ✨ Suggestion: test_last_processed_height unit test reduced to no-wallet-only path, losing with-wallet coverage
The unit test was deliberately trimmed to only assert that update_last_processed_height is a no-op when no wallets are registered. The previous version at least exercised the state machine (values 1000 → 5000 → 10 were stored and checked), giving a local regression signal. Now the only unit-level coverage for the with-wallet path lives in the slower integration test suite, so a bug in the per-wallet update_last_processed_height call inside process_block.rs would not be caught by cargo test -p key-wallet-manager without the integration tests. Add a small with-wallet sub-case: create one wallet, call update_last_processed_height(1000), assert last_processed_height() == 1000 and that the wallet info also shows 1000.
Suggested fix
Suggested change
| let mut manager: WalletManager<ManagedWalletInfo> = WalletManager::new(Network::Testnet); | |
| // After the existing no-wallet assertions, add: | |
| let wallet_id = manager | |
| .create_wallet_from_mnemonic(TEST_MNEMONIC, "", 0, WalletAccountCreationOptions::Default) | |
| .unwrap(); | |
| manager.update_last_processed_height(1000); | |
| assert_eq!(manager.last_processed_height(), 1000); | |
| let info = manager.get_wallet_info(&wallet_id).unwrap(); | |
| assert_eq!(info.last_processed_height(), 1000); |
AI context
{
"file": "key-wallet-manager/src/process_block.rs",
"line": 217,
"severity": "suggestion",
"confidence": "high",
"flaggedBy": [
"Testing & Coverage"
],
"title": "test_last_processed_height unit test reduced to no-wallet-only path, losing with-wallet coverage",
"fix": "// After the existing no-wallet assertions, add:\nlet wallet_id = manager\n .create_wallet_from_mnemonic(TEST_MNEMONIC, \"\", 0, WalletAccountCreationOptions::Default)\n .unwrap();\nmanager.update_las",
"reachability": "reachable"
}| assert_eq!(wallet_info1.last_processed_height(), 30000); | ||
| assert_eq!(wallet_info2.last_processed_height(), 25000); | ||
| assert_eq!(manager.last_processed_height(), 30000); | ||
|
|
There was a problem hiding this comment.
🔴 ✨ Suggestion: No test verifies synced_height() returns min() when wallets diverge
The expanded test_block_height_tracking test calls update_synced_height once and verifies both wallets reach the same value, but never sets the two wallets to different synced_height values to confirm the manager-level min() aggregation. A bug that accidentally uses max() instead of min() would pass all current assertions. Add a step that mutates each wallet's synced_height independently (e.g., wallet1 to 30 000, wallet2 to 20 000) and asserts manager.synced_height() == 20_000.
Suggested fix
Suggested change
| // After the two wallets are at synced_height 40_000: | |
| let wi1 = manager.get_wallet_info_mut(&wallet_id1).unwrap(); | |
| wi1.update_synced_height(50_000); | |
| // wallet2 still at 40_000 | |
| assert_eq!(manager.synced_height(), 40_000, "min() should return the lower per-wallet height"); |
AI context
{
"file": "key-wallet-manager/tests/integration_test.rs",
"line": 237,
"severity": "suggestion",
"confidence": "high",
"flaggedBy": [
"Testing & Coverage"
],
"title": "No test verifies synced_height() returns min() when wallets diverge",
"fix": "// After the two wallets are at synced_height 40_000:\nlet wi1 = manager.get_wallet_info_mut(&wallet_id1).unwrap();\nwi1.update_synced_height(50_000);\n// wallet2 still at 40_000\nassert_eq!(manager.synce",
"reachability": "reachable"
}
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
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.
This was referenced Apr 27, 2026
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.
No description provided.