Merge Subkey into sc-cli by seunlanlege · Pull Request #4954 · paritytech/substrate

seunlanlege · 2020-02-17T16:24:45Z

see #4416

parity-cla-bot · 2020-02-17T16:24:50Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

seunlanlege · 2020-02-19T12:40:18Z

blocked on #4842

joepetrowski · 2020-02-22T19:09:48Z

What is the motivation for this change? Would be nice to have a write up either here or in an issue.

NikVolf · 2020-02-22T20:27:23Z

What is the motivation for this change? Would be nice to have a write up either here or in an issue.

#4416

cecton · 2020-02-24T13:50:50Z

#4842 has been merged. Updating this PR is going to be fun 😁 call me if you need any help @seunlanlege

…-cli-subkey

…substrate into seun-sc-cli-subkey

client/cli/src/arg_enums.rs

cecton

Some small changes

client/cli/src/commands/runcmd.rs

client/cli/src/arg_enums.rs

…-cli-subkey

client/cli/src/commands/utils.rs

bin/node-template/node/src/cli.rs

bkchr

So, I now complained 2 times (this being the third time) that the SharedParams is added by all the cli commands. I understand that this is a current requirement of the API, however this needs to be changed if this pr should be merged.

The following command makes no sense:

cargo run -p node-cli -- verify --dev SIGNATURE

And there are also other cli commands that follow this schema.

A CLI command should never require arguments that are useless for the given command.

bkchr · 2020-08-10T19:16:17Z

client/cli/src/commands/insert.rs

+			.map_err(|e| Error::Other(format!("{:?}", e)))?;
+
+		Ok(())


Suggested change

.map_err(|e| Error::Other(format!("{:?}", e)))?;

Ok(())

.map_err(|e| Error::Other(format!("{:?}", e)).into())

bkchr · 2020-08-10T20:19:06Z

.maintain/prometheus.yml

@@ -0,0 +1,5 @@
+scrape_configs:


What is this?

seunlanlege · 2020-08-10T22:34:21Z

SharedParams is added by all the CLI commands.

Heyyyyy sorry about this, my strategy for removing unused-SharedParams is by removing the CliConfiguration implementation, it somehow skipped my mind to do the same for sign, verify and vanity.

test

bin/node/cli/Cargo.toml

client/cli/src/commands/verify.rs

utils/frame/frame-utilities-cli/src/module_id.rs

…-cli-subkey

…line to Cargo.toml

…-cli-subkey

rakanalh · 2020-08-14T12:41:13Z

client/cli/src/commands/inspect_node_key.rs

+
+use crate::{Error, NetworkSchemeFlag};
+use std::fs;
+use libp2p::identity::{PublicKey, ed25519};


I am curious as to why we're using ed25519 specific types from libp2p.

rakanalh · 2020-08-14T12:43:52Z

client/cli/src/commands/inspect_node_key.rs

+	pub fn run(&self) -> Result<(), Error> {
+		let mut file_content = hex::decode(fs::read(&self.file)?)
+			.map_err(|_| "failed to decode secret as hex")?;
+		let secret = ed25519::SecretKey::from_bytes(&mut file_content)


Is this command only usable for ed25519? would it make sense to generalize this so that i can also get the public key from an sr25519 file for example? I saw the CryptoScheme earlier and i think it can be used here to receive an additional argument for the crypto scheme.

I think for now, we only support ed25519, see

substrate/client/cli/src/arg_enums.rs

Lines 80 to 86 in 0079140

arg_enum! {

#[allow(missing_docs)]

#[derive(Debug, Copy, Clone, PartialEq, Eq)]

pub enum NodeKeyType {

Ed25519

}

}

rakanalh · 2020-08-14T12:47:25Z

client/cli/src/commands/tests.rs

+use super::{SignCmd, VanityCmd};
+use structopt::StructOpt;
+
+#[test]


Can you take each of these 2 tests into their respective command modules?

rakanalh · 2020-08-14T12:47:51Z

client/cli/src/commands/tests.rs

+	assert!(sign.run().is_ok());
+}
+
+#[test]


Same as above

rakanalh

LGTM

…-cli-subkey

gnunicorn · 2020-08-20T08:23:32Z

@seunlanlege test is failing and it seems polkadot pr needs updating.

gnunicorn · 2020-08-20T09:54:58Z

bot merge

ghost · 2020-08-20T09:55:02Z

Trying merge.

draft

73b899b

seunlanlege added A3-in_progress Pull request is in progress. No review needed at this stage. B1-clientnoteworthy labels Feb 17, 2020

seunlanlege mentioned this pull request Feb 18, 2020

Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

Merged

seunlanlege added 4 commits March 4, 2020 15:00

Merge branch 'master' of github.com:paritytech/substrate into seun-sc…

3c6d4fd

…-cli-subkey

revert

742df00

WIP

f118c3a

Merge branch 'seun-merge-sc-cli-and-subkey' of github.com:paritytech/…

0490b61

…substrate into seun-sc-cli-subkey

cecton reviewed Mar 11, 2020

View reviewed changes

client/cli/src/arg_enums.rs Outdated Show resolved Hide resolved

all that remains is tests

0a1df9c

cecton suggested changes Mar 11, 2020

View reviewed changes

client/cli/src/commands/runcmd.rs Outdated Show resolved Hide resolved

client/cli/src/arg_enums.rs Outdated Show resolved Hide resolved

seunlanlege added 2 commits March 13, 2020 08:45

Merge branch 'master' of github.com:paritytech/substrate into seun-sc…

fa92dfa

…-cli-subkey

update Cargo.lock

386cfcf

paritytech-ci added the B2-breaksapi label Mar 13, 2020

seunlanlege added 2 commits March 13, 2020 17:38

tests WIP

fccf7ba

WIP refactor node-template-runtime and node-runtime

26e21da

cecton reviewed Mar 17, 2020

View reviewed changes

client/cli/src/commands/utils.rs Outdated Show resolved Hide resolved

seunlanlege added 3 commits March 17, 2020 13:33

implments sc_cli::RuntimeAdapter for node_template_runtime::Runtime

aea449b

final draft

f96d007

fix update_config for subcommands

ddbf1a8

seunlanlege marked this pull request as ready for review March 18, 2020 07:40

seunlanlege added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 18, 2020

seunlanlege commented Mar 18, 2020

View reviewed changes

bin/node-template/node/src/cli.rs Outdated Show resolved Hide resolved

seunlanlege commented Mar 18, 2020

View reviewed changes

bin/node-template/node/src/cli.rs Outdated Show resolved Hide resolved

github-actions bot removed the A7-needspolkadotpr label Aug 10, 2020

bkchr previously requested changes Aug 10, 2020

View reviewed changes

remove SharedParams from Sign, Vanity, Verify

cd0f083

gnunicorn requested a review from rakanalh August 11, 2020 06:52

expenses reviewed Aug 11, 2020

View reviewed changes

bin/node/cli/Cargo.toml Show resolved Hide resolved

bkchr reviewed Aug 11, 2020

View reviewed changes

client/cli/src/commands/verify.rs Outdated Show resolved Hide resolved

utils/frame/frame-utilities-cli/src/module_id.rs Outdated Show resolved Hide resolved

seunlanlege added 6 commits August 11, 2020 11:34

Merge branch 'master' of github.com:paritytech/substrate into seun-sc…

153c346

…-cli-subkey

remove SharedParams from ModuleIdCmd, remove expect from Verify, new …

4dc44e7

…line to Cargo.toml

remove SharedParams from InsertCmd

3bb63c0

🤦🏾‍♂️

30b49fd

Merge branch 'master' of github.com:paritytech/substrate into seun-sc…

9598c0b

…-cli-subkey

deleted prometheus.yml

f145c8e

rakanalh reviewed Aug 14, 2020

View reviewed changes

move a few things around

94511ab

rakanalh approved these changes Aug 17, 2020

View reviewed changes

Merge branch 'master' of github.com:paritytech/substrate into seun-sc…

b342028

…-cli-subkey

github-actions bot added the A7-needspolkadotpr label Aug 17, 2020

fix vanity test

0a50728

github-actions bot removed the A7-needspolkadotpr label Aug 20, 2020

ghost merged commit 6e098a1 into master Aug 20, 2020

ghost deleted the seun-sc-cli-subkey branch August 20, 2020 09:55

gnunicorn mentioned this pull request Aug 20, 2020

Merge subkey functionality into general cli #4416

Closed

mxinden mentioned this pull request Aug 20, 2020

*: Update to next libp2p version 0.24.0 #6891

Merged

tomaka mentioned this pull request Sep 11, 2020

The peer id generated by the terminal is different from the peer id generated by the subkey #7045

Open

kianenigma mentioned this pull request Sep 22, 2020

can't generate polkadot vanity address by subkey #7177

Closed

This pull request was closed.

	arg_enum! {
	#[allow(missing_docs)]
	#[derive(Debug, Copy, Clone, PartialEq, Eq)]
	pub enum NodeKeyType {
	Ed25519
	}
	}

Conversation

seunlanlege commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Feb 17, 2020

Uh oh!

seunlanlege commented Feb 19, 2020

Uh oh!

joepetrowski commented Feb 22, 2020

Uh oh!

NikVolf commented Feb 22, 2020

Uh oh!

cecton commented Feb 24, 2020

Uh oh!

Uh oh!

cecton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

seunlanlege commented Aug 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rakanalh Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

rakanalh Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

seunlanlege Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

rakanalh Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

rakanalh Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

rakanalh left a comment

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Aug 20, 2020

Uh oh!

gnunicorn commented Aug 20, 2020

Uh oh!

ghost commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

seunlanlege commented Feb 17, 2020 •

edited

Loading