From c4c5085cb212a600963d57e2caba7f0e05554068 Mon Sep 17 00:00:00 2001 From: Harold Ikechukwu Date: Wed, 30 Sep 2026 09:59:15 +0100 Subject: [PATCH] refactor: rename TTL bump entry points for consistency --- Cargo.lock | 1 + README.md | 4 +- contracts/donation-vault/src/lib.rs | 41 +++++--- contracts/donation-vault/src/test.rs | 136 +++++++++++++++++---------- contracts/ngo-registry/src/lib.rs | 4 +- contracts/ngo-registry/src/test.rs | 12 +-- docs/STORAGE.md | 6 +- 7 files changed, 128 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 83646cf..ea8ff05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -630,6 +630,7 @@ dependencies = [ name = "donation-vault" version = "0.1.0" dependencies = [ + "ngo-registry", "proptest", "soroban-sdk", ] diff --git a/README.md b/README.md index 3a5aa0c..619f94f 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,7 @@ funds while the rest of the contract is frozen. money to a donor, so keeping it available means a pause can never trap a donor's unspent deposit. The read-only views (`admin`, `pending_admin`, `get_stream`, `stream_count`, `pending_accrual`, `paused`, `treasury`, -`fee_bps`) and `extend_stream` also keep working, since none of them can +`fee_bps`) and `bump_stream_ttl` also keep working, since none of them can move funds, and `unpause` is of course still reachable. ## Related repositories @@ -132,7 +132,7 @@ facilities that are unavailable on-chain. Persistent storage is retained per key. A stream that is never touched can expire independently of the vault instance, so state-changing calls and the -permissionless `extend_stream` entry point refresh the specific stream that +permissionless `bump_stream_ttl` entry point refresh the specific stream that needs to remain available. ### What is the cancelled-stream grace period? diff --git a/contracts/donation-vault/src/lib.rs b/contracts/donation-vault/src/lib.rs index 588b2ea..9aff9b8 100644 --- a/contracts/donation-vault/src/lib.rs +++ b/contracts/donation-vault/src/lib.rs @@ -99,19 +99,35 @@ pub enum Error { /// leave its type's range. Returned instead of letting the release /// profile's overflow checks panic and abort the transaction. ArithmeticOverflow = 9, - /// `deposit` was below the configured `min_deposit`. - DepositTooLow = 10, + /// `NextStreamId` was missing from instance storage when `create_stream` + /// tried to read it. `init` always sets it, so this should be + /// unreachable in practice, but a missing counter must never be + /// silently treated as `0` — that could collide with an existing + /// stream. Returned instead of defaulting. + StreamCounterMissing = 10, + /// `pause` was called while the vault was already paused. AlreadyPaused = 11, + /// `unpause` was called while the vault was not paused. AlreadyUnpaused = 12, - /// The donor and the NGO are the same address, so the stream would pay - /// the donor back their own deposit. Rejected at creation: a stream that - /// nets to zero still counts as a committed donation in the indexer and - /// on impact pages, which is a way to inflate those totals for free. + /// `create_stream` was called with the same address as both `donor` and + /// `ngo`. Rejected before the deposit is pulled or the amounts are + /// validated, since a self-stream is never a legitimate call. SelfStream = 13, - /// The stream has already been cancelled and closed out. + /// A stream-mutating call (e.g. `top_up`) targeted a stream that + /// `cancel_stream` has already closed out. StreamCancelled = 14, + /// `create_stream` was called with a `deposit` below the configured + /// `min_deposit` floor. + DepositTooLow = 15, + /// The donor already has `max_streams_per_donor` streams. Raised by + /// `create_stream` before the deposit is pulled. See issue #94. + StreamLimitExceeded = 16, + /// A registry is configured and `create_stream`'s `ngo` is not a + /// verified entry in it (missing, unverified, or the registry call + /// itself failed). + NgoNotVerified = 17, /// The proposed administrator is not a valid replacement. - InvalidAdmin = 15, + InvalidAdmin = 18, } /// Fee cap of 10%, enforced by `set_fee_bps` so the admin can never take @@ -642,9 +658,9 @@ impl DonationVault { /// let stream_id = client.create_stream(&donor, &ngo, &sac.address(), &1_000, &10); /// /// // Anyone can keep the stream's storage alive, no auth required. - /// client.extend_stream(&stream_id); + /// client.bump_stream_ttl(&stream_id); /// ``` - pub fn extend_stream(env: Env, stream_id: u64) -> Result<(), Error> { + pub fn bump_stream_ttl(env: Env, stream_id: u64) -> Result<(), Error> { if !env.storage().persistent().has(&DataKey::Stream(stream_id)) { return Err(Error::StreamNotFound); } @@ -969,8 +985,7 @@ impl DonationVault { .instance() .set(&DataKey::MaxStreamsPerDonor, &limit); extend_instance_ttl(&env); - env.events() - .publish((symbol_short!("maxstrm"),), limit); + env.events().publish((symbol_short!("maxstrm"),), limit); Ok(()) } @@ -1479,4 +1494,4 @@ impl DonationVault { } } -mod test; \ No newline at end of file +mod test; diff --git a/contracts/donation-vault/src/test.rs b/contracts/donation-vault/src/test.rs index 7520f50..d2989fc 100644 --- a/contracts/donation-vault/src/test.rs +++ b/contracts/donation-vault/src/test.rs @@ -117,6 +117,7 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { let stream_id = s .client .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let created = last_event(&s.env); // `last_event` only sees the latest top-level call, so assert it before // any other call (such as a balance read) replaces it. @@ -132,15 +133,11 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { 10i128 ) .into_val(&s.env), - ) - .into_val(&s.env), + ), ); assert_eq!(s.token.balance(&s.donor), 0); assert_eq!(s.token.balance(&s.client.address), 1_000); - assert_eq!(s.token.balance(&s.donor), 0); - assert_eq!(s.token.balance(&s.client.address), 1_000); - // 50 seconds pass -> 10/s * 50 = 500 should be withdrawable. s.env.ledger().with_mut(|l| l.timestamp += 50); @@ -164,6 +161,7 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { s.env.ledger().with_mut(|l| l.timestamp += 20); // 200 more settles to the NGO on cancel; the untouched 300 refunds to the donor. let refund = s.client.cancel_stream(&stream_id); + let cancelled = last_event(&s.env); assert_eq!(refund, 300); assert_eq!( cancelled, @@ -175,10 +173,6 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { assert_eq!(s.token.balance(&s.ngo), 700); assert_eq!(s.token.balance(&s.donor), 300); - // 200 more settles to the NGO on cancel; the untouched 300 refunds to the donor. - assert_eq!(s.token.balance(&s.ngo), 700); - assert_eq!(s.token.balance(&s.donor), 300); - let stream = s.client.get_stream(&stream_id); assert_eq!(stream.balance, 0); assert_eq!(stream.rate, 0); @@ -368,6 +362,7 @@ fn propose_then_accept_admin_transfers_control() { let new_admin = Address::generate(&s.env); s.client.propose_admin(&new_admin); + let proposed = last_event(&s.env); assert_eq!( proposed, ( @@ -379,6 +374,7 @@ fn propose_then_accept_admin_transfers_control() { assert_eq!(s.client.admin(), old_admin); s.client.accept_admin(); + let accepted = last_event(&s.env); assert_eq!( accepted, ( @@ -550,6 +546,7 @@ fn pause_blocks_create_but_not_cancel() { .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); s.client.pause(); + let paused_evt = last_event(&s.env); assert_eq!( paused_evt, ( @@ -608,6 +605,7 @@ fn unpause_restores_normal_operation() { s.client.pause(); s.client.unpause(); + let unpaused_evt = last_event(&s.env); assert_eq!( unpaused_evt, ( @@ -697,16 +695,20 @@ fn withdraw_splits_protocol_fee_to_treasury() { let treasury = Address::generate(&s.env); s.client.set_treasury(&treasury); - assert_last_event( - &s.env, - (symbol_short!("treasury"),).into_val(&s.env), - treasury.clone().into_val(&s.env), + assert_eq!( + last_event(&s.env), + ( + (symbol_short!("treasury"),).into_val(&s.env), + treasury.clone().into_val(&s.env), + ) ); s.client.set_fee_bps(&500); // 5% - assert_last_event( - &s.env, - (symbol_short!("feebps"),).into_val(&s.env), - 500u32.into_val(&s.env), + assert_eq!( + last_event(&s.env), + ( + (symbol_short!("feeset"),).into_val(&s.env), + 500u32.into_val(&s.env), + ) ); let stream_id = s @@ -1276,16 +1278,23 @@ fn modify_rate_bumps_instance_and_stream_ttl() { } #[test] -fn extend_stream_bumps_stream_ttl() { +fn bump_stream_ttl_bumps_stream_ttl() { let s = setup(); let stream_id = create_ttl_test_stream(&s); age_past_thresholds(&s, Some(stream_id)); - s.client.extend_stream(&stream_id); + s.client.bump_stream_ttl(&stream_id); assert_eq!(stream_ttl(&s, stream_id), STREAM_BUMP_AMOUNT); } +#[test] +fn bump_nonexistent_stream_ttl_fails() { + let s = setup(); + let result = s.client.try_bump_stream_ttl(&999); + assert_eq!(result, Err(Ok(Error::StreamNotFound))); +} + #[test] fn admin_writes_bump_instance_ttl() { let s = setup(); @@ -1482,14 +1491,14 @@ fn modify_rate_requires_donor_auth() { } #[test] -fn extend_stream_requires_no_auth() { +fn bump_stream_ttl_requires_no_auth() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); let stream_id = s .client .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); - s.client.extend_stream(&stream_id); + s.client.bump_stream_ttl(&stream_id); assert!(s.env.auths().is_empty()); } @@ -1551,9 +1560,6 @@ fn admin_can_set_the_per_donor_cap() { assert_eq!(s.client.max_streams_per_donor(), 5); } - let (net, fee) = s.client.pending_payout(&stream_id); - assert_eq!(fee, 0); // no treasury → no fee, regardless of fee_bps - assert_eq!(net, 500); // full accrual goes to the NGO #[test] fn non_admin_cannot_set_the_per_donor_cap() { let s = setup(); @@ -1569,10 +1575,13 @@ fn create_stream_rejects_the_101st_stream() { // Fill the default cap. for _ in 0..100 { - s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); } - let result = s.client.try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let result = s + .client + .try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); assert!(result.is_err()); } @@ -1582,21 +1591,23 @@ fn raising_the_cap_lets_the_next_stream_through() { s.token_admin.mint(&s.donor, &1_000_000); s.client.set_max_streams_per_donor(&1); - s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); // At the cap, next call fails. - let result = s.client.try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let result = s + .client + .try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); assert!(result.is_err()); // Raise the cap. Next call succeeds. s.client.set_max_streams_per_donor(&2); - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); assert_eq!(s.client.get_stream(&stream_id).donor, s.donor); } - let (net, fee) = s.client.pending_payout(&stream_id); - assert_eq!(fee, 0); // 0 bps → zero fee even with a treasury set - assert_eq!(net, 500); #[test] fn lowering_the_cap_does_not_retroactively_affect_existing_streams() { let s = setup(); @@ -1604,14 +1615,17 @@ fn lowering_the_cap_does_not_retroactively_affect_existing_streams() { s.client.set_max_streams_per_donor(&5); for _ in 0..3 { - s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); } // Drop the cap below the current count. s.client.set_max_streams_per_donor(&1); // New streams are rejected. - let result = s.client.try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let result = s + .client + .try_create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); assert!(result.is_err()); // Existing streams are untouched and still withdrawable. @@ -1629,20 +1643,21 @@ fn separate_donors_have_separate_counters() { s.token_admin.mint(&s.donor, &1_000); s.token_admin.mint(&donor_b, &1_000); - s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); // A different donor is unaffected by donor A's cap. - let donor_b_stream = s.client.create_stream(&donor_b, &s.ngo, &s.token.address, &1_000, &10); + let donor_b_stream = s + .client + .create_stream(&donor_b, &s.ngo, &s.token.address, &1_000, &10); assert_eq!(s.client.get_stream(&donor_b_stream).donor, donor_b); // But donor_b is now at its own cap. - let result = s.client.try_create_stream(&donor_b, &s.ngo, &s.token.address, &1_000, &10); + let result = s + .client + .try_create_stream(&donor_b, &s.ngo, &s.token.address, &1_000, &10); assert!(result.is_err()); } - let (net, fee) = s.client.pending_payout(&stream_id); - assert_eq!(fee, 25); // 5% of 500 - assert_eq!(net, 475); // 500 - 25 - // ============================================================================= // Explicit stream status (issue #92) // ============================================================================= @@ -1651,7 +1666,9 @@ fn separate_donors_have_separate_counters() { fn new_stream_starts_active() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); assert_eq!(s.client.get_stream(&stream_id).status, StreamStatus::Active); } @@ -1659,9 +1676,14 @@ fn new_stream_starts_active() { fn cancelled_stream_reports_cancelled() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); s.client.cancel_stream(&stream_id); - assert_eq!(s.client.get_stream(&stream_id).status, StreamStatus::Cancelled); + assert_eq!( + s.client.get_stream(&stream_id).status, + StreamStatus::Cancelled + ); } #[test] @@ -1669,17 +1691,24 @@ fn fully_withdrawn_stream_reports_drained() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); // 10 units/s for 1000 units ? fully drained after 100 seconds. - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); s.env.ledger().with_mut(|l| l.timestamp += 100); s.client.withdraw(&stream_id); - assert_eq!(s.client.get_stream(&stream_id).status, StreamStatus::Drained); + assert_eq!( + s.client.get_stream(&stream_id).status, + StreamStatus::Drained + ); } #[test] fn partially_withdrawn_stream_stays_active() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); s.env.ledger().with_mut(|l| l.timestamp += 50); s.client.withdraw(&stream_id); assert_eq!(s.client.get_stream(&stream_id).status, StreamStatus::Active); @@ -1689,9 +1718,16 @@ fn partially_withdrawn_stream_stays_active() { fn status_is_queryable_after_cancel_then_further_operations_fail() { let s = setup(); s.token_admin.mint(&s.donor, &1_000); - let stream_id = s.client.create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); s.client.cancel_stream(&stream_id); - assert_eq!(s.client.get_stream(&stream_id).status, StreamStatus::Cancelled); + assert_eq!( + s.client.get_stream(&stream_id).status, + StreamStatus::Cancelled + ); // top_up on a cancelled stream is rejected (cancelled sets rate = 0). s.env.ledger().with_mut(|l| l.timestamp += 10); - let result = s.client.try_withdraw(& \ No newline at end of file + let result = s.client.try_withdraw(&stream_id); + assert!(result.is_err()); // nothing left to withdraw +} diff --git a/contracts/ngo-registry/src/lib.rs b/contracts/ngo-registry/src/lib.rs index ec52e28..17320e4 100644 --- a/contracts/ngo-registry/src/lib.rs +++ b/contracts/ngo-registry/src/lib.rs @@ -447,9 +447,9 @@ impl NgoRegistry { /// # let owner = Address::generate(&env); /// # let name = String::from_str(&env, "Example NGO"); /// # client.register(&owner, &name); - /// client.touch_ngo(&owner); + /// client.bump_ngo_ttl(&owner); /// ``` - pub fn touch_ngo(env: Env, owner: Address) -> Result<(), Error> { + pub fn bump_ngo_ttl(env: Env, owner: Address) -> Result<(), Error> { if !env.storage().persistent().has(&DataKey::Ngo(owner.clone())) { return Err(Error::NotRegistered); } diff --git a/contracts/ngo-registry/src/test.rs b/contracts/ngo-registry/src/test.rs index 8e5ebb5..d93c6b9 100644 --- a/contracts/ngo-registry/src/test.rs +++ b/contracts/ngo-registry/src/test.rs @@ -198,7 +198,7 @@ fn revoke_unregistered_ngo_fails() { } #[test] -fn touch_ngo_leaves_entry_unchanged() { +fn bump_ngo_ttl_leaves_entry_unchanged() { let (env, client, _admin) = setup(); let owner = Address::generate(&env); let name = String::from_str(&env, "Red Cross"); @@ -206,18 +206,18 @@ fn touch_ngo_leaves_entry_unchanged() { client.approve_ngo(&owner); let before = client.get_ngo(&owner); - client.touch_ngo(&owner); + client.bump_ngo_ttl(&owner); let after = client.get_ngo(&owner); assert_eq!(before, after); } #[test] -fn touch_unregistered_ngo_fails() { +fn bump_unregistered_ngo_ttl_fails() { let (env, client, _admin) = setup(); let random = Address::generate(&env); - let result = client.try_touch_ngo(&random); + let result = client.try_bump_ngo_ttl(&random); assert_eq!(result, Err(Ok(Error::NotRegistered))); } @@ -383,13 +383,13 @@ fn update_name_bumps_instance_and_ngo_ttl() { } #[test] -fn touch_ngo_bumps_instance_and_ngo_ttl() { +fn bump_ngo_ttl_bumps_instance_and_ngo_ttl() { let (env, client, _admin) = setup(); let owner = Address::generate(&env); client.register(&owner, &String::from_str(&env, "Red Cross")); age_past_thresholds(&env, &client, &owner); - client.touch_ngo(&owner); + client.bump_ngo_ttl(&owner); assert_ttls_bumped(&env, &client, &owner); } diff --git a/docs/STORAGE.md b/docs/STORAGE.md index ea4ffa6..dcfcf0b 100644 --- a/docs/STORAGE.md +++ b/docs/STORAGE.md @@ -33,7 +33,7 @@ counts below assume a 5-second average ledger close time | `Treasury` | Instance | 30d / 29d | Address that receives the protocol fee cut on withdrawal. | | `FeeBps` | Instance | 30d / 29d | Protocol fee, in basis points, capped at `MAX_FEE_BPS` (1,000 / 10%). | | `CancelGraceLedgers` | Instance | 30d / 29d | Additional ledgers to retain cancelled stream records for indexing. | -| `Stream(u64)` | Persistent | 90d / 89d | One donor→NGO stream record, keyed by stream id. Extended on every `create_stream`, `withdraw`, `top_up`, `cancel_stream`, or rate change touching that stream. | +| `Stream(u64)` | Persistent | 90d / 89d | One donor→NGO stream record, keyed by stream id. Extended on every `create_stream`, `withdraw`, `top_up`, `cancel_stream`, `bump_stream_ttl`, or rate change touching that stream. | All instance keys share one TTL (bumped to 30 days, refreshed once it would otherwise drop below 29 days remaining) via `extend_instance_ttl`, @@ -57,8 +57,8 @@ eligible for archival. As with `donation-vault`, the instance TTL is refreshed on every state-changing call via `extend_instance_ttl`. Each `Ngo(Address)` entry gets its own 90-day TTL via `extend_ngo_ttl`, refreshed by `register`, -`approve_ngo`, and `revoke_ngo` for that specific entry — an NGO that -registers once and is never approved, revoked, or re-touched can still +`approve_ngo`, `revoke_ngo`, and `bump_ngo_ttl` for that specific entry — an NGO that +registers once and is never approved, revoked, or bumped can still have its entry archived independently of the registry's admin data. ## Implication for fee estimation