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

dockerize node in preparation for release #12

Merged
merged 51 commits into from
Jun 20, 2023
Merged

dockerize node in preparation for release #12

merged 51 commits into from
Jun 20, 2023

Conversation

fbielejec
Copy link
Contributor

@fbielejec fbielejec commented Jun 13, 2023

  • dockerized node
  • custom chainspec to cli for it
  • chain can be run with explicit opt-in instant-seal argument
  • basic e2e setup for CI

@fbielejec fbielejec changed the base branch from main to add-pallet-contracts June 13, 2023 13:46
Copy link
Collaborator

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

I've taken a quick look at this and it looks good. @notlesh could please also take a look?

docker/docker_entrypoint.sh Outdated Show resolved Hide resolved
Comment on lines +29 to +52
#[derive(Debug, clap::Parser)]
pub struct BuildSpecCmd {
#[clap(flatten)]
pub base: sc_cli::BuildSpecCmd,

/// Chain name.
#[arg(long, default_value = "Academy PoW")]
pub chain_name: String,

/// Chain ID is a short identifier of the chain
#[arg(long, value_name = "ID", default_value = "academy_pow")]
pub chain_id: String,

/// AccountIds of the optional rich accounts
#[arg(long, value_delimiter = ',', value_parser = parse_account_id, num_args=1..)]
pub endowed_accounts: Option<Vec<AccountId>>,

/// The type of the chain. Possible values: "dev", "local", "live" (default)
#[arg(long, value_name = "TYPE", value_parser = parse_chaintype, default_value = "live")]
pub chain_type: ChainType,

#[arg(long, default_value = "4_000_000")]
pub initial_difficulty: u32,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool idea! I never thought of this.

@@ -151,7 +174,12 @@ pub fn run() -> sc_cli::Result<()> {
.run
.sr25519_public_key
.unwrap_or_else(|| sp_core::sr25519::Public::from_raw([0; 32]));
let instant_seal = cli.run.base.is_dev()?;

let instant_seal = cli.run.base.is_dev()? || cli.run.instant_seal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that you've added a proper flag, we might wat to remove the original --dev check. OTOH, I guess most people using dev would prefer instant seal. So actually, I have no opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just wanted to stay backwards compatible - but same here, not a fan of implicit settings - so removing it.

Comment on lines +63 to 66
/// Generate AccountId based on string command line argument.
fn parse_account_id(s: &str) -> Result<AccountId, String> {
Ok(AccountId::from_string(s).expect("Passed string is not a hex encoding of a public key"))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already a function for this immediately below. If this new one is better please remove the old one. See also #6 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

#6 can of course be a follow up, just wanted you to know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to take #6 but I'd rather not overload this large PR any more

@fbielejec fbielejec merged commit 11c5b40 into main Jun 20, 2023
4 checks passed
@fbielejec fbielejec deleted the dockerize-node branch June 20, 2023 09:59

/// AccountIds of the optional rich accounts
#[arg(long, value_delimiter = ',', value_parser = parse_account_id, num_args=1..)]
pub endowed_accounts: Option<Vec<AccountId>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make this a mapping of account_id -> balance?

@notlesh
Copy link
Contributor

notlesh commented Jun 20, 2023

I've taken a quick look at this and it looks good. @notlesh could please also take a look?

LGTM, but I'm not much of a Docker expert :)

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.

None yet

4 participants