Skip to content

[docs] Add doxygen comments for keypool classes #15777

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 3 commits into from
May 14, 2019
Merged

[docs] Add doxygen comments for keypool classes #15777

maflcko merged 3 commits into from
May 14, 2019

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Apr 9, 2019

Docs/move-only

Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg #15557 (comment)).

These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.

promag
promag reviewed Apr 9, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

👏 very informative! Thanks! Concept ACK.

@laanwj
Copy link
Member

laanwj commented Apr 10, 2019

Thanks for adding documentation!
ACK fb776446810e8f8d3cada1613a30c68d4d3a6fff

@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 10, 2019

rebased

@jnewbery jnewbery force-pushed the 2019_04_keypool_comments branch from fb77644 to 03c5e5e Compare April 10, 2019 13:22
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 10, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15796 (CReserveKey should not allow passive key re-use, KeepKey in destructor by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jnewbery jnewbery force-pushed the 2019_04_keypool_comments branch from 03c5e5e to 7912ce1 Compare April 10, 2019 16:18
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 10, 2019

rebased

@jnewbery jnewbery mentioned this pull request Apr 11, 2019
Merged
@jnewbery
[wallet] move-only: move CReserveKey to be next to CKeyPool
ef2d515 
reviewer tip: use git diff --color-moved=dimmed-zebra
@jnewbery jnewbery force-pushed the 2019_04_keypool_comments branch from 7912ce1 to dc0299e Compare April 14, 2019 13:47
jonatack
jonatack reviewed Apr 15, 2019
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Very helpful documentation! 💯 Concept ACK

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack Apr 15, 2019

Choose a reason for hiding this comment

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

For my understanding: doc/developer-notes.md mentions using //! Description before the member syntax to describe member or variable use to be picked up by Doxygen. Is the // syntax used here with the intent to not be picked up, or for another reason?

Copy link
Contributor Author

@jnewbery jnewbery Apr 29, 2019

Choose a reason for hiding this comment

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

No reason, I just got the doxygen syntax wrong. Thanks for pointing this out!

@jnewbery jnewbery force-pushed the 2019_04_keypool_comments branch from dc0299e to 292f0ee Compare April 29, 2019 20:44
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 29, 2019

Fixed @jonatack's comment.

jonatack
jonatack reviewed May 1, 2019
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 292f0ee

Feel free to ignore the nits I mention. Very helpful documentation, particularly for someone new to the codebase and its history like myself. Kudos for the relevant commit hashes to dive in further.

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack May 1, 2019

Choose a reason for hiding this comment

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

Possibly: "to provide addresses or receive change."

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack May 1, 2019

Choose a reason for hiding this comment

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

Possibly: "that key's existence," or "that key's generation,"

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack May 1, 2019

Choose a reason for hiding this comment

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

/s/, or/ or of/` (missing "of")

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack May 1, 2019

Choose a reason for hiding this comment

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

Beginner perspective: ran git show 02592f4c to see what "HD-split" meant more exactly. Perhaps "The wallet with HD chain split feature (HD-split) added..."

src/wallet/wallet.h Outdated
Copy link
Member

@jonatack jonatack May 1, 2019

Choose a reason for hiding this comment

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

Seems this ought to be either "a set of" or "sets of"

@jnewbery jnewbery force-pushed the 2019_04_keypool_comments branch from 292f0ee to f1a77b0 Compare May 1, 2019 18:54
@jnewbery
Copy link
Contributor Author

jnewbery commented May 1, 2019

Thanks for the review @jonatack . I've taken all your suggestions.

@jonatack
Copy link
Member

jonatack commented May 6, 2019

Thanks, John. Re-ACK f1a77b0, doc-only changes with respect to previous review.

@jb55
Copy link
Contributor

jb55 commented May 14, 2019

ACK f1a77b0

@maflcko maflcko merged commit f1a77b0 into bitcoin:master May 14, 2019
maflcko pushed a commit that referenced this pull request May 14, 2019
Merge #15777: [docs] Add doxygen comments for keypool classes
65526fc 
f1a77b0 [docs] Add doxygen comment for CReserveKey (John Newbery)
37796b2 [docs] Add doxygen comment for CKeyPool (John Newbery)
ef2d515 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery)

Pull request description:

  Docs/move-only

  Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg #15557 (comment)).

  These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.

ACKs for commit f1a77b:
  jonatack:
    Thanks, John. Re-ACK f1a77b0, doc-only changes with respect to previous review.
  jb55:
    ACK f1a77b0

Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 15, 2019
@sidhujag
Merge bitcoin#15777: [docs] Add doxygen comments for keypool classes
ef84484 
f1a77b0 [docs] Add doxygen comment for CReserveKey (John Newbery)
37796b2 [docs] Add doxygen comment for CKeyPool (John Newbery)
ef2d515 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery)

Pull request description:

  Docs/move-only

  Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg bitcoin#15557 (comment)).

  These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.

ACKs for commit f1a77b:
  jonatack:
    Thanks, John. Re-ACK f1a77b0, doc-only changes with respect to previous review.
  jb55:
    ACK f1a77b0

Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 4, 2020
@jnewbery @majcosta
[backport#15777] [wallet] move-only: move CReserveKey to be next to C…
9f4f50f 
…KeyPool

Summary:
reviewer tip: use git diff --color-moved=dimmed-zebra

bitcoin/bitcoin@ef2d515

---

Partial backport of Core [[bitcoin/bitcoin#15777 | PR15777]]

Test Plan:
  ninja

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D6348
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 4, 2020
@jnewbery @majcosta
[backport#15777] [docs] Add doxygen comment for CKeyPool
8a2ed5f 
Summary:
[docs] Add doxygen comment for CReserveKey

bitcoin/bitcoin@37796b2
bitcoin/bitcoin@f1a77b0

---

Concludes backport of Core [[bitcoin/bitcoin#15777 | PR15777]]

Test Plan:
  ninja

Reviewers: #bitcoin_abc, jasonbcox, deadalnix

Reviewed By: #bitcoin_abc, jasonbcox, deadalnix

Subscribers: deadalnix, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D6350
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jonatack jonatack jonatack left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jnewbery @laanwj @DrahtBot @jonatack @jb55 @promag @fanquake @maflcko