Skip to content

Commit

Permalink
Refactor settings setters; use snapshots for events (#456)
Browse files Browse the repository at this point in the history 
* refactor: remove setters; add update_settings function

* refactor: use snapshots for events

* remove needless clone on execution_payload_hash

* refactor: finish using snapshot in events

* chore: formatting

* chore: remappings update

* chore: formatting

* chore: use no update in default

---------

Co-authored-by: Orlando <[email protected]>
  • Loading branch information
@pscott
pscott and Orlando authored Jul 25, 2023
1 parent 20ae041 commit 3bbb8fd
Show file tree
Hide file tree
Showing 5 changed files with 229 additions and 96 deletions.
3 changes: 2 additions & 1 deletion ethereum/remappings.txt
View file
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
@gnosis.pm/safe-contracts=lib/safe-contracts
@gnosis.pm/safe-contracts=lib/safe-contracts
@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts
2 changes: 1 addition & 1 deletion ethereum/src/execution-strategies/StarknetSpaceManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

pragma solidity ^0.8.19;

import "openzeppelin/access/OwnableUpgradeable.sol";
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

/// @title Space Manager - A contract that manages SX spaces on Starknet that are able to execute transactions via this contract
/// @author Snapshot Labs
Expand Down
179 changes: 94 additions & 85 deletions starknet/src/space/space.cairo
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use core::traits::Destruct;
use starknet::ContractAddress;
use sx::utils::types::{Strategy, Proposal, IndexedStrategy, Choice};
use sx::utils::types::{Strategy, Proposal, IndexedStrategy, Choice, UpdateSettingsCalldata};

#[starknet::interface]
trait ISpace<TContractState> {
Expand All @@ -24,16 +24,7 @@ trait ISpace<TContractState> {
// fn get_proposal_status(proposal_id: u256) -> u8;

// Owner Actions
fn set_max_voting_duration(ref self: TContractState, max_voting_duration: u64);
fn set_min_voting_duration(ref self: TContractState, min_voting_duration: u64);
fn set_voting_delay(ref self: TContractState, voting_delay: u64);
fn set_proposal_validation_strategy(
ref self: TContractState, proposal_validation_strategy: Strategy
);
fn add_voting_strategies(ref self: TContractState, voting_strategies: Array<Strategy>);
fn remove_voting_strategies(ref self: TContractState, voting_strategy_indices: Array<u8>);
fn add_authenticators(ref self: TContractState, authenticators: Array<ContractAddress>);
fn remove_authenticators(ref self: TContractState, authenticators: Array<ContractAddress>);
fn update_settings(ref self: TContractState, input: UpdateSettingsCalldata);
fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress);
fn renounce_ownership(ref self: TContractState);
// Actions
Expand Down Expand Up @@ -79,7 +70,8 @@ mod Space {
use sx::utils::{
types::{
Choice, FinalizationStatus, Strategy, IndexedStrategy, Proposal, IndexedStrategyTrait,
IndexedStrategyImpl
IndexedStrategyImpl, UpdateSettingsCalldata, NoUpdateU64, NoUpdateStrategy,
NoUpdateArray
},
bits::BitSetter
};
Expand Down Expand Up @@ -108,14 +100,17 @@ mod Space {
_voting_delay: u64,
_min_voting_duration: u64,
_max_voting_duration: u64,
_proposal_validation_strategy: Strategy,
_voting_strategies: Array<Strategy>,
_authenticators: Array<ContractAddress>
_proposal_validation_strategy: @Strategy,
_voting_strategies: @Array<Strategy>,
_authenticators: @Array<ContractAddress>
) {}

#[event]
fn ProposalCreated(
_proposal_id: u256, _author: ContractAddress, _proposal: Proposal, _payload: Array<felt252>
_proposal_id: u256,
_author: ContractAddress,
_proposal: @Proposal,
_payload: @Array<felt252>
) {}

#[event]
Expand All @@ -127,21 +122,31 @@ mod Space {
fn ProposalExecuted(_proposal_id: u256) {}

#[event]
fn ProposalUpdated(_proposal_id: u256, _execution_stategy: Strategy) {}
fn ProposalUpdated(_proposal_id: u256, _execution_stategy: @Strategy) {}

#[event]
fn ProposalCancelled(_proposal_id: u256) {}

fn VotingStrategiesAdded(_new_voting_strategies: Array<Strategy>) {}
#[event]
fn VotingStrategiesAdded(
_new_voting_strategies: @Array<Strategy>,
_new_voting_strategy_metadata_uris: @Array<Array<felt252>>
) {}

#[event]
fn VotingStrategiesRemoved(_voting_strategy_indices: @Array<u8>) {}

#[event]
fn AuthenticatorsAdded(_new_authenticators: @Array<ContractAddress>) {}

#[event]
fn VotingStrategiesRemoved(_voting_strategy_indices: Array<u8>) {}
fn AuthenticatorsRemoved(_authenticators: @Array<ContractAddress>) {}

#[event]
fn AuthenticatorsAdded(_new_authenticators: Array<ContractAddress>) {}
fn MetadataURIUpdated(_new_metadata_uri: @Array<felt252>) {}

#[event]
fn AuthenticatorsRemoved(_authenticators: Array<ContractAddress>) {}
fn DaoURIUpdated(_new_dao_uri: @Array<felt252>) {}

#[event]
fn MaxVotingDurationUpdated(_new_max_voting_duration: u64) {}
Expand All @@ -150,7 +155,10 @@ mod Space {
fn MinVotingDurationUpdated(_new_min_voting_duration: u64) {}

#[event]
fn ProposalValidationStrategyUpdated(_new_proposal_validation_strategy: Strategy) {}
fn ProposalValidationStrategyUpdated(
_new_proposal_validation_strategy: @Strategy,
_new_proposal_validation_strategy_metadata_URI: @Array<felt252>
) {}

#[event]
fn VotingDelayUpdated(_new_voting_delay: u64) {}
Expand Down Expand Up @@ -184,7 +192,7 @@ mod Space {
// TODO: we use a felt252 for the hash despite felts being discouraged
// a new field would just replace the hash. Might be worth casting to a Uint256 though?
let execution_payload_hash = poseidon::poseidon_hash_span(
execution_strategy.clone().params.span()
execution_strategy.params.span()
);

let proposal = Proposal {
Expand All @@ -198,13 +206,14 @@ mod Space {
finalization_status: FinalizationStatus::Pending(()),
active_voting_strategies: self._active_voting_strategies.read()
};
let snap_proposal = @proposal;

// TODO: Lots of copying, maybe figure out how to pass snapshots to events/storage writers.
self._proposals.write(proposal_id, proposal.clone());
self._proposals.write(proposal_id, proposal);

self._next_proposal_id.write(proposal_id + u256 { low: 1_u128, high: 0_u128 });

ProposalCreated(proposal_id, author, proposal, execution_strategy.params);
ProposalCreated(proposal_id, author, snap_proposal, @execution_strategy.params);
}

fn vote(
Expand Down Expand Up @@ -292,7 +301,7 @@ mod Space {

self._proposals.write(proposal_id, proposal);

ProposalUpdated(proposal_id, execution_strategy);
ProposalUpdated(proposal_id, @execution_strategy);
}

fn cancel_proposal(ref self: ContractState, proposal_id: u256) {
Expand Down Expand Up @@ -356,72 +365,72 @@ mod Space {
self._proposals.read(proposal_id)
}

fn set_max_voting_duration(ref self: ContractState, max_voting_duration: u64) {
fn update_settings(ref self: ContractState, input: UpdateSettingsCalldata) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_set_max_voting_duration(ref self, max_voting_duration);
MaxVotingDurationUpdated(max_voting_duration);
}

fn set_min_voting_duration(ref self: ContractState, min_voting_duration: u64) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_set_min_voting_duration(ref self, min_voting_duration);
MinVotingDurationUpdated(min_voting_duration);
}
// if not NO_UPDATE
if NoUpdateU64::should_update(@input.max_voting_duration) {
_set_max_voting_duration(ref self, input.max_voting_duration);
MaxVotingDurationUpdated(input.max_voting_duration);
}

fn set_voting_delay(ref self: ContractState, voting_delay: u64) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_set_voting_delay(ref self, voting_delay);
VotingDelayUpdated(voting_delay);
}
if NoUpdateU64::should_update(@input.min_voting_duration) {
_set_min_voting_duration(ref self, input.min_voting_duration);
MinVotingDurationUpdated(input.min_voting_duration);
}

fn set_proposal_validation_strategy(
ref self: ContractState, proposal_validation_strategy: Strategy
) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
// TODO: might be possible to remove need to clone by defining the event or setter on a snapshot.
// Similarly for all non value types.
_set_proposal_validation_strategy(ref self, proposal_validation_strategy.clone());
ProposalValidationStrategyUpdated(proposal_validation_strategy);
}
if NoUpdateU64::should_update(@input.voting_delay) {
_set_voting_delay(ref self, input.voting_delay);
VotingDelayUpdated(input.voting_delay);
}

fn add_voting_strategies(ref self: ContractState, voting_strategies: Array<Strategy>) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_add_voting_strategies(ref self, voting_strategies.clone());
VotingStrategiesAdded(voting_strategies);
}
if NoUpdateArray::should_update((@input).metadata_URI) {
MetadataURIUpdated(@input.metadata_URI);
}

fn remove_voting_strategies(ref self: ContractState, voting_strategy_indices: Array<u8>) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_remove_voting_strategies(ref self, voting_strategy_indices.clone());
VotingStrategiesRemoved(voting_strategy_indices);
}
if NoUpdateArray::should_update((@input).dao_URI) {
DaoURIUpdated(@input.dao_URI);
}

fn add_authenticators(ref self: ContractState, authenticators: Array<ContractAddress>) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_add_authenticators(ref self, authenticators.clone());
AuthenticatorsAdded(authenticators);
}
// if not NO_UPDATE
if NoUpdateStrategy::should_update((@input).proposal_validation_strategy) {
// TODO: might be possible to remove need to clone by defining the event or setter on a snapshot.
// Similarly for all non value types.
_set_proposal_validation_strategy(
ref self, input.proposal_validation_strategy.clone()
);
ProposalValidationStrategyUpdated(
@input.proposal_validation_strategy,
@input.proposal_validation_strategy_metadata_URI
);
}

fn remove_authenticators(ref self: ContractState, authenticators: Array<ContractAddress>) {
//TODO: temporary component syntax
let state: Ownable::ContractState = Ownable::unsafe_new_contract_state();
Ownable::assert_only_owner(@state);
_remove_authenticators(ref self, authenticators.clone());
AuthenticatorsRemoved(authenticators);
if NoUpdateArray::should_update((@input).authenticators_to_add) {
_add_authenticators(ref self, input.authenticators_to_add.clone());
AuthenticatorsAdded(@input.authenticators_to_add);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).authenticators_to_remove) {
_remove_authenticators(ref self, input.authenticators_to_remove.clone());
AuthenticatorsRemoved(@input.authenticators_to_remove);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).voting_strategies_to_add) {
_add_voting_strategies(ref self, input.voting_strategies_to_add.clone());
VotingStrategiesAdded(
@input.voting_strategies_to_add, @input.voting_strategies_metadata_URIs_to_add
);
}

// if not NO_UPDATE
if NoUpdateArray::should_update((@input).voting_strategies_to_remove) {
_remove_voting_strategies(ref self, input.voting_strategies_to_remove.clone());
VotingStrategiesRemoved(@input.voting_strategies_to_remove);
}
}

fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) {
Expand Down Expand Up @@ -467,9 +476,9 @@ mod Space {
_voting_delay,
_min_voting_duration,
_max_voting_duration,
_proposal_validation_strategy,
_voting_strategies,
_authenticators
@_proposal_validation_strategy,
@_voting_strategies,
@_authenticators
);
}

Expand Down
14 changes: 9 additions & 5 deletions starknet/src/tests/test_space.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ mod tests {
use sx::voting_strategies::vanilla::VanillaVotingStrategy;
use sx::proposal_validation_strategies::vanilla::VanillaProposalValidationStrategy;
use sx::tests::mocks::proposal_validation_always_fail::AlwaysFailProposalValidationStrategy;
use sx::utils::types::{Strategy, IndexedStrategy, Choice, FinalizationStatus, Proposal};
use sx::utils::types::{
Strategy, IndexedStrategy, Choice, FinalizationStatus, Proposal, UpdateSettingsCalldataImpl
};
use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR};

use Space::Space as SpaceImpl;
Expand Down Expand Up @@ -298,10 +300,12 @@ mod tests {
.unwrap();
testing::set_caller_address(owner);
testing::set_contract_address(owner);
space
.set_proposal_validation_strategy(
Strategy { address: strategy_address, params: ArrayTrait::<felt252>::new() }
);
let mut input = UpdateSettingsCalldataImpl::default();
input.proposal_validation_strategy = Strategy {
address: strategy_address, params: ArrayTrait::<felt252>::new()
};

space.update_settings(input);

let mut propose_calldata = array::ArrayTrait::<felt252>::new();
propose_calldata.append(contract_address_const::<5678>().into());
Expand Down
Loading

0 comments on commit 3bbb8fd

Please sign in to comment.