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

docs: replace zeros comments with ones #28372

Merged

Conversation

peterpme
Copy link
Contributor

@peterpme peterpme commented Oct 13, 2022

Documentation describes PublicKey.default as "all zeros" when its in fact, "all ones"

@mergify mergify bot added the community Community contribution label Oct 13, 2022
@mergify mergify bot requested a review from a team October 13, 2022 01:34
@CriesofCarrots
Copy link
Contributor

The original comment is arguably correct. While the base58-encoded string representation of the default PublicKey is all ones, the underlying BN number is 32 bytes that are all zeros. Would you be willing to rework your PR to make this more clear?

@peterpme
Copy link
Contributor Author

@CriesofCarrots yeah! Thanks for the quick reply and helpful comment. What do you think?

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #28372 (578e3c9) into master (e6687b8) will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #28372     +/-   ##
=========================================
+ Coverage    77.3%    77.5%   +0.1%     
=========================================
  Files          55       55             
  Lines        2889     2926     +37     
  Branches      402      407      +5     
=========================================
+ Hits         2234     2268     +34     
- Misses        514      517      +3     
  Partials      141      141             

Copy link
Contributor

@CriesofCarrots CriesofCarrots 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 quick edit! Looks good to me.
(Disregard the commitlint CI failure. I'll fix that on merge/squash)

@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Oct 13, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Oct 13, 2022
@CriesofCarrots CriesofCarrots merged commit 759dc80 into solana-labs:master Oct 13, 2022
@peterpme peterpme deleted the peterpme/zeros-with-ones-docs branch October 13, 2022 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants