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

refactor: modify enr error message #493

Merged
merged 5 commits into from
May 6, 2022
Merged

Conversation

xenowits
Copy link
Contributor

@xenowits xenowits commented May 5, 2022

Assist users running charon enr in creating their ENRs. See ticket for more details.
Probably may also need to do more than just a helpful message. Thoughts are welcome!

category: refactor

ticket: #490
feature_set: alpha

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #493 (b4b0e5c) into main (47e89c4) will increase coverage by 0.06%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #493      +/-   ##
==========================================
+ Coverage   55.75%   55.81%   +0.06%     
==========================================
  Files          85       84       -1     
  Lines        7614     7578      -36     
==========================================
- Hits         4245     4230      -15     
+ Misses       2790     2773      -17     
+ Partials      579      575       -4     
Impacted Files Coverage Δ
p2p/k1.go 0.00% <0.00%> (ø)
cmd/enr.go 11.76% <66.66%> (+11.76%) ⬆️
testutil/validatormock/validatormock.go 34.80% <0.00%> (-2.13%) ⬇️
core/validatorapi/validatorapi.go 52.50% <0.00%> (ø)
core/validatorapi/signing.go
testutil/keystore/keystore.go
eth2util/keystore/keystore.go 51.28% <0.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 47e89c4...b4b0e5c. Read the comment docs.

@xenowits xenowits changed the title modified enr error message refactor: modify enr error message May 5, 2022
cmd/enr.go Outdated
if err != nil {
return err
return errors.New(helpMsg)
Copy link
Contributor

@corverroos corverroos May 6, 2022

Choose a reason for hiding this comment

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

You are assuming that this is the only error that can occur, which is not true. Also please use structured errors always.

Suggest

key, err := p2p.LoadP2PKey(dir)
if os.IsNotExist(err) {
  return errors.New("ENR private key not found. Maybe create one with `charon create enr`", z.Str("path", p2p.KeyPath(dataDir))
} else if err != nil {
  return err
} 

Also please add a test for 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.

For os.IsNotExist(err) to work, the error coming from calling p2p.LoadPrivKey should not be wrapped. wrapping changes the error type from *fs.PathError to structured.

But, when i attempt to simply return err, like,

func LoadPrivKey(dataDir string) (*ecdsa.PrivateKey, error) {
	key, err := crypto.LoadECDSA(KeyPath(dataDir))
	if err != nil {
		// return nil, errors.Wrap(err, "load key")
                // But, this throws an error by golangci-lint's wrapcheck linter
                	return nil, err
	}

	return key, nil
}

Copy link
Contributor

@corverroos corverroos left a comment

Choose a reason for hiding this comment

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

Please add test

Copy link
Contributor

@corverroos corverroos left a comment

Choose a reason for hiding this comment

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

LGTM

@xenowits xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label May 6, 2022
@obol-bulldozer obol-bulldozer bot merged commit 419dbe5 into main May 6, 2022
@obol-bulldozer obol-bulldozer bot deleted the xenowits/charon-enr-ux branch May 6, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants