Skip to content

Commit

Permalink
annotate ownableImpl with embed_v0
Browse files Browse the repository at this point in the history
  • Loading branch information
@pscott
pscott committed Oct 9, 2024
1 parent 16989be commit fff5cde
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 63 deletions.
23 changes: 2 additions & 21 deletions starknet/src/execution_strategies/timelock.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,6 @@ trait ITimelockExecutionStrategy<TContractState> {
fn disable_space(ref self: TContractState, space: ContractAddress);

fn is_space_enabled(self: @TContractState, space: ContractAddress) -> bool;

/// Transfers the ownership to `new_owner`.
fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress);
/// Renounces the ownership of the contract. CAUTION: This is a one-way operation.
fn renounce_ownership(ref self: TContractState);

fn owner(self: @TContractState) -> ContractAddress;
}

#[starknet::contract]
Expand All @@ -41,8 +34,9 @@ mod TimelockExecutionStrategy {

component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);

impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;
#[abi(embed_v0)]
impl OwnableImpl = OwnableComponent::OwnableImpl<ContractState>;
impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;

component!(path: SimpleQuorumComponent, storage: simple_quorum, event: SimpleQuorumEvent);

Expand Down Expand Up @@ -318,19 +312,6 @@ mod TimelockExecutionStrategy {
let state = SpaceManager::unsafe_new_contract_state();
SpaceManager::InternalImpl::is_space_enabled(@state, space)
}

// TODO: use Ownable impl as abi_embed(v0)?
fn owner(self: @ContractState) -> ContractAddress {
self.ownable.owner()
}

fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) {
self.ownable.transfer_ownership(new_owner);
}

fn renounce_ownership(ref self: ContractState) {
self.ownable.renounce_ownership();
}
}

#[generate_trait]
Expand Down
7 changes: 0 additions & 7 deletions starknet/src/interfaces/i_space.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ use sx::types::{
trait ISpace<TContractState> {
// -- View functions --

/// Returns the owner of the contract.
fn owner(self: @TContractState) -> ContractAddress;

/// The voting delay in seconds between when a proposal is created and the start of the voting period.
fn voting_delay(self: @TContractState) -> u32;

Expand Down Expand Up @@ -60,10 +57,6 @@ trait ISpace<TContractState> {
/// struct is passed to be able to update different settings in a single call. Settings that should not
/// be updated should have the `no_update` value (see `UpdateSettingsCalldata`).
fn update_settings(ref self: TContractState, input: UpdateSettingsCalldata);
/// Transfers the ownership to `new_owner`.
fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress);
/// Renounces the ownership of the contract. CAUTION: This is a one-way operation.
fn renounce_ownership(ref self: TContractState);

// -- Actions --
/// Initializes the contract. Can only be called once.
Expand Down
15 changes: 2 additions & 13 deletions starknet/src/space/space.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ mod Space {

component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);

impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;
#[abi(embed_v0)]
impl OwnableImpl = OwnableComponent::OwnableImpl<ContractState>;
impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;

#[storage]
struct Storage {
Expand Down Expand Up @@ -486,10 +487,6 @@ mod Space {
// We leave the implementation empty.
}

fn owner(self: @ContractState) -> ContractAddress {
self.ownable.owner()
}

fn max_voting_duration(self: @ContractState) -> u32 {
self._max_voting_duration.read()
}
Expand Down Expand Up @@ -710,14 +707,6 @@ mod Space {
fn vote_power(self: @ContractState, proposal_id: u256, choice: Choice) -> u256 {
self._vote_power.read((proposal_id, choice))
}

fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) {
self.ownable.transfer_ownership(new_owner);
}

fn renounce_ownership(ref self: ContractState) {
self.ownable.renounce_ownership();
}
}

#[generate_trait]
Expand Down
58 changes: 38 additions & 20 deletions starknet/src/tests/execution_strategies/timelock.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ mod tests {
use sx::interfaces::{
IQuorum, IQuorumDispatcher, IQuorumDispatcherTrait, ISpaceDispatcher, ISpaceDispatcherTrait
};
use openzeppelin::access::ownable::interface::{IOwnableDispatcher, IOwnableDispatcherTrait};
use sx::execution_strategies::{
TimelockExecutionStrategy, TimelockExecutionStrategy::CallWithSalt,
timelock::{ITimelockExecutionStrategyDispatcher, ITimelockExecutionStrategyDispatcherTrait}
Expand All @@ -27,6 +28,7 @@ mod tests {
let mut config = setup::setup();
config.min_voting_duration = 0;
let (_, space) = setup::deploy(@config);
let ownable_space = IOwnableDispatcher { contract_address: space.contract_address };

let spaces = array![space.contract_address];
let timelock_delay = 100;
Expand Down Expand Up @@ -55,7 +57,7 @@ mod tests {
// Set timelock as space controller
testing::set_caller_address(config.owner);
testing::set_contract_address(config.owner);
space.transfer_ownership(timelock.contract_address);
ownable_space.transfer_ownership(timelock.contract_address);
(config, space, timelock)
}

Expand Down Expand Up @@ -129,7 +131,9 @@ mod tests {
);
// Execute proposal on timelock - should renounce ownership of space
timelock.execute_queued_proposal(payload.span());
assert(space.owner().is_zero(), 'renounce ownership failed');

let ownable_space = IOwnableDispatcher { contract_address: space.contract_address };
assert(ownable_space.owner().is_zero(), 'renounce ownership failed');
}

#[test]
Expand All @@ -150,8 +154,10 @@ mod tests {
}
]
);
testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());

let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
timelock.disable_space(space.contract_address);
assert(timelock.is_space_enabled(space.contract_address) == false, 'disable space failed');
// Try to execute proposal on space, should fail because the space is disabled
Expand Down Expand Up @@ -424,7 +430,9 @@ mod tests {
// Execute proposal on timelock - should renounce ownership of space and update settings
timelock.execute_queued_proposal(payload.span());
assert(space.max_voting_duration() == 1000, 'update settings failed');
assert(space.owner().is_zero(), 'renounce ownership failed');

let ownable_space = IOwnableDispatcher { contract_address: space.contract_address };
assert(ownable_space.owner().is_zero(), 'renounce ownership failed');
}

#[test]
Expand Down Expand Up @@ -515,9 +523,12 @@ mod tests {
]
);
space.execute(proposal_id, payload.clone());

let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());

// Veto proposal from unauthorized account
testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
timelock.veto(poseidon::poseidon_hash_span(payload.span()));
}

Expand All @@ -526,8 +537,9 @@ mod tests {
fn set_timelock_delay() {
let (_, _, timelock) = setup_test();

testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
timelock.set_timelock_delay(1000);
assert(timelock.timelock_delay() == 1000, 'timelock delay not set');
}
Expand All @@ -549,8 +561,9 @@ mod tests {
fn set_veto_guardian() {
let (_, _, timelock) = setup_test();

testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
timelock.set_veto_guardian(timelock.veto_guardian());
assert(timelock.veto_guardian() == timelock.veto_guardian(), 'veto guardian not set');
}
Expand All @@ -571,10 +584,11 @@ mod tests {
fn set_owner() {
let (_, _, timelock) = setup_test();

testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
timelock.transfer_ownership(timelock.owner());
assert(timelock.owner() == timelock.owner(), 'owner not set');
let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
ownable_timelock.transfer_ownership(timelock.veto_guardian());
assert(ownable_timelock.owner() == timelock.veto_guardian(), 'owner not set');
}

#[test]
Expand All @@ -585,16 +599,19 @@ mod tests {
// Only the owner can transfer ownership
testing::set_caller_address(timelock.veto_guardian());
testing::set_contract_address(timelock.veto_guardian());
timelock.transfer_ownership(starknet::contract_address_const::<0x9999>());

let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
ownable_timelock.transfer_ownership(starknet::contract_address_const::<0x9999>());
}

#[test]
#[available_gas(10000000000)]
fn enable_space() {
let (_, _, timelock) = setup_test();

testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
timelock.enable_space(starknet::contract_address_const::<0x123456789>());
assert(
timelock.is_space_enabled(starknet::contract_address_const::<0x123456789>()),
Expand All @@ -618,8 +635,9 @@ mod tests {
fn disable_space() {
let (_, _, timelock) = setup_test();

testing::set_caller_address(timelock.owner());
testing::set_contract_address(timelock.owner());
let ownable_timelock = IOwnableDispatcher { contract_address: timelock.contract_address };
testing::set_caller_address(ownable_timelock.owner());
testing::set_contract_address(ownable_timelock.owner());
timelock.enable_space(starknet::contract_address_const::<0x123456789>());
timelock.disable_space(starknet::contract_address_const::<0x123456789>());
assert(
Expand Down
4 changes: 3 additions & 1 deletion starknet/src/tests/space/space.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
mod tests {
use starknet::{ContractAddress, syscalls, testing, info};
use openzeppelin::tests::utils;
use openzeppelin::access::ownable::interface::{IOwnableDispatcher, IOwnableDispatcherTrait};
use sx::interfaces::{ISpaceDispatcher, ISpaceDispatcherTrait};
use sx::space::space::{Space, Space::{ProposalCreated, VoteCast, ProposalUpdated},};
use sx::tests::mocks::vanilla_authenticator::{
Expand Down Expand Up @@ -111,6 +112,7 @@ mod tests {
.unwrap();

let space = ISpaceDispatcher { contract_address: space_address };
let ownable_space = IOwnableDispatcher { contract_address: space_address };

space
.initialize(
Expand All @@ -127,7 +129,7 @@ mod tests {
array![]
);

assert(space.owner() == owner, 'owner incorrect');
assert(ownable_space.owner() == owner, 'owner incorrect');
assert(space.min_voting_duration() == min_voting_duration, 'min incorrect');
assert(space.max_voting_duration() == max_voting_duration, 'max incorrect');
assert(space.voting_delay() == voting_delay, 'voting delay incorrect');
Expand Down
4 changes: 3 additions & 1 deletion starknet/src/tests/space/upgrade.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod tests {
use sx::tests::mocks::vanilla_authenticator::{
VanillaAuthenticator, IVanillaAuthenticatorDispatcher, IVanillaAuthenticatorDispatcherTrait
};
use openzeppelin::access::ownable::interface::{IOwnableDispatcher, IOwnableDispatcherTrait};
use sx::tests::mocks::vanilla_execution_strategy::VanillaExecutionStrategy;
use sx::tests::mocks::vanilla_voting_strategy::VanillaVotingStrategy;
use sx::tests::mocks::vanilla_proposal_validation::VanillaProposalValidationStrategy;
Expand Down Expand Up @@ -61,7 +62,8 @@ mod tests {

// Set the owner to be the execution strategy
testing::set_contract_address(config.owner);
space.transfer_ownership(execution_contract_address);
let ownable_space = IOwnableDispatcher { contract_address: space.contract_address };
ownable_space.transfer_ownership(execution_contract_address);

let selector = sx::utils::constants::UPGRADE_SELECTOR;
let mut tx_calldata = array![];
Expand Down

0 comments on commit fff5cde

Please sign in to comment.