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

Add verification steps to README #607

Closed
wants to merge 1 commit into from
Closed

Add verification steps to README #607

wants to merge 1 commit into from

Conversation

handshake-enthusiast
Copy link
Contributor

@handshake-enthusiast
Copy link
Contributor Author

bob-2.0.0-arm64.dmg wasn't signed BTW.

Copy link
Collaborator

@rithvikvibhu rithvikvibhu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Yeah even if many won't verify (:disappointed:), having a section may persuade a few more to do so.

README.md Outdated
@@ -24,6 +24,25 @@ For macOS users, Bob is also available through the [Homebrew](https://github.com
brew install kyokan-bob
```

## How to Verify Bob Wallet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## How to Verify Bob Wallet
## Verify downloaded binaries

or similar to match other headings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous section's title is "How to Install Bob Wallet". I tried to match it. Technically "downloaded binaries" is obviously more precise. Also I tried to match the previous title's "camel case". Probably ### Verify downloaded binaries would be better, we could nest it inside "How to Install Bob Wallet". Let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hah you're right, the first heading also doesn't match the others, maybe that can change too (Install or Installation or something).
Yeah makes sense to have verification under install.

## How to Verify Bob Wallet

1. Download a _SHA256SUMS.asc_ file included into the release
2. Paste the file's content into https://keybase.io/verify and click "Verify"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea here was to keep it as short as possible? It makes sense. I'm just thinking if we can say what to expect after clicking "verify". And do they compare the signer's username somewhere? Maybe we should add a SECURITY.md like what hsd does and mention it here: https://github.com/handshake-org/hsd/blob/master/docs/install.md

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the idea here was to keep it as short as possible?

I wanted to contribute with this, a more concise guide enabled me to make it iteratively and submit this PR. A more comprehensive guide and a list of PGP keys would be better. Ok, I can add SECURITY.md and link it here. A final goal is to make a clear guide on how to verify the release binaries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The SECURITY file looks great, even linked in the readme! Everything looks good to me, just a nit: order the signers list in alphabetical order (swap the 2 lines in both places)? And squash if you want to after this, then we can merge this :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

@handshake-enthusiast
Copy link
Contributor Author

having a section may persuade a few more to do so.

Indeed. I noticed the SHA256SUMS.asc file and it wasn't straightforward for me how to use it. I'm a semi power user here, not experienced with this stuff, but was able to understand it could be used.

@handshake-enthusiast
Copy link
Contributor Author

@rithvikvibhu the PR should be ready for another review round. Once you approve I'll squash the commits into one.

@handshake-enthusiast handshake-enthusiast closed this by deleting the head repository Feb 19, 2023
@handshake-enthusiast
Copy link
Contributor Author

Sorry, I used a wrong user and had to delete the repo. Will redo later when not in a hurry.

@handshake-enthusiast
Copy link
Contributor Author

Unfortunately, after deleting my original fork I can't update this PR, so I had to create a new one. See #612. It wasn't my cleanest commit history... A good lesson not to do things in a hurry.

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

2 participants