Fix for HAL-07 ownership transfer.

Now ownership transfer can be done in two steps.
make-software · May 7, 2024 · 4cd215c · 4cd215c
1 parent c14a005
commit 4cd215c
Show file tree

Hide file tree

Showing 34 changed files with 345 additions and 15 deletions.
diff --git a/dao/src/bid_escrow/contract.rs b/dao/src/bid_escrow/contract.rs
@@ -408,6 +408,14 @@ impl BidEscrowContract {
         to self.access_control {
             /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
+            /// [`Read more`](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+
+            /// Changes the ownership of the contract to the new address.
             /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
 

diff --git a/dao/src/core_contracts/dao_nft.rs b/dao/src/core_contracts/dao_nft.rs
@@ -35,6 +35,12 @@ impl DaoNft {
         to self.access_control {
             /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
+            /// [`Read more`](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract to the new address.
             /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a new address to the whitelist.

diff --git a/dao/src/core_contracts/kyc_ntf.rs b/dao/src/core_contracts/kyc_ntf.rs
@@ -37,6 +37,15 @@ impl KycNftContract {
 
     delegate! {
         to self.token {
+            /// Changes the ownership of the contract. Transfers ownership to the `owner`.
+            /// Only the current owner is permitted to call this method.
+            /// [`Read more`](crate::modules::access_control::AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](crate::modules::access_control::AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract to the new address.
+            /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a new address to the whitelist.
             pub fn add_to_whitelist(&mut self, address: Address);

diff --git a/dao/src/core_contracts/reputation/token.rs b/dao/src/core_contracts/reputation/token.rs
@@ -34,7 +34,13 @@ impl ReputationContract {
             /// Changes ownership of the contract. Transfer the ownership to the `owner`. Only the current owner
             /// is permitted to call this method.
             ///
-            /// See [AccessControl](AccessControl::change_ownership())
+            /// See [AccessControl](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// See [AccessControl](AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract to the new address.
+            /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a  new address to the whitelist.
             ///

diff --git a/dao/src/core_contracts/va_nft.rs b/dao/src/core_contracts/va_nft.rs
@@ -39,7 +39,13 @@ impl VaNftContract {
         to self.token {
             /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
-            /// [`Read more`](crate::modules::access_control::AccessControl::change_ownership())
+            /// [`Read more`](crate::modules::access_control::AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](crate::modules::access_control::AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract to the new address.
+            /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a new address to the whitelist.
             /// [`Read more`](crate::modules::access_control::AccessControl::add_to_whitelist())

diff --git a/dao/src/core_contracts/variable_repository.rs b/dao/src/core_contracts/variable_repository.rs
@@ -61,6 +61,12 @@ impl VariableRepositoryContract {
             pub fn get_owner(&self) -> Option<Address>;
             /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
+            /// [`Read more`](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract to the new address.
             /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a new address to the whitelist.

diff --git a/dao/src/modules/access_control.rs b/dao/src/modules/access_control.rs
@@ -1,5 +1,6 @@
 //! AccessControl module.
 use crate::modules::{Owner, Whitelist};
+use odra::contract_env::caller;
 use odra::types::Address;
 
 /// A AccessControl module storage definition.
@@ -23,16 +24,37 @@ impl AccessControl {
         self.whitelist.add_to_whitelist(address);
     }
 
-    /// Changes ownership of the contract. Transfer the ownership to the `owner`. Only the current owner
-    /// is permited to call this method.
+    /// Proposes a change of ownership of the contract. Owner will be changed if accepted by propsed
+    /// new owner. Only the current owner is permited to call this method.
     ///
     /// # Errors
     /// Throws [`NotAnOwner`](crate::utils::Error::NotAnOwner) if caller
     /// is not the current owner.
     ///
+    pub fn propose_new_owner(&mut self, owner: Address) {
+        self.owner.ensure_owner();
+        self.owner.propose_owner(owner);
+    }
+
+    /// Accepts the new owner proposition. This can be called only by the proposed owner.
+    ///
     /// # Events
     /// Emits [`OwnerChanged`](crate::modules::owner::events::OwnerChanged),
     /// [`AddedToWhitelist`](crate::modules::whitelist::events::AddedToWhitelist) events.
+    pub fn accept_new_owner(&mut self) {
+        let caller = caller();
+        self.owner.accept_owner(caller);
+        self.whitelist.add_to_whitelist(caller);
+    }
+
+    /// Changes the ownership of the contract to the new address.
+    ///
+    /// # Events
+    /// Emits [`OwnerChanged`](crate::modules::owner::events::OwnerChanged) event.
+    ///
+    /// # Errors
+    /// Throws [`NotAnOwner`](crate::utils::Error::NotAnOwner) if caller
+    /// is not the current owner.
     pub fn change_ownership(&mut self, owner: Address) {
         self.owner.ensure_owner();
         self.owner.change_ownership(owner);

diff --git a/dao/src/modules/owner.rs b/dao/src/modules/owner.rs
@@ -9,6 +9,7 @@ use odra::Variable;
 #[odra::module]
 pub struct Owner {
     pub owner: Variable<Address>,
+    pub proposed_owner: Variable<Option<Address>>,
 }
 
 /// Module entrypoints implementation.
@@ -20,9 +21,27 @@ impl Owner {
         self.change_ownership(owner);
     }
 
+    /// Sets a new owner proposition.
+    pub fn propose_owner(&mut self, owner: Address) {
+        self.proposed_owner.set(Some(owner));
+    }
+
+    /// Accepts the new owner proposition.
+    pub fn accept_owner(&mut self, caller: Address) {
+        if let Some(proposed_owner) = self.proposed_owner.get_or_default() {
+            if proposed_owner != caller {
+                revert(Error::NotAProposedOwner);
+            }
+            self.change_ownership(proposed_owner);
+        } else {
+            revert(Error::NoProposedOwner);
+        }
+    }
+
     /// Set the owner to the new address.
     pub fn change_ownership(&mut self, owner: Address) {
         self.owner.set(owner);
+        self.proposed_owner.set(None);
 
         OwnerChanged { new_owner: owner }.emit();
     }

diff --git a/dao/src/utils/errors.rs b/dao/src/utils/errors.rs
@@ -69,6 +69,8 @@ execution_error! {
         OnboardingRequestNotFound => 3416,
         OnboardingConfigurationNotFound => 3417,
         AttachedValueMismatch => 3418,
+        NotAProposedOwner => 3419,
+        NoProposedOwner => 3420,
 
         // Bid Escrow Errors.
         CannotPostJobForSelf => 4000,

diff --git a/dao/src/utils_contracts/ids.rs b/dao/src/utils_contracts/ids.rs
@@ -23,8 +23,8 @@ impl DaoIdsContract {
             /// Changes the ownership of the contract. Transfers the ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
             ///
-            /// [`Read more`](AccessControl::change_ownership())
-            pub fn change_ownership(&mut self, owner: Address);
+            /// [`Read more`](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
             /// Adds a new address to the whitelist.
             ///
             /// [`Read more`](AccessControl::add_to_whitelist())

diff --git a/dao/src/voting_contracts/admin.rs b/dao/src/voting_contracts/admin.rs
@@ -46,6 +46,8 @@ impl AdminContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);
@@ -173,15 +175,17 @@ impl AdminVotingCreated {
 pub enum Action {
     AddToWhitelist,
     RemoveFromWhitelist,
-    ChangeOwner,
+    ChangeOwnership,
+    ProposeNewOwner,
 }
 
 impl Action {
     pub(crate) fn get_entry_point(&self) -> String {
         match self {
             Action::AddToWhitelist => "add_to_whitelist",
             Action::RemoveFromWhitelist => "remove_from_whitelist",
-            Action::ChangeOwner => "change_ownership",
+            Action::ChangeOwnership => "change_ownership",
+            Action::ProposeNewOwner => "propose_new_owner",
         }
         .to_string()
     }
@@ -190,7 +194,8 @@ impl Action {
         match self {
             Action::AddToWhitelist => "address",
             Action::RemoveFromWhitelist => "address",
-            Action::ChangeOwner => "owner",
+            Action::ChangeOwnership => "owner",
+            Action::ProposeNewOwner => "owner",
         }
     }
 }
diff --git a/dao/src/voting_contracts/kyc_voter.rs b/dao/src/voting_contracts/kyc_voter.rs
@@ -70,6 +70,8 @@ impl KycVoterContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);

diff --git a/dao/src/voting_contracts/onboarding_request.rs b/dao/src/voting_contracts/onboarding_request.rs
@@ -75,6 +75,12 @@ impl OnboardingRequestContract {
         to self.access_control {
             /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// Only the current owner is permitted to call this method.
+            /// [`Read more`](AccessControl::propose_new_owner())
+            pub fn propose_new_owner(&mut self, owner: Address);
+            /// Accepts the new owner proposition. This can be called only by the proposed owner.
+            /// [`Read more`](AccessControl::accept_new_owner())
+            pub fn accept_new_owner(&mut self);
+            /// Changes the ownership of the contract. Transfers ownership to the `owner`.
             /// [`Read more`](AccessControl::change_ownership())
             pub fn change_ownership(&mut self, owner: Address);
             /// Adds a new address to the whitelist.

diff --git a/dao/src/voting_contracts/repo_voter.rs b/dao/src/voting_contracts/repo_voter.rs
@@ -59,6 +59,8 @@ impl RepoVoterContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);

diff --git a/dao/src/voting_contracts/reputation_voter.rs b/dao/src/voting_contracts/reputation_voter.rs
@@ -63,6 +63,8 @@ impl ReputationVoterContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);

diff --git a/dao/src/voting_contracts/simple_voter.rs b/dao/src/voting_contracts/simple_voter.rs
@@ -54,6 +54,8 @@ impl SimpleVoterContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);

diff --git a/dao/src/voting_contracts/slashing_voter.rs b/dao/src/voting_contracts/slashing_voter.rs
@@ -55,6 +55,8 @@ impl SlashingVoterContract {
         }
 
         to self.access_control {
+            pub fn propose_new_owner(&mut self, owner: Address);
+            pub fn accept_new_owner(&mut self);
             pub fn change_ownership(&mut self, owner: Address);
             pub fn add_to_whitelist(&mut self, address: Address);
             pub fn remove_from_whitelist(&mut self, address: Address);

diff --git a/dao/tests/common/contracts/ownership.rs b/dao/tests/common/contracts/ownership.rs
@@ -5,6 +5,8 @@ use crate::common::{params::Account, DaoWorld};
 #[odra::external_contract]
 trait AccessControl {
     fn change_ownership(&mut self, owner: Address);
+    fn propose_new_owner(&mut self, owner: Address);
+    fn accept_new_owner(&mut self);
     fn is_whitelisted(&self, address: Address) -> bool;
     fn remove_from_whitelist(&mut self, address: Address);
     fn add_to_whitelist(&mut self, address: Address);
@@ -39,11 +41,21 @@ impl DaoWorld {
     }
 
     pub fn change_ownership(&mut self, contract: &Account, caller: &Account, new_owner: &Account) {
+        let new_owner_address = self.get_address(new_owner);
+        let contract = self.get_address(contract);
+        self.set_caller(caller);
+        AccessControlRef::at(&contract).change_ownership(new_owner_address);
+    }
+
+    pub fn propose_new_owner(&mut self, contract: &Account, new_owner: &Account) {
         let new_owner = self.get_address(new_owner);
         let contract = self.get_address(contract);
+        AccessControlRef::at(&contract).propose_new_owner(new_owner);
+    }
 
-        self.set_caller(caller);
-        AccessControlRef::at(&contract).change_ownership(new_owner);
+    pub fn accept_new_owner(&mut self, contract: &Account) {
+        let contract = self.get_address(contract);
+        AccessControlRef::at(&contract).accept_new_owner();
     }
 
     pub fn is_whitelisted(&mut self, contract: &Account, account: &Account) -> bool {

diff --git a/dao/tests/common/contracts/voting/builder.rs b/dao/tests/common/contracts/voting/builder.rs
@@ -19,7 +19,7 @@ pub fn build(world: &DaoWorld, voting: Voting) -> VotingSetup {
             let action = match action.as_str() {
                 "add_to_whitelist" => AdminAction::AddToWhitelist,
                 "remove_from_whitelist" => AdminAction::RemoveFromWhitelist,
-                "change_ownership" => AdminAction::ChangeOwner,
+                "change_ownership" => AdminAction::ChangeOwnership,
                 unknown => panic!("{:?} is not a valid action", unknown),
             };
 

diff --git a/dao/tests/features/voting/admin/admin_actions.feature b/dao/tests/features/voting/admin/admin_actions.feature
@@ -10,8 +10,8 @@ Background:
     | VA2     | true  | 1000        |                 |
     | VA3     | true  | 1000        |                 |
   And Admin is the owner of all contracts
-  Then Alice is not whitelisted in ReputationToken contract 
-  And Bob is whitelisted in ReputationToken contract 
+  Then Alice is not whitelisted in ReputationToken contract
+  And Bob is whitelisted in ReputationToken contract
   And Bob is not the owner of ReputationToken contract
 
 Scenario Outline: Voting passed, action applied

diff --git a/dao/tests/steps/ownership.rs b/dao/tests/steps/ownership.rs
@@ -43,6 +43,15 @@ fn assert_ownership(world: &mut DaoWorld, user: Account, contract: Account) {
     assert_eq!(owner, Some(user_address));
 }
 
+#[then(expr = "{account} is the proposed owner of {account} contract")]
+fn assert_proposed_ownership(world: &mut DaoWorld, user: Account, contract: Account) {
+    let user_address = world.get_address(&user);
+    world.set_caller(&user);
+    world.accept_new_owner(&contract);
+    let owner = world.get_owner(&contract);
+    assert_eq!(owner, Some(user_address));
+}
+
 #[then(expr = "{account} is not the owner of {account} contract")]
 fn assert_ne_ownership(world: &mut DaoWorld, user: Account, contract: Account) {
     let user_address = world.get_address(&user);

diff --git a/resources/admin_contract_schema.json b/resources/admin_contract_schema.json
@@ -173,6 +173,23 @@
       ],
       "return_ty": "Unit"
     },
+    {
+      "name": "propose_new_owner",
+      "is_mutable": true,
+      "args": [
+        {
+          "name": "owner",
+          "ty": "Key"
+        }
+      ],
+      "return_ty": "Unit"
+    },
+    {
+      "name": "accept_new_owner",
+      "is_mutable": true,
+      "args": [],
+      "return_ty": "Unit"
+    },
     {
       "name": "change_ownership",
       "is_mutable": true,