Fix lockup issues found by @ailisp

lockup/README.md

@@ -104,6 +104,12 @@ With the guarantees from the staking pool contracts, whitelist, and voting contr
 - The owner can withdraw rewards from the staking pool before tokens are unlocked unless the vesting termination prevents it.
 - The owner should be able to add a full access key to the account, once all tokens are vested, unlocked and transfers are enabled.
 
+### Contributing
+
+We use Docker to build the contract.
+Configuration could be found [here](https://github.com/near/near-sdk-rs/tree/master/contract-builder).
+Please make sure that Docker is given at least 4Gb of RAM.
+
 ### [Deprecated] Private vesting schedule
 
 Since the vesting schedule usually starts at the date of employment it allows to de-anonymize the owner of the lockup contract.
@@ -394,6 +400,9 @@ pub fn terminate_vesting(
 ///
 /// When the vesting is terminated and there are deficit of the tokens on the account, the
 /// deficit amount of tokens has to be unstaked and withdrawn from the staking pool.
+/// Should be invoked twice:
+/// 1. First, to unstake everything from the staking pool;
+/// 2. Second, after 4 epochs (48 hours) to prepare to withdraw.
 pub fn termination_prepare_to_withdraw(&mut self) -> bool;
 
 /// FOUNDATION'S METHOD
@@ -442,14 +451,14 @@ pub fn get_terminated_unvested_balance(&self) -> WrappedBalance;
 /// the unvested balance from the early-terminated vesting schedule.
 pub fn get_terminated_unvested_balance_deficit(&self) -> WrappedBalance;
 
-/// Get the amount of tokens that are already vested or released, but still locked due to lockup.
+/// Get the amount of tokens that are locked in the account due to lockup or vesting.
 pub fn get_locked_amount(&self) -> WrappedBalance;
 
 /// Get the amount of tokens that are already vested, but still locked due to lockup.
 /// Takes raw vesting schedule, in case the internal vesting schedule is private.
 pub fn get_locked_vested_amount(&self, vesting_schedule: VestingSchedule) -> WrappedBalance;
 
-/// Get the amount of tokens that are locked in this account due to vesting or release schedule.
+/// Get the amount of tokens that are locked in this account due to vesting schedule.
 /// Takes raw vesting schedule, in case the internal vesting schedule is private.
 pub fn get_unvested_amount(&self, vesting_schedule: VestingSchedule) -> WrappedBalance;
 
@@ -698,14 +707,15 @@ it creates the deficit (otherwise the foundation can proceed with withdrawal).
 
 The current termination status should be `VestingTerminatedWithDeficit`.
 
-The NEAR Foundation needs to first unstake tokens in the staking pool and then once tokens
-become liquid, withdraw them from the staking pool to the contract. This is done by calling `termination_prepare_to_withdraw`.
+The NEAR Foundation needs to first unstake tokens in the staking pool and then once tokens become liquid, withdraw them from the staking pool to the contract.
+This is done by calling `termination_prepare_to_withdraw`.
 
 ```bash
 near call lockup1 termination_prepare_to_withdraw '{}' --accountId=near --gas=175000000000000
 ```
 
-The first will unstake everything from the staking pool. This should advance the termination status to `EverythingUnstaked`.
+The first will unstake everything from the staking pool.
+This should advance the termination status to `EverythingUnstaked`.
 In 4 epochs, or about 48 hours, the Foundation can call the same command again:
 
 ```bash

lockup/src/foundation.rs

@@ -52,6 +52,9 @@ impl LockupContract {
     ///
     /// When the vesting is terminated and there are deficit of the tokens on the account, the
     /// deficit amount of tokens has to be unstaked and withdrawn from the staking pool.
+    /// Should be invoked twice:
+    /// 1. First, to unstake everything from the staking pool;
+    /// 2. Second, after 4 epochs (48 hours) to prepare to withdraw.
     pub fn termination_prepare_to_withdraw(&mut self) -> Promise {
         self.assert_called_by_foundation();
         self.assert_staking_pool_is_idle();

lockup/src/foundation_callbacks.rs

@@ -1,5 +1,5 @@
 use crate::*;
-use near_sdk::{near_bindgen, PromiseOrValue};
+use near_sdk::{near_bindgen, PromiseOrValue, assert_self, is_promise_success};
 use std::convert::Into;
 
 #[near_bindgen]
@@ -219,6 +219,9 @@ impl LockupContract {
                     )
                         .as_bytes(),
                 );
+                if self.get_account_balance().0 == 0 {
+                    env::log(b"The withdrawal is completed: no more balance can be withdrawn in a future call");
+                }
             } else {
                 self.foundation_account_id = None;
                 self.vesting_information = VestingInformation::None;

lockup/src/getters.rs

@@ -61,7 +61,7 @@ impl LockupContract {
             .into()
     }
 
-    /// Get the amount of tokens that are locked in this account due to lockup or vesting.
+    /// Get the amount of tokens that are locked in the account due to lockup or vesting.
     pub fn get_locked_amount(&self) -> WrappedBalance {
         let lockup_amount = self.lockup_information.lockup_amount;
         if let TransfersInformation::TransfersEnabled {
@@ -112,13 +112,13 @@ impl LockupContract {
         (lockup_amount - self.lockup_information.termination_withdrawn_tokens).into()
     }
 
-    /// Get the amount of tokens that are already vested or released, but still locked due to lockup.
+    /// Get the amount of tokens that are already vested, but still locked due to lockup.
     /// Takes raw vesting schedule, in case the internal vesting schedule is private.
     pub fn get_locked_vested_amount(&self, vesting_schedule: VestingSchedule) -> WrappedBalance {
         (self.get_locked_amount().0 - self.get_unvested_amount(vesting_schedule).0).into()
     }
 
-    /// Get the amount of tokens that are locked in this account due to vesting or release schedule.
+    /// Get the amount of tokens that are locked in this account due to vesting schedule.
     /// Takes raw vesting schedule, in case the internal vesting schedule is private.
     pub fn get_unvested_amount(&self, vesting_schedule: VestingSchedule) -> WrappedBalance {
         let block_timestamp = env::block_timestamp();

lockup/src/internal.rs

@@ -54,7 +54,8 @@ impl LockupContract {
                 );
                 vesting_schedule.clone()
             }
-            _ => env::panic(b"Vesting was terminated"),
+            VestingInformation::Terminating(_) => env::panic(b"Vesting was terminated"),
+            VestingInformation::None => env::panic(b"Vesting is None"),
         }
     }
 

lockup/src/lib.rs

@@ -11,14 +11,12 @@ pub use crate::internal::*;
 pub use crate::owner::*;
 pub use crate::owner_callbacks::*;
 pub use crate::types::*;
-pub use crate::utils::*;
 
 pub mod foundation;
 pub mod foundation_callbacks;
 pub mod gas;
 pub mod owner_callbacks;
 pub mod types;
-pub mod utils;
 
 pub mod getters;
 pub mod internal;
@@ -30,9 +28,9 @@ static ALLOC: near_sdk::wee_alloc::WeeAlloc = near_sdk::wee_alloc::WeeAlloc::INI
 /// Indicates there are no deposit for a cross contract call for better readability.
 const NO_DEPOSIT: u128 = 0;
 
-/// The contract keeps at least 35 NEAR in the account to avoid being transferred out to cover
+/// The contract keeps at least 3.5 NEAR in the account to avoid being transferred out to cover
 /// contract code storage and some internal state.
-const MIN_BALANCE_FOR_STORAGE: u128 = 35_000_000_000_000_000_000_000_000;
+pub const MIN_BALANCE_FOR_STORAGE: u128 = 3_500_000_000_000_000_000_000_000;
 
 #[ext_contract(ext_staking_pool)]
 pub trait ExtStakingPool {
@@ -153,11 +151,11 @@ impl LockupContract {
     /// Requires 25 TGas (1 * BASE_GAS)
     ///
     /// Initializes lockup contract.
-    /// - `owner_account_id` - the account ID of the owner.  Only this account can call owner's
+    /// - `owner_account_id` - the account ID of the owner. Only this account can call owner's
     ///    methods on this contract.
-    /// - `lockup_duration` - the duration in nanoseconds of the lockup period from the moment
-    ///    the transfers are enabled. During this period tokens are locked and the release doesn't
-    ///    start.
+    /// - `lockup_duration` [deprecated] - the duration in nanoseconds of the lockup period from
+    ///    the moment the transfers are enabled. During this period tokens are locked and
+    ///    the release doesn't start. Instead of this, use `lockup_timestamp` and `release_duration`
     /// - `lockup_timestamp` - the optional absolute lockup timestamp in nanoseconds which locks
     ///    the tokens until this timestamp passes. Until this moment the tokens are locked and the
     ///    release doesn't start.
@@ -229,7 +227,8 @@ impl LockupContract {
             }
         };
         assert!(
-            vesting_information == VestingInformation::None || foundation_account_id.is_some(),
+            vesting_information == VestingInformation::None ||
+                env::is_valid_account_id(foundation_account_id.as_ref().unwrap().as_bytes()),
             "Foundation account should be added for vesting schedule"
         );
 
@@ -314,8 +313,8 @@ mod tests {
                     vesting_schedule,
                     salt: SALT.to_vec().into(),
                 }
-                .hash()
-                .into(),
+                    .hash()
+                    .into(),
             )
         });
         LockupContract::new(
@@ -389,6 +388,39 @@ mod tests {
         contract.add_full_access_key(public_key(4));
     }
 
+    #[test]
+    #[should_panic(expected = "Tokens are still locked/unvested")]
+    fn test_add_full_access_key_when_vesting_is_not_finished() {
+        let mut context = basic_context();
+        testing_env!(context.clone());
+        let vesting_schedule = new_vesting_schedule(YEAR);
+        let mut contract = new_contract(true, Some(vesting_schedule), None, true);
+
+        context.block_timestamp = to_ts(GENESIS_TIME_IN_DAYS + YEAR - 10);
+        context.predecessor_account_id = account_owner();
+        context.signer_account_id = account_owner();
+        context.signer_account_pk = public_key(1).try_into().unwrap();
+        testing_env!(context.clone());
+
+        contract.add_full_access_key(public_key(4));
+    }
+
+    #[test]
+    #[should_panic(expected = "Tokens are still locked/unvested")]
+    fn test_add_full_access_key_when_lockup_is_not_finished() {
+        let mut context = basic_context();
+        testing_env!(context.clone());
+        let mut contract = new_contract(true, None, Some(to_nanos(YEAR).into()), false);
+
+        context.block_timestamp = to_ts(GENESIS_TIME_IN_DAYS + YEAR - 10);
+        context.predecessor_account_id = account_owner();
+        context.signer_account_id = account_owner();
+        context.signer_account_pk = public_key(1).try_into().unwrap();
+        testing_env!(context.clone());
+
+        contract.add_full_access_key(public_key(4));
+    }
+
     #[test]
     #[should_panic(expected = "Can only be called by the owner")]
     fn test_call_by_non_owner() {
@@ -1128,7 +1160,7 @@ mod tests {
             contract.get_vesting_information(),
             VestingInformation::Terminating(TerminationInformation {
                 unvested_amount: to_yocto(250).into(),
-                status: TerminationStatus::ReadyToWithdraw
+                status: TerminationStatus::ReadyToWithdraw,
             })
         );
         assert_eq!(contract.get_owners_balance().0, to_yocto(750));
@@ -1588,10 +1620,10 @@ mod tests {
             VestingInformation::VestingHash(
                 VestingScheduleWithSalt {
                     vesting_schedule: vesting_schedule.clone(),
-                    salt: SALT.to_vec().into()
+                    salt: SALT.to_vec().into(),
                 }
-                .hash()
-                .into()
+                    .hash()
+                    .into()
             )
         );
         assert_eq!(contract.get_owners_balance().0, 0);
@@ -1624,7 +1656,7 @@ mod tests {
             contract.get_vesting_information(),
             VestingInformation::Terminating(TerminationInformation {
                 unvested_amount: lockup_amount.into(),
-                status: TerminationStatus::ReadyToWithdraw
+                status: TerminationStatus::ReadyToWithdraw,
             })
         );
         assert_eq!(contract.get_owners_balance().0, 0);

lockup/src/owner.rs

@@ -488,15 +488,20 @@ impl LockupContract {
     ///
     /// Requires 50 TGas (2 * BASE_GAS)
     ///
-    /// Adds full access key with the given public key to the account once the contract is fully
-    /// vested, lockup duration has expired and transfers are enabled.
-    /// This will allow owner to use this account as a regular account and remove the contract.
+    /// Adds full access key with the given public key to the account.
+    /// The following requirements should be met:
+    /// - The contract is fully vested;
+    /// - Lockup duration has expired;
+    /// - Transfers are enabled;
+    /// - If there’s a termination made by foundation, it has to be finished.
+    /// Full access key will allow owner to use this account as a regular account and remove
+    /// the contract.
     pub fn add_full_access_key(&mut self, new_public_key: Base58PublicKey) -> Promise {
         self.assert_owner();
         self.assert_transfers_enabled();
         self.assert_no_staking_or_idle();
         self.assert_no_termination();
-        assert_eq!(self.get_locked_amount().0, 0);
+        assert_eq!(self.get_locked_amount().0, 0, "Tokens are still locked/unvested");
 
         env::log(b"Adding a full access key");
 

lockup/src/owner_callbacks.rs

@@ -1,5 +1,5 @@
 use crate::*;
-use near_sdk::{near_bindgen, PromiseOrValue};
+use near_sdk::{near_bindgen, PromiseOrValue, assert_self, is_promise_success};
 
 #[near_bindgen]
 impl LockupContract {
@@ -15,6 +15,7 @@ impl LockupContract {
             "The given staking pool account ID is not whitelisted"
         );
         self.assert_staking_pool_is_not_selected();
+        self.assert_no_termination();
         self.staking_information = Some(StakingInformation {
             staking_pool_account_id,
             status: TransactionStatus::Idle,

lockup/src/types.rs

@@ -34,29 +34,35 @@ pub struct LockupInformation {
     /// This amount has to be accounted separately from the lockup_amount to make sure
     /// linear release is not being affected.
     pub termination_withdrawn_tokens: Balance,
-    /// The lockup duration in nanoseconds from the moment when transfers are enabled to unlock the
-    /// lockup amount of tokens.
+    /// [deprecated] - the duration in nanoseconds of the lockup period from
+    /// the moment the transfers are enabled. During this period tokens are locked and
+    /// the release doesn't start. Instead of this, use `lockup_timestamp` and `release_duration`
     pub lockup_duration: Duration,
-    /// If present, the duration when the full lockup amount will be available. The tokens are
-    /// linearly released from the moment transfers are enabled.
+    /// If present, it is the duration when the full lockup amount will be available. The tokens
+    /// are linearly released from the moment tokens are unlocked, defined by:
+    /// `max(transfers_timestamp + lockup_duration, lockup_timestamp)`.
+    /// If not present, the tokens are not locked (though, vesting logic could be used).
     pub release_duration: Option<Duration>,
     /// The optional absolute lockup timestamp in nanoseconds which locks the tokens until this
-    /// timestamp passes.
+    /// timestamp passes. Until this moment the tokens are locked and the release doesn't start.
+    /// If not present, `transfers_timestamp` will be used.
     pub lockup_timestamp: Option<Timestamp>,
-    /// The information to indicate when the lockup period starts.
+    /// The information about the transfers. Either transfers are already enabled, then it contains
+    /// the timestamp when they were enabled. Or the transfers are currently disabled and
+    /// it contains the account ID of the transfer poll contract.
     pub transfers_information: TransfersInformation,
 }
 
 /// Contains information about the transfers. Whether transfers are enabled or disabled.
 #[derive(BorshDeserialize, BorshSerialize, Deserialize, Serialize, Debug)]
 #[serde(crate = "near_sdk::serde")]
 pub enum TransfersInformation {
-    /// The timestamp when the transfers were enabled. The lockup period starts at this timestamp.
+    /// The timestamp when the transfers were enabled.
     TransfersEnabled {
         transfers_timestamp: WrappedTimestamp,
     },
     /// The account ID of the transfers poll contract, to check if the transfers are enabled.
-    /// The lockup period will start when the transfer voted to be enabled.
+    /// The lockup period can start only after the transfer voted to be enabled.
     /// At the launch of the network transfers are disabled for all lockup contracts, once transfers
     /// are enabled, they can't be disabled and don't need to be checked again.
     TransfersDisabled { transfer_poll_account_id: AccountId },
@@ -122,6 +128,7 @@ impl VestingSchedule {
 #[derive(Serialize, Deserialize, Debug)]
 #[serde(crate = "near_sdk::serde")]
 pub enum VestingScheduleOrHash {
+    /// [deprecated] After transfers are enabled, only public schedule is used.
     /// The vesting schedule is private and this is a hash of (vesting_schedule, salt).
     /// In JSON, the hash has to be encoded with base64 to a string.
     VestingHash(Base64VecU8),
@@ -134,6 +141,7 @@ pub enum VestingScheduleOrHash {
 #[serde(crate = "near_sdk::serde")]
 pub enum VestingInformation {
     None,
+    /// [deprecated] After transfers are enabled, only public schedule is used.
     /// Vesting schedule is hashed for privacy and only will be revealed if the NEAR foundation
     /// has to terminate vesting.
     /// The contract assume the vesting schedule doesn't affect lockup release and duration, because