fix(settlement,distribute): reconcile TotalReceived across all credit paths and fix batch_distribute compile - #1362
Open
olaleyeolajide81-sketch wants to merge 4 commits into
Conversation
…tribute signature Closes CalloraOrg#1149, Closes CalloraOrg#1171 ## Issue CalloraOrg#1149 — Reconcile TotalReceived with direct payment credits ### Problem `TotalReceived` was only incremented by `record_deduction`. Both `receive_payment` (pool and developer branches) and `batch_receive_payment` credited balances without touching the metric, causing `get_total_received` to report a number unrelated to what was actually credited. Dashboards and conservation checks comparing `TotalReceived` with pool + developer balances always drifted. ### Fix - `receive_payment` (pool branch): added checked_add of `amount` to `TotalReceived` after pool credit. - `receive_payment` (developer branch): added checked_add of `amount` to `TotalReceived` after developer balance update. - `batch_receive_payment`: accumulate `batch_total` across the loop and write `TotalReceived` once after all items are processed (single storage round-trip, not per-item). - `get_total_received` rustdoc updated to precisely list all contributing paths: both `receive_payment` branches, `batch_receive_payment`, and `record_deduction`. - `record_deduction` rustdoc updated to clarify its role alongside the standard credit paths. ### Tests - `check_invariant` in `test_invariant.rs` extended with an `expected_total_received` parameter that verifies `get_total_received() == expected_credits` after every operation. - `run_trace` now tracks `expected_total_received` separately from `expected_dev_total`: credits increment it, withdrawals do not. - New targeted tests added to `test_invariant.rs`: - `test_total_received_zero_on_init` - `test_total_received_incremented_by_pool_payment` - `test_total_received_incremented_by_batch_receive` - `test_total_received_not_decremented_by_withdrawal` - `test_invariant_pool_only`, `test_invariant_single_dev_full_withdraw`, and `test_invariant_interleaved_dev_and_pool` all now assert `get_total_received` at each step. - Updated `tests/proptest.rs::test_invariant_record_deduction` to assert the correct post-fix value (1800 not 1500 after a 300-unit `receive_payment` following two `record_deduction` calls). - Fixed pre-existing compile errors in `tests/proptest.rs` (.unwrap() on non-Option GlobalPool and i128). ## Issue CalloraOrg#1171 — Unbreak distribute compile on batch payment signature ### Problem `cargo check -p callora-distribute` failed with "generics unsupported on user-defined types in contract functions" because `batch_distribute` declared its `payments` parameter as `SorobanVec<(Address, i128)>` — a `use soroban_sdk::Vec as SorobanVec` alias. The Soroban contract macro resolves parameter types by name and cannot see through import aliases, so it rejected the type. This broke the entire distribute crate, its fuzz crate, and the batch_distribute fuzz crate. ### Fix - Removed the `Vec as SorobanVec` alias; import changed to `Vec`. - `batch_distribute` parameter type changed from `SorobanVec<(Address, i128)>` to `Vec<(Address, i128)>` (concrete SDK type — macro-transparent). - `get_max_batch_size` env parameter renamed from `env` to `_env` to suppress the unused-parameter Clippy/compiler warning. - `events::event_version_v1` symbol fixed from `"callora.v1"` to `"callora_v1"`: the period character (ASCII 46) is not permitted in Soroban `Symbol` values, causing a panic in every test that called `init`. - Dead-code error string constants annotated with `#[allow(dead_code)]` so `-D warnings` passes (they appear only in rustdoc comments). ### Tests - Rewrote `tests/auth_snap.rs` to use the correct `Distribute` / `DistributeClient` API instead of the non-existent `CalloraDistribute` / `CalloraDistributeClient` it previously referenced. - Fixed event structure assertions in `src/test.rs` to match the three-topic layout `(event_name, version, caller/recipient)` emitted by the contract. - Fixed `require_auth_on_all_state_changing_functions` to mock all auths during setup before clearing them for intruder calls. - Added `batch_distribute_accepts_soroban_vec_type` regression test that explicitly uses `soroban_sdk::Vec` (the concrete type) in the call site to guard against the alias regression. - All 63 distribute tests now pass (43 lib + 20 integration).
This was referenced Oct 2, 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
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.
Summary
This pull request resolves two separate but related smart-contract correctness issues:
TotalReceivedwas never updated byreceive_paymentorbatch_receive_payment, makingget_total_received()an unreliable metric.cargo check -p callora-distributefailed becausebatch_distributeused aVec as SorobanVecimport alias the Soroban contract macro cannot resolve.Issue #1149 — Reconcile TotalReceived with direct payment credits
Root cause
TotalReceived(stored underStorageKey::TotalReceived) was only incremented byrecord_deduction. Bothreceive_payment— in its pool branch (to_pool = true) and its developer branch (to_pool = false) — andbatch_receive_paymentcredited balances without touching the metric. As a result,get_total_received()returned the sum of accounting-only vault deductions, while every real on-chain credit went uncounted. Any dashboard or conservation check comparingTotalReceivedagainstpool_balance + sum(dev_balances)showed perpetual drift and could never detect genuine discrepancies.Changes
contracts/settlement/src/lib.rsreceive_payment(pool branch): after writing the updatedglobal_pool.total_balance, readsTotalReceivedand increments it byamountusingchecked_add(panicsPoolOverflowon overflow).receive_payment(developer branch): after writing the updated developer balance and index, readsTotalReceivedand increments it byamountusingchecked_add(panicsDeveloperOverflowon overflow).batch_receive_payment: accumulatesbatch_totalacross the per-item loop, then performs a singleTotalReceivedwrite after all items succeed. This avoids one storage read/write per item and keeps the atomicity model: if any item fails validation the loop never runs andTotalReceivedis not touched.get_total_receivedrustdoc rewritten to exhaustively enumerate every contributing path:receive_payment(pool branch),receive_payment(developer branch),batch_receive_payment, andrecord_deduction.record_deductionrustdoc updated to note that it exists for accounting-only vault deductions outside the standard credit flow, and thatTotalReceivedis also incremented by the standard credit paths.Invariant — definition
TotalReceivedis a monotonically increasing inbound-credit counter. It increases on every credit (receive_paymentandbatch_receive_payment) and every accounting deduction (record_deduction). Withdrawals viawithdraw_developer_balanceare debits from current balances and do not reduceTotalReceived.The full conservation invariant is therefore:
or equivalently before any withdrawals:
Tests —
contracts/settlement/src/test_invariant.rscheck_invariantextended with a newexpected_total_received: i128parameter that callsclient.get_total_received()and asserts it equals the running tally after every operation.run_tracenow tracksexpected_total_receivedindependently:ReceiveDevandReceivePoolops each addamountto it.BatchReceiveDevaddsbatch_totalon success.Withdrawdoes not change it (correctly models the monotonic-counter semantics).test_invariant_pool_only,test_invariant_single_dev_full_withdraw,test_invariant_interleaved_dev_and_pool— to assertget_total_received()at each step.test_total_received_zero_on_init— metric starts at 0.test_total_received_incremented_by_pool_payment— pool and developer payments both increment.test_total_received_incremented_by_batch_receive— batch credits are summed correctly.test_total_received_not_decremented_by_withdrawal— withdrawals leave the counter unchanged.Tests —
contracts/settlement/tests/proptest.rs.unwrap()calls onGlobalPool(a struct, not aResult/Option) and oni128.test_invariant_record_deductionto assertTotalReceived == 1_800(not1_500) after a 300-unitreceive_paymentfollowing tworecord_deductioncalls of 1 000 and 500 — the old assertion encoded the pre-fix broken behaviour.Validation
Issue #1171 — Unbreak distribute compile on batch payment signature
Root cause
contracts/distribute/src/lib.rsimportedsoroban_sdk::Vecunder the aliasSorobanVec:and used the alias as the parameter type for the public contract function
batch_distribute:The Soroban
#[contractimpl]macro resolves parameter types by their literal token-stream name. BecauseSorobanVecis an alias — not the canonical path the macro recognises — it emitted:This broke the distribute crate,
contracts/distribute/fuzz, andcontracts/batch_distribute/fuzz(both fuzz crates importcallora-distribute).A compounding pre-existing bug:
events::event_version_v1returnedSymbol::new(env, "callora.v1"). SorobanSymbolvalues may not contain the period character (ASCII 46). Any test that calledinit— which publishes a version-tagged event — panicked at runtime, leaving the 700-line test file producing zero successful test runs.Changes
contracts/distribute/src/lib.rsVec as SorobanVecchanged to plainVecimport.batch_distributeparameter type:SorobanVec<(Address, i128)>changed toVec<(Address, i128)>(macro-transparent concrete type).get_max_batch_sizeparameter:env: Envchanged to_env: Env(suppresses unused-variable Clippy warning, satisfying-D warnings).#[allow(dead_code)](they appear only in rustdoc# Panicssections; the crate uses typedDistributeErrorvariants for runtime errors).contracts/distribute/src/events.rsevent_version_v1: symbol"callora.v1"changed to"callora_v1"(period replaced with underscore — only[a-zA-Z0-9_]are valid in Soroban Symbols).contracts/distribute/src/test.rstopics.get(1)for address) to two-index (topics.get(2)for address) to match the actual three-topic layout(event_name, version, caller/recipient)emitted by the contract.require_auth_on_all_state_changing_functions: setup now usesenv.mock_all_auths()for the init and mint calls, then callsenv.set_auths(&[])before the intruder tests. The previous approach did not mock auths for the USDCmintcall, causing anInvalidActionhost panic.contracts/distribute/tests/auth_snap.rsRewrote from scratch. The previous version imported
CalloraDistribute,CalloraDistributeClient,Severity, andBatchItem— none of which exist incallora-distribute. The actual contract is namedDistributeand its generated client isDistributeClient. The new file:set_admin,accept_admin,claim_admin,cancel_admin_transfer,pause,unpause,set_max_distribute,distribute,batch_distribute,upgrade) withenv.set_auths(&[])to assert auth is required.get_admin,get_usdc_token,get_paused,get_max_distribute,get_max_batch_size,get_pending_admin,get_version,balance) without auth to assert they succeed.admin_with_auth_can_use_all_entrypoints— full happy-path smoke test exercising every entrypoint.batch_distribute_accepts_soroban_vec_type— explicit regression test for Unbreak distribute compile on batch payment signature #1171 that constructssoroban_sdk::Vec<(Address, i128)>at the call site (the concrete type, not an alias) to guard against the alias regression.Validation
CI pre-checks
cargo fmt -p callora-distribute -p callora-settlement -- --checkcargo clippy -p callora-distribute -- -D warningscargo clippy -p callora-settlement -- -D warningscargo build -p callora-distributecargo build -p callora-settlementcargo test -p callora-distributecargo test -p callora-settlement invariantPre-existing failures unrelated to this PR (confirmed present on
mainbefore any changes):test_ttl_bump::*(18 tests)env.as_contract()wrappertest_events::test_upgrade_emits_upgraded_eventtest_overflow_safe_math::withdraw_daily_amount_overflow_raises_errorSecurity and compatibility
TotalReceivedincrements usechecked_add, consistent with every other balance mutation in the contract. Overflow panics withPoolOverfloworDeveloperOverflowas appropriate.batch_totallocally and writes once after the item loop. If any item fails validation before the loop runs,TotalReceivedis not modified — identical to the developer balance semantics.get_total_received()return type (i128) andbatch_distributeparameter type (Vec<(Address, i128)>) are unchanged from the caller's perspective.StorageKey::TotalReceivedalready existed and was already initialised to0i128ininit. No migration needed."callora_v1"replaces the invalid"callora.v1"in the distribute event version topic. Since the old string caused a runtime panic ininit, no production events with"callora.v1"could ever have been emitted, so no off-chain indexer migration is required.