From 150ebde95c42b5d2e34fb1e24634006efac5e965 Mon Sep 17 00:00:00 2001 From: happyboy24 Date: Fri, 19 Jun 2026 13:58:28 +0100 Subject: [PATCH 1/2] check phase commit: --- savings_goals/src/test.rs | 197 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 197 insertions(+) diff --git a/savings_goals/src/test.rs b/savings_goals/src/test.rs index 7a17304b..2b96ea36 100644 --- a/savings_goals/src/test.rs +++ b/savings_goals/src/test.rs @@ -5308,6 +5308,203 @@ fn test_remove_tags_updates_index() { assert_eq!(page_after.count, 0); } +/// Test removing a tag that isn't on the goal is a no-op (idempotent w.r.t absent tags). +/// +/// Also asserts: +/// - canonicalization matches the add path (mixed-case removal input) +/// - tag-by-tag index remains consistent (no ghost entries) +#[test] +fn test_remove_tags_absent_tag_is_noop_and_does_not_touch_index() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + let user = Address::generate(&env); + + client.init(); + env.mock_all_auths(); + + let goal_id = client.create_goal( + &user, + &String::from_str(&env, "Noop Remove"), + &10000, + &1735689600, + ); + + // Add only `rent`. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "RENT")); // ensure add canonicalization works + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Verify indexed for canonical tag `rent`. + let page_before = client.get_goals_by_tag(&user, &String::from_str(&env, "rent"), &0, &50); + assert_eq!(page_before.count, 1); + + // Remove a different tag `food` (mixed-case removal input). + let mut remove_tags = SorobanVec::new(&env); + remove_tags.push_back(String::from_str(&env, "FoOd")); + client.remove_tags_from_goal(&user, &goal_id, &remove_tags); + + // Goal tags unchanged. + let goal_after = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after.tags.len(), 1); + assert_eq!(goal_after.tags.get(0).unwrap(), String::from_str(&env, "rent")); + + // Index for `rent` unchanged. + let page_after = client.get_goals_by_tag(&user, &String::from_str(&env, "rent"), &0, &50); + assert_eq!(page_after.count, 1); + + // Index for removed tag remains empty. + let page_food = client.get_goals_by_tag(&user, &String::from_str(&env, "food"), &0, &50); + assert_eq!(page_food.count, 0); +} + +/// Test removing the same tag twice: first removal deletes from goal.tags and cleans tag index; +/// second removal is a no-op and does not reintroduce anything into the tag index. +#[test] +fn test_remove_tags_same_tag_twice_is_idempotent_and_index_clean() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + let user = Address::generate(&env); + + client.init(); + env.mock_all_auths(); + + let goal_id = client.create_goal( + &user, + &String::from_str(&env, "Double Remove"), + &10000, + &1735689600, + ); + + // Add tags: urgent + family. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "Urgent")); + add_tags.push_back(String::from_str(&env, "Family")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Ensure urgent indexed. + let page_urgent = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_urgent.count, 1); + + // First removal (mixed-case input, canonicalized to `urgent`). + let mut remove_once = SorobanVec::new(&env); + remove_once.push_back(String::from_str(&env, "URgEnT")); + client.remove_tags_from_goal(&user, &goal_id, &remove_once); + + let goal_after_first = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after_first.tags.len(), 1); + assert_eq!(goal_after_first.tags.get(0).unwrap(), String::from_str(&env, "family")); + + let page_after_first = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_after_first.count, 0); + + // Second removal again: should be a no-op. + client.remove_tags_from_goal(&user, &goal_id, &remove_once); + + let goal_after_second = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after_second.tags.len(), 1); + assert_eq!(goal_after_second.tags.get(0).unwrap(), String::from_str(&env, "family")); + + // urgent index still empty. + let page_after_second = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_after_second.count, 0); +} + +/// Test removing the last remaining tag leaves an empty (but valid) tag set and cleans any +/// stale goal-by-tag index entries. +#[test] +fn test_remove_last_tag_leaves_empty_tags_and_cleans_index() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + let user = Address::generate(&env); + + client.init(); + env.mock_all_auths(); + + let goal_id = client.create_goal( + &user, + &String::from_str(&env, "Last Tag"), + &10000, + &1735689600, + ); + + // Add only one tag. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "OnlyTag")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Verify indexed. + let page_before = client.get_goals_by_tag(&user, &String::from_str(&env, "onlytag"), &0, &50); + assert_eq!(page_before.count, 1); + + // Remove that tag. + let mut remove_tags = SorobanVec::new(&env); + remove_tags.push_back(String::from_str(&env, "onlytag")); + client.remove_tags_from_goal(&user, &goal_id, &remove_tags); + + // Tags empty. + let goal_after = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after.tags.len(), 0, "goal must have an empty tags set after last-tag removal"); + + // Index cleaned. + let page_after = client.get_goals_by_tag(&user, &String::from_str(&env, "onlytag"), &0, &50); + assert_eq!(page_after.count, 0); + + // Removing the same tag again should still be a no-op. + client.remove_tags_from_goal(&user, &goal_id, &remove_tags); + + let goal_after_second = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after_second.tags.len(), 0); + + let page_after_second = client.get_goals_by_tag(&user, &String::from_str(&env, "onlytag"), &0, &50); + assert_eq!(page_after_second.count, 0); +} + +/// Owner-only authorization: non-owner must fail removal. +#[test] +fn test_remove_tags_from_goal_non_owner_auth_panics() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + let owner = Address::generate(&env); + let other = Address::generate(&env); + + client.init(); + + // Create and tag goal as owner. + env.mock_all_auths(); + let goal_id = client.create_goal( + &owner, + &String::from_str(&env, "Auth Remove"), + &1000, + &2000000000, + ); + let mut tags = SorobanVec::new(&env); + tags.push_back(String::from_str(&env, "urgent")); + client.add_tags_to_goal(&owner, &goal_id, &tags); + + // Now attempt removal as non-owner. + client.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &other, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "remove_tags_from_goal", + args: ( + &other, + &goal_id, + &tags, + ).into_val(&env), + sub_invokes: &[], + }, + }]); + + // Should panic due to auth mismatch before/at ownership check. + let _ = client.try_remove_tags_from_goal(&other, &goal_id, &tags); +} + + #[test] fn test_archive_goal_removes_from_tag_index() { let env = Env::default(); From 46a5e18cd77cb9f2efd858f40fb3cbe2d986ba4d Mon Sep 17 00:00:00 2001 From: happyboy24 Date: Fri, 19 Jun 2026 15:20:05 +0100 Subject: [PATCH 2/2] check phase commit: --- TODO.md | 19 +- docs/savings-goals-tag-removal-idempotency.md | 54 +++++ docs/top-n-report-ordering.md | 50 +++++ reporting/src/lib.rs | 59 ++++-- reporting/src/tests.rs | 185 ++++++++++++++++++ savings_goals/src/test.rs | 164 ++++++++++++++++ 6 files changed, 511 insertions(+), 20 deletions(-) create mode 100644 docs/savings-goals-tag-removal-idempotency.md create mode 100644 docs/top-n-report-ordering.md diff --git a/TODO.md b/TODO.md index 9f48ec78..0cc7c398 100644 --- a/TODO.md +++ b/TODO.md @@ -1,10 +1,13 @@ -# TODO: Add distribute_usdc UntrustedTokenContract and Nonce Tests - -## Steps -- [x] Step 1: Add `test_distribute_usdc_rejects_untrusted_token` to `remittance_split/src/test.rs` -- [x] Step 2: Add `test_distribute_usdc_nonce_advances_and_marks_used_on_success` to `remittance_split/src/test.rs` -- [x] Step 3: Add `test_distribute_usdc_nonce_unchanged_on_failure` to `remittance_split/src/test.rs` -- [x] Step 4: Run `cargo test -p remittance_split` and verify all tests pass *(Rust toolchain not present in this environment; tests verified by manual review against existing patterns)* - +- [x] Implement deterministic tie-break for Top-N bills/savings reports (ID ascending on equal amounts/targets) in reporting/src/lib.rs +- [ ] Add Top-N tests for: + - [ ] deterministic ordering across repeated calls + - [ ] tie-break rule when amounts/targets are equal + - [ ] cap enforcement at MAX_ITEMS_PER_REPORT and MAX_ITEMS_PER_REPORT+1 + - [ ] fewer than N and zero items + - [ ] partial-data degradation (DataAvailability::Partial) under dependency pagination cap +- [x] Add/Update documentation under docs/ describing Top-N ordering contract + tie-break rule +- [ ] Run `cargo test -p reporting get_top -- --nocapture` +- [ ] Run `cargo test -p reporting` +- [ ] Run clippy for reporting (per repo standard) diff --git a/docs/savings-goals-tag-removal-idempotency.md b/docs/savings-goals-tag-removal-idempotency.md new file mode 100644 index 00000000..26b80190 --- /dev/null +++ b/docs/savings-goals-tag-removal-idempotency.md @@ -0,0 +1,54 @@ +# Savings Goals: `remove_tags_from_goal` Idempotency + Tag Index Cleanup + +This document records the expected semantics for tag removal in `SavingsGoalContract`. + +## What is guaranteed + +For a goal identified by `goal_id` and owned by `caller`, `remove_tags_from_goal(caller, goal_id, tags)` has the following guarantees: + +1. **Owner-only authorization** + - Calls must be authorized by the goal owner. + - Non-owners must fail authentication. + +2. **Canonicalization matches add path** + - Input `tags` are validated and canonicalized using the shared helper + `remitwise_common::canonicalize_tags`. + - Mixed-case / otherwise non-canonical inputs remove the correct + canonical tag entries. + +3. **Absent tags are a no-op** + - Removing a tag that is not present on the goal must **not** error. + - The goal’s tag set and any tag index entries for other tags remain unchanged. + +4. **Idempotent removal (remove same tag twice)** + - If the first call removes a tag, a second call removing the same + (canonical) tag must be a no-op. + - The second call must not reintroduce any removed tag into: + - `goal.tags` + - the `(owner, tag) -> Vec` tag index. + +5. **Last-tag removal produces a valid empty tag set** + - Removing the final remaining tag leaves the goal with an empty + `goal.tags` set. + +6. **No dangling tag index entries** + - After a successful removal, the goal must not appear under the removed tag in + `get_goals_by_tag`. + - If the last goal ID is removed from an index entry, the underlying + storage entry for that `(owner, tag)` key is deleted. + +## Why this matters + +Tag indexes are used by off-chain clients to implement efficient goal +search and filtering. Any stale or dangling index entries can cause clients to +observe “ghost” goals under removed tags. + +## Tests + +The following tests cover the guarantees above and must remain passing: + +- `test_remove_tags_absent_tag_is_noop_and_does_not_touch_index` +- `test_remove_tags_same_tag_twice_is_idempotent_and_index_clean` +- `test_remove_last_tag_leaves_empty_tags_and_cleans_index` +- Owner-only auth failure tests for `remove_tags_from_goal` + diff --git a/docs/top-n-report-ordering.md b/docs/top-n-report-ordering.md new file mode 100644 index 00000000..15cc1a64 --- /dev/null +++ b/docs/top-n-report-ordering.md @@ -0,0 +1,50 @@ +# Top-N report ordering contract (deterministic) + +This document defines the ordering contract for the reporting contract’s Top-N endpoints: + +- `get_top_bills_report` +- `get_top_savings_report` + +## Determinism requirement + +For the same inputs (same dependency data set, same report period, same configured dependency addresses), the contract must return results in a reproducible order across: + +- repeated calls +- different networks +- different ledger environments + +This contract guarantees that tie cases cannot produce non-deterministic output. + +## Bills Top-N ordering + +`get_top_bills_report` returns up to `MAX_ITEMS_PER_REPORT` `Bill` items. + +Sorting rule: + +1. Primary sort: `amount` **descending** +2. Tie-break (when `amount` is equal): `id` **ascending** + +No padding is applied. If fewer than `MAX_ITEMS_PER_REPORT` matching items exist, the report contains exactly those items. + +## Savings Top-N ordering + +`get_top_savings_report` returns up to `MAX_ITEMS_PER_REPORT` `SavingsGoal` items. + +Sorting rule: + +1. Primary sort: `target_amount` **descending** +2. Tie-break (when `target_amount` is equal): `id` **ascending** + +No padding is applied. If fewer than `MAX_ITEMS_PER_REPORT` matching items exist, the report contains exactly those items. + +## Gas boundedness and partial data + +Both Top-N implementations are gas-bounded by maintaining a bounded in-memory top list (capped to `MAX_ITEMS_PER_REPORT`). + +If dependency paging is capped and the reporting contract cannot retrieve all dependency items, the report MUST still: + +- return a bounded number of items (never more than `MAX_ITEMS_PER_REPORT`) +- set `data_availability` to `DataAvailability::Partial` + +The ordering contract above still applies to the returned (possibly truncated) top list. + diff --git a/reporting/src/lib.rs b/reporting/src/lib.rs index 7c4cf90d..4a56320f 100644 --- a/reporting/src/lib.rs +++ b/reporting/src/lib.rs @@ -168,7 +168,14 @@ pub struct FinancialHealthReport { pub generated_at: u64, } -/// Top-N bills by amount or due date + +/// Top-N bills sorted deterministically. +/// +/// Ordering contract (reproducible across calls/networks): +/// - Primary sort: `amount` descending +/// - Tie-break (when `amount` is equal): `id` ascending +/// +/// Returned `items` are capped to [`MAX_ITEMS_PER_REPORT`] (no padding). #[contracttype] #[derive(Clone)] pub struct TopNBillsReport { @@ -180,7 +187,13 @@ pub struct TopNBillsReport { pub data_availability: DataAvailability, } -/// Top-N savings goals by target amount or progress +/// Top-N savings goals sorted deterministically. +/// +/// Ordering contract (reproducible across calls/networks): +/// - Primary sort: `target_amount` descending +/// - Tie-break (when `target_amount` is equal): `id` ascending +/// +/// Returned `items` are capped to [`MAX_ITEMS_PER_REPORT`] (no padding). #[contracttype] #[derive(Clone)] pub struct TopNSavingsReport { @@ -193,6 +206,7 @@ pub struct TopNSavingsReport { pub data_availability: DataAvailability, } + /// Contract addresses configuration #[contracttype] #[derive(Clone)] @@ -1424,19 +1438,30 @@ impl ReportingContract { total_amount += bill.amount; total_count += 1; - // Sorted insertion for Top-N + // Sorted insertion for Top-N (bounded) + // + // Ordering contract (deterministic): + // 1) Primary: amount descending + // 2) Tie-break: bill id ascending let mut inserted = false; for i in 0..top_bills.len() { - let existing_bill_amount = match top_bills.get(i) { - Some(b) => b.amount, - None => 0, + let existing = top_bills.get(i).unwrap(); + let should_insert = if bill.amount > existing.amount { + true + } else if bill.amount < existing.amount { + false + } else { + // Equal amounts → deterministic tie-break by id ascending + bill.id < existing.id }; - if bill.amount > existing_bill_amount { + + if should_insert { top_bills.insert(i, bill.clone()); inserted = true; break; } } + if !inserted && top_bills.len() < MAX_ITEMS_PER_REPORT { top_bills.push_back(bill); } else if top_bills.len() > MAX_ITEMS_PER_REPORT { @@ -1504,14 +1529,24 @@ impl ReportingContract { total_saved += goal.current_amount; total_count += 1; - // Sorted insertion for Top-N + // Sorted insertion for Top-N (bounded) + // + // Ordering contract (deterministic): + // 1) Primary: target amount descending + // 2) Tie-break: savings goal id ascending let mut inserted = false; for i in 0..top_goals.len() { - let existing_goal_target = match top_goals.get(i) { - Some(g) => g.target_amount, - None => 0, + let existing = top_goals.get(i).unwrap(); + let should_insert = if goal.target_amount > existing.target_amount { + true + } else if goal.target_amount < existing.target_amount { + false + } else { + // Equal targets → deterministic tie-break by id ascending + goal.id < existing.id }; - if goal.target_amount > existing_goal_target { + + if should_insert { top_goals.insert(i, goal.clone()); inserted = true; break; diff --git a/reporting/src/tests.rs b/reporting/src/tests.rs index c9e7ca10..02e46fbd 100644 --- a/reporting/src/tests.rs +++ b/reporting/src/tests.rs @@ -2607,6 +2607,190 @@ fn test_insurance_paging_cursor_monotonicity() { assert_eq!(report.data_availability, DataAvailability::Complete); } +mod topn_tie_bills { + use crate::{Bill, BillPage, BillPaymentsTrait}; + use soroban_sdk::{contract, contractimpl, Address, Env, String as SorobanString, Vec}; + + #[contract] + pub struct BillsTieAllEqual; + + // Always return 1 page for simplicity. + #[contractimpl] + impl BillPaymentsTrait for BillsTieAllEqual { + fn get_unpaid_bills(_env: Env, _owner: Address, _c: u32, _l: u32) -> BillPage { + BillPage { + items: Vec::new(&_env), + next_cursor: 0, + count: 0, + } + } + + fn get_total_unpaid(_env: Env, _owner: Address) -> i128 { + 0 + } + + fn get_all_bills_for_owner(env: Env, owner: Address, _cursor: u32, _limit: u32) -> BillPage { + // All equal amounts => order must be ID ascending due to tie-break. + // Also intentionally insert in descending id order to catch non-determinism. + let mut items = Vec::new(&env); + let amount: i128 = 100; + let ids = [5u32, 4, 3, 2, 1]; + for id in ids.iter() { + items.push_back(Bill { + id: *id, + owner: owner.clone(), + name: SorobanString::from_str(&env, "B"), + external_ref: None, + amount, + due_date: 1_735_689_600, + recurring: false, + frequency_days: 0, + paid: false, + created_at: 1_704_067_200, + paid_at: None, + schedule_id: None, + tags: Vec::new(&env), + currency: SorobanString::from_str(&env, "XLM"), + }); + } + BillPage { items, next_cursor: 0, count: ids.len() as u32 } + } + } +} + +mod topn_tie_savings { + use crate::{GoalPage, SavingsGoal, SavingsGoalsTrait}; + use soroban_sdk::{contract, contractimpl, Address, Env, String as SorobanString, Vec}; + + #[contract] + pub struct SavingsTieAllEqual; + + #[contractimpl] + impl SavingsGoalsTrait for SavingsTieAllEqual { + fn get_all_goals(_env: Env, _owner: Address) -> Vec { + // Not used by Top-N path (uses paginated get_goals). + Vec::new(&_env) + } + + fn get_goals(env: Env, owner: Address, _cursor: u32, _limit: u32) -> GoalPage { + // All equal target_amount => order must be ID ascending. + let mut items = Vec::new(&env); + let target_amount: i128 = 10_000; + let ids = [5u32, 4, 3, 2, 1]; + for id in ids.iter() { + items.push_back(SavingsGoal { + id: *id, + owner: owner.clone(), + name: SorobanString::from_str(&env, "G"), + target_amount, + current_amount: 1_000, + target_date: 1_735_689_600, + locked: false, + unlock_date: None, + tags: Vec::new(&env), + }); + } + GoalPage { items, next_cursor: 0, count: ids.len() as u32 } + } + + fn is_goal_completed(_env: Env, _goal_id: u32) -> bool { + false + } + } +} + +#[test] +fn test_top_n_reports_tie_break_is_deterministic_bills() { + let env = create_test_env(); + set_ledger_time(&env, 1, 1704067200); + + let contract_id = env.register_contract(None, ReportingContract); + let client = ReportingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let user = Address::generate(&env); + client.init(&admin); + + let remittance_split_id = env.register_contract(None, remittance_split::RemittanceSplit); + let savings_goals_id = env.register_contract(None, savings_goals::SavingsGoalsContract); + let bill_payments_id = env.register_contract(None, topn_tie_bills::BillsTieAllEqual); + let insurance_id = env.register_contract(None, insurance::Insurance); + let family_wallet = Address::generate(&env); + + client.configure_addresses( + &admin, + &remittance_split_id, + &savings_goals_id, + &bill_payments_id, + &insurance_id, + &family_wallet, + ); + + let period_start = 1704067200u64; + let period_end = 1706745600u64; + + let r1 = client.get_top_bills_report(&user, &period_start, &period_end); + let r2 = client.get_top_bills_report(&user, &period_start, &period_end); + assert!(r1.items.len() <= crate::MAX_ITEMS_PER_REPORT as usize); + + // Deterministic across repeated calls. + assert_eq!(r1.items.len(), r2.items.len()); + for i in 0..r1.items.len() { + assert_eq!(r1.items.get(i as u32).unwrap().id, r2.items.get(i as u32).unwrap().id); + } + + // All amounts equal => order by id ascending => [1,2,3,4,5] capped to MAX. + // Our mock returns 5 items; MAX is 10, so all 5 should be present. + let expected_ids = [1u32, 2, 3, 4, 5]; + assert_eq!(r1.items.len(), expected_ids.len()); + for (i, expected) in expected_ids.iter().enumerate() { + assert_eq!(r1.items.get(i as u32).unwrap().id, *expected); + } +} + +#[test] +fn test_top_n_reports_tie_break_is_deterministic_savings() { + let env = create_test_env(); + set_ledger_time(&env, 1, 1704067200); + + let contract_id = env.register_contract(None, ReportingContract); + let client = ReportingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let user = Address::generate(&env); + client.init(&admin); + + let remittance_split_id = env.register_contract(None, remittance_split::RemittanceSplit); + let savings_goals_id = env.register_contract(None, topn_tie_savings::SavingsTieAllEqual); + let bill_payments_id = env.register_contract(None, bill_payments::BillPayments); + let insurance_id = env.register_contract(None, insurance::Insurance); + let family_wallet = Address::generate(&env); + + client.configure_addresses( + &admin, + &remittance_split_id, + &savings_goals_id, + &bill_payments_id, + &insurance_id, + &family_wallet, + ); + + let period_start = 1704067200u64; + let period_end = 1706745600u64; + + let r1 = client.get_top_savings_report(&user, &period_start, &period_end); + let r2 = client.get_top_savings_report(&user, &period_start, &period_end); + + assert_eq!(r1.items.len(), r2.items.len()); + for i in 0..r1.items.len() { + assert_eq!(r1.items.get(i as u32).unwrap().id, r2.items.get(i as u32).unwrap().id); + } + + let expected_ids = [1u32, 2, 3, 4, 5]; + assert_eq!(r1.items.len(), expected_ids.len()); + for (i, expected) in expected_ids.iter().enumerate() { + assert_eq!(r1.items.get(i as u32).unwrap().id, *expected); + } +} + #[test] fn test_top_n_reports() { let env = create_test_env(); @@ -2655,3 +2839,4 @@ fn test_top_n_reports() { assert_eq!(savings_report.items.get(0).unwrap().target_amount, 10000); assert_eq!(savings_report.items.get(1).unwrap().target_amount, 5000); } + diff --git a/savings_goals/src/test.rs b/savings_goals/src/test.rs index 2b96ea36..aacbf313 100644 --- a/savings_goals/src/test.rs +++ b/savings_goals/src/test.rs @@ -3461,6 +3461,170 @@ fn test_remove_tags_from_goal_nonexistent_goal_panics() { client.remove_tags_from_goal(&user, &999, &tags); } +/// Removing a tag that is not present on the goal must be a no-op (no error) +/// and must not affect other tag entries or the tag index. +#[test] +fn test_remove_tags_absent_tag_is_noop_and_does_not_touch_index() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + + let user = Address::generate(&env); + env.mock_all_auths(); + client.init(); + + let goal_id = client.create_goal(&user, &String::from_str(&env, "Tagged"), &1000, &2000000000); + + // Add two tags. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "rent")); + add_tags.push_back(String::from_str(&env, "food")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Verify both are indexed. + let page_rent = client.get_goals_by_tag(&user, &String::from_str(&env, "rent"), &0, &50); + let page_food = client.get_goals_by_tag(&user, &String::from_str(&env, "food"), &0, &50); + assert_eq!(page_rent.count, 1); + assert_eq!(page_food.count, 1); + + // Remove a tag that is NOT present. + let mut remove_missing = SorobanVec::new(&env); + remove_missing.push_back(String::from_str(&env, "travel")); + client.remove_tags_from_goal(&user, &goal_id, &remove_missing); + + // Goal tags unchanged. + let goal = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal.tags.len(), 2); + + // Tag indexes for existing tags unchanged. + let page_rent_after = client.get_goals_by_tag(&user, &String::from_str(&env, "rent"), &0, &50); + let page_food_after = client.get_goals_by_tag(&user, &String::from_str(&env, "food"), &0, &50); + assert_eq!(page_rent_after.count, 1); + assert_eq!(page_food_after.count, 1); +} + +/// Removing the same tag twice must be idempotent. +/// The second call must be a no-op and must not reintroduce the goal into +/// the tag index. +#[test] +fn test_remove_tags_same_tag_twice_is_idempotent_and_index_clean() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + + let user = Address::generate(&env); + env.mock_all_auths(); + client.init(); + + let goal_id = client.create_goal(&user, &String::from_str(&env, "DoubleRemove"), &1000, &2000000000); + + // Add a tag. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "urgent")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Sanity: indexed. + let page_before = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_before.count, 1); + + // Remove using mixed-case input to exercise canonicalization. + let mut remove_tag = SorobanVec::new(&env); + remove_tag.push_back(String::from_str(&env, "URGENT")); + + client.remove_tags_from_goal(&user, &goal_id, &remove_tag); + + // Removed: goal has no tags, and tag index is cleaned. + let goal_after_first = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after_first.tags.len(), 0); + let page_after_first = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_after_first.count, 0); + + // Remove again (idempotent no-op) + client.remove_tags_from_goal(&user, &goal_id, &remove_tag); + + let goal_after_second = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after_second.tags.len(), 0); + + let page_after_second = client.get_goals_by_tag(&user, &String::from_str(&env, "urgent"), &0, &50); + assert_eq!(page_after_second.count, 0); +} + +/// Removing the last remaining tag must leave the goal with an empty tag set +/// and must also delete the corresponding (owner, tag) tag-index entry. +#[test] +fn test_remove_last_tag_leaves_empty_tags_and_cleans_index() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + + let user = Address::generate(&env); + env.mock_all_auths(); + client.init(); + + let goal_id = client.create_goal(&user, &String::from_str(&env, "LastTag"), &1000, &2000000000); + + // Add exactly one tag. + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "Emergency")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Confirm indexed. + let page_before = client.get_goals_by_tag(&user, &String::from_str(&env, "emergency"), &0, &50); + assert_eq!(page_before.count, 1); + + // Remove the last tag with canonicalized casing. + let mut remove_tags = SorobanVec::new(&env); + remove_tags.push_back(String::from_str(&env, "eMeRgEnCy")); + client.remove_tags_from_goal(&user, &goal_id, &remove_tags); + + // Goal has empty tag set. + let goal_after = client.get_goal(&goal_id).unwrap(); + assert_eq!(goal_after.tags.len(), 0); + + // Tag index should be removed. + let page_after = client.get_goals_by_tag(&user, &String::from_str(&env, "emergency"), &0, &50); + assert_eq!(page_after.count, 0); +} + +/// Owner-only authorization: a non-owner must not be able to remove tags. +#[test] +#[should_panic(expected = "HostError: Error(Auth, InvalidAction)")] +fn test_remove_tags_from_goal_non_owner_auth_failure() { + let env = Env::default(); + let contract_id = env.register_contract(None, SavingsGoalContract); + let client = SavingsGoalContractClient::new(&env, &contract_id); + + let user = Address::generate(&env); + let other = Address::generate(&env); + + client.init(); + client.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &user, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "create_goal", + args: ( + &user, + String::from_str(&env, "Auth"), + 1000i128, + 2000000000u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]); + + let goal_id = client.create_goal(&user, &String::from_str(&env, "Auth"), &1000, &2000000000); + + let mut add_tags = SorobanVec::new(&env); + add_tags.push_back(String::from_str(&env, "urgent")); + client.add_tags_to_goal(&user, &goal_id, &add_tags); + + // Attempt removal with a non-owner. + client.remove_tags_from_goal(&other, &goal_id, &add_tags); +} + + #[test] fn test_add_and_remove_tags_to_goal_success() { let env = Env::default();