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

Verify key.bin against NodeID in metadata #205

Merged
merged 3 commits into from
Jul 31, 2023

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Jul 31, 2023

Closes #198.

Simple verification of the private key in key.bin against the NodeID in postdata_metadata.json. Will be done when -verify is set.

@fasmat fasmat requested a review from poszu July 31, 2023 04:35
@fasmat fasmat self-assigned this Jul 31, 2023
@fasmat fasmat requested a review from pigmej July 31, 2023 04:35
cmd/postcli/main.go Outdated Show resolved Hide resolved
cmd/postcli/main.go Outdated Show resolved Hide resolved
cmd/postcli/main.go Outdated Show resolved Hide resolved
@fasmat fasmat requested a review from poszu July 31, 2023 08:20
if cmdVerifyPos(opts, fraction, logger) != nil {
os.Exit(1)
}
os.Exit(0)
cmdVerifyPos(opts, fraction, logger)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Previously it was clear that the program terminates after calling cmdVerifyPos(). Now it's hidden and less obvious. How about returning an error and adding a log.Fatal() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the benefit? Errors are not logging messages. I changed the code so it is clear it exits after cmdVerifyPos is called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Errors are not logging messages.

? The point of CLI is to communicate to the user the outcome. What's wrong with communicating by showing them a nice readable error message?

I don't see the benefit?

The benefit is that the next person reading this code immediately understands that the program terminates here.

Copy link
Member Author

@fasmat fasmat Jul 31, 2023

Choose a reason for hiding this comment

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

What's wrong with communicating by showing them a nice readable error message?

I did this by updating the errors with your suggestions and printing them directly? Why first wrap them in an error to return them just to print them?

The benefit is that the next person reading this code immediately understands that the program terminates here.

That's why I added the os.Exit(0) as you suggested?

@fasmat fasmat requested a review from poszu July 31, 2023 09:07
@fasmat fasmat merged commit c66f2dc into develop Jul 31, 2023
@fasmat fasmat deleted the 198-verify-key-against-nodeid branch July 31, 2023 09:34
@xx-134
Copy link

xx-134 commented Jul 31, 2023

Ashampoo_Snap_31 июля 2023 г 15h20m22s_156
What am I doing wrong?

@fasmat
Copy link
Member Author

fasmat commented Jul 31, 2023

@xx-134 thanks for reporting. This was an oversight that will be fixed in this PR: #207

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.

Postcli could verify key.bin vs NodeID in postdata_metatada.json
3 participants