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

Suggested interfaces::Chain cleanups from #15288 #15531

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

ryanofsky
Copy link
Contributor

Mostly documentation improvements requested in the last review of #15288 before it was merged (#15288 (review))

Mostly documentation improvements requested in the last review of bitcoin#15288 before
it was merged
(bitcoin#15288 (review))
@maflcko
Copy link
Member

maflcko commented Mar 4, 2019

ACK de9f7f2

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

utACK de9f7f2

src/interfaces/README.md Outdated Show resolved Hide resolved
src/interfaces/chain.h Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 4, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #10973 (Refactor: separate wallet from node by ryanofsky)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

@ryanofsky
Copy link
Contributor Author

Updated de9f7f2 -> 4d4e4c6 (pr/wclean2.1 -> pr/wclean2.2, compare) with suggested changes

@fanquake
Copy link
Member

fanquake commented Mar 5, 2019

re-utACK 4d4e4c6

//! the future) ability to access to the chain state, receive notifications,
//! estimate fees, and submit transactions.
//!
//! TODO: Current chain methods are too low level, exposing too much of the
Copy link
Member

Choose a reason for hiding this comment

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

nit: I suspect this and below won't be particularly helpful if included in the doxygen output

Copy link
Member

Choose a reason for hiding this comment

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

I think it is helpful to describe the long term goal better (and give ideas to work on for new contributors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #15531 (comment)

nit: I suspect this and below won't be particularly helpful if included in the doxygen output

I don't know if this is a formatting suggestion or a content suggestion. It would help if you could be more specific. I've seen links posted to bitcoin doxygen output once in a while, but I've haven't used the doxygen output myself. I do read comments that appear in code. And I especially like comments that tell me why a particular piece of code was designed a certain way, so I am not left in the dark about things that seem like counterintuitive decision designs.

@maflcko
Copy link
Member

maflcko commented Mar 5, 2019

re-utACK 4d4e4c6

@maflcko maflcko merged commit 4d4e4c6 into bitcoin:master Mar 5, 2019
maflcko pushed a commit that referenced this pull request Mar 5, 2019
4d4e4c6 Suggested interfaces::Chain cleanups from #15288 (Russell Yanofsky)

Pull request description:

  Mostly documentation improvements requested in the last review of #15288 before it was merged (#15288 (review))

Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 28, 2020
…rom #15288

Summary:
4d4e4c6448 Suggested interfaces::Chain cleanups from #15288 (Russell Yanofsky)

Pull request description:

  Mostly documentation improvements requested in the last review of #15288 before it was merged (bitcoin/bitcoin#15288 (review))

Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe

---

Depends on D5869

This is a backport of Core [[bitcoin/bitcoin#15531 | PR15531]]

Test Plan:
  cmake .. -GNinja -DENABLE_WERROR=ON -DCMAKE_BUILD_TYPE=Debug
  ninja check

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D5870
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 25, 2021
…n#15288

4d4e4c6 Suggested interfaces::Chain cleanups from bitcoin#15288 (Russell Yanofsky)

Pull request description:

  Mostly documentation improvements requested in the last review of bitcoin#15288 before it was merged (bitcoin#15288 (review))

Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 30, 2021
…n#15288

4d4e4c6 Suggested interfaces::Chain cleanups from bitcoin#15288 (Russell Yanofsky)

Pull request description:

  Mostly documentation improvements requested in the last review of bitcoin#15288 before it was merged (bitcoin#15288 (review))

Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants