-
Notifications
You must be signed in to change notification settings - Fork 115
Chainspec import members forum versioned store #1190
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
Chainspec import members forum versioned store #1190
Conversation
| wasm-bindgen = { version = "0.2.57", optional = true } | ||
| wasm-bindgen-futures = { version = "0.4.7", optional = true } | ||
| browser-utils = { package = 'substrate-browser-utils', git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4', optional = true} | ||
|
|
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.
Do you mind moving these dependencies to the 'third-party' category?
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.
done in 6e3ebf6
| hex::decode(&self.class[2..].as_bytes()).expect("failed to parse class hex string"); | ||
| let encoded_permissions = hex::decode(&self.permissions[2..].as_bytes()) | ||
| .expect("failed to parse class permissions hex string"); | ||
| ClassAndPermissions { |
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.
Do we need to panic during the initialization? Should we fail on the first error?
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.
Yes I prefer to panic rather than introduce inconsistent state at genesis. Remember this code will only be running at the genesis construction time before any network is launched. So it would NOT affect the runtime if that is your concern?
Should we fail on the first error?
As there are references from some classes to other classes, yes there is no point continuing to import other classes since they will depend on earlier classes in some cases. So its an all or nothing scenario we want at import time.
| self | ||
| } | ||
| pub fn members(mut self, members: Vec<T::AccountId>) -> Self { | ||
| pub fn members(mut self, members: Vec<(T::MemberId, T::AccountId)>) -> Self { |
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.
The public method needs a comment.
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.
updated in 14abbc4
| Some(member.handle.clone().into_bytes()), | ||
| Some(member.avatar_uri.clone().into_bytes()), | ||
| Some(member.about.clone().into_bytes()) | ||
| ).expect("Importing Member Failed"); |
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.
We should avoid panicking in the runtime code. Last time we changed panicking to the printing error message.
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.
This code in the build(|config: &GenesisConfig<T>| { }) block isn't runtime code. It is only native code that runs when building the genesis chain-spec. So it is safe to panic.
| edition = '2018' | ||
|
|
||
| [dependencies] | ||
| serde = { version = "1.0.101", features = ["derive"] } |
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.
Add this line 'serde', to the std feature.
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.
done in a498fef
| let user_info = Self::check_user_registration_info(handle, avatar_uri, about)?; | ||
|
|
||
| // ensure handle is not already registered | ||
| Self::ensure_unique_handle(&user_info.handle)?; |
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.
Do we need this check after copying inside the insert_member?
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.
quite right, I forgot to remove it after adding the check in insert_member
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.
will also move the slashing to come after insert_member
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.
done in 2957b71
| let user_info = Self::check_user_registration_info(handle, avatar_uri, about)?; | ||
|
|
||
| // ensure handle is not already registered | ||
| Self::ensure_unique_handle(&user_info.handle)?; |
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.
Do we need this check after copying inside the insert_member?
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.
quite right, I forgot to remove it after adding the check in insert_member
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.
done in 2957b71
| @@ -0,0 +1,11 @@ | |||
| use node_runtime::{membership, AccountId, Moment}; | |||
| use std::{fs, path::Path}; | |||
|
|
|||
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.
Please add the comments for the public methods.
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.
done in 3a01b78
| @@ -0,0 +1,145 @@ | |||
| use codec::Decode; | |||
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.
Please add the comments for the public methods.
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.
done in 3a01b78
| @@ -0,0 +1,375 @@ | |||
| use codec::Decode; | |||
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.
Please add the comments for the public methods.
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.
done in 3a01b78
shamil-gadelshin
left a comment
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
Update chain-spec-builder to import initial members, forum and versioned store at genesis.
Uses exported state produced by scripts in Joystream/joystream-api-examples#12
The path to the exported state as .json files can be passed as arguments to the chainspec builder:
To test (after
cargo build --release)This will produce a
chain-spec.jsonfile. To run a chain with this configuration: