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

cmd: add gen-p2pkey command #316

Merged
merged 1 commit into from
Mar 30, 2022
Merged

cmd: add gen-p2pkey command #316

merged 1 commit into from
Mar 30, 2022

Conversation

corverroos
Copy link
Contributor

Makes loading vs creating p2pkeys explicit both when running charon and as commands.

category: feature
ticket: #315

@@ -29,7 +29,7 @@ jobs:
tags: "${{ steps.get-version.outputs.version }}"

- name: Generate cli reference
run: docker run ghcr.io/obolnetwork/charon/charon:${{steps.get-version.outputs.version}} charon --help > cli-reference.txt
run: docker run ghcr.io/obolnetwork/charon/charon:${{steps.get-version.outputs.version}} charon run --help > cli-reference.txt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related, but I saw that cli-reference created for the release is incorrect.

@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@f24bf55). Click here to learn what that means.
The diff coverage is 59.61%.

❗ Current head 6cdcd5f differs from pull request most recent head 652fd27. Consider uploading reports for the commit 652fd27 to get more accurate results

@@           Coverage Diff           @@
##             main     #316   +/-   ##
=======================================
  Coverage        ?   55.12%           
=======================================
  Files           ?       59           
  Lines           ?     4747           
  Branches        ?        0           
=======================================
  Hits            ?     2617           
  Misses          ?     1779           
  Partials        ?      351           
Impacted Files Coverage Δ
cmd/cmd.go 73.01% <0.00%> (ø)
cmd/enr.go 0.00% <0.00%> (ø)
cmd/version.go 44.11% <ø> (ø)
p2p/k1.go 0.00% <0.00%> (ø)
app/app.go 63.21% <60.00%> (ø)
cmd/genp2p.go 81.81% <81.81%> (ø)
cmd/gensimnet.go 51.16% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f24bf55...652fd27. Read the comment docs.

dir := "testdata/simnet"
require.NoError(t, os.RemoveAll(dir))
err := os.MkdirAll(dir, 0o755)
dir, err := os.MkdirTemp("", "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing this test that was flapping

Copy link
Contributor

@dB2510 dB2510 left a comment

Choose a reason for hiding this comment

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

LGTM!

cmd/cmd.go Outdated
@@ -41,6 +41,7 @@ func New() *cobra.Command {
return newRootCmd(
newVersionCmd(runVersionCmd),
newEnrCmd(runNewENR),
newGenP2PCmd(runGenP2P),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we name it to newGenP2PKeyCmd to be more specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

cmd/genp2p.go Outdated
"github.com/obolnetwork/charon/p2p"
)

func newGenP2PCmd(runFunc func(io.Writer, p2p.Config, string) error) *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: name as newGenP2PKeyCmd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

cmd/genp2p.go Outdated
}

// runGenP2P stores a new p2pkey to disk and prints the ENR for the provided config.
func runGenP2P(w io.Writer, config p2p.Config, dataDir string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: runGenP2PKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

func newSavedPrivKey(keyPath string) (*ecdsa.PrivateKey, error) {
if err := os.MkdirAll(filepath.Dir(keyPath), 0o755); err != nil {
// NewSavedPrivKey generates a new ecdsa k1 key and saves it to the directory.
func NewSavedPrivKey(datadir string) (*ecdsa.PrivateKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe name as NewP2PKey or NewPrivKey ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewP2PKey would stutter since the package already p2p. and NewPrivKey doesn't indicate that the key is also saved to disk.

func runNewENR(w io.Writer, config p2p.Config, dataDir string) error {
identityKey, loaded, err := p2p.LoadOrCreatePrivKey(dataDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

#893 was fixed here

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.

3 participants