From 3bbb8fde4eec81b8fbd18f3adb47695030f51b68 Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Tue, 25 Jul 2023 14:28:48 +0200 Subject: [PATCH] Refactor settings setters; use snapshots for events (#456) * 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 --- ethereum/remappings.txt | 3 +- .../StarknetSpaceManager.sol | 2 +- starknet/src/space/space.cairo | 179 +++++++++--------- starknet/src/tests/test_space.cairo | 14 +- starknet/src/utils/types.cairo | 127 ++++++++++++- 5 files changed, 229 insertions(+), 96 deletions(-) diff --git a/ethereum/remappings.txt b/ethereum/remappings.txt index ca7495c7..a5e84021 100644 --- a/ethereum/remappings.txt +++ b/ethereum/remappings.txt @@ -1 +1,2 @@ -@gnosis.pm/safe-contracts=lib/safe-contracts \ No newline at end of file +@gnosis.pm/safe-contracts=lib/safe-contracts +@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts \ No newline at end of file diff --git a/ethereum/src/execution-strategies/StarknetSpaceManager.sol b/ethereum/src/execution-strategies/StarknetSpaceManager.sol index af5a99f6..425ad47d 100644 --- a/ethereum/src/execution-strategies/StarknetSpaceManager.sol +++ b/ethereum/src/execution-strategies/StarknetSpaceManager.sol @@ -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 diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index a49995c5..45062bfb 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -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 { @@ -24,16 +24,7 @@ trait ISpace { // 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); - fn remove_voting_strategies(ref self: TContractState, voting_strategy_indices: Array); - fn add_authenticators(ref self: TContractState, authenticators: Array); - fn remove_authenticators(ref self: TContractState, authenticators: Array); + fn update_settings(ref self: TContractState, input: UpdateSettingsCalldata); fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress); fn renounce_ownership(ref self: TContractState); // Actions @@ -79,7 +70,8 @@ mod Space { use sx::utils::{ types::{ Choice, FinalizationStatus, Strategy, IndexedStrategy, Proposal, IndexedStrategyTrait, - IndexedStrategyImpl + IndexedStrategyImpl, UpdateSettingsCalldata, NoUpdateU64, NoUpdateStrategy, + NoUpdateArray }, bits::BitSetter }; @@ -108,14 +100,17 @@ mod Space { _voting_delay: u64, _min_voting_duration: u64, _max_voting_duration: u64, - _proposal_validation_strategy: Strategy, - _voting_strategies: Array, - _authenticators: Array + _proposal_validation_strategy: @Strategy, + _voting_strategies: @Array, + _authenticators: @Array ) {} #[event] fn ProposalCreated( - _proposal_id: u256, _author: ContractAddress, _proposal: Proposal, _payload: Array + _proposal_id: u256, + _author: ContractAddress, + _proposal: @Proposal, + _payload: @Array ) {} #[event] @@ -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) {} + #[event] + fn VotingStrategiesAdded( + _new_voting_strategies: @Array, + _new_voting_strategy_metadata_uris: @Array> + ) {} + + #[event] + fn VotingStrategiesRemoved(_voting_strategy_indices: @Array) {} + + #[event] + fn AuthenticatorsAdded(_new_authenticators: @Array) {} #[event] - fn VotingStrategiesRemoved(_voting_strategy_indices: Array) {} + fn AuthenticatorsRemoved(_authenticators: @Array) {} #[event] - fn AuthenticatorsAdded(_new_authenticators: Array) {} + fn MetadataURIUpdated(_new_metadata_uri: @Array) {} #[event] - fn AuthenticatorsRemoved(_authenticators: Array) {} + fn DaoURIUpdated(_new_dao_uri: @Array) {} #[event] fn MaxVotingDurationUpdated(_new_max_voting_duration: u64) {} @@ -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 + ) {} #[event] fn VotingDelayUpdated(_new_voting_delay: u64) {} @@ -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 { @@ -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( @@ -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) { @@ -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) { - //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) { - //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) { - //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) { - //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) { @@ -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 ); } diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 230e3f94..32714dda 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -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; @@ -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::::new() } - ); + let mut input = UpdateSettingsCalldataImpl::default(); + input.proposal_validation_strategy = Strategy { + address: strategy_address, params: ArrayTrait::::new() + }; + + space.update_settings(input); let mut propose_calldata = array::ArrayTrait::::new(); propose_calldata.append(contract_address_const::<5678>().into()); diff --git a/starknet/src/utils/types.cairo b/starknet/src/utils/types.cairo index 114425d0..2c2d5fe7 100644 --- a/starknet/src/utils/types.cairo +++ b/starknet/src/utils/types.cairo @@ -7,12 +7,13 @@ use option::OptionTrait; use clone::Clone; use integer::{U8IntoU128}; use starknet::{ - ContractAddress, StorageAccess, StorageBaseAddress, SyscallResult, storage_write_syscall, - storage_read_syscall, storage_address_from_base_and_offset, storage_base_address_from_felt252, - contract_address::Felt252TryIntoContractAddress, syscalls::deploy_syscall, - class_hash::Felt252TryIntoClassHash + ContractAddress, contract_address_const, StorageAccess, StorageBaseAddress, SyscallResult, + storage_write_syscall, storage_read_syscall, storage_address_from_base_and_offset, + storage_base_address_from_felt252, contract_address::Felt252TryIntoContractAddress, + syscalls::deploy_syscall, class_hash::Felt252TryIntoClassHash }; use sx::utils::math::pow; +use array::SpanTrait; impl Felt252ArrayIntoU256Array of Into, Array> { fn into(self: Array) -> Array { @@ -534,3 +535,121 @@ impl IndexedStrategyImpl of IndexedStrategyTrait { }; } } + +// TODO: move to u32 +#[derive(Clone, Drop, Serde)] +struct UpdateSettingsCalldata { + min_voting_duration: u64, + max_voting_duration: u64, + voting_delay: u64, + metadata_URI: Array, + dao_URI: Array, + proposal_validation_strategy: Strategy, + proposal_validation_strategy_metadata_URI: Array, + authenticators_to_add: Array, + authenticators_to_remove: Array, + voting_strategies_to_add: Array, + voting_strategies_metadata_URIs_to_add: Array>, + voting_strategies_to_remove: Array, +} + +trait UpdateSettingsCalldataTrait { + fn default() -> UpdateSettingsCalldata; +} + +// Theoretically could derive a value with a proc_macro, +// since NO_UPDATE values are simply the first x bytes of a hash. +trait NoUpdateTrait { + fn no_update() -> T; + fn should_update(self: @T) -> bool; +} + +// Obtained by keccak256 hashing the string "No update", and then taking the corresponding number of bytes. +// Evaluates to: 0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048ba + +impl NoUpdateU32 of NoUpdateTrait { + fn no_update() -> u32 { + 0xf2cda9b1 + } + + fn should_update(self: @u32) -> bool { + *self != 0xf2cda9b1 + } +} + +impl NoUpdateU64 of NoUpdateTrait { + fn no_update() -> u64 { + 0xf2cda9b13ed04e58 + } + + fn should_update(self: @u64) -> bool { + *self != 0xf2cda9b13ed04e58 + } +} + +impl NoUpdateFelt252 of NoUpdateTrait { + fn no_update() -> felt252 { + // First 248 bits + 0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048 + } + + fn should_update(self: @felt252) -> bool { + *self != 0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048 + } +} + +impl NoUpdateContractAddress of NoUpdateTrait { + fn no_update() -> ContractAddress { + // First 248 bits + contract_address_const::<0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048>() + } + + fn should_update(self: @ContractAddress) -> bool { + *self != contract_address_const::<0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048>() + } +} + +impl NoUpdateStrategy of NoUpdateTrait { + fn no_update() -> Strategy { + Strategy { + address: contract_address_const::<0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048>(), + params: array::ArrayTrait::new(), + } + } + + fn should_update(self: @Strategy) -> bool { + *self + .address != contract_address_const::<0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048>() + } +} + +// TODO: find a way for "Strings" +impl NoUpdateArray of NoUpdateTrait> { + fn no_update() -> Array { + array::ArrayTrait::::new() + } + + fn should_update(self: @Array) -> bool { + self.len() != 0 + } +} + + +impl UpdateSettingsCalldataImpl of UpdateSettingsCalldataTrait { + fn default() -> UpdateSettingsCalldata { + UpdateSettingsCalldata { + min_voting_duration: NoUpdateU64::no_update(), + max_voting_duration: NoUpdateU64::no_update(), + voting_delay: NoUpdateU64::no_update(), + metadata_URI: NoUpdateArray::no_update(), + dao_URI: NoUpdateArray::no_update(), + proposal_validation_strategy: NoUpdateStrategy::no_update(), + proposal_validation_strategy_metadata_URI: NoUpdateArray::no_update(), + authenticators_to_add: NoUpdateArray::no_update(), + authenticators_to_remove: NoUpdateArray::no_update(), + voting_strategies_to_add: NoUpdateArray::no_update(), + voting_strategies_metadata_URIs_to_add: NoUpdateArray::no_update(), + voting_strategies_to_remove: NoUpdateArray::no_update(), + } + } +}