Procházet zdrojové kódy

Single-source the overdraft decision across resolve and validate

The overdraft policy was encoded twice. resolve_envelope granted a
shortfall offset posting based on a HashSet<AccountId> that the async
layer assembled from account.forbids_overdraft(), while validate_and_plan
read the same flag directly off each Account. The HashSet was a lossy
re-encoding: if the async layer built it wrong, resolve and validate
could silently disagree and neither pure module would reveal the bug.

Have both passes read the decision from one place. ResolveInput now takes
the debit accounts (HashMap<AccountId, Account>) instead of a pre-derived
permit set, and resolve_envelope consults Account::forbids_overdraft(),
the same accessor validation uses. A missing account still counts as
forbidding overdraft, so the prior unknown-account behavior is preserved.
The async resolve() loads the debit accounts into a map and hands it
straight to the pure pass, dropping the now-unused set.
Cesar Rodas před 2 týdny
rodič
revize
3e040dba69

+ 49 - 31
crates/kuatia-core/src/posting_resolution.rs

@@ -5,19 +5,21 @@
 //!
 //! 1. [`draft_movements`] aggregates movements into output postings and
 //!    per-(account, asset) net debits. It tells the async layer exactly which
-//!    postings to load and which debit accounts permit overdraft.
+//!    postings to load and which accounts to fetch.
 //! 2. [`resolve_envelope`] selects postings for each debit, computes change, and
-//!    covers an overdraft shortfall with a negative offset posting.
+//!    covers an overdraft shortfall with a negative offset posting. Whether an
+//!    account may overdraw is read straight from its [`Account::forbids_overdraft`]
+//!    flag, the same accessor validation reads, so the two passes cannot disagree.
 //!
 //! The async ledger loads state; this module decides. The change-making and
 //! shortfall branches are the parts most worth property-testing, and living here
 //! they are reachable without standing up a store.
 
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 
 use kuatia_types::{
-    AccountId, AssetId, Cent, Envelope, EnvelopeBuilder, NewPosting, OverflowError, Posting,
-    PostingId, Transfer,
+    Account, AccountId, AssetId, Cent, Envelope, EnvelopeBuilder, NewPosting, OverflowError,
+    Posting, PostingId, Transfer,
 };
 
 // ---------------------------------------------------------------------------
@@ -149,8 +151,8 @@ pub fn draft_movements(transfer: &Transfer) -> Result<MovementDraft, OverflowErr
 // ---------------------------------------------------------------------------
 
 /// Pre-loaded state for resolution pass 2. The async layer gathers `available`
-/// and `overdraft_allowed` for the debits produced by [`draft_movements`]; this
-/// pass is pure.
+/// and `accounts` for the debits produced by [`draft_movements`]; this pass is
+/// pure.
 pub struct ResolveInput<'a> {
     /// The transfer being resolved (for its book and metadata).
     pub transfer: &'a Transfer,
@@ -160,13 +162,14 @@ pub struct ResolveInput<'a> {
     /// Active postings available for each debit's (account, asset). A missing or
     /// empty entry means no positive postings to draw on.
     pub available: &'a HashMap<(AccountId, AssetId), Vec<Posting>>,
-    /// Accounts that permit overdraft (i.e. that do *not* carry
-    /// `DEBIT_MUST_NOT_EXCEED_CREDIT`). A debit short of positive postings gets a
-    /// negative offset posting only if its account is in this set; otherwise it
-    /// fails with [`InsufficientFunds`]. A missing account is
-    /// treated as forbidding overdraft, so an unknown account never gets an
-    /// offset position on unknown terms.
-    pub overdraft_allowed: &'a HashSet<AccountId>,
+    /// The debit accounts, keyed by id. A debit short of positive postings gets a
+    /// negative offset posting only if its account permits overdraft (does *not*
+    /// carry `DEBIT_MUST_NOT_EXCEED_CREDIT`, read via
+    /// [`Account::forbids_overdraft`]); otherwise it fails with
+    /// [`InsufficientFunds`]. This is the same flag validation reads, so resolve
+    /// and validate cannot disagree. A missing account is treated as forbidding
+    /// overdraft, so an unknown account never gets an offset posting.
+    pub accounts: &'a HashMap<AccountId, Account>,
 }
 
 /// Pass 2: for each debit, either select postings and compute change, or (for an
@@ -179,7 +182,7 @@ pub fn resolve_envelope(input: ResolveInput<'_>) -> Result<Envelope, ResolveErro
         transfer,
         draft,
         available,
-        overdraft_allowed,
+        accounts,
     } = input;
     let MovementDraft {
         mut creates,
@@ -228,8 +231,12 @@ pub fn resolve_envelope(input: ResolveInput<'_>) -> Result<Envelope, ResolveErro
         } else {
             // Not enough positive postings. An account that permits overdraft
             // covers the shortfall with a negative posting (an offset position);
-            // one that forbids it — or an unknown account — fails.
-            if overdraft_allowed.contains(&debit.account) {
+            // one that forbids it, or an unknown account, fails. The decision is
+            // the account's own flag, the same one validation reads.
+            let permits_overdraft = accounts
+                .get(&debit.account)
+                .is_some_and(|a| !a.forbids_overdraft());
+            if permits_overdraft {
                 let positives: Vec<PostingId> = avail
                     .iter()
                     .filter(|p| p.value.is_positive())
@@ -287,6 +294,15 @@ mod tests {
             .build()
     }
 
+    fn accounts(list: impl IntoIterator<Item = Account>) -> HashMap<AccountId, Account> {
+        list.into_iter().map(|a| (a.id, a)).collect()
+    }
+
+    /// An account that forbids overdraft (carries `DEBIT_MUST_NOT_EXCEED_CREDIT`).
+    fn no_overdraft(id: AccountId) -> Account {
+        Account::debit_must_not_exceed_credit(id)
+    }
+
     #[test]
     fn draft_aggregates_net_debit_and_output() {
         let draft = draft_movements(&pay(acct(1), acct(2), 100)).unwrap();
@@ -329,12 +345,12 @@ mod tests {
             (acct(1), AssetId::new(1)),
             vec![posting(acct(1), 0, 60), posting(acct(1), 1, 40)],
         )]);
-        let overdraft_allowed = HashSet::new();
+        let accounts = accounts([no_overdraft(acct(1))]);
         let env = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap();
         assert_eq!(env.consumes().len(), 2);
@@ -349,12 +365,12 @@ mod tests {
         let draft = draft_movements(&transfer).unwrap();
         let available =
             HashMap::from([((acct(1), AssetId::new(1)), vec![posting(acct(1), 0, 100)])]);
-        let overdraft_allowed = HashSet::new();
+        let accounts = accounts([no_overdraft(acct(1))]);
         let env = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap();
         assert_eq!(env.consumes().len(), 1);
@@ -382,12 +398,12 @@ mod tests {
                 posting(acct(1), 2, 50),
             ],
         )]);
-        let overdraft_allowed = HashSet::new();
+        let accounts = accounts([no_overdraft(acct(1))]);
         let env = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap();
         assert_eq!(env.consumes().len(), 1);
@@ -406,12 +422,12 @@ mod tests {
         let draft = draft_movements(&transfer).unwrap();
         let available =
             HashMap::from([((acct(1), AssetId::new(1)), vec![posting(acct(1), 0, 40)])]);
-        let overdraft_allowed = HashSet::new();
+        let accounts = accounts([no_overdraft(acct(1))]);
         let err = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap_err();
         assert_eq!(
@@ -428,12 +444,14 @@ mod tests {
         let transfer = pay(acct(1), acct(2), 100);
         let draft = draft_movements(&transfer).unwrap();
         let available = HashMap::new();
-        let overdraft_allowed = HashSet::new();
+        // The payer account is absent from the map, so it must be treated as
+        // forbidding overdraft.
+        let accounts = accounts([]);
         let err = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap_err();
         assert_eq!(
@@ -451,12 +469,12 @@ mod tests {
         let draft = draft_movements(&transfer).unwrap();
         let available =
             HashMap::from([((acct(1), AssetId::new(1)), vec![posting(acct(1), 0, 30)])]);
-        let overdraft_allowed = HashSet::from([acct(1)]);
+        let accounts = accounts([Account::new(acct(1))]);
         let env = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap();
         // The single positive posting is consumed.
@@ -478,12 +496,12 @@ mod tests {
         let transfer = pay(acct(1), acct(2), 100);
         let draft = draft_movements(&transfer).unwrap();
         let available = HashMap::new();
-        let overdraft_allowed = HashSet::from([acct(1)]);
+        let accounts = accounts([Account::new(acct(1))]);
         let env = resolve_envelope(ResolveInput {
             transfer: &transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })
         .unwrap();
         assert!(env.consumes().is_empty());

+ 13 - 17
crates/kuatia/src/ledger/commit.rs

@@ -6,7 +6,7 @@
 //! lets [`Ledger::recover`] complete or safely abandon a commit interrupted by a
 //! crash.
 
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::sync::Arc;
 
 use legend::ExecutionResult;
@@ -172,18 +172,20 @@ impl Ledger {
     /// The decision is pure ([`kuatia_core::draft_movements`] +
     /// [`kuatia_core::resolve_envelope`]); this method only loads the state those
     /// functions need. Pass 1 aggregates net debits and tells us which postings
-    /// to load and which accounts permit overdraft; pass 2 selects postings,
-    /// computes change, and covers any overdraft shortfall.
+    /// and accounts to load; pass 2 selects postings, computes change, and covers
+    /// any overdraft shortfall (reading each account's own flag, the same one
+    /// validation reads).
     #[instrument(skip(self, transfer), name = "ledger.resolve")]
     pub async fn resolve(&self, transfer: &Transfer) -> Result<Envelope, LedgerError> {
         let draft = draft_movements(transfer)?;
 
-        // Load the active postings for each debit, and note which debit accounts
-        // permit overdraft. A deposit nets to zero on the system account, so it
-        // produces no debit and loads nothing here.
+        // Load the active postings for each debit and the debit accounts
+        // themselves. Pass 2 reads the overdraft decision off each account's flag,
+        // so we hand it the accounts rather than a re-derived set. A deposit nets
+        // to zero on the system account, so it produces no debit and loads nothing
+        // here.
         let mut available: HashMap<(AccountId, AssetId), Vec<Posting>> = HashMap::new();
-        let mut overdraft_allowed: HashSet<AccountId> = HashSet::new();
-        let mut checked: HashSet<AccountId> = HashSet::new();
+        let mut accounts: HashMap<AccountId, kuatia_core::Account> = HashMap::new();
         for debit in &draft.debits {
             let postings = self
                 .store
@@ -195,14 +197,8 @@ impl Ledger {
                 )
                 .await?;
             available.insert((debit.account, debit.asset), postings);
-            if checked.insert(debit.account)
-                && !self
-                    .store
-                    .get_account(&debit.account)
-                    .await?
-                    .forbids_overdraft()
-            {
-                overdraft_allowed.insert(debit.account);
+            if let std::collections::hash_map::Entry::Vacant(e) = accounts.entry(debit.account) {
+                e.insert(self.store.get_account(&debit.account).await?);
             }
         }
 
@@ -210,7 +206,7 @@ impl Ledger {
             transfer,
             draft,
             available: &available,
-            overdraft_allowed: &overdraft_allowed,
+            accounts: &accounts,
         })?;
 
         // Resolve account snapshots for optimistic concurrency