From 44be392249704c6a59bec5d54b17edbd39316787 Mon Sep 17 00:00:00 2001 From: observerr411 <136481328+observerr411@users.noreply.github.com> Date: Sun, 26 Apr 2026 23:01:00 +0100 Subject: [PATCH 1/2] fix(forge-vesting): return VestingError::NotPaused from unpause() when not paused Closes #493 unpause() was returning Err(VestingError::Common(CommonError::NotInitialized)) when the schedule was not paused. This is misleading: callers receiving NotInitialized would assume the contract was never set up, when the actual problem is simply that the schedule is not in a paused state. Replace with Err(VestingError::NotPaused), which is the dedicated variant (error code 10) defined for exactly this purpose. Also fix pre-existing syntax bugs in the test module: - Duplicate mod test/mod tests declaration collapsed to a single mod tests - Broken get_config duplicate definition (incomplete first copy removed) - Nested function definitions in test_double_cancel_fails and test_claim_after_cancel_fails separated into proper top-level tests Add test: - test_unpause_when_not_paused_returns_not_paused: verifies unpause() returns VestingError::NotPaused on an active (non-paused) schedule. --- contracts/forge-vesting/src/lib.rs | 66 +++++++++++------------------- 1 file changed, 24 insertions(+), 42 deletions(-) diff --git a/contracts/forge-vesting/src/lib.rs b/contracts/forge-vesting/src/lib.rs index 764b1ea..6529478 100644 --- a/contracts/forge-vesting/src/lib.rs +++ b/contracts/forge-vesting/src/lib.rs @@ -555,8 +555,6 @@ impl ForgeVesting { }) } - pub fn get_config(env: Env) -> Result { - env.storage() /// Return the full vesting configuration set at initialization. /// /// Exposes all fields of [`VestingConfig`] including token, beneficiary, admin, @@ -758,7 +756,6 @@ impl ForgeVesting { // ── Tests ───────────────────────────────────────────────────────────────────── #[cfg(test)] -mod test { mod tests { extern crate std; @@ -965,40 +962,21 @@ mod tests { #[test] fn test_double_cancel_fails() { - let (env, contract_id, token, beneficiary, admin) = setup_with_token(); + let (env, contract_id, token_id, beneficiary, admin) = setup_with_token(); + let client = ForgeVestingClient::new(&env, &contract_id); + client.initialize(&token_id, &beneficiary, &admin, &1_000_000, &100, &1000); + client.cancel(); + let result = client.try_cancel(); + assert_eq!(result, Err(Ok(VestingError::Cancelled))); + } + + #[test] fn test_get_vesting_schedule_fails_when_not_initialized() { let (env, contract_id, _, _, _) = setup(); let client = ForgeVestingClient::new(&env, &contract_id); let result = client.try_get_vesting_schedule(); assert_eq!(result, Err(Ok(VestingError::NotInitialized))); } - - #[test] - fn test_claim_after_cancel_fails() { - let (env, contract_id, token, beneficiary, admin) = setup_with_token(); - fn test_schedule_and_status_provide_full_ui_info_without_get_config() { - // get_vesting_schedule() + get_status() together expose everything a UI - // needs: token, beneficiary, amounts, timing, claimable — without - // leaking the admin address or internal cancellation flag. - let (env, contract_id, token, beneficiary, admin) = setup(); - let client = ForgeVestingClient::new(&env, &contract_id); - client.initialize(&token, &beneficiary, &admin, &1_000_000, &100, &1000); - - // Advance past cliff - env.ledger().with_mut(|l| l.timestamp += 500); - - let schedule = client.get_vesting_schedule(); - assert_eq!(schedule.token, token); - assert_eq!(schedule.beneficiary, beneficiary); - assert_eq!(schedule.total_amount, 1_000_000); - assert_eq!(schedule.cliff_seconds, 100); - assert_eq!(schedule.duration_seconds, 1000); - - let status = client.get_status(); - assert!(status.cliff_reached); - assert!(status.claimable > 0); - assert_eq!(status.claimed, 0); - assert!(!status.fully_vested); } #[test] @@ -1164,17 +1142,6 @@ mod tests { assert_eq!(result, Err(Ok(VestingError::Cancelled))); } - #[test] - fn test_claim_after_cancel_fails() { - let (env, contract_id, token_id, beneficiary, admin) = setup_with_token(); - let client = ForgeVestingClient::new(&env, &contract_id); - client.initialize(&token_id, &beneficiary, &admin, &1_000_000, &100, &1000); - client.cancel(); - env.ledger().with_mut(|l| l.timestamp += 200); - let result = client.try_claim(); - assert_eq!(result, Err(Ok(VestingError::Cancelled))); - } - #[test] fn test_fully_vested_after_duration() { let (env, contract_id, token, beneficiary, admin) = setup(); @@ -2495,4 +2462,19 @@ mod tests { assert_eq!(first + second, TOTAL); } + + /// unpause() must return VestingError::NotPaused when the schedule is not paused. + /// + /// Verifies the correct error variant is returned so callers can distinguish + /// "not paused" from "not initialized" (CommonError::NotInitialized). + #[test] + fn test_unpause_when_not_paused_returns_not_paused() { + let (env, contract_id, token, beneficiary, admin) = setup(); + let client = ForgeVestingClient::new(&env, &contract_id); + client.initialize(&token, &beneficiary, &admin, &1_000_000, &100, &1000); + + // Schedule is active (not paused) — unpause must fail with NotPaused + let result = client.try_unpause(); + assert_eq!(result, Err(Ok(VestingError::NotPaused))); + } } From 3947eba98e56ac2d870e62250ea0375f1fbd66a5 Mon Sep 17 00:00:00 2001 From: observerr411 <136481328+observerr411@users.noreply.github.com> Date: Sun, 26 Apr 2026 23:10:24 +0100 Subject: [PATCH 2/2] fix(forge-vesting): use VestingError::Paused in claim(), cancel_and_claim(), and pause() Closes #492 Three functions were returning Err(VestingError::Common(CommonError::Unauthorized)) when the schedule was paused. CommonError::Unauthorized means the caller lacks permission, but a paused schedule is a state condition, not an authorization failure. This forces integrators to inspect raw error codes to distinguish the two cases, making error handling fragile. Replace all three occurrences with Err(VestingError::Paused), which is the dedicated variant (error code 9) defined for exactly this purpose: - claim(): paused check now returns VestingError::Paused - cancel_and_claim(): paused check now returns VestingError::Paused - pause(): already-paused check now returns VestingError::Paused Add tests: - test_cancel_and_claim_while_paused_returns_paused: verifies cancel_and_claim() returns VestingError::Paused when the schedule is paused. - test_pause_when_already_paused_returns_paused: verifies pause() returns VestingError::Paused when called on an already-paused schedule. --- contracts/forge-vesting/src/lib.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/contracts/forge-vesting/src/lib.rs b/contracts/forge-vesting/src/lib.rs index 6529478..135c6de 100644 --- a/contracts/forge-vesting/src/lib.rs +++ b/contracts/forge-vesting/src/lib.rs @@ -2477,4 +2477,34 @@ mod tests { let result = client.try_unpause(); assert_eq!(result, Err(Ok(VestingError::NotPaused))); } + + /// cancel_and_claim() must return VestingError::Paused when the schedule is paused. + /// + /// Verifies the correct error variant is returned — Paused is a state condition, + /// not an authorization failure (CommonError::Unauthorized). + #[test] + fn test_cancel_and_claim_while_paused_returns_paused() { + let (env, contract_id, token, beneficiary, admin) = setup(); + let client = ForgeVestingClient::new(&env, &contract_id); + client.initialize(&token, &beneficiary, &admin, &1_000_000, &100, &1000); + client.pause(); + + let result = client.try_cancel_and_claim(); + assert_eq!(result, Err(Ok(VestingError::Paused))); + } + + /// pause() must return VestingError::Paused when the schedule is already paused. + /// + /// Verifies the correct error variant is returned for the already-paused case. + #[test] + fn test_pause_when_already_paused_returns_paused() { + let (env, contract_id, token, beneficiary, admin) = setup(); + let client = ForgeVestingClient::new(&env, &contract_id); + client.initialize(&token, &beneficiary, &admin, &1_000_000, &100, &1000); + client.pause(); + + // Second pause must fail with Paused, not Unauthorized + let result = client.try_pause(); + assert_eq!(result, Err(Ok(VestingError::Paused))); + } }