Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: upgradeability with initializer #497

Merged
merged 7 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions starknet/src/factory/factory.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ trait IFactory<TContractState> {
self: @TContractState,
class_hash: ClassHash,
contract_address_salt: felt252,
calldata: Span<felt252>
initialize_calldata: Span<felt252>
) -> ContractAddress;
}

Expand All @@ -17,9 +17,11 @@ mod Factory {
use super::IFactory;
use starknet::ContractAddress;
use starknet::contract_address_const;
use starknet::syscalls::deploy_syscall;
use starknet::syscalls::{deploy_syscall, call_contract_syscall};
use starknet::ClassHash;
use result::ResultTrait;
use array::{ArrayTrait, SpanTrait};
use sx::utils::constants::INITIALIZE_SELECTOR;

#[event]
fn SpaceDeployed(class_hash: ClassHash, space_address: ContractAddress) {}
Expand All @@ -33,13 +35,17 @@ mod Factory {
self: @ContractState,
class_hash: ClassHash,
contract_address_salt: felt252,
calldata: Span<felt252>
initialize_calldata: Span<felt252>
) -> ContractAddress {
let (space_address, _) = deploy_syscall(
class_hash, contract_address_salt, calldata, false
class_hash, contract_address_salt, array![].span(), false
)
.unwrap();

// Call initializer.
call_contract_syscall(space_address, INITIALIZE_SELECTOR, initialize_calldata)
.unwrap_syscall();

SpaceDeployed(class_hash, space_address);

space_address
Expand Down
138 changes: 86 additions & 52 deletions starknet/src/space/space.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ trait ISpace<TContractState> {
fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress);
fn renounce_ownership(ref self: TContractState);
// Actions
fn initialize(
ref self: TContractState,
owner: ContractAddress,
min_voting_duration: u32,
max_voting_duration: u32,
voting_delay: u32,
proposal_validation_strategy: Strategy,
proposal_validation_strategy_metadata_URI: Array<felt252>,
voting_strategies: Array<Strategy>,
voting_strategy_metadata_URIs: Array<Array<felt252>>,
authenticators: Array<ContractAddress>,
metadata_URI: Array<felt252>,
dao_URI: Array<felt252>,
);
fn propose(
ref self: TContractState,
author: UserAddress,
Expand All @@ -53,13 +67,15 @@ trait ISpace<TContractState> {
metadata_URI: Array<felt252>,
);
fn cancel_proposal(ref self: TContractState, proposal_id: u256);
fn upgrade(ref self: TContractState, class_hash: ClassHash);
fn upgrade(
ref self: TContractState, class_hash: ClassHash, initialize_calldata: Array<felt252>
);
}

#[starknet::contract]
mod Space {
use super::ISpace;
use starknet::{ClassHash, ContractAddress, info, Store};
use starknet::{ClassHash, ContractAddress, info, Store, syscalls};
use zeroable::Zeroable;
use array::{ArrayTrait, SpanTrait};
use clone::Clone;
Expand All @@ -77,14 +93,17 @@ mod Space {
IndexedStrategyTrait, IndexedStrategyImpl, UpdateSettingsCalldata, NoUpdateU32,
NoUpdateStrategy, NoUpdateArray
};
use sx::utils::reinitializable::Reinitializable;
use sx::utils::ReinitializableImpl;
use sx::utils::bits::BitSetter;
use sx::utils::legacy_hash::LegacyHashChoice;
use sx::external::ownable::Ownable;
use sx::utils::constants::INITIALIZE_SELECTOR;

#[storage]
struct Storage {
_max_voting_duration: u32,
_min_voting_duration: u32,
_max_voting_duration: u32,
_next_proposal_id: u256,
_voting_delay: u32,
_active_voting_strategies: u256,
Expand All @@ -101,9 +120,9 @@ mod Space {
fn SpaceCreated(
_space: ContractAddress,
_owner: ContractAddress,
_voting_delay: u32,
_min_voting_duration: u32,
_max_voting_duration: u32,
_voting_delay: u32,
_proposal_validation_strategy: @Strategy,
_proposal_validation_strategy_metadata_URI: @Array<felt252>,
_voting_strategies: @Array<Strategy>,
Expand Down Expand Up @@ -179,10 +198,56 @@ mod Space {
fn VotingDelayUpdated(_new_voting_delay: u32) {}

#[event]
fn Upgraded(class_hash: ClassHash) {}
fn Upgraded(class_hash: ClassHash, initialize_calldata: Array<felt252>) {}

#[external(v0)]
impl Space of ISpace<ContractState> {
fn initialize(
ref self: ContractState,
owner: ContractAddress,
min_voting_duration: u32,
max_voting_duration: u32,
voting_delay: u32,
proposal_validation_strategy: Strategy,
proposal_validation_strategy_metadata_URI: Array<felt252>,
voting_strategies: Array<Strategy>,
voting_strategy_metadata_URIs: Array<Array<felt252>>,
authenticators: Array<ContractAddress>,
metadata_URI: Array<felt252>,
dao_URI: Array<felt252>,
) {
SpaceCreated(
info::get_contract_address(),
owner,
min_voting_duration,
max_voting_duration,
voting_delay,
@proposal_validation_strategy,
@proposal_validation_strategy_metadata_URI,
@voting_strategies,
@voting_strategy_metadata_URIs,
@authenticators,
@metadata_URI,
@dao_URI
);
// Checking that the contract is not already initialized
//TODO: temporary component syntax (see imports too)
let mut state: Reinitializable::ContractState =
Reinitializable::unsafe_new_contract_state();
ReinitializableImpl::initialize(ref state);

//TODO: temporary component syntax
let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::initializer(ref state);
Ownable::transfer_ownership(ref state, owner);
_set_min_voting_duration(ref self, min_voting_duration);
_set_max_voting_duration(ref self, max_voting_duration);
_set_voting_delay(ref self, voting_delay);
_set_proposal_validation_strategy(ref self, proposal_validation_strategy);
_add_voting_strategies(ref self, voting_strategies);
_add_authenticators(ref self, authenticators);
self._next_proposal_id.write(1_u256);
}
fn propose(
ref self: ContractState,
author: UserAddress,
Expand Down Expand Up @@ -344,15 +409,26 @@ mod Space {
ProposalCancelled(proposal_id);
}

fn upgrade(ref self: ContractState, class_hash: ClassHash) {
fn upgrade(
ref self: ContractState, class_hash: ClassHash, initialize_calldata: Array<felt252>
) {
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);

assert(
class_hash.is_non_zero(), 'Class Hash cannot be zero'
); // TODO: not sure this is needed
assert(class_hash.is_non_zero(), 'Class Hash cannot be zero');
starknet::replace_class_syscall(class_hash).unwrap_syscall();
Upgraded(class_hash);

// Allowing initializer to be called again.
let mut state: Reinitializable::ContractState =
Reinitializable::unsafe_new_contract_state();
ReinitializableImpl::reinitialize(ref state);

// Call initializer on the new version.
syscalls::call_contract_syscall(
info::get_contract_address(), INITIALIZE_SELECTOR, initialize_calldata.span()
)
.unwrap_syscall();
Upgraded(class_hash, initialize_calldata);
}

fn owner(self: @ContractState) -> ContractAddress {
Expand Down Expand Up @@ -485,48 +561,6 @@ mod Space {
}
}

#[constructor]
fn constructor(
ref self: ContractState,
_owner: ContractAddress,
_max_voting_duration: u32,
_min_voting_duration: u32,
_voting_delay: u32,
_proposal_validation_strategy: Strategy,
_proposal_validation_strategy_metadata_URI: Array<felt252>,
_voting_strategies: Array<Strategy>,
_voting_strategies_metadata_URIs: Array<Array<felt252>>,
_authenticators: Array<ContractAddress>,
_metadata_URI: Array<felt252>,
_dao_URI: Array<felt252>
) {
//TODO: temporary component syntax
let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::initializer(ref state);
Ownable::transfer_ownership(ref state, _owner);
_set_max_voting_duration(ref self, _max_voting_duration);
_set_min_voting_duration(ref self, _min_voting_duration);
_set_voting_delay(ref self, _voting_delay);
_set_proposal_validation_strategy(ref self, _proposal_validation_strategy.clone());
_add_voting_strategies(ref self, _voting_strategies.clone());
_add_authenticators(ref self, _authenticators.clone());
self._next_proposal_id.write(1_u256);
SpaceCreated(
info::get_contract_address(),
_owner,
_voting_delay,
_min_voting_duration,
_max_voting_duration,
@_proposal_validation_strategy,
@_proposal_validation_strategy_metadata_URI,
@_voting_strategies,
@_voting_strategies_metadata_URIs,
@_authenticators,
@_metadata_URI,
@_dao_URI
);
}

///
/// Internals
///
Expand Down
1 change: 1 addition & 0 deletions starknet/src/tests/mocks.cairo
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
mod proposal_validation_always_fail;
mod executor;
mod space_v2;
27 changes: 27 additions & 0 deletions starknet/src/tests/mocks/space_v2.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#[starknet::interface]
trait ISpaceV2<TContractState> {
fn initialize(ref self: TContractState, var: felt252);
fn get_var(self: @TContractState) -> felt252;
}

#[starknet::contract]
mod SpaceV2 {
use super::ISpaceV2;
#[storage]
struct Storage {
_initialized: bool,
_var: felt252
}

#[external(v0)]
impl SpaceV2 of ISpaceV2<ContractState> {
fn initialize(ref self: ContractState, var: felt252) {
assert(self._initialized.read() == false, 'Contract already initialized');
self._initialized.write(true);
self._var.write(var);
Copy link
Contributor

@pscott pscott Aug 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we make SpaceV2 use the reinitializable functions ? Just to make sure it can indeed be used again (not sure how storage works)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

}
fn get_var(self: @ContractState) -> felt252 {
self._var.read()
}
}
}
40 changes: 20 additions & 20 deletions starknet/src/tests/setup/setup.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ mod setup {
.append(Strategy { address: vanilla_voting_strategy_address, params: array![] });

// Deploy Vanilla Execution Strategy
let mut constructor_calldata = array![];
quorum.serialize(ref constructor_calldata);
let mut initializer_calldata = array![];
quorum.serialize(ref initializer_calldata);
let (vanilla_execution_strategy_address, _) = deploy_syscall(
VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(),
0,
constructor_calldata.span(),
initializer_calldata.span(),
false
)
.unwrap();
Expand All @@ -96,7 +96,7 @@ mod setup {
}
}

fn get_constructor_calldata(
fn get_initialize_calldata(
owner: @ContractAddress,
min_voting_duration: @u64,
max_voting_duration: @u64,
Expand All @@ -106,20 +106,20 @@ mod setup {
authenticators: @Array<ContractAddress>
) -> Array<felt252> {
// Using empty arrays for all the metadata fields
let mut constructor_calldata = array![];
constructor_calldata.append((*owner).into());
constructor_calldata.append((*max_voting_duration).into());
constructor_calldata.append((*min_voting_duration).into());
constructor_calldata.append((*voting_delay).into());
proposal_validation_strategy.serialize(ref constructor_calldata);
ArrayTrait::<felt252>::new().serialize(ref constructor_calldata);
voting_strategies.serialize(ref constructor_calldata);
ArrayTrait::<felt252>::new().serialize(ref constructor_calldata);
authenticators.serialize(ref constructor_calldata);
ArrayTrait::<felt252>::new().serialize(ref constructor_calldata);
ArrayTrait::<felt252>::new().serialize(ref constructor_calldata);

constructor_calldata
let mut initializer_calldata = array![];
owner.serialize(ref initializer_calldata);
min_voting_duration.serialize(ref initializer_calldata);
max_voting_duration.serialize(ref initializer_calldata);
voting_delay.serialize(ref initializer_calldata);
proposal_validation_strategy.serialize(ref initializer_calldata);
ArrayTrait::<felt252>::new().serialize(ref initializer_calldata);
voting_strategies.serialize(ref initializer_calldata);
ArrayTrait::<felt252>::new().serialize(ref initializer_calldata);
authenticators.serialize(ref initializer_calldata);
ArrayTrait::<felt252>::new().serialize(ref initializer_calldata);
ArrayTrait::<felt252>::new().serialize(ref initializer_calldata);

initializer_calldata
}

fn deploy(config: @Config) -> (IFactoryDispatcher, ISpaceDispatcher) {
Expand All @@ -133,7 +133,7 @@ mod setup {

let factory = IFactoryDispatcher { contract_address: factory_address };

let constructor_calldata = get_constructor_calldata(
let initializer_calldata = get_initialize_calldata(
config.owner,
config.min_voting_duration,
config.max_voting_duration,
Expand All @@ -143,7 +143,7 @@ mod setup {
config.authenticators
);
let space_address = factory
.deploy(space_class_hash, contract_address_salt, constructor_calldata.span());
.deploy(space_class_hash, contract_address_salt, initializer_calldata.span());

let space = ISpaceDispatcher { contract_address: space_address };

Expand Down
4 changes: 2 additions & 2 deletions starknet/src/tests/test_factory.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ mod tests {
use sx::types::Strategy;
use starknet::ClassHash;

use sx::tests::setup::setup::setup::{setup, get_constructor_calldata, deploy};
use sx::tests::setup::setup::setup::{setup, get_initialize_calldata, deploy};

#[test]
#[available_gas(10000000000)]
Expand Down Expand Up @@ -39,7 +39,7 @@ mod tests {
let contract_address_salt = 0;

let config = setup();
let constructor_calldata = get_constructor_calldata(
let constructor_calldata = get_initialize_calldata(
@config.owner,
@config.min_voting_duration,
@config.max_voting_duration,
Expand Down
Loading
Loading