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.

Sign up for GitHub

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

Jump to bottom

Removes the backwards compatibility of storage for governor config #60

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Removes the backwards compatibility of storage for governor config #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cc94a9
Removes the backwards compatibility of storage for governor config
moodysalem Jul 4, 2024
1863818
improve the comment
moodysalem Jul 5, 2024
51e29a8
Merge branch 'main' into remove-backwards-compat-storage
moodysalem Sep 4, 2024
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
38 changes: 31 additions & 7 deletions src/governor.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ pub trait IGovernor<TContractState> {

// Replaces the code at this address. This must be self-called via a proposal.
fn upgrade(ref self: TContractState, class_hash: ClassHash);

// Migrates to the latest version of storage layout, from the version of storage before v2.1.0
fn _migrate_old_config_storage(ref self: TContractState);
}

#[starknet::contract(account)]
Expand All @@ -97,6 +100,7 @@ pub mod Governor {
use core::poseidon::{PoseidonTrait};
use governance::call_trait::{HashSerializable, CallTrait};
use governance::staker::{IStakerDispatcherTrait};
use starknet::storage_access::{Store, storage_base_address_from_felt252};
use starknet::{
get_block_timestamp, get_caller_address, get_contract_address,
syscalls::{replace_class_syscall}, AccountContract, get_tx_info
Expand Down Expand Up @@ -159,7 +163,6 @@ pub mod Governor {
#[storage]
struct Storage {
staker: IStakerDispatcher,
config: Config,
config_versions: LegacyMap<u64, Config>,
latest_config_version: u64,
nonce: u64,
Expand All @@ -171,7 +174,9 @@ pub mod Governor {
#[constructor]
fn constructor(ref self: ContractState, staker: IStakerDispatcher, config: Config) {
self.staker.write(staker);
self.config.write(config);

self.config_versions.write(0, config);
self.emit(Reconfigured { new_config: config, version: 0 });
}

pub fn get_proposal_id(address: ContractAddress, nonce: u64) -> felt252 {
Expand Down Expand Up @@ -405,11 +410,7 @@ pub mod Governor {
}

fn get_config_version(self: @ContractState, version: u64) -> Config {
Copy link
Contributor

Choose a reason for hiding this comment

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

to rename get_config? or get_config_by_version ?

it returns a Config, not a version, get_config_version is probably not the better name for this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, but slightly concerned about breaking compatibility with any calling contracts, and it's also confusing with get_config_with_version anyway

if version.is_zero() {
self.config.read()
} else {
self.config_versions.read(version)
}
self.config_versions.read(version)
}

fn get_staker(self: @ContractState) -> IStakerDispatcher {
Expand Down Expand Up @@ -447,6 +448,29 @@ pub mod Governor {

replace_class_syscall(class_hash).unwrap();
}

fn _migrate_old_config_storage(ref self: ContractState) {
let old_config_storage_address = storage_base_address_from_felt252(selector!("config"));
let old_config: Config = Store::read(0, old_config_storage_address).unwrap();
// voting period of 0 is assumed to be invalid
assert(old_config.voting_period.is_non_zero(), 'NO_OLD_CONFIG');
self.config_versions.write(0, old_config);
self.emit(Reconfigured { version: 0, new_config: old_config });
Store::write(
0,
old_config_storage_address,
Config {
voting_start_delay: 0,
voting_period: 0,
voting_weight_smoothing_duration: 0,
quorum: 0,
proposal_creation_threshold: 0,
execution_delay: 0,
execution_window: 0
}
)
Copy link
Contributor

@TAdev0 TAdev0 Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

code looks good!
can you explain me what you mean exactly by // Migrates to the fixed storage layout, i.e. the storage layout from before v2.1.0 in the interface?

For me you just removed the config variable in storage, reset all storage slot of the config to 0 using Store trait that replaced StorageAccess in v 2.1.0, and store the current config (and all previous configs associated with previous versions) in config_versions mapping

Copy link
Member Author

@moodysalem moodysalem Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

v2.1.0 is referring to the governor release, not cairo/scarb

Since we added versioned configs in governor v2.1.0, we have a new storage layout for storing configs. But without the migration, we had to keep both storage variables

Having a storage variable for config as well as another variable for versioned configs is less elegant than just having all configs be versioned (and the initial config is version 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

All clear thanks a lot!

.expect('FAILED_TO_DELETE');
}
}

// This implementation exists solely for the purpose of allowing simulation of calls from the governor with the flag to skip validation
Expand Down
5 changes: 5 additions & 0 deletions src/governor_test.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ fn test_describe_proposal_successful() {
id,
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
);
pop_log::<Governor::Reconfigured>(governor.contract_address).unwrap();
pop_log::<Governor::Proposed>(governor.contract_address).unwrap();
assert_eq!(
pop_log::<Governor::Described>(governor.contract_address).unwrap(),
Expand Down Expand Up @@ -320,6 +321,7 @@ fn test_propose_and_describe_successful() {
);
set_contract_address(address_before);

pop_log::<Governor::Reconfigured>(governor.contract_address).unwrap();
pop_log::<Governor::Proposed>(governor.contract_address).unwrap();
assert_eq!(
pop_log::<Governor::Described>(governor.contract_address).unwrap(),
Expand Down Expand Up @@ -892,6 +894,7 @@ fn test_execute_emits_logs_from_data() {
// both transfers suceeded
assert_eq!(result, expected);

pop_log::<Governor::Reconfigured>(governor.contract_address).unwrap();
pop_log::<Governor::Proposed>(governor.contract_address).unwrap();
pop_log::<Governor::Voted>(governor.contract_address).unwrap();
pop_log::<Governor::Voted>(governor.contract_address).unwrap();
Expand Down Expand Up @@ -1111,6 +1114,8 @@ fn test_reconfigure_succeeds_self_call() {
.span()
);

// the first one is from constructor
pop_log::<Governor::Reconfigured>(governor.contract_address).unwrap();
pop_log::<Governor::Proposed>(governor.contract_address).unwrap();
pop_log::<Governor::Voted>(governor.contract_address).unwrap();
let reconfigured = pop_log::<Governor::Reconfigured>(governor.contract_address).unwrap();
Expand Down
Loading