refactor: Single network WalletManager#299
Conversation
WalkthroughThe pull request refactors network configuration management across all crates by moving network binding from per-method-call parameters to initialization time. WalletManager constructors now require a Network argument, while trait methods and wallet operations that previously accepted network parameters have them removed. BlockProcessor's network field is eliminated, and FFI APIs are updated to reflect this centralized approach. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The changes span multiple crates with heterogeneous modifications to trait definitions, struct layouts, and method signatures. While individual changes follow a consistent pattern (moving network to initialization), the breadth of affected call sites, integration across FFI boundaries, and the need to verify state management changes (particularly around height and network scoping) require substantial review effort. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
1232c3b to
61198ae
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
refactor/ffi-networks
branch
from
3271b7a to
4a4014c
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
61198ae to
a09352a
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
xdustinface
force-pushed
the
refactor/ffi-networks
branch
2 times, most recently
from
3df2035 to
c896c14
Compare
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
a09352a to
2cd41fa
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/ffi-networks
branch
from
c896c14 to
1a26acc
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
2cd41fa to
583a170
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/ffi-networks
branch
from
1a26acc to
5eff2d1
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
refactor/ffi-networks
branch
from
5eff2d1 to
db44dd5
Compare
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
583a170 to
da1af64
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/ffi-networks
branch
from
db44dd5 to
87c0601
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
da1af64 to
b984765
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/wallet-manager
branch
from
b984765 to
432e895
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
key-wallet-manager/src/wallet_manager/mod.rs (3)
332-360: Missing network-consistency check when importing from extended private key
import_wallet_from_extended_priv_keybuilds aWalletviaWallet::from_extended_key(...)without verifying thatwallet.networkmatchesself.network. Given thatWalletManageris now explicitly single-network, this allows importing a wallet for a different network into the same manager, violating the “single network” invariant and the project preference to avoid mixing mainnet/testnet contexts.Consider rejecting imports where the wallet’s network does not match the manager’s:
Suggested guard in import_wallet_from_extended_priv_key
- // Create wallet from extended private key - let wallet = Wallet::from_extended_key(extended_priv_key, account_creation_options) - .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + // Create wallet from extended private key + let wallet = Wallet::from_extended_key(extended_priv_key, account_creation_options) + .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + + // Enforce single-network invariant + if wallet.network != self.network { + return Err(WalletError::InvalidNetwork); + }Based on learnings, this helps enforce “never mix mainnet and testnet operations” when managing wallets.
375-407: Also enforce network match when importing from xpubSimilarly,
import_wallet_from_xpubcreates a wallet viaWallet::from_xpub(...)and then adds it to a manager whosenetworkmay not match the xpub’s implied network. To keep the single-network invariant, add the same kind of check before inserting:Suggested guard in import_wallet_from_xpub
- // Create watch-only or externally signable wallet from extended public key - let wallet = Wallet::from_xpub(extended_pub_key, accounts, can_sign_externally) - .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + // Create watch-only or externally signable wallet from extended public key + let wallet = Wallet::from_xpub(extended_pub_key, accounts, can_sign_externally) + .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + + // Enforce that imported wallet matches this manager's network + if wallet.network != self.network { + return Err(WalletError::InvalidNetwork); + }This prevents accidentally loading a mainnet watch-only wallet into a testnet manager (or vice versa).
Based on learnings, …
422-451: Import from serialized bytes should also validate network vs manager
import_wallet_from_bytesdeserializes aWalletand then unconditionally inserts it, only usingself.current_heightas a default birth height. If the serialized wallet is for a different network thanself.network, the manager will now be managing cross-network state.Recommend adding a network check similar to the other import paths:
Suggested guard in import_wallet_from_bytes
- // Compute wallet ID from the wallet's root public key - let wallet_id = wallet.compute_wallet_id(); + // Ensure the imported wallet matches this manager's network + if wallet.network != self.network { + return Err(WalletError::InvalidNetwork); + } + + // Compute wallet ID from the wallet's root public key + let wallet_id = wallet.compute_wallet_id();Based on learnings, this keeps imported backups from silently breaking the single-network invariant.
♻️ Duplicate comments (1)
key-wallet-manager/src/wallet_manager/mod.rs (1)
869-879: wallet_utxos delegates cleanly to wallet_info.utxosAside from the type-consistency concern already mentioned for
get_all_utxos, this method is straightforward: it resolves the wallet info and hands back the UTXO set. Once the sharedutxos()contract is confirmed, this is fine.
🧹 Nitpick comments (10)
key-wallet-ffi/src/address_pool.rs (1)
1056-1063: FFI wallet_manager_create usage matches new network-bound APIBoth test sites now construct the manager with an explicit
FFINetwork::Testnetargument, matching the updatedwallet_manager_create(network, error)signature and keeping the testnet address generation consistent with the manager’s network. Looks good; if this pattern expands further, consider a small helper to create a test manager and reduce duplication.Also applies to: 1155-1162
key-wallet-ffi/src/wallet_manager_tests.rs (1)
883-1076: Serialized-wallet roundtrip tests use network-bound managers consistentlyIn
test_create_wallet_from_mnemonic_return_serialized_bytes, all managers (manager,manager2,manager3,manager4,manager5) are created viawallet_manager_create(FFINetwork::Testnet, error), aligning with the new API. The watch-only and externally signable flows still validate ID stability and error handling for invalid mnemonics. You could optionally factor out a tiny helper for “fresh Testnet manager + error” to DRY these repeated patterns.key-wallet-ffi/tests/test_passphrase_wallets.rs (1)
60-60: Consider adding Mainnet test coverage.All tests in this file use
FFINetwork::Testnet. Per coding guidelines, tests should cover both mainnet and testnet configurations to ensure network-specific behavior is validated.Consider adding parallel test cases for
FFINetwork::Dash(mainnet) to verify passphrase wallet functionality on both networks.Based on coding guidelines.
key-wallet-manager/tests/test_serialized_wallets.rs (1)
11-11: Consider adding Mainnet test coverage.All
WalletManagerinstances are created withNetwork::Testnet. Per coding guidelines, tests should cover both mainnet and testnet configurations.Consider adding test variants that use
Network::Dash(mainnet) to ensure wallet serialization and passphrase handling work correctly across both networks.Based on coding guidelines.
Also applies to: 30-30, 48-48, 64-64, 72-72, 90-90
key-wallet-ffi/src/wallet_manager_serialization_tests.rs (1)
20-20: Consider adding Mainnet test coverage.All tests create wallet managers with
FFINetwork::Testnet. Per coding guidelines, tests should validate behavior on both mainnet and testnet networks.Consider duplicating key serialization tests to also run with
FFINetwork::Dash(mainnet), ensuring serialization compatibility across networks.Based on coding guidelines.
Also applies to: 71-71, 119-119, 167-167, 215-215, 246-246, 282-282, 320-320, 355-355
key-wallet-ffi/tests/test_import_wallet.rs (1)
17-17: Consider adding Mainnet test coverage.The test creates wallet managers with
FFINetwork::Testnet. Per coding guidelines, tests should validate both mainnet and testnet configurations.Consider adding a test variant using
FFINetwork::Dash(mainnet) to ensure wallet import functionality works correctly on both networks.Based on coding guidelines.
Also applies to: 52-52
key-wallet-ffi/tests/integration_test.rs (1)
28-28: Consider adding Mainnet integration test coverage.All integration tests use
FFINetwork::Testnet. Per coding guidelines, integration tests should cover both mainnet and testnet configurations to validate network operations thoroughly.Consider duplicating
test_full_wallet_workflowand other critical integration tests to also run withFFINetwork::Dash(mainnet), ensuring the complete workflow functions correctly across both networks.Based on coding guidelines.
key-wallet-manager/tests/spv_integration_tests.rs (1)
120-150: Consider adding mainnet test variant.The filter caching tests work well with the new single-network API. Per coding guidelines, tests should cover both mainnet and testnet configurations. Consider adding a parameterized test or separate test function for
Network::Dashto ensure network consistency validation works across networks.key-wallet-ffi/src/managed_account.rs (1)
1343-1349: Minor: mutable binding may be unnecessary.Line 1348 declares
manageras mutable (let mut manager), but it's later reassigned viawallet_manager_create. The first assignment doesn't needmutsince the variable is only being initialized, not mutated.🔎 Suggested fix
- let mut manager = wallet_manager_create(FFINetwork::Testnet, &mut error); + let manager = wallet_manager_create(FFINetwork::Testnet, &mut error);Note: Line 1437 does require
mutsince the same variable is reassigned later in the test.key-wallet-ffi/FFI_API.md (1)
480-480: Clarify network context in descriptions.Several function descriptions still reference "for a network" or "for a given network" but no longer accept a network parameter:
- Line 480:
wallet_manager_current_height- "Get current height for a network"- Line 496:
wallet_manager_describe- "Describe the wallet manager for a given network"- Line 676:
wallet_manager_update_height- "Update block height for a network"Since this is auto-generated documentation, consider updating the source docstrings to clarify that these operations apply to the network specified when the wallet manager was created (via
wallet_manager_create).Suggested description improvements
For consistency, descriptions could be updated to:
wallet_manager_current_height: "Get current height for the wallet manager's network"wallet_manager_describe: "Describe the wallet manager"wallet_manager_update_height: "Update block height for the wallet manager's network"Also applies to: 496-496, 676-676
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Didn't go into too much detail on the review.
2 participants
Refactors the
WalletManagerto be a single network wallet only. Follow up to finalize the refactoring started in:WalletandManagedWalletInfo#271FFINetworksand useFFINetworkonly #294Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.