From e2a149ae174733ff9f1f37f820a707c59abe7bf2 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Thu, 2 Nov 2023 16:38:39 +0100 Subject: [PATCH 01/25] dev: two step ownable --- src/access/ownable/interface.cairo | 16 +++ src/access/ownable/ownable.cairo | 104 +++++++++++++++++++- src/tests/access.cairo | 1 + src/tests/access/test_ownable_twostep.cairo | 37 +++++++ src/tests/mocks/ownable_mocks.cairo | 33 +++++++ 5 files changed, 187 insertions(+), 4 deletions(-) create mode 100644 src/tests/access/test_ownable_twostep.cairo diff --git a/src/access/ownable/interface.cairo b/src/access/ownable/interface.cairo index e350eed8a..20daf3f5c 100644 --- a/src/access/ownable/interface.cairo +++ b/src/access/ownable/interface.cairo @@ -15,3 +15,19 @@ 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 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 78ec49fe9..2bfdf9b6d 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -9,6 +9,8 @@ /// /// The initial owner can be set by using the `initializer` function in /// construction time. This can later be changed with `transfer_ownership`. +/// +/// TODO: add a mention about two step transfer #[starknet::component] mod OwnableComponent { use openzeppelin::access::ownable::interface; @@ -17,23 +19,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'; } @@ -43,6 +58,7 @@ mod OwnableComponent { TContractState, +HasComponent > of interface::IOwnable> { /// Returns the address of the current owner. + #[inline(always)] fn owner(self: @ComponentState) -> ContractAddress { self.Ownable_owner.read() } @@ -70,11 +86,68 @@ mod OwnableComponent { 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) { + Ownable::renounce_ownership(ref self); + } + } + + /// 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() + } + + /// Return the address of the pending owner. + fn pending_owner(self: @ComponentState) -> ContractAddress { + self.Ownable_pending_owner.read() + } + + /// Finish 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: ContractAddress = get_caller_address(); + let pending_owner: ContractAddress = self.Ownable_pending_owner.read(); + assert(caller == pending_owner, Errors::NOT_PENDING_OWNER); + self._accept_ownership(); + } + + /// Start the two step ownership transfer process by setting the pending owner. + fn transfer_ownership( + ref self: ComponentState, new_owner: ContractAddress + ) { + assert(!new_owner.is_zero(), Errors::ZERO_ADDRESS_OWNER); + 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); + } + } + + #[embeddable_as(OwnableTwoStepCamelOnlyImpl)] + impl OwnableTwoStepCamelOnly< + TContractState, +HasComponent + > of interface::IOwnableTwoStepCamelOnly> { + fn acceptOwnership(ref self: ComponentState) { + OwnableTwoStep::accept_ownership(ref self); + } + + fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { + OwnableTwoStep::transfer_ownership(ref self, newOwner); } fn renounceOwnership(ref self: ComponentState) { - self.renounce_ownership(); + OwnableTwoStep::renounce_ownership(ref self); } } @@ -98,6 +171,29 @@ mod OwnableComponent { assert(caller == owner, Errors::NOT_OWNER); } + /// Transfers the ownership to the pending owner. + /// + /// Internal function without access restriction. + fn _accept_ownership(ref self: ComponentState) { + let pending_owner: ContractAddress = self.Ownable_pending_owner.read(); + self.Ownable_pending_owner.write(Zeroable::zero()); + self._transfer_ownership(pending_owner); + } + + /// Sets a new pending owner of the contract. + /// + /// Internal function without access restriction. + fn _propose_owner( + ref self: ComponentState, new_owner: ContractAddress + ) { + let previous_owner: ContractAddress = 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_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo new file mode 100644 index 000000000..9605b1bda --- /dev/null +++ b/src/tests/access/test_ownable_twostep.cairo @@ -0,0 +1,37 @@ +use openzeppelin::access::ownable::OwnableComponent::InternalTrait; +use openzeppelin::access::ownable::interface::{IOwnableTwoStep, IOwnableTwoStepCamelOnly}; +use openzeppelin::tests::mocks::ownable_mocks::DualCaseTwoStepOwnableMock; +use openzeppelin::tests::utils::constants::{ZERO, OWNER, NEW_OWNER}; +use openzeppelin::tests::utils; +use starknet::testing; + +fn STATE() -> DualCaseTwoStepOwnableMock::ContractState { + DualCaseTwoStepOwnableMock::contract_state_for_testing() +} + +fn setup() -> DualCaseTwoStepOwnableMock::ContractState { + let mut state = STATE(); + state.ownable.initializer(OWNER()); + utils::drop_event(ZERO()); + state +} + +#[test] +#[available_gas(2000000)] +fn test_two_step_transfer() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transfer_ownership(NEW_OWNER()); + + // TODO: event test + + assert(state.ownable.pending_owner() == NEW_OWNER(), 'Should set pending owner'); + + testing::set_caller_address(NEW_OWNER()); + state.ownable.accept_ownership(); + + // TODO: event test + + assert(state.ownable.owner() == NEW_OWNER(), 'Should transfer ownership'); + assert(state.ownable.pending_owner() == ZERO(), 'Should clear pending owner'); +} diff --git a/src/tests/mocks/ownable_mocks.cairo b/src/tests/mocks/ownable_mocks.cairo index 5ed7e107c..448b364e9 100644 --- a/src/tests/mocks/ownable_mocks.cairo +++ b/src/tests/mocks/ownable_mocks.cairo @@ -145,3 +145,36 @@ mod CamelOwnablePanicMock { panic_with_felt252('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 OwnableTwoStep = OwnableComponent::OwnableTwoStepImpl; + #[abi(embed_v0)] + impl OwnableTwoStepCamelOnly = + 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); + } +} From 34f2dc04fa17a6163547fc1d454f7f8bb310e207 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Thu, 2 Nov 2023 17:04:11 +0100 Subject: [PATCH 02/25] chore: lint --- src/access/ownable/ownable.cairo | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index 2bfdf9b6d..d8e3d28d4 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -174,7 +174,7 @@ mod OwnableComponent { /// Transfers the ownership to the pending owner. /// /// Internal function without access restriction. - fn _accept_ownership(ref self: ComponentState) { + fn _accept_ownership(ref self: ComponentState) { let pending_owner: ContractAddress = self.Ownable_pending_owner.read(); self.Ownable_pending_owner.write(Zeroable::zero()); self._transfer_ownership(pending_owner); @@ -183,14 +183,14 @@ mod OwnableComponent { /// Sets a new pending owner of the contract. /// /// Internal function without access restriction. - fn _propose_owner( - ref self: ComponentState, new_owner: ContractAddress - ) { + fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { let previous_owner: ContractAddress = self.Ownable_owner.read(); self.Ownable_pending_owner.write(new_owner); self .emit( - OwnershipTransferStarted { previous_owner: previous_owner, new_owner: new_owner } + OwnershipTransferStarted { + previous_owner: previous_owner, new_owner: new_owner + } ); } From 0ad5629a8467d771eae4ffa52faa7d33536122a5 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Fri, 3 Nov 2023 15:57:46 +0100 Subject: [PATCH 03/25] test(ownable): two step test suite --- src/access/ownable/ownable.cairo | 8 +- src/tests/access/test_ownable_twostep.cairo | 325 +++++++++++++++++++- 2 files changed, 322 insertions(+), 11 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index d8e3d28d4..423e584cf 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -10,7 +10,9 @@ /// The initial owner can be set by using the `initializer` function in /// construction time. This can later be changed with `transfer_ownership`. /// -/// TODO: add a mention about two step transfer +/// 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; @@ -171,7 +173,7 @@ mod OwnableComponent { assert(caller == owner, Errors::NOT_OWNER); } - /// Transfers the ownership to the pending owner. + /// Transfers ownership to the pending owner. /// /// Internal function without access restriction. fn _accept_ownership(ref self: ComponentState) { @@ -180,7 +182,7 @@ mod OwnableComponent { self._transfer_ownership(pending_owner); } - /// Sets a new pending owner of the contract. + /// Sets a new pending owner. /// /// Internal function without access restriction. fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index 9605b1bda..5bf17cfb6 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -1,9 +1,17 @@ use openzeppelin::access::ownable::OwnableComponent::InternalTrait; +use openzeppelin::access::ownable::OwnableComponent::OwnershipTransferStarted; use openzeppelin::access::ownable::interface::{IOwnableTwoStep, IOwnableTwoStepCamelOnly}; use openzeppelin::tests::mocks::ownable_mocks::DualCaseTwoStepOwnableMock; -use openzeppelin::tests::utils::constants::{ZERO, OWNER, NEW_OWNER}; +use openzeppelin::tests::utils::constants::{ZERO, OWNER, OTHER, NEW_OWNER}; use openzeppelin::tests::utils; +use starknet::ContractAddress; +use starknet::storage::StorageMemberAccessTrait; use starknet::testing; +use super::test_ownable::assert_event_ownership_transferred; + +// +// Setup +// fn STATE() -> DualCaseTwoStepOwnableMock::ContractState { DualCaseTwoStepOwnableMock::contract_state_for_testing() @@ -16,22 +24,323 @@ fn setup() -> DualCaseTwoStepOwnableMock::ContractState { state } +// +// initializer +// + +#[test] +#[available_gas(2000000)] +fn test_initializer_owner_pending_owner() { + let mut state = STATE(); + assert(state.ownable.Ownable_owner.read() == ZERO(), 'Owner should be ZERO'); + assert(state.ownable.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); + state.ownable.initializer(OWNER()); + + assert_event_ownership_transferred(ZERO(), OWNER()); + + assert(state.ownable.Ownable_owner.read() == OWNER(), 'Owner should be set'); + assert(state.ownable.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.Ownable_pending_owner.write(OTHER()); + + state.ownable._accept_ownership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// _propose_owner +// + #[test] #[available_gas(2000000)] -fn test_two_step_transfer() { +fn test__propose_owner() { + let mut state = setup(); + + state.ownable._propose_owner(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.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.ownable.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + // transferring to yet another owner while pending is set should work state.ownable.transfer_ownership(NEW_OWNER()); - // TODO: event test + assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('New owner is the zero address',))] +fn test_transfer_ownership_to_zero() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transfer_ownership(ZERO()); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_transfer_ownership_from_zero() { + let mut state = setup(); + state.ownable.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.ownable.transfer_ownership(OTHER()); +} + +#[test] +#[available_gas(2000000)] +fn test_transferOwnership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transferOwnership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + // transferring to yet another owner while pending is set should work + state.ownable.transferOwnership(NEW_OWNER()); + + assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('New owner is the zero address',))] +fn test_transferOwnership_to_zero() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transferOwnership(ZERO()); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Caller is the zero address',))] +fn test_transferOwnership_from_zero() { + let mut state = setup(); + state.ownable.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.ownable.transferOwnership(OTHER()); +} + +// +// accept_ownership & acceptOwnership +// + +#[test] +#[available_gas(2000000)] +fn test_accept_ownership() { + let mut state = setup(); + state.ownable.Ownable_pending_owner.write(OTHER()); + testing::set_caller_address(OTHER()); + + state.ownable.accept_ownership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.ownable.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.Ownable_pending_owner.write(NEW_OWNER()); + testing::set_caller_address(OTHER()); + state.ownable.accept_ownership(); +} + +#[test] +#[available_gas(2000000)] +fn test_acceptOwnership() { + let mut state = setup(); + state.ownable.Ownable_pending_owner.write(OTHER()); + testing::set_caller_address(OTHER()); + + state.ownable.acceptOwnership(); + + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.ownable.pending_owner() == 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.Ownable_pending_owner.write(NEW_OWNER()); + testing::set_caller_address(OTHER()); + state.ownable.acceptOwnership(); +} + +// +// renounce_ownership & renounceOwnership +// + +#[test] +#[available_gas(2000000)] +fn test_renounce_ownership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.renounce_ownership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + + assert(state.ownable.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.ownable.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.ownable.renounce_ownership(); +} + +#[test] +#[available_gas(2000000)] +fn test_renounceOwnership() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.renounceOwnership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + + assert(state.ownable.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.ownable.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.ownable.renounceOwnership(); +} - assert(state.ownable.pending_owner() == NEW_OWNER(), 'Should set pending owner'); +// +// full two step ownership transfer (happy path) +// - testing::set_caller_address(NEW_OWNER()); +#[test] +#[available_gas(2000000)] +fn test_full_two_step_transfer() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + testing::set_caller_address(OTHER()); state.ownable.accept_ownership(); - // TODO: event test + assert_event_ownership_transferred(OWNER(), OTHER()); + assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// test accept ownership after original owner renounced +// + +#[test] +#[available_gas(2000000)] +fn test_pending_accept_after_owner_renounce() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.ownable.transfer_ownership(OTHER()); + + assert_event_ownership_transfer_started(OWNER(), OTHER()); + assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + + state.ownable.renounce_ownership(); + + assert_event_ownership_transferred(OWNER(), ZERO()); + assert(state.ownable.owner() == ZERO(), 'Should renounce ownership'); + + testing::set_caller_address(OTHER()); + state.ownable.accept_ownership(); + + assert_event_ownership_transferred(ZERO(), OTHER()); + assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); +} + +// +// Helpers +// - assert(state.ownable.owner() == NEW_OWNER(), 'Should transfer ownership'); - assert(state.ownable.pending_owner() == ZERO(), 'Should clear pending owner'); +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()); } From def6cc9ce9f8a1f63fc8da427c9377a5fb932588 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 8 Nov 2023 21:15:44 +0100 Subject: [PATCH 04/25] dev: apply suggestions from code review Co-authored-by: Eric Nordelo --- src/access/ownable/ownable.cairo | 20 ++++++++++---------- src/tests/access/test_ownable_twostep.cairo | 12 ++---------- src/tests/mocks/ownable_mocks.cairo | 4 ++-- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index 423e584cf..134b69b2c 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -10,7 +10,7 @@ /// 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 +/// 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] @@ -60,7 +60,6 @@ mod OwnableComponent { TContractState, +HasComponent > of interface::IOwnable> { /// Returns the address of the current owner. - #[inline(always)] fn owner(self: @ComponentState) -> ContractAddress { self.Ownable_owner.read() } @@ -106,21 +105,21 @@ mod OwnableComponent { self.Ownable_owner.read() } - /// Return the address of the pending owner. + /// Returns the address of the pending owner. fn pending_owner(self: @ComponentState) -> ContractAddress { self.Ownable_pending_owner.read() } - /// Finish the two step ownership transfer process by accepting the ownership. + /// 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: ContractAddress = get_caller_address(); - let pending_owner: ContractAddress = self.Ownable_pending_owner.read(); + let caller = get_caller_address(); + let pending_owner = self.Ownable_pending_owner.read(); assert(caller == pending_owner, Errors::NOT_PENDING_OWNER); self._accept_ownership(); } - /// Start the two step ownership transfer process by setting the pending owner. + /// Starts the two-step ownership transfer process by setting the pending owner. fn transfer_ownership( ref self: ComponentState, new_owner: ContractAddress ) { @@ -136,12 +135,13 @@ mod OwnableComponent { } } + /// Adds camelCase support for `IOwnableTwoStep`. #[embeddable_as(OwnableTwoStepCamelOnlyImpl)] impl OwnableTwoStepCamelOnly< TContractState, +HasComponent > of interface::IOwnableTwoStepCamelOnly> { fn acceptOwnership(ref self: ComponentState) { - OwnableTwoStep::accept_ownership(ref self); + self.accept_ownership(); } fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { @@ -177,7 +177,7 @@ mod OwnableComponent { /// /// Internal function without access restriction. fn _accept_ownership(ref self: ComponentState) { - let pending_owner: ContractAddress = self.Ownable_pending_owner.read(); + let pending_owner = self.Ownable_pending_owner.read(); self.Ownable_pending_owner.write(Zeroable::zero()); self._transfer_ownership(pending_owner); } @@ -186,7 +186,7 @@ mod OwnableComponent { /// /// Internal function without access restriction. fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { - let previous_owner: ContractAddress = self.Ownable_owner.read(); + let previous_owner = self.Ownable_owner.read(); self.Ownable_pending_owner.write(new_owner); self .emit( diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index 5bf17cfb6..0f42ff848 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -88,7 +88,7 @@ fn test_transfer_ownership() { assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); - // transferring to yet another owner while pending is set should work + // Transferring to yet another owner while pending is set should work state.ownable.transfer_ownership(NEW_OWNER()); assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); @@ -133,7 +133,7 @@ fn test_transferOwnership() { assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); - // transferring to yet another owner while pending is set should work + // Transferring to yet another owner while pending is set should work state.ownable.transferOwnership(NEW_OWNER()); assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); @@ -281,10 +281,6 @@ fn test_renounceOwnership_from_nonowner() { state.ownable.renounceOwnership(); } -// -// full two step ownership transfer (happy path) -// - #[test] #[available_gas(2000000)] fn test_full_two_step_transfer() { @@ -304,10 +300,6 @@ fn test_full_two_step_transfer() { assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); } -// -// test accept ownership after original owner renounced -// - #[test] #[available_gas(2000000)] fn test_pending_accept_after_owner_renounce() { diff --git a/src/tests/mocks/ownable_mocks.cairo b/src/tests/mocks/ownable_mocks.cairo index 448b364e9..76cc6741a 100644 --- a/src/tests/mocks/ownable_mocks.cairo +++ b/src/tests/mocks/ownable_mocks.cairo @@ -154,9 +154,9 @@ mod DualCaseTwoStepOwnableMock { component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); #[abi(embed_v0)] - impl OwnableTwoStep = OwnableComponent::OwnableTwoStepImpl; + impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl; #[abi(embed_v0)] - impl OwnableTwoStepCamelOnly = + impl OwnableTwoStepCamelOnlyImpl = OwnableComponent::OwnableTwoStepCamelOnlyImpl; impl InternalImpl = OwnableComponent::InternalImpl; From 4b992a6a8c92e9aaf029e163b6987f357afbcf47 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 8 Nov 2023 21:16:57 +0100 Subject: [PATCH 05/25] chore: removing explicit types --- src/access/ownable/ownable.cairo | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index 134b69b2c..f27e1484e 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -167,8 +167,8 @@ 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); } From 54365bbb44b5d01f40d9b6fa81fa7a4edc8df847 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 8 Nov 2023 21:23:22 +0100 Subject: [PATCH 06/25] dev: asserting ownable events keys --- src/tests/access/test_ownable.cairo | 6 ++++++ src/tests/access/test_ownable_twostep.cairo | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/tests/access/test_ownable.cairo b/src/tests/access/test_ownable.cairo index c88448916..475b70a41 100644 --- a/src/tests/access/test_ownable.cairo +++ b/src/tests/access/test_ownable.cairo @@ -4,6 +4,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; @@ -234,4 +235,9 @@ fn assert_event_ownership_transferred(previous_owner: ContractAddress, new_owner 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/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index 0f42ff848..7c2811af8 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -4,6 +4,7 @@ use openzeppelin::access::ownable::interface::{IOwnableTwoStep, IOwnableTwoStepC 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; @@ -335,4 +336,9 @@ fn assert_event_ownership_transfer_started( 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()); } From 7e1b537becbdd6daa88a0ed0bdded4020a3a10a1 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 8 Nov 2023 22:49:01 +0100 Subject: [PATCH 07/25] docs: OwnableTwoStep API --- docs/modules/ROOT/pages/api/access.adoc | 184 ++++++++++++++++++++++++ 1 file changed, 184 insertions(+) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 28855ddce..07eb16191 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -1,6 +1,7 @@ :github-icon: pass:[] :AccessControl: xref:AccessControlComponent[AccessControl] :Ownable: xref:OwnableComponent[Ownable] +:OwnableTwoStep: xref:OwnableTwoStep[OwnableTwoStep] :src5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SRC5] :inner-src5: xref:api/introspection.adoc#ISRC5[SRC5 ID] :_set_role_admin: xref:#AccessControlComponent-_set_role_admin[_set_role_admin] @@ -11,6 +12,8 @@ This directory provides ways to restrict who can access the functions of a contr - {Ownable} is a simple mechanism with a single "owner" role that can be assigned to a single account. This mechanism can be useful in simple scenarios, but fine grained access needs are likely to outgrow it. +- {OwnableTwoStep} is like Ownable, but ownership transfer is a two-step process where the newly proposed +owner has to accept the transfer before it becomes active. - {AccessControl} provides a general role based access control mechanism. Multiple hierarchical roles can be created and assigned each to multiple accounts. @@ -142,6 +145,187 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. Emitted when the ownership is transferred. +[.contract] +[[OwnableTwoStep]] +=== `++OwnableTwoStep++` link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.8.0-beta.0/src/access/ownable/ownable.cairo[{github-icon},role=heading-link] + +```javascript +use openzeppelin::access::ownable::OwnableComponent; +``` + +`OwnableTwoStep` provides a basic access control mechanism where an account + (an owner) can be granted exclusive access to specific functions. Ownership can be + transferred to a new address in a two-step process, as opposed to {Ownable}, where + the transfer is immediate. + +This module includes the internal `assert_only_owner` to restrict a function to be used only by the owner. + +[.contract-index] +.Embeddable Implementations +-- +.OwnableTwoStepImpl + +* xref:OwnableTwoStep-owner[`++owner(self)++`] +* xref:OwnableTwoStep-pending_owner[`++pending_owner(self)++`] +* xref:OwnableTwoStep-accept_ownership[`++accept_ownership(self)++`] +* xref:OwnableTwoStep-transfer_ownership[`++transfer_ownership(self, new_owner)++`] +* xref:OwnableTwoStep-renounce_ownership[`++renounce_ownership(self)++`] +-- + +[.contract-index] +.Embeddable Implementations (camelCase) +-- +.OwnableTwoStepCamelOnlyImpl + +* xref:OwnableTwoStep-acceptOwnership[`++acceptOwnership(self)++`] +* xref:OwnableTwoStep-transferOwnership[`++transferOwnership(self, newOwner)++`] +* xref:OwnableTwoStep-renounceOwnership[`++renounceOwnership(self)++`] +-- + +[.contract-index] +.Internal Implementations +-- +.InternalImpl + +* xref:OwnableTwoStep-initializer[`++initializer(self, owner)++`] +* xref:OwnableTwoStep-assert_only_owner[`++assert_only_owner(self)++`] +* xref:OwnableTwoStep-_accept_ownership[`++_accept_ownership(self)++`] +* xref:OwnableTwoStep-_propose_owner[`++_propose_owner(self, new_owner)++`] +* xref:OwnableTwoStep-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`] +-- + +[.contract-index] +.Events +-- +* xref:OwnableTwoStep-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] +* xref:OwnableTwoStep-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] +-- + +[#OwnableTwoStep-Embeddable-Functions] +==== Embeddable Functions + +[.contract-item] +[[OwnableTwoStep-owner]] +==== `[.contract-item-name]#++owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# + +Returns the address of the current owner. + +[.contract-item] +[[OwnableTwoStep-pending_owner]] +==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# + +Returns the address of the pending owner. + +[.contract-item] +[[OwnableTwoStep-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:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. + +[.contract-item] +[[OwnableTwoStep-transfer_ownership]] +==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# + +Sets pending owner to a new account (`new_owner`). +Can only be called by the current owner. + +Emits an xref:OwnableTwoStep-OwnershipTransferStarted[OwnershipTransferStarted] event. + +[.contract-item] +[[OwnableTwoStep-renounce_ownership]] +==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# + +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. + +[#OwnableTwoStep-camelCase-Support] +==== camelCase Support + +[.contract-item] +[[OwnableTwoStep-acceptOwnership]] +==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableTwoStep-accept_ownership[transfer_ownership]. + +[.contract-item] +[[OwnableTwoStep-transferOwnership]] +==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# + +See xref:OwnableTwoStep-transfer_ownership[transfer_ownership]. + +[.contract-item] +[[OwnableTwoStep-renounceOwnership]] +==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableTwoStep-renounce_ownership[renounce_ownership]. + +[#Ownable-Internal-Functions] +==== Internal Functions + +[.contract-item] +[[OwnableTwoStep-initializer]] +==== `[.contract-item-name]#++initializer++#++(ref self: ContractState, owner: ContractAddress)++` [.item-kind]#internal# + +Initializes the contract and sets `owner` as the initial owner. + +Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. + +[.contract-item] +[[OwnableTwoStep-assert_only_owner]] +==== `[.contract-item-name]#++assert_only_owner++#++(self: @ContractState)++` [.item-kind]#internal# + +Panics if called by any account other than the owner. + +[.contract-item] +[[OwnableTwoStep-_accept_ownership]] +==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal# + +Transfers ownership of the contract to the pending owner. +Sets the pending owner to zero address. +Internal function without access restriction. + +Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. + +[.contract-item] +[[OwnableTwoStep-_propose_owner]] +==== `[.contract-item-name]#++_propose_owner++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# + +Sets pending owner to a new account (`new_owner`). +Internal function without access restriction. + +Emits an xref:OwnableTwoStep-OwnershipTransferStarted[OwnershipTransferStarted] event. + +[.contract-item] +[[OwnableTwoStep-_transfer_ownership]] +==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# + +Transfers ownership of the contract to a new account (`new_owner`). +Internal function without access restriction. + +Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. + +[#OwnableTwoStep-Events] +==== Events + +[.contract-item] +[[OwnableTwoStep-OwnershipTransferStarted]] +==== `[.contract-item-name]#++OwnershipTransferStarted++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# + +Emitted when the pending owner is updated. + +[.contract-item] +[[OwnableTwoStep-OwnershipTransferred]] +==== `[.contract-item-name]#++OwnershipTransferred++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# + +Emitted when the ownership is transferred. + [.contract] [[IAccessControl]] === `++IAccessControl++` link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.8.0/src/access/accesscontrol/interface.cairo[{github-icon},role=heading-link] From 7551934a793af8aecd7bc61e7ea988eb102c1997 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Thu, 9 Nov 2023 22:34:54 +0100 Subject: [PATCH 08/25] docs: OwnableTwoStep usage and impl section --- docs/modules/ROOT/pages/access.adoc | 134 +++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 3 deletions(-) diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index d8d6f66f9..f4f05e01b 100644 --- a/docs/modules/ROOT/pages/access.adoc +++ b/docs/modules/ROOT/pages/access.adoc @@ -106,6 +106,134 @@ 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! +== `TwoStepOwnable` + +The `TwoStepOwnable` offers a more robust way of transferring ownership when compared to `Ownable`. +The difference lies in the fact that the new owner has to first xref:/api/access.adoc#OwnableTwoStep-accept_ownership[accept] +their ownership. Only after this call is made, the ownersihp is transferred. This mechanism helps to prevent +unintended and irreversible owner transfers. + +=== Usage + +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#AccessControl-initializer[`initializer`] like this: + +[,javascript] +---- +#[starknet::contract] +mod MyContract { + 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) { + // Set the initial owner of the contract + self.ownable.initializer(owner); + } + + (...) +} +---- + +To restrict a function's access to the owner only, add in the `assert_only_owner` method: + +[,javascript] +---- +#[starknet::contract] +mod MyContract { + (...) + + #[external(v0)] + fn only_owner_allowed(ref self: ContractState) { + // This function can only be called by the owner + self.ownable.assert_only_owner(); + + (...) + } +} +---- + +=== Interface + +This is the full interface of the `TwoStepOwnable` implementation: + +[,javascript] +---- +trait IOwnableTwoStep { + /// Returns the current owner + fn owner() -> ContractAddress; + + /// Returns the pending owner. + fn pending_owner() -> ContractAddress; + + /// Finishes the two-step ownership transfer process + /// by accepting the ownership. + fn accept_ownership(ref self: TState); + + /// Starts the two-step ownership transfer process + /// by setting the pending owner. + fn transfer_ownership(ref self: TState, new_owner: ContractAddress); + + /// Renounces the ownership of the contract. + fn renounce_ownership(ref self: TState); +} +---- + +TODO: what else, if anything, should be mentioned here? just copy-paste from Ownable? + maybe mention that `transfer_ownership` can be called multiple times to overwrite the + proposed owner? and that after accepting, the pending owner is set to zero? + +=== Upgrading from `Ownable` + +If your contract already is xref:ownership_and_ownable[Ownable] and you want to xref:/upgrades.adoc[upgrade] it to `TwoStepOwnable`, +the only thing necessary is to replace the declared implementations. Change + +[,javascript] +---- +#[abi(embed_v0)] +impl OwnableImpl = OwnableComponent::OwnableImpl; +#[abi(embed_v0)] +impl OwnableCamelOnlyImpl = + OwnableComponent::OwnableCamelOnlyImpl; +---- + +into + +[,javascript] +---- +#[abi(embed_v0)] +impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl; +#[abi(embed_v0)] +impl OwnableTwoStepCamelOnlyImpl = + OwnableComponent::OwnableTwoStepCamelOnlyImpl; +---- + +After the upgrade, the set `owner` will be preserved and you will gain the functionality of +a two-step ownable implementation. + == Role-Based `AccessControl` While the simplicity of ownership can be useful for simple systems or quick prototyping, different levels of @@ -326,8 +454,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 +465,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: From 22f9c113e2d6def3301521769085a0cd84a5815b Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Thu, 9 Nov 2023 22:56:58 +0100 Subject: [PATCH 09/25] docs: using OwnableTwoStepComponent everywhere --- docs/modules/ROOT/pages/access.adoc | 6 +- docs/modules/ROOT/pages/api/access.adoc | 84 ++++++++++++------------- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index f4f05e01b..fe0386677 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] ---- @@ -109,7 +109,7 @@ will no longer be callable! == `TwoStepOwnable` The `TwoStepOwnable` offers a more robust way of transferring ownership when compared to `Ownable`. -The difference lies in the fact that the new owner has to first xref:/api/access.adoc#OwnableTwoStep-accept_ownership[accept] +The difference lies in the fact that the new owner has to first xref:/api/access.adoc#OwnableTwoStepComponent-accept_ownership[accept] their ownership. Only after this call is made, the ownersihp is transferred. This mechanism helps to prevent unintended and irreversible owner transfers. @@ -117,7 +117,7 @@ unintended and irreversible owner transfers. 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#AccessControl-initializer[`initializer`] like this: +xref:/api/access.adoc#OwnableTwoStepComponent-initializer[`initializer`] like this: [,javascript] ---- diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 07eb16191..52803ec77 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -1,7 +1,7 @@ :github-icon: pass:[] :AccessControl: xref:AccessControlComponent[AccessControl] :Ownable: xref:OwnableComponent[Ownable] -:OwnableTwoStep: xref:OwnableTwoStep[OwnableTwoStep] +:OwnableTwoStep: xref:OwnableTwoStepComponent[OwnableTwoStep] :src5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SRC5] :inner-src5: xref:api/introspection.adoc#ISRC5[SRC5 ID] :_set_role_admin: xref:#AccessControlComponent-_set_role_admin[_set_role_admin] @@ -146,8 +146,8 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. Emitted when the ownership is transferred. [.contract] -[[OwnableTwoStep]] -=== `++OwnableTwoStep++` link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.8.0-beta.0/src/access/ownable/ownable.cairo[{github-icon},role=heading-link] +[[OwnableTwoStepComponent]] +=== `++OwnableTwoStepComponent++` link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.8.0-beta.0/src/access/ownable/ownable.cairo[{github-icon},role=heading-link] ```javascript use openzeppelin::access::ownable::OwnableComponent; @@ -165,11 +165,11 @@ This module includes the internal `assert_only_owner` to restrict a function to -- .OwnableTwoStepImpl -* xref:OwnableTwoStep-owner[`++owner(self)++`] -* xref:OwnableTwoStep-pending_owner[`++pending_owner(self)++`] -* xref:OwnableTwoStep-accept_ownership[`++accept_ownership(self)++`] -* xref:OwnableTwoStep-transfer_ownership[`++transfer_ownership(self, new_owner)++`] -* xref:OwnableTwoStep-renounce_ownership[`++renounce_ownership(self)++`] +* xref:OwnableTwoStepComponent-owner[`++owner(self)++`] +* xref:OwnableTwoStepComponent-pending_owner[`++pending_owner(self)++`] +* xref:OwnableTwoStepComponent-accept_ownership[`++accept_ownership(self)++`] +* xref:OwnableTwoStepComponent-transfer_ownership[`++transfer_ownership(self, new_owner)++`] +* xref:OwnableTwoStepComponent-renounce_ownership[`++renounce_ownership(self)++`] -- [.contract-index] @@ -177,9 +177,9 @@ This module includes the internal `assert_only_owner` to restrict a function to -- .OwnableTwoStepCamelOnlyImpl -* xref:OwnableTwoStep-acceptOwnership[`++acceptOwnership(self)++`] -* xref:OwnableTwoStep-transferOwnership[`++transferOwnership(self, newOwner)++`] -* xref:OwnableTwoStep-renounceOwnership[`++renounceOwnership(self)++`] +* xref:OwnableTwoStepComponent-acceptOwnership[`++acceptOwnership(self)++`] +* xref:OwnableTwoStepComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] +* xref:OwnableTwoStepComponent-renounceOwnership[`++renounceOwnership(self)++`] -- [.contract-index] @@ -187,56 +187,56 @@ This module includes the internal `assert_only_owner` to restrict a function to -- .InternalImpl -* xref:OwnableTwoStep-initializer[`++initializer(self, owner)++`] -* xref:OwnableTwoStep-assert_only_owner[`++assert_only_owner(self)++`] -* xref:OwnableTwoStep-_accept_ownership[`++_accept_ownership(self)++`] -* xref:OwnableTwoStep-_propose_owner[`++_propose_owner(self, new_owner)++`] -* xref:OwnableTwoStep-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`] +* xref:OwnableTwoStepComponent-initializer[`++initializer(self, owner)++`] +* xref:OwnableTwoStepComponent-assert_only_owner[`++assert_only_owner(self)++`] +* xref:OwnableTwoStepComponent-_accept_ownership[`++_accept_ownership(self)++`] +* xref:OwnableTwoStepComponent-_propose_owner[`++_propose_owner(self, new_owner)++`] +* xref:OwnableTwoStepComponent-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`] -- [.contract-index] .Events -- -* xref:OwnableTwoStep-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] -* xref:OwnableTwoStep-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] +* xref:OwnableTwoStepComponent-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] +* xref:OwnableTwoStepComponent-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] -- -[#OwnableTwoStep-Embeddable-Functions] +[#OwnableTwoStepComponent-Embeddable-Functions] ==== Embeddable Functions [.contract-item] -[[OwnableTwoStep-owner]] +[[OwnableTwoStepComponent-owner]] ==== `[.contract-item-name]#++owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# Returns the address of the current owner. [.contract-item] -[[OwnableTwoStep-pending_owner]] +[[OwnableTwoStepComponent-pending_owner]] ==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# Returns the address of the pending owner. [.contract-item] -[[OwnableTwoStep-accept_ownership]] +[[OwnableTwoStepComponent-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:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. +Emits an xref:OwnableTwoStepComponent-OwnershipTransferred[OwnershipTransferred] event. [.contract-item] -[[OwnableTwoStep-transfer_ownership]] +[[OwnableTwoStepComponent-transfer_ownership]] ==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# Sets pending owner to a new account (`new_owner`). Can only be called by the current owner. -Emits an xref:OwnableTwoStep-OwnershipTransferStarted[OwnershipTransferStarted] event. +Emits an xref:OwnableTwoStepComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. [.contract-item] -[[OwnableTwoStep-renounce_ownership]] +[[OwnableTwoStepComponent-renounce_ownership]] ==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# Leaves the contract without owner. It will not be possible to call @@ -245,32 +245,32 @@ Leaves the contract without owner. It will not be possible to call NOTE: Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner. -[#OwnableTwoStep-camelCase-Support] +[#OwnableTwoStepComponent-camelCase-Support] ==== camelCase Support [.contract-item] -[[OwnableTwoStep-acceptOwnership]] +[[OwnableTwoStepComponent-acceptOwnership]] ==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# -See xref:OwnableTwoStep-accept_ownership[transfer_ownership]. +See xref:OwnableTwoStepComponent-accept_ownership[transfer_ownership]. [.contract-item] -[[OwnableTwoStep-transferOwnership]] +[[OwnableTwoStepComponent-transferOwnership]] ==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# -See xref:OwnableTwoStep-transfer_ownership[transfer_ownership]. +See xref:OwnableTwoStepComponent-transfer_ownership[transfer_ownership]. [.contract-item] -[[OwnableTwoStep-renounceOwnership]] +[[OwnableTwoStepComponent-renounceOwnership]] ==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# -See xref:OwnableTwoStep-renounce_ownership[renounce_ownership]. +See xref:OwnableTwoStepComponent-renounce_ownership[renounce_ownership]. [#Ownable-Internal-Functions] ==== Internal Functions [.contract-item] -[[OwnableTwoStep-initializer]] +[[OwnableTwoStepComponent-initializer]] ==== `[.contract-item-name]#++initializer++#++(ref self: ContractState, owner: ContractAddress)++` [.item-kind]#internal# Initializes the contract and sets `owner` as the initial owner. @@ -278,13 +278,13 @@ Initializes the contract and sets `owner` as the initial owner. Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. [.contract-item] -[[OwnableTwoStep-assert_only_owner]] +[[OwnableTwoStepComponent-assert_only_owner]] ==== `[.contract-item-name]#++assert_only_owner++#++(self: @ContractState)++` [.item-kind]#internal# Panics if called by any account other than the owner. [.contract-item] -[[OwnableTwoStep-_accept_ownership]] +[[OwnableTwoStepComponent-_accept_ownership]] ==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal# Transfers ownership of the contract to the pending owner. @@ -294,34 +294,34 @@ Internal function without access restriction. Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. [.contract-item] -[[OwnableTwoStep-_propose_owner]] +[[OwnableTwoStepComponent-_propose_owner]] ==== `[.contract-item-name]#++_propose_owner++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# Sets pending owner to a new account (`new_owner`). Internal function without access restriction. -Emits an xref:OwnableTwoStep-OwnershipTransferStarted[OwnershipTransferStarted] event. +Emits an xref:OwnableTwoStepComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. [.contract-item] -[[OwnableTwoStep-_transfer_ownership]] +[[OwnableTwoStepComponent-_transfer_ownership]] ==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# Transfers ownership of the contract to a new account (`new_owner`). Internal function without access restriction. -Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. +Emits an xref:OwnableTwoStepComponent-OwnershipTransferred[OwnershipTransferred] event. [#OwnableTwoStep-Events] ==== Events [.contract-item] -[[OwnableTwoStep-OwnershipTransferStarted]] +[[OwnableTwoStepComponent-OwnershipTransferStarted]] ==== `[.contract-item-name]#++OwnershipTransferStarted++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# Emitted when the pending owner is updated. [.contract-item] -[[OwnableTwoStep-OwnershipTransferred]] +[[OwnableTwoStepComponent-OwnershipTransferred]] ==== `[.contract-item-name]#++OwnershipTransferred++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# Emitted when the ownership is transferred. From 81cc84fde262769993ca3c06ad26d8bb219faa46 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Fri, 10 Nov 2023 21:49:44 +0100 Subject: [PATCH 10/25] dev: adding pendingOwner --- docs/modules/ROOT/pages/api/access.adoc | 9 ++++++++- src/access/ownable/interface.cairo | 1 + src/access/ownable/ownable.cairo | 4 ++++ src/tests/access/test_ownable_twostep.cairo | 6 +++--- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 52803ec77..bb39e5173 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -177,6 +177,7 @@ This module includes the internal `assert_only_owner` to restrict a function to -- .OwnableTwoStepCamelOnlyImpl +* xref:OwnableTwoStepComponent-pendingOwner[`++pendingOwner(self)++`] * xref:OwnableTwoStepComponent-acceptOwnership[`++acceptOwnership(self)++`] * xref:OwnableTwoStepComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] * xref:OwnableTwoStepComponent-renounceOwnership[`++renounceOwnership(self)++`] @@ -248,11 +249,17 @@ thereby removing any functionality that is only available to the owner. [#OwnableTwoStepComponent-camelCase-Support] ==== camelCase Support +[.contract-item] +[[OwnableTwoStepComponent-acceptOwnership]] +==== `[.contract-item-name]#++pendingOwner++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableTwoStepComponent-pending_owner[pending_owner]. + [.contract-item] [[OwnableTwoStepComponent-acceptOwnership]] ==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# -See xref:OwnableTwoStepComponent-accept_ownership[transfer_ownership]. +See xref:OwnableTwoStepComponent-accept_ownership[accept_ownership]. [.contract-item] [[OwnableTwoStepComponent-transferOwnership]] diff --git a/src/access/ownable/interface.cairo b/src/access/ownable/interface.cairo index 20daf3f5c..fc62cccc5 100644 --- a/src/access/ownable/interface.cairo +++ b/src/access/ownable/interface.cairo @@ -27,6 +27,7 @@ trait IOwnableTwoStep { #[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 f27e1484e..f469f8833 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -140,6 +140,10 @@ mod OwnableComponent { impl OwnableTwoStepCamelOnly< TContractState, +HasComponent > of interface::IOwnableTwoStepCamelOnly> { + fn pendingOwner(self: @ComponentState) -> ContractAddress { + OwnableTwoStep::pending_owner(self) + } + fn acceptOwnership(ref self: ComponentState) { self.accept_ownership(); } diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index 7c2811af8..0f00cff11 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -132,14 +132,14 @@ fn test_transferOwnership() { assert_event_ownership_transfer_started(OWNER(), OTHER()); assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + assert(state.ownable.pendingOwner() == OTHER(), 'Pending owner should be OTHER'); // Transferring to yet another owner while pending is set should work state.ownable.transferOwnership(NEW_OWNER()); assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); + assert(state.ownable.pendingOwner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); } #[test] @@ -207,7 +207,7 @@ fn test_acceptOwnership() { assert_event_ownership_transferred(OWNER(), OTHER()); assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.ownable.pendingOwner() == ZERO(), 'Pending owner should be ZERO'); } #[test] From 9c0fa087afbfbaea00d279f8a1cc1f08da0177b9 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Fri, 1 Dec 2023 21:37:51 +0100 Subject: [PATCH 11/25] doc: no more OwnableTwoStepComponent --- docs/modules/ROOT/pages/access.adoc | 130 +------------ docs/modules/ROOT/pages/api/access.adoc | 235 ++++++------------------ 2 files changed, 65 insertions(+), 300 deletions(-) diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index fe0386677..c840a3ee4 100644 --- a/docs/modules/ROOT/pages/access.adoc +++ b/docs/modules/ROOT/pages/access.adoc @@ -106,133 +106,11 @@ 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! -== `TwoStepOwnable` +=== Two step transfer -The `TwoStepOwnable` offers a more robust way of transferring ownership when compared to `Ownable`. -The difference lies in the fact that the new owner has to first xref:/api/access.adoc#OwnableTwoStepComponent-accept_ownership[accept] -their ownership. Only after this call is made, the ownersihp is transferred. This mechanism helps to prevent -unintended and irreversible owner transfers. - -=== Usage - -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#OwnableTwoStepComponent-initializer[`initializer`] like this: - -[,javascript] ----- -#[starknet::contract] -mod MyContract { - 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) { - // Set the initial owner of the contract - self.ownable.initializer(owner); - } - - (...) -} ----- - -To restrict a function's access to the owner only, add in the `assert_only_owner` method: - -[,javascript] ----- -#[starknet::contract] -mod MyContract { - (...) - - #[external(v0)] - fn only_owner_allowed(ref self: ContractState) { - // This function can only be called by the owner - self.ownable.assert_only_owner(); - - (...) - } -} ----- - -=== Interface - -This is the full interface of the `TwoStepOwnable` implementation: - -[,javascript] ----- -trait IOwnableTwoStep { - /// Returns the current owner - fn owner() -> ContractAddress; - - /// Returns the pending owner. - fn pending_owner() -> ContractAddress; - - /// Finishes the two-step ownership transfer process - /// by accepting the ownership. - fn accept_ownership(ref self: TState); - - /// Starts the two-step ownership transfer process - /// by setting the pending owner. - fn transfer_ownership(ref self: TState, new_owner: ContractAddress); - - /// Renounces the ownership of the contract. - fn renounce_ownership(ref self: TState); -} ----- - -TODO: what else, if anything, should be mentioned here? just copy-paste from Ownable? - maybe mention that `transfer_ownership` can be called multiple times to overwrite the - proposed owner? and that after accepting, the pending owner is set to zero? - -=== Upgrading from `Ownable` - -If your contract already is xref:ownership_and_ownable[Ownable] and you want to xref:/upgrades.adoc[upgrade] it to `TwoStepOwnable`, -the only thing necessary is to replace the declared implementations. Change - -[,javascript] ----- -#[abi(embed_v0)] -impl OwnableImpl = OwnableComponent::OwnableImpl; -#[abi(embed_v0)] -impl OwnableCamelOnlyImpl = - OwnableComponent::OwnableCamelOnlyImpl; ----- - -into - -[,javascript] ----- -#[abi(embed_v0)] -impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl; -#[abi(embed_v0)] -impl OwnableTwoStepCamelOnlyImpl = - OwnableComponent::OwnableTwoStepCamelOnlyImpl; ----- - -After the upgrade, the set `owner` will be preserved and you will gain the functionality of -a two-step ownable implementation. +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. == Role-Based `AccessControl` diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index bb39e5173..77e49b742 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -1,7 +1,6 @@ :github-icon: pass:[] :AccessControl: xref:AccessControlComponent[AccessControl] :Ownable: xref:OwnableComponent[Ownable] -:OwnableTwoStep: xref:OwnableTwoStepComponent[OwnableTwoStep] :src5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SRC5] :inner-src5: xref:api/introspection.adoc#ISRC5[SRC5 ID] :_set_role_admin: xref:#AccessControlComponent-_set_role_admin[_set_role_admin] @@ -12,8 +11,6 @@ This directory provides ways to restrict who can access the functions of a contr - {Ownable} is a simple mechanism with a single "owner" role that can be assigned to a single account. This mechanism can be useful in simple scenarios, but fine grained access needs are likely to outgrow it. -- {OwnableTwoStep} is like Ownable, but ownership transfer is a two-step process where the newly proposed -owner has to accept the transfer before it becomes active. - {AccessControl} provides a general role based access control mechanism. Multiple hierarchical roles can be created and assigned each to multiple accounts. @@ -42,6 +39,19 @@ This module includes the internal `assert_only_owner` to restrict a function to * xref:OwnableComponent-renounce_ownership[`++renounce_ownership(self)++`] -- +[.contract-index] +[[OwnableTwoStepImpl]] +.Embeddable Implementations (two-step transfer) +-- +.OwnableTwoStepImpl + +* xref:OwnableComponent-owner[`++owner(self)++`] +* xref:OwnableComponent-pending_owner[`++pending_owner(self)++`] +* xref:OwnableComponent-transfer_ownership[`++transfer_ownership(self, new_owner)++`] +* xref:OwnableComponent-renounce_ownership[`++renounce_ownership(self)++`] +* xref:OwnableComponent-accept_ownership[`++accept_ownership(self)++`] +-- + [.contract-index] .Embeddable Implementations (camelCase) -- @@ -51,6 +61,18 @@ This module includes the internal `assert_only_owner` to restrict a function to * xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] -- +[.contract-index] +.Embeddable Implementations (camelCase two-step transfer) +-- +.OwnableTwoStepCamelOnlyImpl + +* xref:OwnableComponent-owner[`++owner(self)++`] +* xref:OwnableComponent-pendingOwner[`++pendingOwner(self)++`] +* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, new_owner)++`] +* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] +* xref:OwnableComponent-acceptOwnership[`++acceptOwnership(self)++`] +-- + [.contract-index] .Internal Implementations -- @@ -64,6 +86,7 @@ This module includes the internal `assert_only_owner` to restrict a function to [.contract-index] .Events -- +* xref:OwnableComponent-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] * xref:OwnableComponent-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] -- @@ -76,6 +99,12 @@ This module includes the internal `assert_only_owner` to restrict a function to Returns the address of the current owner. +[.contract-item] +[[OwnableComponent-pending_owner]] +==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# + +Returns the address of the pending owner. + [.contract-item] [[OwnableComponent-transfer_ownership]] ==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# @@ -95,9 +124,25 @@ Leaves the contract without owner. It will not be possible to call NOTE: Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner. +[.contract-item] +[[OwnableComponent-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. + [#OwnableComponent-camelCase-Support] ==== camelCase Support +[.contract-item] +[[OwnableComponent-acceptOwnership]] +==== `[.contract-item-name]#++pendingOwner++#++(self: @ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-pending_owner[pending_owner]. + [.contract-item] [[OwnableComponent-transferOwnership]] ==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# @@ -110,6 +155,12 @@ See xref:OwnableComponent-transfer_ownership[transfer_ownership]. See xref:OwnableComponent-renounce_ownership[renounce_ownership]. +[.contract-item] +[[OwnableComponent-acceptOwnership]] +==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-accept_ownership[accept_ownership]. + [#OwnableComponent-Internal-Functions] ==== Internal Functions @@ -136,199 +187,35 @@ Internal function without access restriction. Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. -[#OwnableComponent-Events] -==== Events - -[.contract-item] -[[OwnableComponent-OwnershipTransferred]] -==== `[.contract-item-name]#++OwnershipTransferred++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# - -Emitted when the ownership is transferred. - -[.contract] -[[OwnableTwoStepComponent]] -=== `++OwnableTwoStepComponent++` link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.8.0-beta.0/src/access/ownable/ownable.cairo[{github-icon},role=heading-link] - -```javascript -use openzeppelin::access::ownable::OwnableComponent; -``` - -`OwnableTwoStep` provides a basic access control mechanism where an account - (an owner) can be granted exclusive access to specific functions. Ownership can be - transferred to a new address in a two-step process, as opposed to {Ownable}, where - the transfer is immediate. - -This module includes the internal `assert_only_owner` to restrict a function to be used only by the owner. - -[.contract-index] -.Embeddable Implementations --- -.OwnableTwoStepImpl - -* xref:OwnableTwoStepComponent-owner[`++owner(self)++`] -* xref:OwnableTwoStepComponent-pending_owner[`++pending_owner(self)++`] -* xref:OwnableTwoStepComponent-accept_ownership[`++accept_ownership(self)++`] -* xref:OwnableTwoStepComponent-transfer_ownership[`++transfer_ownership(self, new_owner)++`] -* xref:OwnableTwoStepComponent-renounce_ownership[`++renounce_ownership(self)++`] --- - -[.contract-index] -.Embeddable Implementations (camelCase) --- -.OwnableTwoStepCamelOnlyImpl - -* xref:OwnableTwoStepComponent-pendingOwner[`++pendingOwner(self)++`] -* xref:OwnableTwoStepComponent-acceptOwnership[`++acceptOwnership(self)++`] -* xref:OwnableTwoStepComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] -* xref:OwnableTwoStepComponent-renounceOwnership[`++renounceOwnership(self)++`] --- - -[.contract-index] -.Internal Implementations --- -.InternalImpl - -* xref:OwnableTwoStepComponent-initializer[`++initializer(self, owner)++`] -* xref:OwnableTwoStepComponent-assert_only_owner[`++assert_only_owner(self)++`] -* xref:OwnableTwoStepComponent-_accept_ownership[`++_accept_ownership(self)++`] -* xref:OwnableTwoStepComponent-_propose_owner[`++_propose_owner(self, new_owner)++`] -* xref:OwnableTwoStepComponent-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`] --- - -[.contract-index] -.Events --- -* xref:OwnableTwoStepComponent-OwnershipTransferStarted[`++OwnershipTransferStarted(previous_owner, new_owner)++`] -* xref:OwnableTwoStepComponent-OwnershipTransferred[`++OwnershipTransferred(previous_owner, new_owner)++`] --- - -[#OwnableTwoStepComponent-Embeddable-Functions] -==== Embeddable Functions - -[.contract-item] -[[OwnableTwoStepComponent-owner]] -==== `[.contract-item-name]#++owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# - -Returns the address of the current owner. - -[.contract-item] -[[OwnableTwoStepComponent-pending_owner]] -==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# - -Returns the address of the pending owner. - -[.contract-item] -[[OwnableTwoStepComponent-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:OwnableTwoStepComponent-OwnershipTransferred[OwnershipTransferred] event. - -[.contract-item] -[[OwnableTwoStepComponent-transfer_ownership]] -==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# - -Sets pending owner to a new account (`new_owner`). -Can only be called by the current owner. - -Emits an xref:OwnableTwoStepComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. - -[.contract-item] -[[OwnableTwoStepComponent-renounce_ownership]] -==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# - -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. - -[#OwnableTwoStepComponent-camelCase-Support] -==== camelCase Support - -[.contract-item] -[[OwnableTwoStepComponent-acceptOwnership]] -==== `[.contract-item-name]#++pendingOwner++#++(self: @ContractState)++` [.item-kind]#external# - -See xref:OwnableTwoStepComponent-pending_owner[pending_owner]. - -[.contract-item] -[[OwnableTwoStepComponent-acceptOwnership]] -==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# - -See xref:OwnableTwoStepComponent-accept_ownership[accept_ownership]. - -[.contract-item] -[[OwnableTwoStepComponent-transferOwnership]] -==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# - -See xref:OwnableTwoStepComponent-transfer_ownership[transfer_ownership]. - -[.contract-item] -[[OwnableTwoStepComponent-renounceOwnership]] -==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# - -See xref:OwnableTwoStepComponent-renounce_ownership[renounce_ownership]. - -[#Ownable-Internal-Functions] -==== Internal Functions - -[.contract-item] -[[OwnableTwoStepComponent-initializer]] -==== `[.contract-item-name]#++initializer++#++(ref self: ContractState, owner: ContractAddress)++` [.item-kind]#internal# - -Initializes the contract and sets `owner` as the initial owner. - -Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. - -[.contract-item] -[[OwnableTwoStepComponent-assert_only_owner]] -==== `[.contract-item-name]#++assert_only_owner++#++(self: @ContractState)++` [.item-kind]#internal# - -Panics if called by any account other than the owner. - [.contract-item] -[[OwnableTwoStepComponent-_accept_ownership]] +[[OwnableComponent-_accept_ownership]] ==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal# Transfers ownership of the contract to the pending owner. Sets the pending owner to zero address. Internal function without access restriction. - -Emits an xref:OwnableTwoStep-OwnershipTransferred[OwnershipTransferred] event. +Calls xref:OwnableComponent-_transfer_ownership[_transfer_ownership]. [.contract-item] -[[OwnableTwoStepComponent-_propose_owner]] +[[OwnableComponent-_propose_owner]] ==== `[.contract-item-name]#++_propose_owner++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# Sets pending owner to a new account (`new_owner`). Internal function without access restriction. -Emits an xref:OwnableTwoStepComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. - -[.contract-item] -[[OwnableTwoStepComponent-_transfer_ownership]] -==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# - -Transfers ownership of the contract to a new account (`new_owner`). -Internal function without access restriction. - -Emits an xref:OwnableTwoStepComponent-OwnershipTransferred[OwnershipTransferred] event. +Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. -[#OwnableTwoStep-Events] +[#OwnableComponent-Events] ==== Events [.contract-item] -[[OwnableTwoStepComponent-OwnershipTransferStarted]] +[[OwnableComponent-OwnershipTransferStarted]] ==== `[.contract-item-name]#++OwnershipTransferStarted++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# Emitted when the pending owner is updated. [.contract-item] -[[OwnableTwoStepComponent-OwnershipTransferred]] +[[OwnableComponent-OwnershipTransferred]] ==== `[.contract-item-name]#++OwnershipTransferred++#++(previous_owner: ContractAddress, new_owner: ContractAddress)++` [.item-kind]#event# Emitted when the ownership is transferred. From 611001271338ed9ebe325f28e96cb3468ff75809 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 2 Dec 2023 21:08:05 +0100 Subject: [PATCH 12/25] docs: adding OwnableTwoStepImpl snippet --- docs/modules/ROOT/pages/access.adoc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index c840a3ee4..c92c8631b 100644 --- a/docs/modules/ROOT/pages/access.adoc +++ b/docs/modules/ROOT/pages/access.adoc @@ -110,7 +110,17 @@ will no longer be callable! 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. +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; +---- == Role-Based `AccessControl` From 14c1766654e1b026ae6bb6aa7b8e0ae6081f69bd Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 2 Dec 2023 21:51:18 +0100 Subject: [PATCH 13/25] test: using component_state_for_testing --- src/tests/access/test_ownable_twostep.cairo | 144 ++++++++++---------- 1 file changed, 74 insertions(+), 70 deletions(-) diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index 0f00cff11..de6e0d9a7 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -1,5 +1,6 @@ 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}; @@ -14,13 +15,16 @@ use super::test_ownable::assert_event_ownership_transferred; // Setup // -fn STATE() -> DualCaseTwoStepOwnableMock::ContractState { - DualCaseTwoStepOwnableMock::contract_state_for_testing() +type ComponentState = OwnableComponent::ComponentState; + + +fn COMPONENT_STATE() -> ComponentState { + OwnableComponent::component_state_for_testing() } -fn setup() -> DualCaseTwoStepOwnableMock::ContractState { - let mut state = STATE(); - state.ownable.initializer(OWNER()); +fn setup() -> ComponentState { + let mut state = COMPONENT_STATE(); + state.initializer(OWNER()); utils::drop_event(ZERO()); state } @@ -32,15 +36,15 @@ fn setup() -> DualCaseTwoStepOwnableMock::ContractState { #[test] #[available_gas(2000000)] fn test_initializer_owner_pending_owner() { - let mut state = STATE(); - assert(state.ownable.Ownable_owner.read() == ZERO(), 'Owner should be ZERO'); - assert(state.ownable.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); - state.ownable.initializer(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.Ownable_owner.read() == OWNER(), 'Owner should be set'); - assert(state.ownable.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); + assert(state.Ownable_owner.read() == OWNER(), 'Owner should be set'); + assert(state.Ownable_pending_owner.read() == ZERO(), 'Pending owner should be ZERO'); } // @@ -51,13 +55,13 @@ fn test_initializer_owner_pending_owner() { #[available_gas(2000000)] fn test__accept_ownership() { let mut state = setup(); - state.ownable.Ownable_pending_owner.write(OTHER()); + state.Ownable_pending_owner.write(OTHER()); - state.ownable._accept_ownership(); + state._accept_ownership(); assert_event_ownership_transferred(OWNER(), OTHER()); - assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); } // @@ -69,11 +73,11 @@ fn test__accept_ownership() { fn test__propose_owner() { let mut state = setup(); - state.ownable._propose_owner(OTHER()); + state._propose_owner(OTHER()); assert_event_ownership_transfer_started(OWNER(), OTHER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); } // transfer_ownership & transferOwnership @@ -83,18 +87,18 @@ fn test__propose_owner() { fn test_transfer_ownership() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transfer_ownership(OTHER()); + state.transfer_ownership(OTHER()); assert_event_ownership_transfer_started(OWNER(), OTHER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be 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.ownable.transfer_ownership(NEW_OWNER()); + state.transfer_ownership(NEW_OWNER()); assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); } #[test] @@ -103,7 +107,7 @@ fn test_transfer_ownership() { fn test_transfer_ownership_to_zero() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transfer_ownership(ZERO()); + state.transfer_ownership(ZERO()); } #[test] @@ -111,7 +115,7 @@ fn test_transfer_ownership_to_zero() { #[should_panic(expected: ('Caller is the zero address',))] fn test_transfer_ownership_from_zero() { let mut state = setup(); - state.ownable.transfer_ownership(OTHER()); + state.transfer_ownership(OTHER()); } #[test] @@ -120,7 +124,7 @@ fn test_transfer_ownership_from_zero() { fn test_transfer_ownership_from_nonowner() { let mut state = setup(); testing::set_caller_address(OTHER()); - state.ownable.transfer_ownership(OTHER()); + state.transfer_ownership(OTHER()); } #[test] @@ -128,18 +132,18 @@ fn test_transfer_ownership_from_nonowner() { fn test_transferOwnership() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transferOwnership(OTHER()); + state.transferOwnership(OTHER()); assert_event_ownership_transfer_started(OWNER(), OTHER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pendingOwner() == OTHER(), 'Pending owner should be 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.ownable.transferOwnership(NEW_OWNER()); + state.transferOwnership(NEW_OWNER()); assert_event_ownership_transfer_started(OWNER(), NEW_OWNER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pendingOwner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pendingOwner() == NEW_OWNER(), 'Pending should be NEW_OWNER'); } #[test] @@ -148,7 +152,7 @@ fn test_transferOwnership() { fn test_transferOwnership_to_zero() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transferOwnership(ZERO()); + state.transferOwnership(ZERO()); } #[test] @@ -156,7 +160,7 @@ fn test_transferOwnership_to_zero() { #[should_panic(expected: ('Caller is the zero address',))] fn test_transferOwnership_from_zero() { let mut state = setup(); - state.ownable.transferOwnership(OTHER()); + state.transferOwnership(OTHER()); } #[test] @@ -165,7 +169,7 @@ fn test_transferOwnership_from_zero() { fn test_transferOwnership_from_nonowner() { let mut state = setup(); testing::set_caller_address(OTHER()); - state.ownable.transferOwnership(OTHER()); + state.transferOwnership(OTHER()); } // @@ -176,14 +180,14 @@ fn test_transferOwnership_from_nonowner() { #[available_gas(2000000)] fn test_accept_ownership() { let mut state = setup(); - state.ownable.Ownable_pending_owner.write(OTHER()); + state.Ownable_pending_owner.write(OTHER()); testing::set_caller_address(OTHER()); - state.ownable.accept_ownership(); + state.accept_ownership(); assert_event_ownership_transferred(OWNER(), OTHER()); - assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); } #[test] @@ -191,23 +195,23 @@ fn test_accept_ownership() { #[should_panic(expected: ('Caller is not the pending owner',))] fn test_accept_ownership_from_nonpending() { let mut state = setup(); - state.ownable.Ownable_pending_owner.write(NEW_OWNER()); + state.Ownable_pending_owner.write(NEW_OWNER()); testing::set_caller_address(OTHER()); - state.ownable.accept_ownership(); + state.accept_ownership(); } #[test] #[available_gas(2000000)] fn test_acceptOwnership() { let mut state = setup(); - state.ownable.Ownable_pending_owner.write(OTHER()); + state.Ownable_pending_owner.write(OTHER()); testing::set_caller_address(OTHER()); - state.ownable.acceptOwnership(); + state.acceptOwnership(); assert_event_ownership_transferred(OWNER(), OTHER()); - assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pendingOwner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pendingOwner() == ZERO(), 'Pending owner should be ZERO'); } #[test] @@ -215,9 +219,9 @@ fn test_acceptOwnership() { #[should_panic(expected: ('Caller is not the pending owner',))] fn test_acceptOwnership_from_nonpending() { let mut state = setup(); - state.ownable.Ownable_pending_owner.write(NEW_OWNER()); + state.Ownable_pending_owner.write(NEW_OWNER()); testing::set_caller_address(OTHER()); - state.ownable.acceptOwnership(); + state.acceptOwnership(); } // @@ -229,11 +233,11 @@ fn test_acceptOwnership_from_nonpending() { fn test_renounce_ownership() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.renounce_ownership(); + state.renounce_ownership(); assert_event_ownership_transferred(OWNER(), ZERO()); - assert(state.ownable.owner() == ZERO(), 'Should renounce ownership'); + assert(state.owner() == ZERO(), 'Should renounce ownership'); } #[test] @@ -241,7 +245,7 @@ fn test_renounce_ownership() { #[should_panic(expected: ('Caller is the zero address',))] fn test_renounce_ownership_from_zero_address() { let mut state = setup(); - state.ownable.renounce_ownership(); + state.renounce_ownership(); } #[test] @@ -250,7 +254,7 @@ fn test_renounce_ownership_from_zero_address() { fn test_renounce_ownership_from_nonowner() { let mut state = setup(); testing::set_caller_address(OTHER()); - state.ownable.renounce_ownership(); + state.renounce_ownership(); } #[test] @@ -258,11 +262,11 @@ fn test_renounce_ownership_from_nonowner() { fn test_renounceOwnership() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.renounceOwnership(); + state.renounceOwnership(); assert_event_ownership_transferred(OWNER(), ZERO()); - assert(state.ownable.owner() == ZERO(), 'Should renounce ownership'); + assert(state.owner() == ZERO(), 'Should renounce ownership'); } #[test] @@ -270,7 +274,7 @@ fn test_renounceOwnership() { #[should_panic(expected: ('Caller is the zero address',))] fn test_renounceOwnership_from_zero_address() { let mut state = setup(); - state.ownable.renounceOwnership(); + state.renounceOwnership(); } #[test] @@ -279,7 +283,7 @@ fn test_renounceOwnership_from_zero_address() { fn test_renounceOwnership_from_nonowner() { let mut state = setup(); testing::set_caller_address(OTHER()); - state.ownable.renounceOwnership(); + state.renounceOwnership(); } #[test] @@ -287,18 +291,18 @@ fn test_renounceOwnership_from_nonowner() { fn test_full_two_step_transfer() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transfer_ownership(OTHER()); + state.transfer_ownership(OTHER()); assert_event_ownership_transfer_started(OWNER(), OTHER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be 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.ownable.accept_ownership(); + state.accept_ownership(); assert_event_ownership_transferred(OWNER(), OTHER()); - assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); } #[test] @@ -306,23 +310,23 @@ fn test_full_two_step_transfer() { fn test_pending_accept_after_owner_renounce() { let mut state = setup(); testing::set_caller_address(OWNER()); - state.ownable.transfer_ownership(OTHER()); + state.transfer_ownership(OTHER()); assert_event_ownership_transfer_started(OWNER(), OTHER()); - assert(state.ownable.owner() == OWNER(), 'Owner should be OWNER'); - assert(state.ownable.pending_owner() == OTHER(), 'Pending owner should be OTHER'); + assert(state.owner() == OWNER(), 'Owner should be OWNER'); + assert(state.pending_owner() == OTHER(), 'Pending owner should be OTHER'); - state.ownable.renounce_ownership(); + state.renounce_ownership(); assert_event_ownership_transferred(OWNER(), ZERO()); - assert(state.ownable.owner() == ZERO(), 'Should renounce ownership'); + assert(state.owner() == ZERO(), 'Should renounce ownership'); testing::set_caller_address(OTHER()); - state.ownable.accept_ownership(); + state.accept_ownership(); assert_event_ownership_transferred(ZERO(), OTHER()); - assert(state.ownable.owner() == OTHER(), 'Owner should be OTHER'); - assert(state.ownable.pending_owner() == ZERO(), 'Pending owner should be ZERO'); + assert(state.owner() == OTHER(), 'Owner should be OTHER'); + assert(state.pending_owner() == ZERO(), 'Pending owner should be ZERO'); } // From d17b08afb26ba34946ea5d3208ffb0dd82783f2d Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 2 Dec 2023 21:51:44 +0100 Subject: [PATCH 14/25] chore: updating CHANGELOG --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15a61afa6..45b99bcf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,3 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + +### Added + +- Two-step transfer functionality of the `Ownable` component + +### Changed + +- `assert_event_ownership_transferred` checks for indexed keys now From 8e37b194b132ef35a1c7ecd8b3c34ee2b738db9a Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 2 Dec 2023 22:19:11 +0100 Subject: [PATCH 15/25] chore: update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martín Triay --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45b99bcf6..150a8d894 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Two-step transfer functionality of the `Ownable` component +- Two-step transfer functionality of the `Ownable` component (#809) ### Changed -- `assert_event_ownership_transferred` checks for indexed keys now +- `assert_event_ownership_transferred` checks for indexed keys now (#809) From 2e085306a9a0ec5ba30d5d729cb7fc8193735366 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 2 Dec 2023 22:06:57 +0100 Subject: [PATCH 16/25] dev: allow two step ownable transfer to zero --- src/access/ownable/ownable.cairo | 1 - src/tests/access/test_ownable_twostep.cairo | 10 ++++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index f469f8833..bee474ccf 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -123,7 +123,6 @@ mod OwnableComponent { fn transfer_ownership( ref self: ComponentState, new_owner: ContractAddress ) { - assert(!new_owner.is_zero(), Errors::ZERO_ADDRESS_OWNER); self.assert_only_owner(); self._propose_owner(new_owner); } diff --git a/src/tests/access/test_ownable_twostep.cairo b/src/tests/access/test_ownable_twostep.cairo index de6e0d9a7..9708fc033 100644 --- a/src/tests/access/test_ownable_twostep.cairo +++ b/src/tests/access/test_ownable_twostep.cairo @@ -103,11 +103,14 @@ fn test_transfer_ownership() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('New owner is the zero address',))] 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] @@ -148,11 +151,14 @@ fn test_transferOwnership() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('New owner is the zero address',))] 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] From fd5732368f75b6df30c0a1c2fc97d3afa49b79d7 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Mon, 11 Dec 2023 15:40:49 +0100 Subject: [PATCH 17/25] docs: two step interface --- docs/modules/ROOT/pages/access.adoc | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/modules/ROOT/pages/access.adoc b/docs/modules/ROOT/pages/access.adoc index c92c8631b..b429827f4 100644 --- a/docs/modules/ROOT/pages/access.adoc +++ b/docs/modules/ROOT/pages/access.adoc @@ -122,6 +122,33 @@ 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 From 76e0dfbfe667665b220229a5a30348e6e6fff3d6 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Mon, 11 Dec 2023 19:35:15 +0100 Subject: [PATCH 18/25] docs: access API with separate two step sections --- docs/modules/ROOT/pages/api/access.adoc | 156 ++++++++++++++++-------- 1 file changed, 103 insertions(+), 53 deletions(-) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 77e49b742..0a95d1eb0 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -40,37 +40,36 @@ This module includes the internal `assert_only_owner` to restrict a function to -- [.contract-index] -[[OwnableTwoStepImpl]] -.Embeddable Implementations (two-step transfer) +.Embeddable Implementations (camelCase) -- -.OwnableTwoStepImpl +.OwnableCamelOnlyImpl -* xref:OwnableComponent-owner[`++owner(self)++`] -* xref:OwnableComponent-pending_owner[`++pending_owner(self)++`] -* xref:OwnableComponent-transfer_ownership[`++transfer_ownership(self, new_owner)++`] -* xref:OwnableComponent-renounce_ownership[`++renounce_ownership(self)++`] -* xref:OwnableComponent-accept_ownership[`++accept_ownership(self)++`] +* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] +* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] -- [.contract-index] -.Embeddable Implementations (camelCase) +[[OwnableTwoStepImpl]] +.Embeddable Implementations (two step transfer) -- -.OwnableCamelOnlyImpl +.OwnableTwoStepImpl -* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] -* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] +* 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] -.Embeddable Implementations (camelCase two-step transfer) +.Embeddable Implementations (camelCase two step transfer) -- .OwnableTwoStepCamelOnlyImpl -* xref:OwnableComponent-owner[`++owner(self)++`] -* xref:OwnableComponent-pendingOwner[`++pendingOwner(self)++`] -* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, new_owner)++`] -* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] -* xref:OwnableComponent-acceptOwnership[`++acceptOwnership(self)++`] +* 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] @@ -80,6 +79,8 @@ 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)++`] -- @@ -96,14 +97,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. - -[.contract-item] -[[OwnableComponent-pending_owner]] -==== `[.contract-item-name]#++pending_owner++#++(self: @ContractState) → ContractAddress++` [.item-kind]#external# - -Returns the address of the pending owner. +// end::owner[] [.contract-item] [[OwnableComponent-transfer_ownership]] @@ -117,15 +113,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-camelCase-Support] +==== camelCase Support + +[.contract-item] +[[OwnableComponent-transferOwnership]] +==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# + +See xref:OwnableComponent-transfer_ownership[transfer_ownership]. + +[.contract-item] +[[OwnableComponent-renounceOwnership]] +==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-renounce_ownership[renounce_ownership]. + +[.contract-item] +[[OwnableComponent-acceptOwnership]] +==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-accept_ownership[accept_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-accept_ownership]] +[[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. @@ -134,32 +166,46 @@ Resets pending owner to zero address. Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. -[#OwnableComponent-camelCase-Support] -==== camelCase Support +[.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-acceptOwnership]] +[[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-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-pending_owner[pending_owner]. +See xref:OwnableComponent-two-step-pending_owner[pending_owner]. [.contract-item] -[[OwnableComponent-transferOwnership]] -==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# +[[OwnableComponent-two-step-acceptOwnership]] +==== `[.contract-item-name]#++acceptOwnership++#++(self: @ContractState)++` [.item-kind]#external# -See xref:OwnableComponent-transfer_ownership[transfer_ownership]. +See xref:OwnableComponent-two-step-accept_ownership[accept_ownership]. [.contract-item] -[[OwnableComponent-renounceOwnership]] -==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# +[[OwnableComponent-two-step-transferOwnership]] +==== `[.contract-item-name]#++transferOwnership++#++(self: @ContractState)++` [.item-kind]#external# -See xref:OwnableComponent-renounce_ownership[renounce_ownership]. +See xref:OwnableComponent-two-step-transfer_ownership[transfer_ownership]. [.contract-item] -[[OwnableComponent-acceptOwnership]] -==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# +[[OwnableComponent-two-step-renounceOwnership]] +==== `[.contract-item-name]#++renounceOwnership++#++(self: @ContractState)++` [.item-kind]#external# -See xref:OwnableComponent-accept_ownership[accept_ownership]. +See xref:OwnableComponent-two-step-renounce_ownership[renounce_ownership]. [#OwnableComponent-Internal-Functions] ==== Internal Functions @@ -178,33 +224,37 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. Panics if called by any account other than the owner. -[.contract-item] -[[OwnableComponent-_transfer_ownership]] -==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal# - -Transfers ownership of the contract to a new account (`new_owner`). -Internal function without access restriction. - -Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. - [.contract-item] [[OwnableComponent-_accept_ownership]] ==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal# -Transfers ownership of the contract to the pending owner. -Sets the pending owner to zero address. -Internal function without access restriction. +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 pending owner to a new account (`new_owner`). +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# + +Transfers ownership of the contract to a new account (`new_owner`). +Internal function without access restriction. + +Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. + [#OwnableComponent-Events] ==== Events From 5077d0519389e41f4abfa358e21466767767ce6a Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Tue, 19 Dec 2023 13:27:32 +0100 Subject: [PATCH 19/25] lint: rm extra blank line --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3ced0d58..83cb298f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,4 +19,3 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Two-step transfer functionality of the `Ownable` component (#809) - Use ComponentState in tests (#836) - Docsite navbar (#838) - From 535b16456303e8a6b665f6e1b775124ea8f83f4e Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Sat, 30 Dec 2023 22:00:02 +0100 Subject: [PATCH 20/25] chore: PR comments --- CHANGELOG.md | 2 +- docs/modules/ROOT/pages/api/access.adoc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83cb298f2..21e3b25b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Usage docs (#823) - Utilities documentation (#825) - Documentation for presets (#832) +- Ownable two-step functionality (#809) ### Changed -- Two-step transfer functionality of the `Ownable` component (#809) - Use ComponentState in tests (#836) - Docsite navbar (#838) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 0a95d1eb0..3c4a23c2a 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -181,7 +181,7 @@ Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted include::./access.adoc[tag=renounce_ownership] [#OwnableComponent-camelCase-Support-Two-Step] -=== camelCase Support (two step transfer) +==== camelCase Support (two step transfer) [.contract-item] [[OwnableComponent-two-step-pendingOwner]] From 3fd74661eab302a620a36c92df057acb38e576fb Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 31 Jan 2024 10:44:31 +0100 Subject: [PATCH 21/25] fix: impl order --- src/access/ownable/ownable.cairo | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/access/ownable/ownable.cairo b/src/access/ownable/ownable.cairo index bd459f980..dcda0ff13 100644 --- a/src/access/ownable/ownable.cairo +++ b/src/access/ownable/ownable.cairo @@ -94,20 +94,6 @@ mod OwnableComponent { } } - /// Adds camelCase support for `IOwnable`. - #[embeddable_as(OwnableCamelOnlyImpl)] - impl OwnableCamelOnly< - TContractState, +HasComponent - > of interface::IOwnableCamelOnly> { - fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { - Ownable::transfer_ownership(ref self, newOwner); - } - - fn renounceOwnership(ref self: ComponentState) { - Ownable::renounce_ownership(ref self); - } - } - /// Adds support for two step ownership transfer. #[embeddable_as(OwnableTwoStepImpl)] impl OwnableTwoStep< @@ -147,6 +133,20 @@ mod OwnableComponent { } } + /// Adds camelCase support for `IOwnable`. + #[embeddable_as(OwnableCamelOnlyImpl)] + impl OwnableCamelOnly< + TContractState, +HasComponent + > of interface::IOwnableCamelOnly> { + fn transferOwnership(ref self: ComponentState, newOwner: ContractAddress) { + Ownable::transfer_ownership(ref self, newOwner); + } + + fn renounceOwnership(ref self: ComponentState) { + Ownable::renounce_ownership(ref self); + } + } + /// Adds camelCase support for `IOwnableTwoStep`. #[embeddable_as(OwnableTwoStepCamelOnlyImpl)] impl OwnableTwoStepCamelOnly< From baec9e12ebb5da789b71945b7a448436ac3547b5 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 31 Jan 2024 10:54:58 +0100 Subject: [PATCH 22/25] doc: small fixes --- docs/modules/ROOT/pages/api/access.adoc | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 3c4a23c2a..364d66ebc 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -50,7 +50,7 @@ This module includes the internal `assert_only_owner` to restrict a function to [.contract-index] [[OwnableTwoStepImpl]] -.Embeddable Implementations (two step transfer) +.Embeddable Implementations (Two Step Transfer) -- .OwnableTwoStepImpl @@ -62,7 +62,7 @@ This module includes the internal `assert_only_owner` to restrict a function to -- [.contract-index] -.Embeddable Implementations (camelCase two step transfer) +.Embeddable Implementations (camelCase Two Step Transfer) -- .OwnableTwoStepCamelOnlyImpl @@ -136,14 +136,8 @@ See xref:OwnableComponent-transfer_ownership[transfer_ownership]. See xref:OwnableComponent-renounce_ownership[renounce_ownership]. -[.contract-item] -[[OwnableComponent-acceptOwnership]] -==== `[.contract-item-name]#++acceptOwnership++#++(ref self: ContractState)++` [.item-kind]#external# - -See xref:OwnableComponent-accept_ownership[accept_ownership]. - [#OwnableComponent-Embeddable-Functions-Two-Step] -==== Embeddable Functions (Two step transfer) +==== Embeddable Functions (Two Step Transfer) [.contract-item] [[OwnableComponent-two-step-owner]] @@ -181,7 +175,7 @@ Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted include::./access.adoc[tag=renounce_ownership] [#OwnableComponent-camelCase-Support-Two-Step] -==== camelCase Support (two step transfer) +==== camelCase Support (Two Step Transfer) [.contract-item] [[OwnableComponent-two-step-pendingOwner]] From fb2be79a73bde145d533852b6612e35f6eb55171 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 31 Jan 2024 11:00:18 +0100 Subject: [PATCH 23/25] doc: combining section overview --- docs/modules/ROOT/pages/api/access.adoc | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 364d66ebc..b83fbf946 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -37,21 +37,7 @@ 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)++`] --- - -[.contract-index] -.Embeddable Implementations (camelCase) --- -.OwnableCamelOnlyImpl - -* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] -* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] --- -[.contract-index] -[[OwnableTwoStepImpl]] -.Embeddable Implementations (Two Step Transfer) --- .OwnableTwoStepImpl * xref:OwnableComponent-two-step-owner[`++owner(self)++`] @@ -62,8 +48,13 @@ This module includes the internal `assert_only_owner` to restrict a function to -- [.contract-index] -.Embeddable Implementations (camelCase Two Step Transfer) +.Embeddable Implementations (camelCase) -- +.OwnableCamelOnlyImpl + +* xref:OwnableComponent-transferOwnership[`++transferOwnership(self, newOwner)++`] +* xref:OwnableComponent-renounceOwnership[`++renounceOwnership(self)++`] + .OwnableTwoStepCamelOnlyImpl * xref:OwnableComponent-two-step-pendingOwner[`++pendingOwner(self)++`] From dde5ab6fe7ae36cacba854e80906d77617f8f649 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 31 Jan 2024 11:09:56 +0100 Subject: [PATCH 24/25] lint: changelog.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07c697592..269a79f74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased ### Added + - Ownable two-step functionality (#809) ### Changed From 16ee09361f41a440ea0ba735368b2f2d5b8befe9 Mon Sep 17 00:00:00 2001 From: Milan Cermak Date: Wed, 31 Jan 2024 19:41:16 +0100 Subject: [PATCH 25/25] doc: section ordering --- docs/modules/ROOT/pages/api/access.adoc | 30 ++++++++++++------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 423a70cac..bbb140ff0 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -112,21 +112,6 @@ 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-camelCase-Support] -==== camelCase Support - -[.contract-item] -[[OwnableComponent-transferOwnership]] -==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# - -See xref:OwnableComponent-transfer_ownership[transfer_ownership]. - -[.contract-item] -[[OwnableComponent-renounceOwnership]] -==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# - -See xref:OwnableComponent-renounce_ownership[renounce_ownership]. - [#OwnableComponent-Embeddable-Functions-Two-Step] ==== Embeddable Functions (Two Step Transfer) @@ -165,6 +150,21 @@ Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted ==== `[.contract-item-name]#++renounce_ownership++#++(ref self: ContractState)++` [.item-kind]#external# include::./access.adoc[tag=renounce_ownership] +[#OwnableComponent-camelCase-Support] +==== camelCase Support + +[.contract-item] +[[OwnableComponent-transferOwnership]] +==== `[.contract-item-name]#++transferOwnership++#++(ref self: ContractState, newOwner: ContractAddress)++` [.item-kind]#external# + +See xref:OwnableComponent-transfer_ownership[transfer_ownership]. + +[.contract-item] +[[OwnableComponent-renounceOwnership]] +==== `[.contract-item-name]#++renounceOwnership++#++(ref self: ContractState)++` [.item-kind]#external# + +See xref:OwnableComponent-renounce_ownership[renounce_ownership]. + [#OwnableComponent-camelCase-Support-Two-Step] ==== camelCase Support (Two Step Transfer)