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

Dice doc #403

Merged
merged 19 commits into from
Aug 4, 2023
Merged

Dice doc #403

merged 19 commits into from
Aug 4, 2023

Conversation

jahangir13
Copy link
Contributor

@jahangir13 jahangir13 commented Jul 16, 2023

My first Pull Request ever^^
New documentation for the dice_verification.md.

New dice_verification documentation using new screenshots and including Sparrow, Ian Coleman BIP39 tool and Seed Tool for verification.
I did not see/understand how to verify the change addresses. This adds this now.
added Sparrow zpub comparison
docs/dice_verification.md Outdated Show resolved Hide resolved
@jdlcdl
Copy link

jdlcdl commented Jul 17, 2023

Nice work @jahangir13!

After a first pass review, really nice textual corrections and additions, and updates of images.

This deserves fresh eyes and another pass, I will get back to this soon.

docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
@Marc-Gee
Copy link
Contributor

Great work @jahangir13 !
Some suggestions by me are above !

- new introduction
- changes from review comments
- more consistent headings compared to other .md documents
- more spacing between the topics
- no use of the lists (1., 2., 3.) anymore to allow more spacing here
- add a chapter with example values for doing the same tutorial with 50 dice rolls / 12 words seed
- added Sparrow Wallet URL
- entered conclusion at the end
@jahangir13
Copy link
Contributor Author

jahangir13 commented Jul 19, 2023

So I've had another look based on the comments. Thanks for these!
I am not so happy with the kdb tags I use for creating borders around the white screens especially from the coleman tool. Not sure if there is a better solution, I didn't find one.

corrected to textual errors
- fixed some typos / unconsistencies
docs/dice_verification.md Outdated Show resolved Hide resolved
- proof --> prove
- Seedsigner --> SeedSigner
@jahangir13 jahangir13 requested a review from Marc-Gee July 20, 2023 18:06
@jdlcdl
Copy link

jdlcdl commented Jul 21, 2023

Instead of just running through the changes, I've read top to bottom (I'll do so again looking for typos/word suggestions) to follow the flow of how a user would verify. This is very thorough, a great exercise for building confidence in seedsigner, in your wallet, in other related tools... and especially that user knows how to use them. For dice values, I also verified against Keith's pr #404 and results were as expected/documented.

docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
- suggest: "which fits to" becomes "supported by"
- "The wallets settings screen" becomes "The wallet's settings screen"
- suggest: added comma like: "To verify the change addresses, change..." (2 times)
- fix for the strange wrapping ( (4): )
@jdlcdl
Copy link

jdlcdl commented Jul 21, 2023

Very nice work.

ACK

- lowercase for fingerprints, zpubs and addresses in the conclusion
Copy link
Contributor

@Marc-Gee Marc-Gee left a comment

Choose a reason for hiding this comment

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

I have suggested some grammar changes in this review. I am not that familiar with how this appears to you all once submitted, as it is very different to the MS Word / Google Docs track changes style with which I am more familiar.

So @jahangir13 , please let me know how these suggestions appear for you - I expect they do not function like the "Accept changes" feature of MS Word , but we can try these few suggestions initially.

docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
docs/dice_verification.md Outdated Show resolved Hide resolved
@jdlcdl
Copy link

jdlcdl commented Jul 25, 2023

Where I "thumbs-up"ed Marc's comments, it's because I agree that they are improvements.

...but I'm still very impressed with your English @jahangir13!

At this Friday's meeting, I hope that we can address some of these documentation changes as candidates ready for merge/release. Also, how these might be merged w/ Keith's pr-404 which allows cli access to seedsigner's dice-rolls mnemonic generation.

- Marc's review changes
@jahangir13
Copy link
Contributor Author

Where I "thumbs-up"ed Marc's comments, it's because I agree that they are improvements.

...but I'm still very impressed with your English @jahangir13!

At this Friday's meeting, I hope that we can address some of these documentation changes as candidates ready for merge/release. We should also discuss how these might be merged w/ Keith's pr-404 which allows cli access to seedsigner's dice-rolls mnemonic generation.

Hm, the more I work on this, I think that there is more to be verified. Not sure how to combine all of that together to reduce redundancy. E.g. verify calculation of the 13th/24th words...verify BIP85 account address derivation...seed via camera will maybe not be possible.
There could be several docs later or maybe 1 complete one...I was wondering why there is only one for dice-based seed creation.

@jdlcdl
Copy link

jdlcdl commented Jul 25, 2023

Just my 2c. If someone wants quick answers, they'll check out a video. If they want in-depth, then as much as they can read, even if partially redundant, is a resource worth having. None of these docs make it into the seedsigner-os images that are released; we are not limited in how thorough our docs can be.

This is good progress, good work. It's definitely MUCH better than leaving old docs with old interface-screens that would confuse readers, or lead them not to read it at all.

@jahangir13
Copy link
Contributor Author

jahangir13 commented Jul 25, 2023

Some aspects:

  • Verify calulation of last word: create seed (manually), enter in SeSi and let it create the last word. Check in the tools that the same last word is calculated (check in Seedpicker?)
  • Verify xpub: export xpub to check that the right xpub is exported (check in Sparrow, Coleman, Seed Tool)
  • Dice-based seed creation verification, check in Sparrow, coleman, seed tool (as we already have)
  • Camera-based seed creation?
  • BIP85 verification, check in Sparrow, Coleman, Seed Tool
  • check addresses of any seed in Seedsigner (check in SeSi address explorer, Sparrow, Coleman, Seed Tool)
  • What else can be/needs to be verified?

In the end after entering the entropy details or seed words into the tools, the verification is often the same. Not sure about a good flow here.

To be honest..using the Github Wiki might be a better approach than having single .md files in a Docs directory. The Wiki could be ordered/organized for topics and sub-topics, I guess existing links to already available docs could just be linked there. So the people do not need to search for topics in single files. Maybe something for the future^^

@jdlcdl
Copy link

jdlcdl commented Jul 25, 2023

Why not.

You have a point that more can be verified at the same time. The doc is about mnemonic_generation and that fits much more than simply dice rolls. I agree that whatever can be checked at the same time might be documented. These documents serve marketing for our project, building trust and community, and at the same time they beg our users to be verifiers that the project is worth trusting... once verified.

However, it doesn't all need to be there before merge. Can always issue a new pr for additional changes. What is there needs to be verified and ACKed by others, so that "perfect" does not delay "considerably better than we have now" -- which you've already accomplished.

@jahangir13 jahangir13 requested a review from Marc-Gee July 26, 2023 22:54
@Marc-Gee
Copy link
Contributor

Hi, I should have time tomorrow afternoon (PST) to complete the requested review 👍

@newtonick newtonick added the documentation Improvements or additions to documentation label Aug 1, 2023
@newtonick
Copy link
Collaborator

ACK reviewed, great work!

Copy link
Contributor

@Marc-Gee Marc-Gee left a comment

Choose a reason for hiding this comment

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

Ack! The most recent changes are great. The document reads really well now from top to bottom! ! Great work @jahangir13

Marc.

@newtonick newtonick merged commit 714d648 into SeedSigner:dev Aug 4, 2023
@SeedSigner
Copy link
Owner

@jahangir13 - please contact me at [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants