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

Apply updated configuration to Zeitgeist runtime #749

Merged
merged 12 commits into from
Aug 12, 2022
Merged

Conversation

sea212
Copy link
Member

@sea212 sea212 commented Aug 11, 2022

Closes #595

This PR essentially applies the configuration as discussed in issue #595 . To be able to configure different values for the Zeitgeist pallet within the different runtimes, it was necessary to move the parameters from the primitives crate into the runtime-* crate. Since all mocks relied on the constants defined in the primitives crate, the primitives::constants module was extended by a mock module which contains all those constants.

The configuration values for the parachain-staking pallet were not applied. This is due to the fact that the implications of changing them are unknown yet. We need more time to figure out what the implications are and which migrations we'll have to execute. Issue #751 reflects that.

In regards to implications, changing the bonds for the prediction markets pallet could lead to problems, but since no prediction markets exist on the mainnet before we will apply those changes, we don't have to worry about this.

@sea212 sea212 added the s:in-progress The pull requests is currently being worked on label Aug 11, 2022
@sea212 sea212 added this to the v0.3.5 milestone Aug 11, 2022
@sea212 sea212 self-assigned this Aug 11, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews s:in-progress The pull requests is currently being worked on and removed s:in-progress The pull requests is currently being worked on s:review-needed The pull request requires reviews labels Aug 11, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews s:in-progress The pull requests is currently being worked on and removed s:in-progress The pull requests is currently being worked on s:review-needed The pull request requires reviews labels Aug 11, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 11, 2022
Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

Looks fine. 👍

I wonder if we should organize this a little differently, though. Maybe I'm just not used to this setup, but it starts seeming quite complex. Maybe we should accept a certain degree of code duplication in favor of some clearner imports/cargo.toml files.

(I don't have a clean plan for this, just thinking on the spot)

@@ -2150,6 +2148,12 @@ mod pallet {
Ok(())
}

/// The reserve ID of the court pallet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The reserve ID of the court pallet.
/// The reserve ID of the prediction-markets pallet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mitigated in 587e90e

@sea212 sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Aug 11, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 11, 2022
@sea212 sea212 requested a review from Chralt98 August 12, 2022 10:23
@sea212 sea212 requested a review from vivekvpandya August 12, 2022 10:23
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

👍 Love the comments for the runtime constants.

@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Aug 12, 2022
@sea212
Copy link
Member Author

sea212 commented Aug 12, 2022

I wonder if we should organize this a little differently, though. Maybe I'm just not used to this setup, but it starts seeming quite complex. Maybe we should accept a certain degree of code duplication in favor of some clearner imports/cargo.toml files.

This is definitely a drawback that's not nice at all. We copy the code from the common crate to the runtime crates, but the imports are required in the runtime crates. One does not see directly why those imports are necessary. I am open to change it, I'd label it low priority though since for now it works and only minor changes are necessary to add new stuff.

@sea212
Copy link
Member Author

sea212 commented Aug 12, 2022

I wonder if we should organize this a little differently, though. Maybe I'm just not used to this setup, but it starts seeming quite complex. Maybe we should accept a certain degree of code duplication in favor of some clearner imports/cargo.toml files.

This is definitely a drawback that's not nice at all. We copy the code from the common crate to the runtime crates, but the imports are required in the runtime crates. One does not see directly why those imports are necessary. I am open to change it, I'd label it low priority though since for now it works and only minor changes are necessary to add new stuff.

Let's keep it like it is for a couple of weeks and see how we feel about that after that time.

@sea212 sea212 merged commit 8788f42 into main Aug 12, 2022
@sea212 sea212 deleted the sea212-update-config branch August 12, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure runtime constants
3 participants