-
Notifications
You must be signed in to change notification settings - Fork 680
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
chain-spec-builder: Add support for codeSubstitutes
#4685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, do we explain anywhere in which clusterfuck situations one needs to even use this? 😂 would be good to maybe post to the postmortem of when we used it at least for some reference.
@@ -0,0 +1,14 @@ | |||
[package] | |||
name = "cumulus-client-chain-spec-extension" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a new crate for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was that we can refer to this crate from small tools like chain-spec-builder without pulling in the whole world of dependencies. Which would be the case if I put it in some other bigger crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be also done with features, one of which could be 'api' exposing necessary types only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe move it to sc_chain_spec::common, as it is typical/common of parachains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4685 (comment) given this, we should revert these changes. Merge the pr with the codeSubstitute
support and add a new one that ignores the unknown fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking of follow up #4873
|
||
/// Chain-spec extension that provides access to optional `relay_chain` and `para_id` fields. | ||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, ChainSpecGroup, ChainSpecExtension)] | ||
pub struct OptionalParaFieldsExtension { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. If you are asking why we need it at all, without any extension relay_chain
and para_id
fields are silently removed from the chain specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should change this to pass through any unknown fields and just modify the stuff we know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, modified this PR to just allow adding substitutes
codeSubstitutes
and unify chain-spec-extensionscodeSubstitutes
and unify chain-spec-extensions
let chain_spec_json = serde_json::to_string_pretty(&chain_spec_json) | ||
.map_err(|e| format!("to pretty failed: {e}"))?; | ||
fs::write(chain_spec_path, chain_spec_json).map_err(|err| err.to_string())?; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to avoid code duplication in UpdateCode
and AddCodeSubstitute
match arms we could add a function update_chain_spec_file_in_place
which could take a closure that modifies &mut chain_spec_json
provided as its input.
/// Add a code substitute in the chain spec. | ||
/// | ||
/// The `codeSubstitute` object of the chain spec will be updated with the block height as key and | ||
/// runtime code as value. This operation supports both plain and raw formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth mentioning why would we want to do this?
/// The block height at which the code should be substituted. | ||
pub block_height: u64, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this does not modify the file in-place. Maybe would be good to add a note about this. (Same case as in updateCode).
codeSubstitutes
and unify chain-spec-extensionscodeSubstitutes
While working on paritytech#4600 I found that it would be nice if `chain-spec-builder` supported `codeSubstitutes`. After this PR is merged you can do: ``` chain-spec-builder add-code-substitute chain_spec.json my_runtime.compact.compressed.wasm 1234 ``` In addition, the `chain-spec-builder` was silently removing `relay_chain` and `para_id` fields when used on parachain chain-specs. This is now fixed by providing a custom chain-spec extension that has these fields marked as optional.
While working on #4600 I found that it would be nice if
chain-spec-builder
supportedcodeSubstitutes
. After this PR is merged you can do:In addition, the
chain-spec-builder
was silently removingrelay_chain
andpara_id
fields when used on parachain chain-specs. This is now fixed by providing a custom chain-spec extension that has these fields marked as optional.