feat: new managed platform account to track platform balances #404
Conversation
📝 WalkthroughWalkthroughThis PR refactors the wallet account system by renaming Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Wallet as ManagedWalletInfo
participant Collection as ManagedAccountCollection
participant PlatAccount as ManagedPlatformAccount
participant AddressPool as AddressPool
Client->>Wallet: get_platform_payment_account(account_idx, key_class)
Wallet->>Collection: get_platform_account(key)
Collection->>PlatAccount: return reference
PlatAccount->>AddressPool: maintain_gap_limit()
AddressPool->>PlatAccount: next_unused_platform_address()
PlatAccount->>PlatAccount: set_address_credit_balance()
PlatAccount->>Client: return PlatformP2PKHAddress
sequenceDiagram
participant Transaction as Transaction
participant Wallet as ManagedWalletInfo
participant Checker as WalletPlatformChecker
participant PlatAccounts as Platform Accounts
Transaction->>Wallet: check_core_transaction()
Wallet->>Checker: set_platform_address_balance()
Checker->>PlatAccounts: find_account_for_address()
PlatAccounts->>PlatAccounts: add_address_credit_balance()
PlatAccounts->>Checker: return updated balance
Checker->>Wallet: balance updated
Wallet->>Transaction: return TransactionCheckResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dash/src/blockdata/transaction/mod.rs (1)
597-606:⚠️ Potential issue | 🟡 MinorComment is incomplete — decoding also disables SegWit for
QuorumCommitmentandMnhfSignal.The comment on line 597 only mentions
AssetUnlock, but lines 601-606 also forcesegwit = falseforQuorumCommitmentandMnhfSignal. This creates a discrepancy between the documentation and actual behavior.Note: The asymmetry with the encoding path (which only handles
AssetUnlock) may be intentional for backward compatibility when decoding legacy transactions, but the comment should reflect the complete behavior.📝 Suggested comment update
- // Forcing segwit to false for AssetUnlock, as currently Core doesn't support BIP141 SegWit. + // Forcing segwit to false for AssetUnlock, QuorumCommitment, and MnhfSignal, + // as currently Core doesn't support BIP141 SegWit for these special transaction types.key-wallet-ffi/src/managed_wallet.rs (1)
975-985:⚠️ Potential issue | 🟠 MajorUse
address_array_freefor external range cleanup.The external range is allocated via Rust (
Box::into_raw), so freeing it withlibc::freeis UB on non‑system allocators. Use the sameaddress_array_freehelper as the internal range test and avoid manually freeing individual CStrings.🔧 Suggested fix
- unsafe { - let addresses = std::slice::from_raw_parts(addresses_out, count_out); - for &addr_ptr in addresses { - let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); - assert!(!addr_str.is_empty()); - println!("External address: {}", addr_str); - let _ = CString::from_raw(addr_ptr); - } - libc::free(addresses_out as *mut libc::c_void); - } + unsafe { + let addresses = std::slice::from_raw_parts(addresses_out, count_out); + for &addr_ptr in addresses { + let addr_str = CStr::from_ptr(addr_ptr).to_string_lossy(); + assert!(!addr_str.is_empty()); + println!("External address: {}", addr_str); + } + crate::address::address_array_free(addresses_out, count_out); + }Based on learnings: Be careful with FFI memory management.
key-wallet-ffi/src/managed_account.rs (1)
205-215:⚠️ Potential issue | 🟠 MajorFree
FFIErrormessages before returning error results.When
wallet_manager_get_managed_wallet_infofails,error.messageis allocated but never freed; the returned result contains a new CString, so the original message leaks. Please free the originalFFIErrormessage before returning. (Apply the same pattern in each early-return path.)🔧 Suggested fix (example for `managed_wallet_get_account`)
- if managed_wallet_ptr.is_null() { - return FFIManagedCoreAccountResult::error( - error.code, - if error.message.is_null() { - "Failed to get managed wallet info".to_string() - } else { - let c_str = std::ffi::CStr::from_ptr(error.message); - c_str.to_string_lossy().to_string() - }, - ); - } + if managed_wallet_ptr.is_null() { + let message = if error.message.is_null() { + "Failed to get managed wallet info".to_string() + } else { + let c_str = std::ffi::CStr::from_ptr(error.message); + c_str.to_string_lossy().to_string() + }; + error.free_message(); + return FFIManagedCoreAccountResult::error(error.code, message); + }Based on learnings: Be careful with FFI memory management.
Also applies to: 317-327, 388-397, 450-457, 1103-1113
🤖 Fix all issues with AI agents
In `@key-wallet-ffi/FFI_API.md`:
- Around line 1276-1281: The generated Markdown uses inline `*` that
Markdownlint treats as emphasis in the "Arguments" section of the Platform
Payment account doc; update the generator to emit a proper bulleted list (one
`-` or `*` item per line) for the three arguments `wallet`, `account_index`, and
`key_class`, ensuring a blank line before the list so each becomes a standalone
list item rather than inline emphasis and thus fixes MD037.
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 498-508: Before calling to_account_type() on account_type_rust,
guard against variants that require extra params (PlatformPayment,
DashpayReceivingFunds, DashpayExternalAccount) so the conversion cannot panic
and unwind across the FFI; check account_type_rust first and if it matches any
of those forbidden variants call FFIError::set_error(error,
FFIErrorCode::InvalidInput, "Platform Payment accounts cannot be used for
address pool operations".to_string()) and return false, otherwise proceed to
call account_type_rust.try_into() (producing account_type_to_check) and continue
as before. Ensure you reference the existing symbols account_type_rust,
to_account_type(), account_type_to_check, FFIError::set_error,
FFIErrorCode::InvalidInput and error when implementing the guard.
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 939-987: Validate the untrusted FFI inputs account_index and
key_class in wallet_add_platform_payment_account before creating
AccountType::PlatformPayment: explicitly check that both values are less than
2^31 (i.e. reject >= 2_147_483_648) and return
crate::types::FFIAccountResult::error with FFIErrorCode::InvalidInput and a
clear message if out of range; only after these guards succeed proceed to
construct AccountType::PlatformPayment and call w.add_account (so you never pass
invalid indices to derivation/ChildNumber::from_hardened_idx).
In `@key-wallet/src/managed_account/managed_platform_account.rs`:
- Around line 363-392: The Decode impl for ManagedPlatformAccount currently
trusts decoded network and credit_balance; update the bincode::Decode::decode
implementation to enforce invariants by (1) verifying that every key in
address_balances and addresses.network are consistent (or at minimum set network
= addresses.network) and (2) recomputing credit_balance as the sum of values in
address_balances and using that computed total instead of the decoded
credit_balance when constructing Self; make these adjustments inside the Decode
for ManagedPlatformAccount (the function that decodes account, key_class,
network, credit_balance, address_balances, addresses, metadata, is_watch_only)
so the returned instance always has network == addresses.network and
credit_balance == sum(address_balances).
In `@key-wallet/src/managed_account/platform_address.rs`:
- Around line 62-71: The to_address method uses
Hash160::from_slice(&self.0).expect(...) which calls expect in library code;
replace that with the infallible Hash160::from_byte_array(self.0) (or equivalent
method taking [u8;20]) to construct pubkey_hash without panicking, then build
Payload::PubkeyHash(pubkey_hash.into()) and return Address::new(network,
payload); update references to self.0 accordingly and remove the expect call.
🧹 Nitpick comments (1)
key-wallet/src/wallet/balance.rs (1)
55-58: Consider checked arithmetic fortotal()to avoid panic in library code.The
total()method can panic on overflow (as demonstrated intest_balance_overflow). While unlikely in practice, library code guidelines recommend avoiding panics. Consider returningResult<u64, _>or usingsaturating_add.♻️ Optional: Use saturating arithmetic
/// Get the total balance. - pub fn total(&self) -> u64 { - self.spendable + self.unconfirmed + self.immature + self.locked + pub fn total(&self) -> u64 { + self.spendable + .saturating_add(self.unconfirmed) + .saturating_add(self.immature) + .saturating_add(self.locked) }As per coding guidelines: "Avoid
unwrap()andexpect()in library code; use explicit error types" - the same principle applies to operations that can panic.
| **Description:** | ||
| Add a Platform Payment account (DIP-17) to the wallet Platform Payment accounts use the derivation path: `m/9'/coin_type'/17'/account'/key_class'/index` # Arguments * `wallet` - Pointer to the wallet * `account_index` - The account index (hardened) in the derivation path * `key_class` - The key class (hardened) - typically 0' for main addresses # Safety This function dereferences a raw pointer to FFIWallet. The caller must ensure that: - The wallet pointer is either null or points to a valid FFIWallet - The FFIWallet remains valid for the duration of this call | ||
|
|
||
| **Safety:** | ||
| This function dereferences a raw pointer to FFIWallet. The caller must ensure that: - The wallet pointer is either null or points to a valid FFIWallet - The FFIWallet remains valid for the duration of this call | ||
|
|
There was a problem hiding this comment.
Fix MD037 inline emphasis spacing by breaking the arguments list into bullets.
Markdownlint flags the inline * list as emphasis with spaces; please update the generator to emit a proper list.
📝 Proposed formatting fix (update generator)
-Add a Platform Payment account (DIP-17) to the wallet Platform Payment accounts use the derivation path: `m/9'/coin_type'/17'/account'/key_class'/index` # Arguments * `wallet` - Pointer to the wallet * `account_index` - The account index (hardened) in the derivation path * `key_class` - The key class (hardened) - typically 0' for main addresses # Safety This function dereferences a raw pointer to FFIWallet. The caller must ensure that: - The wallet pointer is either null or points to a valid FFIWallet - The FFIWallet remains valid for the duration of this call
+Add a Platform Payment account (DIP-17) to the wallet. Platform Payment accounts use the derivation path: `m/9'/coin_type'/17'/account'/key_class'/index`
+
+**Arguments:**
+- `wallet` - Pointer to the wallet
+- `account_index` - The account index (hardened) in the derivation path
+- `key_class` - The key class (hardened) - typically 0' for main addresses
+
+# Safety
+This function dereferences a raw pointer to FFIWallet. The caller must ensure that:
+- The wallet pointer is either null or points to a valid FFIWallet
+- The FFIWallet remains valid for the duration of this call🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 1277-1277: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 1277-1277: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
In `@key-wallet-ffi/FFI_API.md` around lines 1276 - 1281, The generated Markdown
uses inline `*` that Markdownlint treats as emphasis in the "Arguments" section
of the Platform Payment account doc; update the generator to emit a proper
bulleted list (one `-` or `*` item per line) for the three arguments `wallet`,
`account_index`, and `key_class`, ensuring a blank line before the list so each
becomes a standalone list item rather than inline emphasis and thus fixes MD037.
| let account_type_to_check = match account_type_rust.try_into() { | ||
| Ok(check_type) => check_type, | ||
| Err(_) => { | ||
| FFIError::set_error( | ||
| error, | ||
| FFIErrorCode::InvalidInput, | ||
| "Platform Payment accounts cannot be used for address pool operations".to_string(), | ||
| ); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the to_account_type implementation
echo "=== Searching for to_account_type implementation ==="
rg -n "fn to_account_type" key-wallet-ffi/src/
echo -e "\n=== Looking for account type conversions and panic patterns ==="
rg -n "PlatformPayment|DashpayReceivingFunds|DashpayExternalAccount|unreachable\!|panic\!" key-wallet-ffi/src/types.rs -A 2 -B 2
echo -e "\n=== Checking call sites of to_account_type in address_pool.rs ==="
rg -n "to_account_type" key-wallet-ffi/src/address_pool.rs -B 5 -A 10Repository: dashpay/rust-dashcore
Length of output: 2406
🏁 Script executed:
#!/bin/bash
# Read the to_account_type implementation
echo "=== to_account_type implementation ==="
sed -n '190,220p' key-wallet-ffi/src/types.rs
echo -e "\n=== Look for unimplemented/panic for account types ==="
rg -n "unimplemented|panic" key-wallet-ffi/src/types.rs -B 2 -A 2
echo -e "\n=== Look for AccountType enum definition ==="
rg -n "enum FFIAccountType" key-wallet-ffi/src/types.rs -A 20
echo -e "\n=== Check try_into/Into implementations for AccountType ==="
rg -n "impl.*Into.*AccountType|impl.*TryInto" key-wallet-ffi/src/types.rs -A 30Repository: dashpay/rust-dashcore
Length of output: 6472
🏁 Script executed:
#!/bin/bash
# Check context around the other two call sites
echo "=== Call site 1 (line 293) - full context ==="
sed -n '275,310p' key-wallet-ffi/src/address_pool.rs
echo -e "\n=== Call site 2 (line 399) - full context ==="
sed -n '385,420p' key-wallet-ffi/src/address_pool.rs
echo -e "\n=== Verify which account types are valid for address pools ==="
rg -n "get_managed_account_by_type|address pool" key-wallet-ffi/src/address_pool.rs -B 2 -A 2 | head -40Repository: dashpay/rust-dashcore
Length of output: 4176
🏁 Script executed:
#!/bin/bash
# Search for all calls to to_account_type in the entire codebase
echo "=== All calls to to_account_type in key-wallet-ffi ==="
rg -n "to_account_type\(" key-wallet-ffi/src/ -B 3 | grep -E "\.rs:|to_account_type"
echo -e "\n=== Check if there's any pre-validation of account types in the codebase ==="
rg -n "matches.*FFIAccountType|if.*account_type.*==" key-wallet-ffi/src/ | head -20Repository: dashpay/rust-dashcore
Length of output: 2306
🏁 Script executed:
#!/bin/bash
# Check wallet.rs call sites for error handling
echo "=== wallet.rs line 520 context ==="
sed -n '510,535p' key-wallet-ffi/src/wallet.rs
echo -e "\n=== wallet.rs line 749 context ==="
sed -n '740,765p' key-wallet-ffi/src/wallet.rs
echo -e "\n=== wallet.rs line 872 context ==="
sed -n '863,888p' key-wallet-ffi/src/wallet.rs
echo -e "\n=== managed_account.rs line 218 context ==="
sed -n '210,235p' key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 3724
🏁 Script executed:
#!/bin/bash
# Verify the exact code at lines 498-508 in address_pool.rs
echo "=== address_pool.rs lines 498-508 ==="
sed -n '498,508p' key-wallet-ffi/src/address_pool.rs
# Also check the function signature
echo -e "\n=== Function containing lines 498-508 ==="
sed -n '460,475p' key-wallet-ffi/src/address_pool.rsRepository: dashpay/rust-dashcore
Length of output: 960
Add account type validation before to_account_type() to prevent FFI panics.
The to_account_type() call at line 496 panics for PlatformPayment, DashpayReceivingFunds, and DashpayExternalAccount without the required identity/index parameters. This panic can unwind across the FFI boundary. The try_into() error handler at line 498 is unreachable for these types.
Validate the account type before calling to_account_type():
Suggested guard
+ if matches!(
+ account_type,
+ FFIAccountType::PlatformPayment
+ | FFIAccountType::DashpayReceivingFunds
+ | FFIAccountType::DashpayExternalAccount
+ ) {
+ FFIError::set_error(
+ error,
+ FFIErrorCode::InvalidInput,
+ "Account type not supported for address pool operations".to_string(),
+ );
+ return false;
+ }
+
let account_type_rust = account_type.to_account_type(account_index);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let account_type_to_check = match account_type_rust.try_into() { | |
| Ok(check_type) => check_type, | |
| Err(_) => { | |
| FFIError::set_error( | |
| error, | |
| FFIErrorCode::InvalidInput, | |
| "Platform Payment accounts cannot be used for address pool operations".to_string(), | |
| ); | |
| return false; | |
| } | |
| }; | |
| if matches!( | |
| account_type, | |
| FFIAccountType::PlatformPayment | |
| | FFIAccountType::DashpayReceivingFunds | |
| | FFIAccountType::DashpayExternalAccount | |
| ) { | |
| FFIError::set_error( | |
| error, | |
| FFIErrorCode::InvalidInput, | |
| "Account type not supported for address pool operations".to_string(), | |
| ); | |
| return false; | |
| } | |
| let account_type_rust = account_type.to_account_type(account_index); | |
| let account_type_to_check = match account_type_rust.try_into() { | |
| Ok(check_type) => check_type, | |
| Err(_) => { | |
| FFIError::set_error( | |
| error, | |
| FFIErrorCode::InvalidInput, | |
| "Platform Payment accounts cannot be used for address pool operations".to_string(), | |
| ); | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@key-wallet-ffi/src/address_pool.rs` around lines 498 - 508, Before calling
to_account_type() on account_type_rust, guard against variants that require
extra params (PlatformPayment, DashpayReceivingFunds, DashpayExternalAccount) so
the conversion cannot panic and unwind across the FFI; check account_type_rust
first and if it matches any of those forbidden variants call
FFIError::set_error(error, FFIErrorCode::InvalidInput, "Platform Payment
accounts cannot be used for address pool operations".to_string()) and return
false, otherwise proceed to call account_type_rust.try_into() (producing
account_type_to_check) and continue as before. Ensure you reference the existing
symbols account_type_rust, to_account_type(), account_type_to_check,
FFIError::set_error, FFIErrorCode::InvalidInput and error when implementing the
guard.
| pub unsafe extern "C" fn wallet_add_platform_payment_account( | ||
| wallet: *mut FFIWallet, | ||
| account_index: c_uint, | ||
| key_class: c_uint, | ||
| ) -> crate::types::FFIAccountResult { | ||
| use key_wallet::account::AccountType; | ||
|
|
||
| if wallet.is_null() { | ||
| return crate::types::FFIAccountResult::error( | ||
| FFIErrorCode::InvalidInput, | ||
| "Wallet is null".to_string(), | ||
| ); | ||
| } | ||
|
|
||
| let wallet = &mut *wallet; | ||
|
|
||
| let account_type = AccountType::PlatformPayment { | ||
| account: account_index, | ||
| key_class, | ||
| }; | ||
|
|
||
| match wallet.inner_mut() { | ||
| Some(w) => { | ||
| // Use the proper add_account method | ||
| match w.add_account(account_type, None) { | ||
| Ok(()) => { | ||
| // Get the account we just added | ||
| if let Some(account) = w.accounts.account_of_type(account_type) { | ||
| let ffi_account = crate::types::FFIAccount::new(account); | ||
| return crate::types::FFIAccountResult::success(Box::into_raw(Box::new( | ||
| ffi_account, | ||
| ))); | ||
| } | ||
| crate::types::FFIAccountResult::error( | ||
| FFIErrorCode::WalletError, | ||
| "Failed to retrieve account after adding".to_string(), | ||
| ) | ||
| } | ||
| Err(e) => crate::types::FFIAccountResult::error( | ||
| FFIErrorCode::WalletError, | ||
| format!("Failed to add platform payment account: {}", e), | ||
| ), | ||
| } | ||
| } | ||
| None => crate::types::FFIAccountResult::error( | ||
| FFIErrorCode::InvalidState, | ||
| "Cannot modify wallet".to_string(), | ||
| ), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for PlatformPayment definition and usage
rg -n "PlatformPayment" -g '*.rs' --context=5Repository: dashpay/rust-dashcore
Length of output: 50378
🏁 Script executed:
# Check AccountType definition and constraints
fd -e rs -path "*/account/*" | head -20Repository: dashpay/rust-dashcore
Length of output: 233
🏁 Script executed:
# Look for validation of account_index and key_class parameters
rg -n "account_index|key_class" -g '*.rs' --context=3 | head -100Repository: dashpay/rust-dashcore
Length of output: 8905
🏁 Script executed:
# Look for derivation_path implementation for PlatformPayment
sed -n '421,450p' key-wallet/src/account/account_type.rsRepository: dashpay/rust-dashcore
Length of output: 1191
🏁 Script executed:
# Check if there's any validation during child key derivation
rg -A 10 "fn derive.*platform\|fn child_key" -i --context=3Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Look for any bounds checking or validation error types
rg -n "invalid.*account\|invalid.*key_class\|bounds\|range" -i | grep -i "platform\|dip.17\|account.*key"Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Find ChildNumber implementation to see hardened index bounds
rg -n "from_hardened_idx" --type rust -A 5Repository: dashpay/rust-dashcore
Length of output: 38867
🏁 Script executed:
# Check the dashcore crate for ChildNumber constraints
fd -e rs | xargs rg -l "from_hardened_idx" | head -5Repository: dashpay/rust-dashcore
Length of output: 264
🏁 Script executed:
# Look for any documentation or constants about valid index ranges
rg -n "0x[0-9a-f]+|2\^31|hardened.*bound|max.*index" --context=2 | head -50Repository: dashpay/rust-dashcore
Length of output: 3351
🏁 Script executed:
# Check the add_account method to see if it validates these parameters
rg -A 15 "fn add_account" key-wallet/src/wallet/*.rs | head -80Repository: dashpay/rust-dashcore
Length of output: 2494
🏁 Script executed:
# Look for any error handling or validation before account creation
rg -B 3 -A 10 "fn add_account.*account_type" key-wallet/src/wallet/mod.rs | head -60Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check the exact bounds validation in ChildNumber::from_hardened_idx
sed -n '622,630p' key-wallet/src/bip32.rsRepository: dashpay/rust-dashcore
Length of output: 326
🏁 Script executed:
# Verify no other validation happens at the FFI boundary
sed -n '939,965p' key-wallet-ffi/src/wallet.rsRepository: dashpay/rust-dashcore
Length of output: 846
Add input validation for platform account indices before account creation.
The FFI inputs account_index and key_class are untrusted and must be validated at the FFI boundary. ChildNumber::from_hardened_idx() rejects indices ≥ 2³¹, but validation occurs only during later derivation, allowing creation of invalid accounts. Add explicit range checks to reject values ≥ 2³¹ before calling add_account().
🤖 Prompt for AI Agents
In `@key-wallet-ffi/src/wallet.rs` around lines 939 - 987, Validate the untrusted
FFI inputs account_index and key_class in wallet_add_platform_payment_account
before creating AccountType::PlatformPayment: explicitly check that both values
are less than 2^31 (i.e. reject >= 2_147_483_648) and return
crate::types::FFIAccountResult::error with FFIErrorCode::InvalidInput and a
clear message if out of range; only after these guards succeed proceed to
construct AccountType::PlatformPayment and call w.add_account (so you never pass
invalid indices to derivation/ChildNumber::from_hardened_idx).
| #[cfg(feature = "bincode")] | ||
| impl<Context> bincode::Decode<Context> for ManagedPlatformAccount { | ||
| fn decode<D: bincode::de::Decoder<Context = Context>>( | ||
| decoder: &mut D, | ||
| ) -> core::result::Result<Self, bincode::error::DecodeError> { | ||
| let account = bincode::Decode::decode(decoder)?; | ||
| let key_class = bincode::Decode::decode(decoder)?; | ||
| let network = bincode::Decode::decode(decoder)?; | ||
| let credit_balance = bincode::Decode::decode(decoder)?; | ||
|
|
||
| // Decode address_balances from vec of tuples | ||
| let address_balances_vec: Vec<(PlatformP2PKHAddress, u64)> = | ||
| bincode::Decode::decode(decoder)?; | ||
| let address_balances: BTreeMap<PlatformP2PKHAddress, u64> = | ||
| address_balances_vec.into_iter().collect(); | ||
|
|
||
| let addresses = bincode::Decode::decode(decoder)?; | ||
| let metadata = bincode::Decode::decode(decoder)?; | ||
| let is_watch_only = bincode::Decode::decode(decoder)?; | ||
|
|
||
| Ok(Self { | ||
| account, | ||
| key_class, | ||
| network, | ||
| credit_balance, | ||
| address_balances, | ||
| addresses, | ||
| metadata, | ||
| is_watch_only, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and examine the bincode Decode implementation
git ls-files key-wallet/src/managed_account/ | head -20Repository: dashpay/rust-dashcore
Length of output: 527
🏁 Script executed:
# Check the specific file and lines mentioned
cd key-wallet && find . -name "managed_platform_account.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 114
🏁 Script executed:
# Let's also search for AddressPool definition to understand the structure
rg "struct AddressPool|enum AddressPool" key-wallet/src --type rust -A 5Repository: dashpay/rust-dashcore
Length of output: 1587
🏁 Script executed:
# Search for the bincode Decode implementation for ManagedPlatformAccount
rg "impl.*bincode::Decode.*ManagedPlatformAccount" key-wallet/src --type rust -B 2 -A 35Repository: dashpay/rust-dashcore
Length of output: 3715
🏁 Script executed:
# Get the ManagedPlatformAccount struct definition
rg "pub struct ManagedPlatformAccount" key-wallet/src --type rust -A 15Repository: dashpay/rust-dashcore
Length of output: 1715
🏁 Script executed:
# Check what type the 'addresses' field is
rg "addresses:" key-wallet/src/managed_account/managed_platform_account.rs --type rust -B 2 -A 2Repository: dashpay/rust-dashcore
Length of output: 524
🏁 Script executed:
# Look at the full struct to understand field types
head -150 key-wallet/src/managed_account/managed_platform_account.rs | tail -50Repository: dashpay/rust-dashcore
Length of output: 2187
🏁 Script executed:
# Check the bincode Encode implementation to see how data is serialized
rg "impl.*bincode::Encode.*ManagedPlatformAccount" key-wallet/src --type rust -B 2 -A 35Repository: dashpay/rust-dashcore
Length of output: 3929
🏁 Script executed:
# Check if there are any validation methods or invariants documented
rg "invariant|validate|check" key-wallet/src/managed_account/managed_platform_account.rs --type rust -B 2 -A 2Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Look at how ManagedPlatformAccount is constructed and used elsewhere
rg "ManagedPlatformAccount::new" key-wallet/src --type rust -B 2 -A 5Repository: dashpay/rust-dashcore
Length of output: 13478
🏁 Script executed:
# Check if there's any validation in the constructor
rg "pub fn new" key-wallet/src/managed_account/managed_platform_account.rs --type rust -A 20Repository: dashpay/rust-dashcore
Length of output: 724
🏁 Script executed:
# Verify AddressPool contains network field and check its access
rg "pub network" key-wallet/src/managed_account/address_pool.rs --type rust -B 1 -A 1Repository: dashpay/rust-dashcore
Length of output: 163
🏁 Script executed:
# Final check: Look for any tests covering bincode deserialization with mismatched data
rg "test.*bincode|bincode.*test" key-wallet/src/managed_account/managed_platform_account.rs --type rust -A 10Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check if there are any other deserialization validators or invariant checks
rg "validate|check_invariant" key-wallet/src/managed_account/managed_platform_account.rs --type rust -B 2 -A 5Repository: dashpay/rust-dashcore
Length of output: 47
Validate network consistency and total balance on deserialization from untrusted bincode data.
ManagedPlatformAccount accepts deserialized network and credit_balance values without validation against the AddressPool and per-address balances. This violates the invariant established in the constructor (where network = addresses.network) and in normal balance operations (where credit_balance equals the sum of per-address balances). Untrusted input could cause mixed-network states or incorrect totals.
🔧 Suggested fix (normalize to consistent state)
- let network = bincode::Decode::decode(decoder)?;
- let credit_balance = bincode::Decode::decode(decoder)?;
+ let mut network = bincode::Decode::decode(decoder)?;
+ let mut credit_balance: u64 = bincode::Decode::decode(decoder)?;
@@
- let addresses = bincode::Decode::decode(decoder)?;
+ let addresses: AddressPool = bincode::Decode::decode(decoder)?;
+ if addresses.network != network {
+ network = addresses.network;
+ }
@@
- Ok(Self {
+ credit_balance = address_balances.values().sum();
+ Ok(Self {
account,
key_class,
network,🤖 Prompt for AI Agents
In `@key-wallet/src/managed_account/managed_platform_account.rs` around lines 363
- 392, The Decode impl for ManagedPlatformAccount currently trusts decoded
network and credit_balance; update the bincode::Decode::decode implementation to
enforce invariants by (1) verifying that every key in address_balances and
addresses.network are consistent (or at minimum set network = addresses.network)
and (2) recomputing credit_balance as the sum of values in address_balances and
using that computed total instead of the decoded credit_balance when
constructing Self; make these adjustments inside the Decode for
ManagedPlatformAccount (the function that decodes account, key_class, network,
credit_balance, address_balances, addresses, metadata, is_watch_only) so the
returned instance always has network == addresses.network and credit_balance ==
sum(address_balances).
| /// Convert to a dashcore::Address (P2PKH) | ||
| /// | ||
| /// This creates a standard P2PKH address from the hash. | ||
| /// Note: This is for interoperability - Platform addresses typically | ||
| /// use bech32m encoding rather than base58 P2PKH format. | ||
| pub fn to_address(&self, network: Network) -> Address { | ||
| let pubkey_hash = Hash160::from_slice(&self.0).expect("20 bytes is valid for Hash160"); | ||
| let payload = Payload::PubkeyHash(pubkey_hash.into()); | ||
| Address::new(network, payload) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and understand the code
find . -name "platform_address.rs" -type f | head -5Repository: dashpay/rust-dashcore
Length of output: 117
🏁 Script executed:
# Check if Hash160 is used elsewhere in the codebase
rg "Hash160" --type rs | head -20Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Look for the PlatformP2PKHAddress definition to understand the structure
rg "struct PlatformP2PKHAddress\|type PlatformP2PKHAddress" --type rs -A 5Repository: dashpay/rust-dashcore
Length of output: 91
🌐 Web query:
Hash160 constructor from_byte_array dashcore rust crate
💡 Result:
In dashcore, Hash160 is the HASH160 newtype dashcore::hashes::hash160::Hash, and the constructor you want is:
use dashcore::hashes::hash160;
let bytes: [u8; 20] = /* ... */;
let h160 = hash160::Hash::from_byte_array(bytes);hash160::Hash also provides zero-copy conversions if you already have a fixed-size array reference:
use dashcore::hashes::hash160;
let bytes_ref: &[u8; 20] = /* ... */;
let h160_ref: &hash160::Hash = hash160::Hash::from_bytes_ref(bytes_ref);[1] [2]
🏁 Script executed:
cat -n ./key-wallet/src/managed_account/platform_address.rs | head -75Repository: dashpay/rust-dashcore
Length of output: 3110
🏁 Script executed:
# Get the struct definition
sed -n '1,100p' ./key-wallet/src/managed_account/platform_address.rsRepository: dashpay/rust-dashcore
Length of output: 3428
Use Hash160::from_byte_array() to eliminate expect() in library code.
Hash160::from_slice(&self.0).expect(...) at line 68 violates the guideline to avoid expect() in library code. Since self.0 is already a fixed-size [u8; 20] array, use the infallible Hash160::from_byte_array() constructor instead.
Fix
- let pubkey_hash = Hash160::from_slice(&self.0).expect("20 bytes is valid for Hash160");
+ let pubkey_hash = Hash160::from_byte_array(self.0);🤖 Prompt for AI Agents
In `@key-wallet/src/managed_account/platform_address.rs` around lines 62 - 71, The
to_address method uses Hash160::from_slice(&self.0).expect(...) which calls
expect in library code; replace that with the infallible
Hash160::from_byte_array(self.0) (or equivalent method taking [u8;20]) to
construct pubkey_hash without panicking, then build
Payload::PubkeyHash(pubkey_hash.into()) and return Address::new(network,
payload); update references to self.0 accordingly and remove the expect call.
PastaPastaPasta
force-pushed
the
codex/pr-368-clean
branch
from
c233cfc to
a871a8b
Compare
this is a vesion of #368, with cleaned git history by codex. there should be no changes in the diff
ManagedAccounttoManagedCoreAccountthroughout the codebase to align with the new account structure.WalletBalancetoWalletCoreBalanceto reflect the updated balance management.This update enhances the overall architecture by standardizing account management and balance tracking across the application.
Summary by CodeRabbit
Release Notes
New Features
Refactoring
Bug Fixes