diff --git a/CHANGELOG.md b/CHANGELOG.md index d97fc9898..3e77df989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - EthAccount component and preset (#853) +- Ownable two-step functionality (#809) ### Changed - Bump scarb to v2.4.4 (#853) - Bump scarb to v2.5.3 (#898) +- OwnershipTransferred event args are indexed (#809) ### Removed diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index 39aebf551..997f22b2e 100644 --- a/docs/modules/ROOT/pages/access.adoc +++ b/docs/modules/ROOT/pages/access.adoc @@ -20,7 +20,7 @@ OpenZeppelin Contracts for Cairo provides {ownable-cairo} for implementing owner Integrating this component into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's -xref:/api/access.adoc#AccessControlComponent-initializer[`initializer`] like this: +xref:/api/access.adoc#OwnableComponent-initializer[`initializer`] like this: [,javascript] ---- @@ -106,6 +106,49 @@ after an initial stage with centralized administration is over. WARNING: Removing the owner altogether will mean that administrative tasks that are protected by `assert_only_owner` will no longer be callable! +=== Two step transfer + +The component also offers a more robust way of transferring ownership via the +xref:/api/access.adoc#OwnableTwoStepImpl[OwnableTwoStepImpl] implementation. A two step transfer mechanism helps +to prevent unintended and irreversible owner transfers. Simply replace the `OwnableImpl` and `OwnableCamelOnlyImpl` +with their respective two step variants: + +[,javascript] +---- +#[abi(embed_v0)] +impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl; +#[abi(embed_v0)] +impl OwnableTwoStepCamelOnlyImpl = + OwnableComponent::OwnableTwoStepCamelOnlyImpl; +---- + +[#interface-twostep] +==== Interface + +This is the full interface of the two step `Ownable` implementation: + +[,javascript] +---- +trait IOwnableTwoStep { + /// Returns the address of the current owner. + fn owner() -> ContractAddress; + + /// Returns the address of the pending owner. + fn pending_owner() -> ContractAddress; + + /// Finishes the two-step ownership transfer process + /// by accepting the ownership. + fn accept_ownership(); + + /// Starts the two-step ownership transfer process + /// by setting the pending owner. + fn transfer_ownership(new_owner: ContractAddress); + + /// Renounces the ownership of the contract. + fn renounce_ownership(); +} +---- + == Role-Based `AccessControl` While the simplicity of ownership can be useful for simple systems or quick prototyping, different levels of @@ -326,8 +369,8 @@ roles (such as during construction). But what if we later want to grant the 'min By default, *accounts with a role cannot grant it or revoke it from other accounts*: all having a role does is making the xref:api/access.adoc#AccessControlComponent-assert_only_role[`assert_only_role`] check pass. To grant and revoke roles dynamically, you will need help from the role's _admin_. -Every role has an associated admin role, which grants permission to call the -xref:api/access.adoc#AccessControlComponent-grant_role[`grant_role`] and +Every role has an associated admin role, which grants permission to call the +xref:api/access.adoc#AccessControlComponent-grant_role[`grant_role`] and xref:api/access.adoc#AccessControlComponent-revoke_role[`revoke_role`] functions. A role can be granted or revoked by using these if the calling account has the corresponding admin role. Multiple roles may have the same admin role to make management easier. @@ -337,7 +380,7 @@ to also grant and revoke it. This mechanism can be used to create complex permissioning structures resembling organizational charts, but it also provides an easy way to manage simpler applications. `AccessControl` includes a special role with the role identifier of `0`, called `DEFAULT_ADMIN_ROLE`, which acts as the *default admin role for all roles*. -An account with this role will be able to manage any other role, unless +An account with this role will be able to manage any other role, unless xref:api/access.adoc#AccessControlComponent-_set_role_admin[`_set_role_admin`] is used to select a new admin role. Let's take a look at the ERC20 token example, this time taking advantage of the default admin role: diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 03a331f72..bbb140ff0 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -37,6 +37,14 @@ This module includes the internal `assert_only_owner` to restrict a function to * xref:OwnableComponent-owner[`++owner(self)++`] * xref:OwnableComponent-transfer_ownership[`++transfer_ownership(self, new_owner)++`] * xref:OwnableComponent-renounce_ownership[`++renounce_ownership(self)++`] + +.OwnableTwoStepImpl + +* xref:OwnableComponent-two-step-owner[`++owner(self)++`] +* xref:OwnableComponent-two-step-pending_owner[`++pending_owner(self)++`] +* xref:OwnableComponent-two-step-accept_ownership[`++accept_ownership(self)++`] +* xref:OwnableComponent-two-step-transfer_ownership[`++transfer_ownership(self, new_owner)++`] +* xref:OwnableComponent-two-step-renounce_ownership[`++renounce_ownership(self)++`] -- [.contract-index] @@ -46,6 +54,13 @@ This module includes the internal `assert_only_owner` to restrict a function to * xref:OwnableComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] * xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] + +.OwnableTwoStepCamelOnlyImpl + +* xref:OwnableComponent-two-step-pendingOwner[`++pendingOwner(self)++`] +* xref:OwnableComponent-two-step-acceptOwnership[`++acceptOwnership(self)++`] +* xref:OwnableComponent-two-step-transferOwnership[`++transferOwnership(self, new_owner)++`] +* xref:OwnableComponent-two-step-renounceOwnership[`++renounceOwnership(self)++`] -- [.contract-index] @@ -55,12 +70,15 @@ This module includes the internal `assert_only_owner` to restrict a function to * xref:OwnableComponent-initializer[`++initializer(self, owner)++`] * xref:OwnableComponent-assert_only_owner[`++assert_only_owner(self)++`] +* xref:OwnableComponent-_accept_ownership[`++_accept_ownership(self)++`] +* xref:OwnableComponent-_propose_owner[`++_propose_owner(self, new_owner)++`] * xref:OwnableComponent-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`] -- [.contract-index] .Events -- +* xref:OwnableComponent-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] * xref:OwnableComponent-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] -- @@ -70,8 +88,9 @@ This module includes the internal `assert_only_owner` to restrict a function to [.contract-item] [[OwnableComponent-owner]] ==== `[.contract-item-name]#++owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# - +// tag::owner[] Returns the address of the current owner. +// end::owner[] [.contract-item] [[OwnableComponent-transfer_ownership]] @@ -85,12 +104,51 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. [.contract-item] [[OwnableComponent-renounce_ownership]] ==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# - +// tag::renounce_ownership[] Leaves the contract without owner. It will not be possible to call `assert_only_owner` functions anymore. Can only be called by the current owner. NOTE: Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner. +//end::renounce_ownership[] + +[#OwnableComponent-Embeddable-Functions-Two-Step] +==== Embeddable Functions (Two Step Transfer) + +[.contract-item] +[[OwnableComponent-two-step-owner]] +==== `[.contract-item-name]#++owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# +include::./access.adoc[tag=owner] + +[.contract-item] +[[OwnableComponent-two-step-pending_owner]] +==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# + +Returns the address of the pending owner. + +[.contract-item] +[[OwnableComponent-two-step-accept_ownership]] +==== `[.contract-item-name]#++accept_ownership++#++(ref self: ContractState)++` [.item-kind]#external# + +Transfers ownership of the contract to the pending owner. +Can only be called by the pending owner. +Resets pending owner to zero address. + +Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. + +[.contract-item] +[[OwnableComponent-two-step-transfer_ownership]] +==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# + +Starts the two step ownership transfer process, by setting the pending owner. +Can only be called by the current owner. + +Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. + +[.contract-item] +[[OwnableComponent-two-step-renounce_ownership]] +==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# +include::./access.adoc[tag=renounce_ownership] [#OwnableComponent-camelCase-Support] ==== camelCase Support @@ -107,6 +165,33 @@ See xref:OwnableComponent-transfer_ownership[transfer_ownership]. See xref:OwnableComponent-renounce_ownership[renounce_ownership]. +[#OwnableComponent-camelCase-Support-Two-Step] +==== camelCase Support (Two Step Transfer) + +[.contract-item] +[[OwnableComponent-two-step-pendingOwner]] +==== `[.contract-item-name]#++pendingOwner++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-two-step-pending_owner[pending_owner]. + +[.contract-item] +[[OwnableComponent-two-step-acceptOwnership]] +==== `[.contract-item-name]#++acceptOwnership++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-two-step-accept_ownership[accept_ownership]. + +[.contract-item] +[[OwnableComponent-two-step-transferOwnership]] +==== `[.contract-item-name]#++transferOwnership++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-two-step-transfer_ownership[transfer_ownership]. + +[.contract-item] +[[OwnableComponent-two-step-renounceOwnership]] +==== `[.contract-item-name]#++renounceOwnership++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-two-step-renounce_ownership[renounce_ownership]. + [#OwnableComponent-Internal-Functions] ==== Internal Functions @@ -124,6 +209,28 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. Panics if called by any account other than the owner. +[.contract-item] +[[OwnableComponent-_accept_ownership]] +==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal# + +Transfers ownership to the pending owner. Resets pending owner to zero address. +Calls xref:OwnableComponent-_transfer_ownership[_transfer_ownership]. + +Internal function without access restriction. + +[.contract-item] +[[OwnableComponent-_transfer_ownership]] + +[.contract-item] +[[OwnableComponent-_propose_owner]] +==== `[.contract-item-name]#++_propose_owner++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# + +Sets a new pending owner in a two step transfer. + +Internal function without access restriction. + +Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. + [.contract-item] [[OwnableComponent-_transfer_ownership]] ==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# @@ -136,6 +243,12 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. [#OwnableComponent-Events] ==== Events +[.contract-item] +[[OwnableComponent-OwnershipTransferStarted]] +==== `[.contract-item-name]#++OwnershipTransferStarted++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# + +Emitted when the pending owner is updated. + [.contract-item] [[OwnableComponent-OwnershipTransferred]] ==== `[.contract-item-name]#++OwnershipTransferred++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# diff --git a/src/access/ownable/interface.cairo b/src/access/ownable/interface.cairo index 34227016c..11a9e7220 100644 --- a/src/access/ownable/interface.cairo +++ b/src/access/ownable/interface.cairo @@ -15,3 +15,20 @@ trait IOwnableCamelOnly { fn transferOwnership(ref self: TState, newOwner: ContractAddress); fn renounceOwnership(ref self: TState); } + +#[starknet::interface] +trait IOwnableTwoStep { + fn owner(self: @TState) -> ContractAddress; + fn pending_owner(self: @TState) -> ContractAddress; + fn accept_ownership(ref self: TState); + fn transfer_ownership(ref self: TState, new_owner: ContractAddress); + fn renounce_ownership(ref self: TState); +} + +#[starknet::interface] +trait IOwnableTwoStepCamelOnly { + fn pendingOwner(self: @TState) -> ContractAddress; + fn acceptOwnership(ref self: TState); + fn transferOwnership(ref self: TState, newOwner: ContractAddress); + fn renounceOwnership(ref self: TState); +} diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index de071e61d..d797af905 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -9,6 +9,10 @@ /// /// The initial owner can be set by using the `initializer` function in /// construction time. This can later be changed with `transfer_ownership`. +/// +/// The component also offers functionality for a two-step ownership +/// transfer where the new owner first has to accept their ownership to +/// finalize the transfer. #[starknet::component] mod OwnableComponent { use openzeppelin::access::ownable::interface; @@ -17,23 +21,36 @@ mod OwnableComponent { #[storage] struct Storage { - Ownable_owner: ContractAddress + Ownable_owner: ContractAddress, + Ownable_pending_owner: ContractAddress } #[event] #[derive(Drop, starknet::Event)] enum Event { - OwnershipTransferred: OwnershipTransferred + OwnershipTransferred: OwnershipTransferred, + OwnershipTransferStarted: OwnershipTransferStarted } #[derive(Drop, starknet::Event)] struct OwnershipTransferred { + #[key] previous_owner: ContractAddress, + #[key] + new_owner: ContractAddress, + } + + #[derive(Drop, starknet::Event)] + struct OwnershipTransferStarted { + #[key] + previous_owner: ContractAddress, + #[key] new_owner: ContractAddress, } mod Errors { const NOT_OWNER: felt252 = 'Caller is not the owner'; + const NOT_PENDING_OWNER: felt252 = 'Caller is not the pending owner'; const ZERO_ADDRESS_CALLER: felt252 = 'Caller is the zero address'; const ZERO_ADDRESS_OWNER: felt252 = 'New owner is the zero address'; } @@ -77,17 +94,78 @@ mod OwnableComponent { } } + /// Adds support for two step ownership transfer. + #[embeddable_as(OwnableTwoStepImpl)] + impl OwnableTwoStep< + TContractState, +HasComponent + > of interface::IOwnableTwoStep> { + /// Returns the address of the current owner. + fn owner(self: @ComponentState) -> ContractAddress { + self.Ownable_owner.read() + } + + /// Returns the address of the pending owner. + fn pending_owner(self: @ComponentState) -> ContractAddress { + self.Ownable_pending_owner.read() + } + + /// Finishes the two-step ownership transfer process by accepting the ownership. + /// Can only be called by the pending owner. + fn accept_ownership(ref self: ComponentState) { + let caller = get_caller_address(); + let pending_owner = self.Ownable_pending_owner.read(); + assert(caller == pending_owner, Errors::NOT_PENDING_OWNER); + self._accept_ownership(); + } + + /// Starts the two-step ownership transfer process by setting the pending owner. + fn transfer_ownership( + ref self: ComponentState, new_owner: ContractAddress + ) { + self.assert_only_owner(); + self._propose_owner(new_owner); + } + + /// Leaves the contract without owner. It will not be possible to call `assert_only_owner` + /// functions anymore. Can only be called by the current owner. + fn renounce_ownership(ref self: ComponentState) { + Ownable::renounce_ownership(ref self); + } + } + /// Adds camelCase support for `IOwnable`. #[embeddable_as(OwnableCamelOnlyImpl)] impl OwnableCamelOnly< TContractState, +HasComponent > of interface::IOwnableCamelOnly> { fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { - self.transfer_ownership(newOwner); + Ownable::transfer_ownership(ref self, newOwner); } fn renounceOwnership(ref self: ComponentState) { - self.renounce_ownership(); + Ownable::renounce_ownership(ref self); + } + } + + /// Adds camelCase support for `IOwnableTwoStep`. + #[embeddable_as(OwnableTwoStepCamelOnlyImpl)] + impl OwnableTwoStepCamelOnly< + TContractState, +HasComponent + > of interface::IOwnableTwoStepCamelOnly> { + fn pendingOwner(self: @ComponentState) -> ContractAddress { + OwnableTwoStep::pending_owner(self) + } + + fn acceptOwnership(ref self: ComponentState) { + self.accept_ownership(); + } + + fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { + OwnableTwoStep::transfer_ownership(ref self, newOwner); + } + + fn renounceOwnership(ref self: ComponentState) { + OwnableTwoStep::renounce_ownership(ref self); } } @@ -105,12 +183,35 @@ mod OwnableComponent { /// Panics if called by any account other than the owner. Use this /// to restrict access to certain functions to the owner. fn assert_only_owner(self: @ComponentState) { - let owner: ContractAddress = self.Ownable_owner.read(); - let caller: ContractAddress = get_caller_address(); + let owner = self.Ownable_owner.read(); + let caller = get_caller_address(); assert(!caller.is_zero(), Errors::ZERO_ADDRESS_CALLER); assert(caller == owner, Errors::NOT_OWNER); } + /// Transfers ownership to the pending owner. + /// + /// Internal function without access restriction. + fn _accept_ownership(ref self: ComponentState) { + let pending_owner = self.Ownable_pending_owner.read(); + self.Ownable_pending_owner.write(Zeroable::zero()); + self._transfer_ownership(pending_owner); + } + + /// Sets a new pending owner. + /// + /// Internal function without access restriction. + fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { + let previous_owner = self.Ownable_owner.read(); + self.Ownable_pending_owner.write(new_owner); + self + .emit( + OwnershipTransferStarted { + previous_owner: previous_owner, new_owner: new_owner + } + ); + } + /// Transfers ownership of the contract to a new address. /// /// Internal function without access restriction. diff --git a/src/tests/access.cairo b/src/tests/access.cairo index 2599d6d1f..078ca4837 100644 --- a/src/tests/access.cairo +++ b/src/tests/access.cairo @@ -2,3 +2,4 @@ mod test_accesscontrol; mod test_dual_accesscontrol; mod test_dual_ownable; mod test_ownable; +mod test_ownable_twostep; diff --git a/src/tests/access/test_ownable.cairo b/src/tests/access/test_ownable.cairo index c4f1fa407..1f723901e 100644 --- a/src/tests/access/test_ownable.cairo +++ b/src/tests/access/test_ownable.cairo @@ -5,6 +5,7 @@ use openzeppelin::access::ownable::interface::{IOwnable, IOwnableCamelOnly}; use openzeppelin::tests::mocks::ownable_mocks::DualCaseOwnableMock; use openzeppelin::tests::utils::constants::{ZERO, OTHER, OWNER}; use openzeppelin::tests::utils; +use openzeppelin::utils::serde::SerializedAppend; use starknet::ContractAddress; use starknet::storage::StorageMemberAccessTrait; use starknet::testing; @@ -219,4 +220,9 @@ fn assert_event_ownership_transferred(previous_owner: ContractAddress, new_owner assert_eq!(event.previous_owner, previous_owner); assert_eq!(event.new_owner, new_owner); utils::assert_no_events_left(ZERO()); + + let mut indexed_keys = array![]; + indexed_keys.append_serde(previous_owner); + indexed_keys.append_serde(new_owner); + utils::assert_indexed_keys(event, indexed_keys.span()); } diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo new file mode 100644 index 000000000..9708fc033 --- /dev/null +++ b/src/tests/access/test_ownable_twostep.cairo @@ -0,0 +1,354 @@ +use openzeppelin::access::ownable::OwnableComponent::InternalTrait; +use openzeppelin::access::ownable::OwnableComponent::OwnershipTransferStarted; +use openzeppelin::access::ownable::OwnableComponent; +use openzeppelin::access::ownable::interface::{IOwnableTwoStep, IOwnableTwoStepCamelOnly}; +use openzeppelin::tests::mocks::ownable_mocks::DualCaseTwoStepOwnableMock; +use openzeppelin::tests::utils::constants::{ZERO, OWNER, OTHER, NEW_OWNER}; +use openzeppelin::tests::utils; +use openzeppelin::utils::serde::SerializedAppend; +use starknet::ContractAddress; +use starknet::storage::StorageMemberAccessTrait; +use starknet::testing; +use super::test_ownable::assert_event_ownership_transferred; + +// +// Setup +// + +type ComponentState = OwnableComponent::ComponentState; + + +fn COMPONENT_STATE() -> ComponentState { + OwnableComponent::component_state_for_testing() +} + +fn setup() -> ComponentState { + let mut state = COMPONENT_STATE(); + state.initializer(OWNER()); + utils::drop_event(ZERO()); + state +} + +// +// initializer +// + +#[test] +#[available_gas(2000000)] +fn test_initializer_owner_pending_owner() { + let mut state = COMPONENT_STATE(); + assert(state.Ownable_owner.read() == ZERO(), 'Owner should be ZERO'); + assert(state.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); + state.initializer(OWNER()); + + assert_event_ownership_transferred(ZERO(), OWNER()); + + assert(state.Ownable_owner.read() == OWNER(), 'Owner should be set'); + assert(state.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// _accept_ownership +// + +#[test] +#[available_gas(2000000)] +fn test__accept_ownership() { + let mut state = setup(); + state.Ownable_pending_owner.write(OTHER()); + + state._accept_ownership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// _propose_owner +// + +#[test] +#[available_gas(2000000)] +fn test__propose_owner() { + let mut state = setup(); + + state._propose_owner(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); +} + +// transfer_ownership & transferOwnership + +#[test] +#[available_gas(2000000)] +fn test_transfer_ownership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + // Transferring to yet another owner while pending is set should work + state.transfer_ownership(NEW_OWNER()); + + assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); +} + +#[test] +#[available_gas(2000000)] +fn test_transfer_ownership_to_zero() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transfer_ownership(ZERO()); + + assert_event_ownership_transfer_started(OWNER(), ZERO()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_transfer_ownership_from_zero() { + let mut state = setup(); + state.transfer_ownership(OTHER()); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the owner',))] +fn test_transfer_ownership_from_nonowner() { + let mut state = setup(); + testing::set_caller_address(OTHER()); + state.transfer_ownership(OTHER()); +} + +#[test] +#[available_gas(2000000)] +fn test_transferOwnership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transferOwnership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pendingOwner() == OTHER(), 'Pending owner should be OTHER'); + + // Transferring to yet another owner while pending is set should work + state.transferOwnership(NEW_OWNER()); + + assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pendingOwner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); +} + +#[test] +#[available_gas(2000000)] +fn test_transferOwnership_to_zero() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transferOwnership(ZERO()); + + assert_event_ownership_transfer_started(OWNER(), ZERO()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pendingOwner() == ZERO(), 'Pending owner should be ZERO'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_transferOwnership_from_zero() { + let mut state = setup(); + state.transferOwnership(OTHER()); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the owner',))] +fn test_transferOwnership_from_nonowner() { + let mut state = setup(); + testing::set_caller_address(OTHER()); + state.transferOwnership(OTHER()); +} + +// +// accept_ownership & acceptOwnership +// + +#[test] +#[available_gas(2000000)] +fn test_accept_ownership() { + let mut state = setup(); + state.Ownable_pending_owner.write(OTHER()); + testing::set_caller_address(OTHER()); + + state.accept_ownership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the pending owner',))] +fn test_accept_ownership_from_nonpending() { + let mut state = setup(); + state.Ownable_pending_owner.write(NEW_OWNER()); + testing::set_caller_address(OTHER()); + state.accept_ownership(); +} + +#[test] +#[available_gas(2000000)] +fn test_acceptOwnership() { + let mut state = setup(); + state.Ownable_pending_owner.write(OTHER()); + testing::set_caller_address(OTHER()); + + state.acceptOwnership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pendingOwner() == ZERO(), 'Pending owner should be ZERO'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the pending owner',))] +fn test_acceptOwnership_from_nonpending() { + let mut state = setup(); + state.Ownable_pending_owner.write(NEW_OWNER()); + testing::set_caller_address(OTHER()); + state.acceptOwnership(); +} + +// +// renounce_ownership & renounceOwnership +// + +#[test] +#[available_gas(2000000)] +fn test_renounce_ownership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.renounce_ownership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + + assert(state.owner() == ZERO(), 'Should renounce ownership'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_renounce_ownership_from_zero_address() { + let mut state = setup(); + state.renounce_ownership(); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the owner',))] +fn test_renounce_ownership_from_nonowner() { + let mut state = setup(); + testing::set_caller_address(OTHER()); + state.renounce_ownership(); +} + +#[test] +#[available_gas(2000000)] +fn test_renounceOwnership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.renounceOwnership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + + assert(state.owner() == ZERO(), 'Should renounce ownership'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_renounceOwnership_from_zero_address() { + let mut state = setup(); + state.renounceOwnership(); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is not the owner',))] +fn test_renounceOwnership_from_nonowner() { + let mut state = setup(); + testing::set_caller_address(OTHER()); + state.renounceOwnership(); +} + +#[test] +#[available_gas(2000000)] +fn test_full_two_step_transfer() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + testing::set_caller_address(OTHER()); + state.accept_ownership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +#[test] +#[available_gas(2000000)] +fn test_pending_accept_after_owner_renounce() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + state.renounce_ownership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + assert(state.owner() == ZERO(), 'Should renounce ownership'); + + testing::set_caller_address(OTHER()); + state.accept_ownership(); + + assert_event_ownership_transferred(ZERO(), OTHER()); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// Helpers +// + +fn assert_event_ownership_transfer_started( + previous_owner: ContractAddress, new_owner: ContractAddress +) { + let event = utils::pop_log::(ZERO()).unwrap(); + assert(event.previous_owner == previous_owner, 'Invalid `previous_owner`'); + assert(event.new_owner == new_owner, 'Invalid `new_owner`'); + utils::assert_no_events_left(ZERO()); + + let mut indexed_keys = array![]; + indexed_keys.append_serde(previous_owner); + indexed_keys.append_serde(new_owner); + utils::assert_indexed_keys(event, indexed_keys.span()); +} diff --git a/src/tests/mocks/ownable_mocks.cairo b/src/tests/mocks/ownable_mocks.cairo index e6a6a294d..c0a8f5fe7 100644 --- a/src/tests/mocks/ownable_mocks.cairo +++ b/src/tests/mocks/ownable_mocks.cairo @@ -145,3 +145,36 @@ mod CamelOwnablePanicMock { panic!("Some error"); } } + +#[starknet::contract] +mod DualCaseTwoStepOwnableMock { + use openzeppelin::access::ownable::OwnableComponent; + use starknet::ContractAddress; + + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl; + #[abi(embed_v0)] + impl OwnableTwoStepCamelOnlyImpl = + OwnableComponent::OwnableTwoStepCamelOnlyImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + + #[storage] + struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage + } + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event + } + + #[constructor] + fn constructor(ref self: ContractState, owner: ContractAddress) { + self.ownable.initializer(owner); + } +}