fix(#467): standardize validation ordering for set_settlement_rule and set_default_rule - #3
Open
Seunfunmi-319509 wants to merge 309 commits into
Open
fix(#467): standardize validation ordering for set_settlement_rule and set_default_rule#3Seunfunmi-319509 wants to merge 309 commits into
Seunfunmi-319509 wants to merge 309 commits into
Conversation
…mount-docstring Settlement Contract — Fee Split & Payment Reference Library
…file-targets Add Common Development Convenience Targets to the Makefile
…ring Document InvalidSettlementDelay Panic Condition in set_default_rule
…-convention docs: document event-topic convention in module docstrings
…oban-typo fix: rename misspelled Makefile variable SORBORN to SOROBAN
…ring fix: validate caller in transfer_admin and update docstring
…or-test-naming chore(governance): rename anchor_removal_test.rs -> anchor_removal_tests.rs (closes Betta-Pay#346)
docs: remove stray conversational text from README.md
docs: add .env.example for deployment configuration
…mount-error # Conflicts: # governance_contract/src/lib.rs # settlement_contract/src/lib.rs
…etta-Pay#259) Add initialization check to is_paused() that panics with GovernanceError::NotInitialized when the contract has not been initialized (i.e., when Admin key is not present in instance storage). This prevents misinterpretation of the result as 'running normally' for an uninitialized contract. Also rename the internal is_paused helper to is_paused_internal to avoid confusion with the public method.
…est-suite Rewrite settlement test suite
…-Pay#310, Betta-Pay#312, Betta-Pay#313 main currently fails cargo build, cargo test, cargo clippy, and cargo fmt --check outright, independent of the four issues below, so none of that work could land on a green CI without also fixing it: - governance_contract: upsert_anchor referenced an undefined `old_anchor` variable (production code, wouldn't compile at all). - governance_contract: Symbol's length check used `.to_string()`, whose ToString impl is gated `not(target_family = "wasm")` in soroban-sdk 21.7.7 - it can never compile for the wasm32 release target. Replaced with a SymbolStr-based length check that works on every target. - governance_contract::get_anchor and settlement_contract::get_payment_reference called `.get_ttl()` in production code, a method that only exists via the SDK's test-only `testutils::storage` traits. It happened to compile under `cargo test`/`clippy --all-targets` (dev-deps pull the feature in) but fails a plain `cargo build`/`cargo build --release --target wasm32-unknown-unknown` - the exact "Compile WASM Release Binaries" CI step. Replaced with an unconditional `extend_ttl(threshold, bump)` call, which already only writes when the current TTL is below `threshold` (per the SDK's own doc comment), so behavior is unchanged. - settlement_contract had a stray unconditional `use soroban_sdk::testutils::storage::Persistent;` in production code (a leftover attempt at the get_ttl fix above) that doesn't compile outside test builds. - Several tests never actually compiled/ran before (main didn't build), so their own bugs were never caught: two governance test files called the old one-argument `init(&admin)` signature, one referenced an undefined `recovery_address`, one used a nonexistent `Address::from_str`, test code calling `.get_ttl()` was missing the testutils trait import, a TTL assertion mixed up absolute vs. relative ledger math, and `admin_functions_work_while_paused` asserted `set_fee_config`/ `remove_anchor` are pause-exempt when they are not (only `update_system_param` is, by design). - clippy.toml used a schema from an incompatible clippy version (`too-many-arguments = "allow"` etc.), which made `cargo clippy` fail immediately with "unknown field" before reaching any crate's code. Converted to the numeric-threshold keys this clippy version expects. - The tree didn't satisfy `cargo fmt --all -- --check`; ran `cargo fmt --all`. - A handful of pre-existing clippy lints (`len_zero`, `unnecessary_unwrap`, `int_plus_one`) surfaced once the above compile errors were fixed. Issue Betta-Pay#309 (rejects_oversized_symbol_key relies on SDK internals): the requested fix - assert `GovernanceError::InvalidParamValue` instead of a bare SDK panic - turns out to be unreachable through the public SDK. Symbol's 32-character limit is enforced by the Stellar protocol itself (SCSYMBOL_LIMIT) at Symbol construction, not merely by an SDK convenience check, confirmed empirically: `Symbol::new` panics with `Error(Value, InvalidInput)` before the contract is ever invoked, so the contract's own length guard (kept, now portable, as defense-in-depth) can never actually fire. Updated the three affected tests (rejects_oversized_symbol_key and two near-duplicates) to assert that specific, verified SDK/protocol-level panic instead of a bare `#[should_panic]`, with comments explaining why. Issue Betta-Pay#310 (duplicated setup() across 3 governance test files): extracted one `pub(crate) fn setup()` at the crate root; anchor_event_tests.rs and anchor_removal_tests.rs now pick it up via their existing glob imports. anchor_auth_tests.rs existed on disk but was never wired into lib.rs via `mod anchor_auth_tests;` - it's wired in now, so its own duplicate setup() (and the tests it guarded) actually compile and run for the first time. Issues Betta-Pay#312/Betta-Pay#313 (read_admin, and is_paused/assert_not_paused, duplicated across both contracts): added a `shared` workspace crate exposing read_admin, is_paused, and assert_not_paused, generic over each contract's own `#[contracterror]` type via `E: Into<soroban_sdk::Error>`. Its private Admin/Paused key enum is safe to use for the same storage slots each contract's own DataKey enum already reads/writes elsewhere, since a Soroban #[contracttype] enum's storage encoding depends only on the variant name/shape, never the enclosing Rust type (verified against soroban-sdk-macros). Adopted governance's existing named TTL constants (the values two of its tests already lock in) as the shared canonical values; settlement's read_admin previously used unrelated bare literals with no test depending on the exact number. Verified: cargo fmt --all -- --check, cargo clippy --all-targets [--all-features] -- -D warnings, cargo test --all (154 tests), and cargo build --target wasm32-unknown-unknown --release all pass.
…etta-Pay#310 fixes main moved forward significantly while this branch was open - most notably 9171394 introduced a bettapay_common shared crate that already covers what this branch's own `shared` crate did for issues Betta-Pay#312/Betta-Pay#313 (read_admin, is_paused/assert_not_paused), plus constants and event helpers neither contract had unified before. Dropped this branch's `shared` crate entirely in favor of it rather than duplicating the same DRY-up two different ways. That merge (and others layered on top of it since) left main in a worse compiling state than when this branch started: 47 fresh errors across both contracts from what looks like an incomplete migration - missing `use bettapay_common::...` imports, a local helper function renamed at some call sites but not its definition (`assert_not_zero` vs `validate_nonzero_address`, same story as `assert_not_zero_address`), a local `DataKey` enum that still declared Admin/Paused/RecoveryAddress/PendingRecovery redundantly alongside the new shared `CommonDataKey` (with two straggler call sites still writing through the old local variant instead of the new one - not a storage-format issue since Soroban encodes contracttype enum variants by name/shape only, confirmed empirically, but still worth cleaning up for one source of truth), and a duplicate `SettlementError` discriminant from two independently-added error variants both claiming codes 20/21 (renumbered the newer ones to 23/24, verified no test asserts the exact numeric code). Also found and fixed, only reachable now that both contracts compile again: an `init` missing the `initialized` event its own test expects, a `transfer_admin`/pause-model doc comment (added by a separate, unrelated PR) that doesn't match what the code actually guards in either contract, and two `store_payment_reference` tests still asserting the pre-refactor `InvalidAmount` (Betta-Pay#7) error code instead of the `AmountTooSmall` (Betta-Pay#22) the split-error refactor actually produces. This branch's own fixes for issues Betta-Pay#309 (oversized-symbol test) and Betta-Pay#310 (duplicated setup()) were untouched by any of this - main never addressed either - and are preserved as-is on top of the reconciled base. Re-verified after resolving: cargo fmt --all -- --check, cargo clippy --all-targets [--all-features] -- -D warnings, cargo test --all (154 tests), and cargo build --target wasm32-unknown-unknown --release all pass.
Wrap long get_payment_reference chains and collapse an over-wrapped from_contract_error line in the orphan-policy tests so the new code is fmt-clean.
Wrap long get_payment_reference chains in the new auth-gating tests so the new code is fmt-clean.
Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
test: cover governance authorization paths
fix: cover multisig scheduled operations
…ment-rule-cleared-event fix: canonical settlement_rule_cleared event shape across both paths (closes Betta-Pay#491)
…rage test: add property coverage for fees and thresholds
…h-gating fix: gate payment-record reads behind merchant auth (closes Betta-Pay#492)
…on-merchant-unregister fix: orphan payment records when a merchant is unregistered (closes Betta-Pay#490)
…ment-reference-scope fix: scope payment references per merchant (closes Betta-Pay#493)
…order Step 2 previously said the merchant-specific rule is "stored globally via set_default_rule", which is self-contradictory and doesn't match the code: merchant-specific rules are stored per-merchant via set_settlement_rule (settlement.rs), while set_default_rule only writes the global DefaultRule key. Cross-checked against read_rule_or_default in storage.rs, which reads the merchant key before falling back to the global default key.
…rule-resolution-step
…scheduled ops Fixes Betta-Pay#572 and Betta-Pay#570. Betta-Pay#572: settlement's cross-contract decode of governance's FeeConfig already decodes by field name (soroban-sdk encodes named structs as a name-keyed ScMap, not positional), so it was already order-independent. Added fee_config_ordering_tests.rs to lock that guarantee in with an explicit regression test using a governance stub whose fields are declared in reverse order. Betta-Pay#570: ScheduledOperation was keyed by sha256(operation) alone, trusting a hash match as proof of content. schedule/execute/cancel now store and verify the full operation XDR (ScheduledOp) so a hash collision is detected (new OperationHashCollision error) instead of silently letting an unrelated operation ride another's pending slot. Also fixes three pre-existing, unrelated cargo test --workspace breakages found while verifying the above: missing imports in timelock_tests.rs, a stale try_update_governance() call in governance's real_auth_tests.rs, two governance proptest cases missing mock_all_auths(), a wrong argument type in integration_tests.rs, and a missing #[should_panic] on calculate_fee_split_rejects_amount_zero.
fix: order-independent fee decode test + hash-collision handling for scheduled ops
feat: implement ci jobs atterst wasm and recovery_executed event payload
Issue Betta-Pay#507. Ships SchemaVersion (u32) written at init, plus an admin-gated, idempotent migrate entry point so the first real storage migration has a defined baseline to distinguish 'pre-marker' from 'current' data. Adds MIGRATED_EVENT constant to bettapay_common, updates DEVELOPMENT.md, and regenerates the 60 committed test snapshots that now include the SchemaVersion storage write.
Issue Betta-Pay#514. execute_recovery loaded the old admin set just to build the recovery event, so a missing or empty Admin entry panicked (NotInitialized or primary_admin().unwrap()) before recovery could replace it — defeating recovery as a repair mechanism. Resolve the old admin as Option and fall back to the zero-address sentinel for the event, so recovery always succeeds. Adds a corruption-tolerance test.
Issue Betta-Pay#515. Governance read_admins bumped the instance entry with the standard 14/30-day policy while settlement used 50k/100k for the same conceptual admin entry, and ADR 003 prescribes 50k/100k for Admin & Governance reads. Switch read_admins to the shared 50k/100k policy and pin it with a TTL test. Regenerates the 7 snapshots that record the instance live-until value.
Issue Betta-Pay#516. ADR 001 (aligned with the code by commit b10289a) and the code already treat transfer_admin/change_threshold/upgrade/recovery as allowed while paused; only set_fee_config, upsert_anchor and remove_anchor are blocked. Make the lib.rs Pause Model doc and ADR 001 state the full matrix explicitly for both contracts, and pin it with pause_blocks_fee_and_anchor_writes plus pause_allows_admin_transfer_threshold_and_recovery.
Seunfunmi-319509
force-pushed
the
fix/467-validation-ordering-settlement-rule
branch
from
August 26, 2026 22:30
6d399e2 to
71ae2a1
Compare
…-515-516-governance-alignment fix: harden governance — schema version, recovery repair, admin TTL, pause matrix (Betta-Pay#507 Betta-Pay#514 Betta-Pay#515 Betta-Pay#516)
…t fallback drift, and dynamic min payment - Betta-Pay#687: execute_recovery now uses read_optional_primary_admin so corrupt admin state cannot brick the recovery mechanism - Betta-Pay#688: _register_merchant writes () to Merchant key, matching register_merchant's storage type - Betta-Pay#689: clear_settlement_rule and unregister_merchant events now reflect the governance fee fallback, not just the bootstrap default - Betta-Pay#690: MIN_PAYMENT_AMOUNT is read dynamically from governance get_system_param("min_payment"), falling back to 100 closes Betta-Pay#687, closes Betta-Pay#688, closes Betta-Pay#689, closes Betta-Pay#690
fix: resolve settlement recovery panic, merchant storage desync, even…
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
19 participants
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
Standardizes the validation ordering between the direct path (
settlement.rs) and the scheduled/timelocked path (admin.rs) so both paths return the same error for the same invalid input.Targets upstream: Betta-Pay/BettaPay-Contract#640
Problem
set_settlement_rule(settlement.rs) checked merchant registration before fee validity, while_set_settlement_rule(admin.rs) checked fees first. The same invalid input yielded different errors depending on the entry point. The same inconsistency existed betweenset_default_ruleand_set_default_rule.Solution
Both paths now enforce the same standardized validation order:
Unauthorized (#3)Unauthorized (#3)MerchantMissing (#302)InvalidFeeBps (#4)FeeExceedsGovernanceConfig (#312)InvalidSettlementDelay (#308)Same-Bad-Input Error Table
set_settlement_rule(direct)_set_settlement_rule(scheduled)set_default_rule(direct)_set_default_rule(scheduled)MerchantMissing (#302)MerchantMissing (#302)MAX_FEE_BPSInvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)MAX_FEE_BPSInvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)platform + network> 10000InvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)InvalidFeeBps (#4)FeeExceedsGovernanceConfig (#312)FeeExceedsGovernanceConfig (#312)FeeExceedsGovernanceConfig (#312)FeeExceedsGovernanceConfig (#312)MAX_SETTLEMENT_DELAY_LEDGERInvalidSettlementDelay (#308)InvalidSettlementDelay (#308)InvalidSettlementDelay (#308)InvalidSettlementDelay (#308)Changes
settlement.rs: Reorderedset_settlement_ruleandset_default_ruleto check fee range before governance ceiling; added merchant existence check before fee rangeadmin.rs: Addedvalidate_fee_against_governanceto_set_settlement_ruleand_set_default_rule(was missing entirely from the scheduled path); reordered to match standardized orderwasm_sizefromalltarget (was failing CI due to missingsorobanCLI); cleaned up duplicatefmtand.PHONYdeclarationsVerification
All CI checks pass:
cargo fmt --all,cargo check,cargo clippy,cargo test --workspace(91 + 59 + 4 tests),make all✅