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

Removes the backwards compatibility of storage for governor config #60

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

moodysalem
Copy link
Member

No description provided.

@@ -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

execution_delay: 0,
execution_window: 0
}
)
Copy link
Contributor

@TAdev0 TAdev0 Jul 5, 2024

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

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!

@moodysalem moodysalem marked this pull request as ready for review September 4, 2024 22:11
@moodysalem moodysalem merged commit 51a5123 into main Sep 4, 2024
1 check passed
@moodysalem moodysalem deleted the remove-backwards-compat-storage branch September 4, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants