-
Notifications
You must be signed in to change notification settings - Fork 58
feat(platform-wallet): report the balance a pooled build can actually spend #4582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/core-tx-builder-only-added-inputs
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,6 +299,61 @@ pub(crate) fn resolve_source_accounts( | |
| impl<B: TransactionBroadcaster + ?Sized> CoreWallet<B> { | ||
| /// Consume a configured builder, atomically fund and reserve its selected | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc comment got captured by the new function. The insertion landed between
|
||
| /// inputs, then sign without holding the wallet-manager lock. | ||
| /// The balance a pooled build could actually select from — the same | ||
| /// accounts [`Self::finalize_transaction`] funds from, counting only UTXOs | ||
| /// coin selection would accept. | ||
| /// | ||
| /// Hosts gate their amount entry on this. The wallet-level balance is a | ||
| /// strict superset: it sums every funding account, CoinJoin included, and | ||
| /// never consults a reservation set, so gating on it offers money the build | ||
| /// then refuses — the shortfall surfacing as | ||
| /// [`CorePooledInsufficientFunds`](PlatformWalletError::CorePooledInsufficientFunds) | ||
| /// after the user has already committed to an amount. | ||
| /// | ||
| /// Missing sources are skipped, as in a pooled build: a wallet without a | ||
| /// BIP32 account or without DashPay contacts still has a spendable balance. | ||
| /// | ||
| /// Reservations are NOT subtracted: key-wallet keeps each account's | ||
| /// `ReservationSet` private, so reading it needs an accessor there and a pin | ||
| /// bump. The figure is therefore optimistic by whatever another in-flight | ||
| /// build currently holds — transient by construction, since a reservation is | ||
| /// released when its spend is processed, on a definitive broadcast | ||
| /// rejection, at the TTL, or on restart. The account-set mismatch this fixes | ||
| /// is permanent, and was the whole of the shortfall in the report that | ||
| /// prompted it (support ticket 32081: 0.0054 DASH offered as spendable | ||
| /// against a 94 DASH balance, all of it CoinJoin). | ||
| pub async fn pooled_spendable_balance( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth a test — the fixtures are already there.
Nothing currently pins this function to |
||
| &self, | ||
| sources: &[AccountTypePreference], | ||
| source_index: u32, | ||
| ) -> Result<u64, PlatformWalletError> { | ||
| let mut manager = self.wallet_manager.write().await; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Write lock for a pure read. Nothing here mutates. Read-only equivalents exist and are what the rest of the package uses:
so this can be It matters because of how this is meant to be called — gating amount entry means potentially one call per keystroke, each taking the exclusive manager lock against concurrent finalizers, broadcast reconciliation and SPV sync writers. Every other read-only path in the package already takes the read lock: |
||
| let (_wallet, info) = manager | ||
| .get_wallet_and_info_mut(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound("wallet not found".into()))?; | ||
| let height = info.core_wallet.last_processed_height(); | ||
|
|
||
| let mut seen: HashSet<AccountType> = HashSet::new(); | ||
| let mut total: u64 = 0; | ||
| for &preference in sources { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Single-source selectors silently return 0 instead of the strict not-found error.
That also makes the doc's "the same accounts |
||
| for at in resolve_source_accounts(&info.core_wallet.accounts, preference, source_index) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This loop is a second copy of the one in The PR's argument is that the host's hand-copy of the pooling rule drifted, so hosts should read it from the source of truth instead. But the new source of truth is itself a hand-copy of the funding loop — and it has already drifted twice before merge (the two comments above). Extracting something like fn resolved_funding_accounts(
accounts: &ManagedAccountCollection,
wallet: &Wallet,
sources: &[AccountTypePreference],
source_index: u32,
strict: bool,
) -> Result<Vec<AccountType>, PlatformWalletError>and driving both call sites from it makes the two impossible to desynchronise, and drops the duplicated dedup |
||
| { | ||
| if !seen.insert(at) { | ||
| continue; | ||
| } | ||
| let Some(managed) = info.core_wallet.accounts.funds_account_mut(&at) else { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Diverges from
let (Some(account), Some(managed)) = (
wallet.accounts.account_of_type(at),
info.core_wallet.accounts.funds_account_mut(&at),
) else { ... continue; };This counts an account when only The discarded |
||
| continue; | ||
| }; | ||
| total += managed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fee isn't subtracted, so this ceiling is still unspendable as an amount. The doc above says hosts should gate amount entry on this, but the value returned is the gross sum of spendable UTXOs. A build needs
With the 500-UTXO sweep landing alongside (#4548), the input count and therefore the fee can be well above dust, so this isn't a rounding concern. Either subtract an estimated fee here, or state in the doc that the figure is pre-fee and hosts must reserve headroom — but given the failure mode this is meant to prevent, I'd rather it be handled here than left as a second thing for hosts to mirror. |
||
| .spendable_utxos(height) | ||
| .iter() | ||
| .map(|utxo| utxo.value()) | ||
| .sum::<u64>(); | ||
| } | ||
| } | ||
| Ok(total) | ||
| } | ||
|
|
||
| pub async fn finalize_transaction<S: TransactionSigner + ?Sized + Sync>( | ||
| &self, | ||
| builder: TransactionBuilder, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same capture here.
The doc block starting at line 629 ("Fund the build from the inputs
core_wallet_tx_builder_add_inputs_from_outpointssupplied, and nothing else" … "fails with a too-many-inputs error") belongs tocore_wallet_tx_builder_use_only_added_inputs, which now sits at line 674 with only the boilerplate# Safetynote.This one leaks further than the Rust-side twin: cbindgen emits these into the generated C header the Swift SDK imports, so the public header will describe a balance getter in terms of batched-drain funding, while
use_only_added_inputs— new in the base PR, and whose entire rationale lived in that block — ships with no explanation at all.