Minor improvements for the finance module (#1209)

* feat: improvements * feat: add CHANGELOG entry * fix: linter * feat: apply review updates
OpenZeppelin · Nov 14, 2024 · f84b30d · f84b30d
1 parent 4d40f5e
commit f84b30d
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed (Breaking)
 
+- VestingComponent `release` function won't emit an event or attempt to transfer when the amount is zero (#1209)
 - Bump snforge_std to v0.33.0 (#1203)
 
 ## 0.19.0 (2024-11-08)

diff --git a/docs/modules/ROOT/pages/api/finance.adoc b/docs/modules/ROOT/pages/api/finance.adoc
@@ -91,7 +91,7 @@ Returns the total vested amount of a specified `token` at a given `timestamp`.
 
 Releases the amount of a given `token` that has already vested and returns that amount.
 
-Emits {AmountReleased} event.
+May emit an {AmountReleased} event.
 
 [#IVesting-Events]
 ==== Events
@@ -208,11 +208,14 @@ Returns the total vested amount of a specified `token` at a given `timestamp`.
 
 Releases the amount of a given `token` that has already vested and returns that amount.
 
+NOTE: If the releasable amount is zero, this function won't emit the event
+or attempt to transfer the tokens.
+
 Requirements:
 
 - `transfer` call to the `token` must return `true` indicating a successful transfer.
 
-Emits {AmountReleased} event.
+May emit an {AmountReleased} event.
 
 [#VestingComponent-Internal-Functions]
 ==== Internal functions

diff --git a/packages/finance/src/tests/test_vesting_linear.cairo b/packages/finance/src/tests/test_vesting_linear.cairo
@@ -6,6 +6,7 @@ use openzeppelin_access::ownable::interface::{IOwnableDispatcher, IOwnableDispat
 use openzeppelin_test_common::mocks::vesting::LinearVestingMock;
 use openzeppelin_test_common::vesting::VestingSpyHelpers;
 use openzeppelin_testing::constants::{OWNER, OTHER};
+use openzeppelin_testing::events::EventSpyExt;
 use openzeppelin_token::erc20::interface::{IERC20Dispatcher, IERC20DispatcherTrait};
 use snforge_std::{spy_events, start_cheat_caller_address, start_cheat_block_timestamp_global};
 
@@ -103,6 +104,22 @@ fn test_vesting_schedule_with_cliff() {
     assert_eq!(vesting.vested_amount(token, end_timestamp), data.total_allocation);
 }
 
+#[test]
+fn test_release_zero_amount() {
+    let data = TEST_DATA();
+    let (vesting, token) = setup(data);
+
+    start_cheat_block_timestamp_global(data.start);
+    let mut spy = spy_events();
+
+    assert_eq!(vesting.releasable(token), 0);
+
+    let actual_release_amount = vesting.release(token);
+    assert_eq!(actual_release_amount, 0);
+
+    spy.assert_no_events_left();
+}
+
 #[test]
 fn test_release_single_call_within_duration() {
     let data = TEST_DATA();

diff --git a/packages/finance/src/vesting/vesting.cairo b/packages/finance/src/vesting/vesting.cairo
@@ -79,6 +79,10 @@ pub mod VestingComponent {
         ) -> u256;
     }
 
+    //
+    // External
+    //
+
     #[embeddable_as(VestingImpl)]
     impl Vesting<
         TContractState,
@@ -132,21 +136,26 @@ pub mod VestingComponent {
 
         /// Releases the amount of a given `token` that has already vested.
         ///
+        /// NOTE: If the releasable amount is zero, this function won't emit the event
+        /// or attempt to transfer the tokens.
         ///
         /// Requirements:
         ///
         /// - `transfer` call to the `token` must return `true` indicating a successful transfer.
         ///
-        /// Emits an `AmountReleased` event.
+        /// May emit an `AmountReleased` event.
         fn release(ref self: ComponentState<TContractState>, token: ContractAddress) -> u256 {
             let amount = self.releasable(token);
-            self.Vesting_released.write(token, self.Vesting_released.read(token) + amount);
-            self.emit(AmountReleased { token, amount });
-
-            let beneficiary = get_dep_component!(@self, Ownable).owner();
-            let token_dispatcher = IERC20Dispatcher { contract_address: token };
-            assert(token_dispatcher.transfer(beneficiary, amount), Errors::TOKEN_TRANSFER_FAILED);
-
+            if amount > 0 {
+                self.Vesting_released.write(token, self.Vesting_released.read(token) + amount);
+                self.emit(AmountReleased { token, amount });
+
+                let beneficiary = get_dep_component!(@self, Ownable).owner();
+                let token_dispatcher = IERC20Dispatcher { contract_address: token };
+                assert(
+                    token_dispatcher.transfer(beneficiary, amount), Errors::TOKEN_TRANSFER_FAILED
+                );
+            }
             amount
         }
     }
@@ -159,13 +168,10 @@ pub mod VestingComponent {
     > of InternalTrait<TContractState> {
         /// Initializes the component by setting the vesting `start`, `duration` and
         /// `cliff_duration`.
-        /// To prevent reinitialization, this should only be used inside of a contract's
-        /// constructor.
         ///
         /// Requirements:
         ///
         /// - `cliff_duration` must be less than or equal to `duration`.
-        ///
         fn initializer(
             ref self: ComponentState<TContractState>, start: u64, duration: u64, cliff_duration: u64
         ) {