From d00e1c13d2d253f753a10f5d09c70af61e12efdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20Hrastnik?= Date: Tue, 9 Jan 2024 14:29:11 +0900 Subject: [PATCH] Refactor Ownable as a component --- .../access_control/access_controller.cairo | 70 +++---- .../src/emergency/sequencer_uptime_feed.cairo | 68 ++----- contracts/src/libraries/ownable.cairo | 191 +++++++++++------- contracts/src/ocr2/aggregator.cairo | 79 +++----- contracts/src/ocr2/aggregator_proxy.cairo | 73 +++---- .../src/tests/test_access_controller.cairo | 2 +- contracts/src/tests/test_aggregator.cairo | 8 +- .../src/tests/test_aggregator_proxy.cairo | 2 +- contracts/src/tests/test_link_token.cairo | 2 +- contracts/src/tests/test_ownable.cairo | 90 ++++++--- contracts/src/tests/test_upgradeable.cairo | 1 - contracts/src/token/link_token.cairo | 50 ++--- 12 files changed, 293 insertions(+), 343 deletions(-) diff --git a/contracts/src/access_control/access_controller.cairo b/contracts/src/access_control/access_controller.cairo index dd50197db..676128220 100644 --- a/contracts/src/access_control/access_controller.cairo +++ b/contracts/src/access_control/access_controller.cairo @@ -6,16 +6,31 @@ mod AccessController { use starknet::class_hash::ClassHash; use chainlink::libraries::access_control::{AccessControl, IAccessController}; - use chainlink::libraries::ownable::{Ownable, IOwnable}; + use chainlink::libraries::ownable::{OwnableComponent, IOwnable}; use chainlink::libraries::upgradeable::{Upgradeable, IUpgradeable}; + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event, + } + #[storage] - struct Storage {} + struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage, + } #[constructor] fn constructor(ref self: ContractState, owner_address: ContractAddress) { - let mut ownable = Ownable::unsafe_new_contract_state(); - Ownable::constructor(ref ownable, owner_address); + self.ownable.initializer(owner_address); let mut access_control = AccessControl::unsafe_new_contract_state(); AccessControl::constructor(ref access_control); } @@ -28,66 +43,30 @@ mod AccessController { } fn add_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::add_access(ref state, user); } fn remove_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::remove_access(ref state, user); } fn enable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::enable_access_check(ref state); } fn disable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::disable_access_check(ref state); } } - /// - /// Ownable - /// - - #[external(v0)] - impl OwnableImpl of IOwnable { - fn owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::owner(@state) - } - - fn proposed_owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::proposed_owner(@state) - } - - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::transfer_ownership(ref state, new_owner) - } - - fn accept_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::accept_ownership(ref state) - } - - fn renounce_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::renounce_ownership(ref state) - } - } - /// /// Upgradeable /// @@ -100,8 +79,7 @@ mod AccessController { #[external(v0)] impl UpgradeableImpl of IUpgradeable { fn upgrade(ref self: ContractState, new_impl: ClassHash) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); Upgradeable::upgrade(new_impl); } } diff --git a/contracts/src/emergency/sequencer_uptime_feed.cairo b/contracts/src/emergency/sequencer_uptime_feed.cairo index df2420ee9..c8f8e9936 100644 --- a/contracts/src/emergency/sequencer_uptime_feed.cairo +++ b/contracts/src/emergency/sequencer_uptime_feed.cairo @@ -27,15 +27,25 @@ mod SequencerUptimeFeed { use option::OptionTrait; use zeroable::Zeroable; - use chainlink::libraries::ownable::{Ownable, IOwnable}; + use chainlink::libraries::ownable::{OwnableComponent, IOwnable}; use chainlink::libraries::access_control::{AccessControl, IAccessController}; use chainlink::ocr2::aggregator::Round; use chainlink::ocr2::aggregator::IAggregator; use chainlink::ocr2::aggregator::{Transmission}; use chainlink::libraries::upgradeable::Upgradeable; + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + + #[storage] struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage, + // l1 sender is an starknet validator ethereum address _l1_sender: felt252, // maps round id to round transmission @@ -46,6 +56,8 @@ mod SequencerUptimeFeed { #[event] #[derive(Drop, starknet::Event)] enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event, RoundUpdated: RoundUpdated, NewRound: NewRound, AnswerUpdated: AnswerUpdated, @@ -167,8 +179,7 @@ mod SequencerUptimeFeed { #[external(v0)] impl SequencerUptimeFeedImpl of super::ISequencerUptimeFeed { fn set_l1_sender(ref self: ContractState, address: EthAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); assert(!address.is_zero(), '0 address not allowed'); @@ -198,44 +209,10 @@ mod SequencerUptimeFeed { #[external(v0)] fn upgrade(ref self: ContractState, new_impl: ClassHash) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); Upgradeable::upgrade(new_impl) } - /// - /// Ownership - /// - - #[external(v0)] - impl OwnableImpl of IOwnable { - fn owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::owner(@state) - } - - fn proposed_owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::proposed_owner(@state) - } - - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::transfer_ownership(ref state, new_owner) - } - - fn accept_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::accept_ownership(ref state) - } - - fn renounce_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::renounce_ownership(ref state) - } - } - - /// /// Access Control /// @@ -248,29 +225,25 @@ mod SequencerUptimeFeed { } fn add_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::add_access(ref state, user) } fn remove_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::remove_access(ref state, user) } fn enable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::enable_access_check(ref state) } fn disable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::disable_access_check(ref state) } @@ -292,8 +265,7 @@ mod SequencerUptimeFeed { fn _initializer( ref self: ContractState, initial_status: u128, owner_address: ContractAddress ) { - let mut ownable = Ownable::unsafe_new_contract_state(); - Ownable::constructor(ref ownable, owner_address); + self.ownable.initializer(owner_address); let mut access_control = AccessControl::unsafe_new_contract_state(); AccessControl::constructor(ref access_control); let round_id = 1_u128; diff --git a/contracts/src/libraries/ownable.cairo b/contracts/src/libraries/ownable.cairo index 6646d5cf9..d880eb672 100644 --- a/contracts/src/libraries/ownable.cairo +++ b/contracts/src/libraries/ownable.cairo @@ -1,110 +1,155 @@ use starknet::ContractAddress; +// TODO: consider replacing with OwnableTwoStep if https://github.com/OpenZeppelin/cairo-contracts/pull/809/ lands + #[starknet::interface] -trait IOwnable { - fn owner(self: @TContractState) -> ContractAddress; - fn proposed_owner(self: @TContractState) -> ContractAddress; - fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress); - fn accept_ownership(ref self: TContractState); - fn renounce_ownership(ref self: TContractState); +trait IOwnable { + fn owner(self: @TState) -> ContractAddress; + fn proposed_owner(self: @TState) -> ContractAddress; + fn transfer_ownership(ref self: TState, new_owner: ContractAddress); + fn accept_ownership(ref self: TState); + fn renounce_ownership(ref self: TState); } -#[starknet::contract] -mod Ownable { +#[starknet::component] +mod OwnableComponent { use starknet::ContractAddress; - use starknet::contract_address_const; - use zeroable::Zeroable; + use starknet::get_caller_address; #[storage] struct Storage { - _owner: ContractAddress, - _proposed_owner: ContractAddress, + Ownable_owner: ContractAddress, + Ownable_proposed_owner: ContractAddress } - // - // Events - // - #[event] - fn OwnershipTransferred(previous_owner: ContractAddress, newOwner: ContractAddress) {} - - #[event] - fn OwnershipTransferRequested(from: starknet::ContractAddress, to: starknet::ContractAddress) {} + #[derive(Drop, starknet::Event)] + enum Event { + OwnershipTransferred: OwnershipTransferred, + OwnershipTransferRequested: OwnershipTransferRequested + } - // - // Constructor - // + #[derive(Drop, starknet::Event)] + struct OwnershipTransferred { + #[key] + previous_owner: ContractAddress, + #[key] + new_owner: ContractAddress, + } - #[constructor] - fn constructor(ref self: ContractState, owner: ContractAddress) { - assert(!owner.is_zero(), 'Ownable: transfer to 0'); - self._accept_ownership_transfer(owner); + #[derive(Drop, starknet::Event)] + struct OwnershipTransferRequested { + #[key] + previous_owner: ContractAddress, + #[key] + new_owner: ContractAddress, } - // - // Modifiers - // - fn assert_only_owner(self: @ContractState) { - let owner = self._owner.read(); - let caller = starknet::get_caller_address(); - assert(caller == owner, 'Ownable: caller is not owner'); + mod Errors { + const NOT_OWNER: felt252 = 'Caller is not the owner'; + const NOT_PROPOSED_OWNER: felt252 = 'Caller is not proposed owner'; + const ZERO_ADDRESS_CALLER: felt252 = 'Caller is the zero address'; + const ZERO_ADDRESS_OWNER: felt252 = 'New owner is the zero address'; } - #[external(v0)] - impl OwnableImpl of super::IOwnable { - // - // Getters - // - fn owner(self: @ContractState) -> ContractAddress { - self._owner.read() + /// Adds support for two step ownership transfer. + #[embeddable_as(OwnableImpl)] + impl Ownable< + TContractState, +HasComponent + > of super::IOwnable> { + /// Returns the address of the current owner. + fn owner(self: @ComponentState) -> ContractAddress { + self.Ownable_owner.read() } - fn proposed_owner(self: @ContractState) -> ContractAddress { - self._proposed_owner.read() + /// Returns the address of the pending owner. + fn proposed_owner(self: @ComponentState) -> ContractAddress { + self.Ownable_proposed_owner.read() } - // - // Setters - // + /// Finishes the two-step ownership transfer process by accepting the ownership. + /// Can only be called by the pending owner. + fn accept_ownership(ref self: ComponentState) { + let caller = get_caller_address(); + let proposed_owner = self.Ownable_proposed_owner.read(); + assert(caller == proposed_owner, Errors::NOT_PROPOSED_OWNER); + self._accept_ownership(); + } - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { + /// Starts 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(), 'Ownable: transfer to 0'); - assert_only_owner(@self); - - self._proposed_owner.write(new_owner); - let previous_owner = self._owner.read(); - OwnershipTransferRequested(previous_owner, new_owner); + self.assert_only_owner(); + self._propose_owner(new_owner); } - fn accept_ownership(ref self: ContractState) { - let proposed_owner = self._proposed_owner.read(); - let caller = starknet::get_caller_address(); - - assert(caller == proposed_owner, 'Ownable: not proposed_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) { + self.assert_only_owner(); + self._transfer_ownership(Zeroable::zero()); + } + } - self._accept_ownership_transfer(proposed_owner); + #[generate_trait] + impl InternalImpl< + TContractState, +HasComponent + > of InternalTrait { + /// Sets the contract's initial owner. + /// + /// This function should be called at construction time. + fn initializer(ref self: ComponentState, owner: ContractAddress) { + self._transfer_ownership(owner); } - fn renounce_ownership(ref self: ContractState) { - assert_only_owner(@self); - self._accept_ownership_transfer(starknet::contract_address_const::<0>()); + /// 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 = self.Ownable_owner.read(); + let caller = get_caller_address(); + assert(!caller.is_zero(), Errors::ZERO_ADDRESS_CALLER); + assert(caller == owner, Errors::NOT_OWNER); } - } + /// Transfers ownership to the pending owner. + /// + /// Internal function without access restriction. + fn _accept_ownership(ref self: ComponentState) { + let proposed_owner = self.Ownable_proposed_owner.read(); + self.Ownable_proposed_owner.write(Zeroable::zero()); + self._transfer_ownership(proposed_owner); + } - // - // Internal - // + /// Sets a new pending owner. + /// + /// Internal function without access restriction. + fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { + let previous_owner = self.Ownable_owner.read(); + self.Ownable_proposed_owner.write(new_owner); + self + .emit( + OwnershipTransferRequested { + previous_owner: previous_owner, new_owner: new_owner + } + ); + } - #[generate_trait] - impl InternalImpl of InternalTrait { - fn _accept_ownership_transfer( - ref self: ContractState, new_owner: starknet::ContractAddress + /// Transfers ownership of the contract to a new address. + /// + /// Internal function without access restriction. + /// + /// Emits an `OwnershipTransferred` event. + fn _transfer_ownership( + ref self: ComponentState, new_owner: ContractAddress ) { - let previous_owner = self._owner.read(); - self._owner.write(new_owner); - self._proposed_owner.write(starknet::contract_address_const::<0>()); - OwnershipTransferred(previous_owner, new_owner); + let previous_owner: ContractAddress = self.Ownable_owner.read(); + self.Ownable_owner.write(new_owner); + self + .emit( + OwnershipTransferred { previous_owner: previous_owner, new_owner: new_owner } + ); } } } diff --git a/contracts/src/ocr2/aggregator.cairo b/contracts/src/ocr2/aggregator.cairo index f23f07347..542dfbd38 100644 --- a/contracts/src/ocr2/aggregator.cairo +++ b/contracts/src/ocr2/aggregator.cairo @@ -195,7 +195,7 @@ mod Aggregator { use starknet::class_hash::ClassHash; use chainlink::utils::split_felt; - use chainlink::libraries::ownable::{Ownable, IOwnable}; + use chainlink::libraries::ownable::{OwnableComponent, IOwnable}; use chainlink::libraries::access_control::AccessControl; use chainlink::libraries::upgradeable::{Upgradeable, IUpgradeable}; @@ -205,6 +205,12 @@ mod Aggregator { IAccessController, IAccessControllerDispatcher, IAccessControllerDispatcherTrait }; + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + const GIGA: u128 = 1000000000_u128; const MAX_ORACLES: u32 = 31_u32; @@ -212,6 +218,8 @@ mod Aggregator { #[event] #[derive(Drop, starknet::Event)] enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event, NewTransmission: NewTransmission, ConfigSet: ConfigSet, LinkTokenSet: LinkTokenSet, @@ -266,6 +274,9 @@ mod Aggregator { #[storage] struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage, + /// Maximum number of faulty oracles _f: u8, _latest_epoch_and_round: u64, // (u32, u32) @@ -292,8 +303,7 @@ mod Aggregator { _billing: BillingConfig, // payee management _payees: LegacyMap, // - _proposed_payees: LegacyMap // + _proposed_payees: LegacyMap // } fn _require_read_access() { @@ -357,8 +367,7 @@ mod Aggregator { decimals: u8, description: felt252 ) { - let mut ownable = Ownable::unsafe_new_contract_state(); - Ownable::constructor(ref ownable, owner); // Ownable::initializer(owner); + self.ownable.initializer(owner); let mut access_control = AccessControl::unsafe_new_contract_state(); AccessControl::constructor(ref access_control); self._link_token.write(link); @@ -377,42 +386,11 @@ mod Aggregator { #[external(v0)] impl UpgradeableImpl of IUpgradeable { fn upgrade(ref self: ContractState, new_impl: ClassHash) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); Upgradeable::upgrade(new_impl) } } - // --- Ownership --- - - #[external(v0)] - impl OwnableImpl of IOwnable { - fn owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::owner(@state) - } - - fn proposed_owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::proposed_owner(@state) - } - - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::transfer_ownership(ref state, new_owner) - } - - fn accept_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::accept_ownership(ref state) - } - - fn renounce_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::renounce_ownership(ref state) - } - } - // -- Access Control -- #[external(v0)] @@ -423,29 +401,25 @@ mod Aggregator { } fn add_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::add_access(ref state, user) } fn remove_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::remove_access(ref state, user) } fn enable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::enable_access_check(ref state) } fn disable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::disable_access_check(ref state) } @@ -529,8 +503,7 @@ mod Aggregator { offchain_config_version: u64, offchain_config: Array, ) -> felt252 { // digest - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); assert(oracles.len() <= MAX_ORACLES, 'too many oracles'); assert((3_u8 * f).into() < oracles.len(), 'faulty-oracle f too high'); assert(f > 0_u8, 'f must be positive'); @@ -900,8 +873,7 @@ mod Aggregator { fn set_link_token( ref self: ContractState, link_token: ContractAddress, recipient: ContractAddress ) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let old_token = self._link_token.read(); @@ -968,8 +940,7 @@ mod Aggregator { fn set_billing_access_controller( ref self: ContractState, access_controller: ContractAddress ) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let old_controller = self._billing_access_controller.read(); if access_controller == old_controller { @@ -1068,8 +1039,7 @@ mod Aggregator { impl BillingHelperImpl of BillingHelperTrait { fn has_billing_access(self: @ContractState) { let caller = starknet::info::get_caller_address(); - let ownable = Ownable::unsafe_new_contract_state(); - let owner = Ownable::OwnableImpl::owner(@ownable); + let owner = self.ownable.owner(); // owner always has access if caller == owner { @@ -1228,8 +1198,7 @@ mod Aggregator { #[external(v0)] impl PayeeManagementImpl of super::PayeeManagement { fn set_payees(ref self: ContractState, mut payees: Array) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); loop { match payees.pop_front() { Option::Some(payee) => { diff --git a/contracts/src/ocr2/aggregator_proxy.cairo b/contracts/src/ocr2/aggregator_proxy.cairo index bbb06c881..e26332d90 100644 --- a/contracts/src/ocr2/aggregator_proxy.cairo +++ b/contracts/src/ocr2/aggregator_proxy.cairo @@ -49,7 +49,7 @@ mod AggregatorProxy { use chainlink::ocr2::aggregator::IAggregator; use chainlink::ocr2::aggregator::Round; - use chainlink::libraries::ownable::{Ownable, IOwnable}; + use chainlink::libraries::ownable::{OwnableComponent, IOwnable}; use chainlink::libraries::access_control::{AccessControl, IAccessController}; use chainlink::utils::split_felt; use chainlink::libraries::upgradeable::{Upgradeable, IUpgradeable}; @@ -63,13 +63,30 @@ mod AggregatorProxy { aggregator: ContractAddress } + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + #[storage] struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage, + _current_phase: Phase, _proposed_aggregator: ContractAddress, _phases: LegacyMap } + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event, + } + + // TODO: refactor these events #[event] fn AggregatorProposed(current: ContractAddress, proposed: ContractAddress) {} @@ -132,50 +149,18 @@ mod AggregatorProxy { #[constructor] fn constructor(ref self: ContractState, owner: ContractAddress, address: ContractAddress) { // TODO: ownable and access control need to share owners and update when needed - let mut ownable = Ownable::unsafe_new_contract_state(); - Ownable::constructor(ref ownable, owner); + self.ownable.initializer(owner); let mut access_control = AccessControl::unsafe_new_contract_state(); AccessControl::constructor(ref access_control); self._set_aggregator(address); } - // --- Ownership --- - - #[external(v0)] - impl OwnableImpl of IOwnable { - fn owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::owner(@state) - } - - fn proposed_owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::proposed_owner(@state) - } - - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::transfer_ownership(ref state, new_owner) - } - - fn accept_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::accept_ownership(ref state) - } - - fn renounce_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::renounce_ownership(ref state) - } - } - // -- Upgradeable -- #[external(v0)] impl UpgradeableImpl of IUpgradeable { fn upgrade(ref self: ContractState, new_impl: ClassHash) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); Upgradeable::upgrade(new_impl) } } @@ -191,29 +176,25 @@ mod AggregatorProxy { } fn add_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::add_access(ref state, user) } fn remove_access(ref self: ContractState, user: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::remove_access(ref state, user) } fn enable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::enable_access_check(ref state) } fn disable_access_check(ref self: ContractState) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); let mut state = AccessControl::unsafe_new_contract_state(); AccessControl::disable_access_check(ref state) } @@ -224,8 +205,7 @@ mod AggregatorProxy { #[external(v0)] impl AggregatorProxyInternal of super::IAggregatorProxyInternal { fn propose_aggregator(ref self: ContractState, address: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); assert(!address.is_zero(), 'proposed address is 0'); self._proposed_aggregator.write(address); @@ -234,8 +214,7 @@ mod AggregatorProxy { } fn confirm_aggregator(ref self: ContractState, address: ContractAddress) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); assert(!address.is_zero(), 'confirm address is 0'); let phase = self._current_phase.read(); let previous = phase.aggregator; diff --git a/contracts/src/tests/test_access_controller.cairo b/contracts/src/tests/test_access_controller.cairo index b267d24f5..4b96b2f30 100644 --- a/contracts/src/tests/test_access_controller.cairo +++ b/contracts/src/tests/test_access_controller.cairo @@ -31,7 +31,7 @@ fn setup() -> ContractAddress { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_upgrade_not_owner() { let sender = setup(); let mut state = STATE(); diff --git a/contracts/src/tests/test_aggregator.cairo b/contracts/src/tests/test_aggregator.cairo index 439e62c41..6268c9071 100644 --- a/contracts/src/tests/test_aggregator.cairo +++ b/contracts/src/tests/test_aggregator.cairo @@ -107,7 +107,7 @@ fn setup() -> ( } #[test] -#[available_gas(2000000)] +#[available_gas(3000000)] fn test_ownable() { let (account, _, _, _) = setup(); // Deploy aggregator @@ -153,7 +153,7 @@ fn test_access_control() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_upgrade_non_owner() { let sender = setup(); let mut state = STATE(); @@ -165,7 +165,7 @@ fn test_upgrade_non_owner() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_set_billing_access_controller_not_owner() { let (owner, acc2, billingAccessController, _) = setup(); let mut state = STATE(); @@ -281,7 +281,7 @@ fn test_set_billing_config_as_acc_with_access() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_set_payees_caller_not_owner() { let (owner, acc2, _, _) = setup(); let mut state = STATE(); diff --git a/contracts/src/tests/test_aggregator_proxy.cairo b/contracts/src/tests/test_aggregator_proxy.cairo index 4ae15a1ee..79d3457bb 100644 --- a/contracts/src/tests/test_aggregator_proxy.cairo +++ b/contracts/src/tests/test_aggregator_proxy.cairo @@ -91,7 +91,7 @@ fn test_access_control() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_upgrade_non_owner() { let (_, _, _, _, _) = setup(); let mut state = STATE(); diff --git a/contracts/src/tests/test_link_token.cairo b/contracts/src/tests/test_link_token.cairo index 5d24f0e86..75e1a21ff 100644 --- a/contracts/src/tests/test_link_token.cairo +++ b/contracts/src/tests/test_link_token.cairo @@ -147,7 +147,7 @@ fn test_permissioned_burn_from_nonminter() { #[test] #[available_gas(2000000)] -#[should_panic(expected: ('Ownable: caller is not owner',))] +#[should_panic(expected: ('Caller is not the owner',))] fn test_upgrade_non_owner() { let sender = setup(); let mut state = STATE(); diff --git a/contracts/src/tests/test_ownable.cairo b/contracts/src/tests/test_ownable.cairo index 63593fda3..a67933d0e 100644 --- a/contracts/src/tests/test_ownable.cairo +++ b/contracts/src/tests/test_ownable.cairo @@ -3,11 +3,43 @@ use starknet::ContractAddress; use starknet::testing::set_caller_address; use starknet::testing::set_contract_address; use zeroable::Zeroable; -use chainlink::libraries::ownable::{Ownable, IOwnable, IOwnableDispatcher, IOwnableDispatcherTrait}; -use chainlink::libraries::ownable::Ownable::OwnableImpl; +use chainlink::libraries::ownable::{OwnableComponent, IOwnable, IOwnableDispatcher, IOwnableDispatcherTrait}; +use OwnableComponent::InternalTrait; + +#[starknet::contract] +mod OwnableMock { + use super::OwnableComponent; + use starknet::ContractAddress; + + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); + + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + 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); + } +} + +type ComponentState = OwnableComponent::ComponentState; -fn STATE() -> Ownable::ContractState { - Ownable::contract_state_for_testing() +fn STATE() -> ComponentState { + OwnableComponent::component_state_for_testing() } fn setup() -> (ContractAddress, ContractAddress) { @@ -23,8 +55,8 @@ fn test_assert_only_owner() { let (owner, _) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); - Ownable::assert_only_owner(@state); + state.initializer(owner); + state.assert_only_owner(); } #[test] @@ -34,9 +66,9 @@ fn test_assert_only_owner_panics_if_not_owner() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); + state.initializer(owner); set_caller_address(other_user); - Ownable::assert_only_owner(@state); + state.assert_only_owner(); } #[test] @@ -45,9 +77,9 @@ fn test_owner() { let (owner, _) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); + state.initializer(owner); - assert(owner == OwnableImpl::owner(@state), 'should equal owner'); + assert(owner == state.owner(), 'should equal owner'); } #[test] @@ -56,10 +88,10 @@ fn test_transfer_ownership() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); - OwnableImpl::transfer_ownership(ref state, other_user); + state.initializer(owner); + state.transfer_ownership(other_user); - assert(other_user == OwnableImpl::proposed_owner(@state), 'should equal proposed owner'); + assert(other_user == state.proposed_owner(), 'should equal proposed owner'); } #[test] @@ -69,8 +101,8 @@ fn test_transfer_ownership_panics_if_zero_address() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); - OwnableImpl::transfer_ownership(ref state, Zeroable::zero()); + state.initializer(owner); + state.transfer_ownership(Zeroable::zero()); } #[test] @@ -80,9 +112,9 @@ fn test_transfer_ownership_panics_if_not_owner() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); + state.initializer(owner); set_caller_address(other_user); - OwnableImpl::transfer_ownership(ref state, other_user); + state.transfer_ownership(other_user); } #[test] @@ -91,12 +123,12 @@ fn test_accept_ownership() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); - OwnableImpl::transfer_ownership(ref state, other_user); + state.initializer(owner); + state.transfer_ownership(other_user); set_caller_address(other_user); - OwnableImpl::accept_ownership(ref state); + state.accept_ownership(); - assert(OwnableImpl::owner(@state) == other_user, 'failed to accept ownership'); + assert(state.owner() == other_user, 'failed to accept ownership'); } #[test] @@ -106,11 +138,11 @@ fn test_accept_ownership_panics_if_not_proposed_owner() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); - OwnableImpl::transfer_ownership(ref state, other_user); + state.initializer(owner); + state.transfer_ownership(other_user); set_caller_address(contract_address_const::<3>()); - OwnableImpl::accept_ownership(ref state); + state.accept_ownership(); } #[test] @@ -120,10 +152,10 @@ fn test_renounce_ownership() { let mut state = STATE(); set_caller_address(owner); - Ownable::constructor(ref state, owner); - OwnableImpl::renounce_ownership(ref state); + state.initializer(owner); + state.renounce_ownership(); - assert(OwnableImpl::owner(@state).is_zero(), 'owner not 0 after renounce'); + assert(state.owner().is_zero(), 'owner not 0 after renounce'); } #[test] @@ -133,9 +165,9 @@ fn test_renounce_ownership_panics_if_not_owner() { let (owner, other_user) = setup(); let mut state = STATE(); - Ownable::constructor(ref state, owner); + state.initializer(owner); set_caller_address(other_user); - OwnableImpl::renounce_ownership(ref state); + state.renounce_ownership(); } // // General ownable contract tests diff --git a/contracts/src/tests/test_upgradeable.cairo b/contracts/src/tests/test_upgradeable.cairo index 1605b0439..f63cc7954 100644 --- a/contracts/src/tests/test_upgradeable.cairo +++ b/contracts/src/tests/test_upgradeable.cairo @@ -7,7 +7,6 @@ use starknet::class_hash::class_hash_const; use starknet::syscalls::deploy_syscall; use chainlink::libraries::upgradeable::Upgradeable; -use chainlink::libraries::ownable::Ownable; use chainlink::libraries::mocks::mock_upgradeable::{ MockUpgradeable, IMockUpgradeableDispatcher, IMockUpgradeableDispatcherTrait, IMockUpgradeableDispatcherImpl diff --git a/contracts/src/token/link_token.cairo b/contracts/src/token/link_token.cairo index 48a1283c7..8b12f1eb1 100644 --- a/contracts/src/token/link_token.cairo +++ b/contracts/src/token/link_token.cairo @@ -16,16 +16,21 @@ mod LinkToken { use openzeppelin::token::erc20::interface::{IERC20, IERC20Dispatcher, IERC20DispatcherTrait}; use chainlink::libraries::token::erc677::ERC677Component; - use chainlink::libraries::ownable::{Ownable, IOwnable}; + use chainlink::libraries::ownable::{OwnableComponent, IOwnable}; use chainlink::libraries::upgradeable::{Upgradeable, IUpgradeable}; use openzeppelin::token::erc20::ERC20Component; use starknet::ContractAddress; use starknet::class_hash::ClassHash; + component!(path: OwnableComponent, storage: ownable, event: OwnableEvent); component!(path: ERC20Component, storage: erc20, event: ERC20Event); component!(path: ERC677Component, storage: erc677, event: ERC677Event); + #[abi(embed_v0)] + impl OwnableImpl = OwnableComponent::OwnableImpl; + impl InternalImpl = OwnableComponent::InternalImpl; + #[abi(embed_v0)] impl ERC20Impl = ERC20Component::ERC20Impl; #[abi(embed_v0)] @@ -40,6 +45,9 @@ mod LinkToken { #[storage] struct Storage { + #[substorage(v0)] + ownable: OwnableComponent::Storage, + _minter: ContractAddress, #[substorage(v0)] @@ -52,6 +60,8 @@ mod LinkToken { #[event] #[derive(Drop, starknet::Event)] enum Event { + #[flat] + OwnableEvent: OwnableComponent::Event, #[flat] ERC20Event: ERC20Component::Event, #[flat] @@ -80,8 +90,7 @@ mod LinkToken { self.erc20.initializer(NAME, SYMBOL); assert(!minter.is_zero(), 'minter is 0'); self._minter.write(minter); - let mut ownable = Ownable::unsafe_new_contract_state(); - Ownable::constructor(ref ownable, owner); // Ownable::initializer(owner); + self.ownable.initializer(owner); } // TODO #[view] @@ -113,44 +122,11 @@ mod LinkToken { #[external(v0)] impl UpgradeableImpl of IUpgradeable { fn upgrade(ref self: ContractState, new_impl: ClassHash) { - let ownable = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@ownable); + self.ownable.assert_only_owner(); Upgradeable::upgrade(new_impl) } } - // - // Ownership - // - - #[external(v0)] - impl OwnableImpl of IOwnable { - fn owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::owner(@state) - } - - fn proposed_owner(self: @ContractState) -> ContractAddress { - let state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::proposed_owner(@state) - } - - fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::transfer_ownership(ref state, new_owner) - } - - fn accept_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::accept_ownership(ref state) - } - - fn renounce_ownership(ref self: ContractState) { - let mut state = Ownable::unsafe_new_contract_state(); - Ownable::OwnableImpl::renounce_ownership(ref state) - } - } - // fn increase_allowance(ref self: ContractState, spender: ContractAddress, added_value: u256) -> bool { // let mut state = ERC20::unsafe_new_contract_state(); // ERC20::ERC20Impl::increase_allowance(ref state, spender, added_value)