Skip to content

refactor, wallet: Nuke coincontrol circular dependency #17518

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
meshcollider merged 1 commit into from
Nov 24, 2019
Merged

refactor, wallet: Nuke coincontrol circular dependency #17518

meshcollider merged 1 commit into from
Nov 24, 2019

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Nov 19, 2019

This PR gets rid of wallet/coincontrol -> wallet/wallet -> wallet/coincontrol circular dependency.

@practicalswift
Copy link
Contributor

practicalswift commented Nov 19, 2019

Concept ACK

Very nice :)

@Sjors
Copy link
Member

Sjors commented Nov 19, 2019

ACK 80303fa2b9a8eaaa00d8c3ddf665a5026675508d. Tested on macOS 10.15.1

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 19, 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:

  • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

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.

promag
promag reviewed Nov 21, 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.

ACK 80303fa2b9a8eaaa00d8c3ddf665a5026675508d.

src/wallet/init.cpp Outdated
Copy link
Contributor

@promag promag Nov 21, 2019

Choose a reason for hiding this comment

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

While here also add #include <ui_interface.h> because of InitError.

nit, does the comment really matter?

Copy link
Member Author

@hebasto hebasto Nov 22, 2019

Choose a reason for hiding this comment

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

Done.

src/wallet/coincontrol.h Outdated
Copy link
Contributor

@promag promag Nov 21, 2019

Choose a reason for hiding this comment

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

I've tried to move these to a new header wallet/constants.h - which would allow to avoid #include <wallet/wallet.h> in some places - but suddenly it was already a lot of moved code.. so I think this is fine.

Copy link
Member Author

@hebasto hebasto Nov 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've tried to move these to a new header...

That was my first attempt too ;)

@hebasto hebasto force-pushed the 20191119-coincontrol-dep branch from 80303fa to abe9546 Compare November 22, 2019 19:00
@hebasto
Copy link
Member Author

hebasto commented Nov 22, 2019

@promag @Sjors

Thank you for your reviews. @promag's comments have been addressed.

Would you mind re-reviewing?

@hebasto hebasto force-pushed the 20191119-coincontrol-dep branch from abe9546 to 3ed5e68 Compare November 23, 2019 06:36
@hebasto
Copy link
Member Author

hebasto commented Nov 23, 2019

Rebased after #16944 has been merged.

@Sjors
Copy link
Member

Sjors commented Nov 23, 2019

re-ACK 3ed5e68

meshcollider
meshcollider reviewed Nov 24, 2019
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

utACK 3ed5e68

meshcollider added a commit that referenced this pull request Nov 24, 2019
@meshcollider
Merge #17518: refactor, wallet: Nuke coincontrol circular dependency
9cbd87d 
3ed5e68 refactor: Nuke coincontrol circular dependency (Hennadii Stepanov)

Pull request description:

  This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency.

ACKs for top commit:
  Sjors:
    re-ACK 3ed5e68
  meshcollider:
    utACK 3ed5e68

Tree-SHA512: 7fbceb74a9cd04157170df158d2deb979cf397df818376b478224d2423f1d8504a8688e3a9b8fc527da73e4a34ab6bc4a98be0cc2937e102a063ab2ac553e86d
@meshcollider meshcollider merged commit 3ed5e68 into bitcoin:master Nov 24, 2019
@hebasto hebasto deleted the 20191119-coincontrol-dep branch November 24, 2019 05:40
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 24, 2019
@meshcollider
Merge bitcoin#17518: refactor, wallet: Nuke coincontrol circular depe…
0dd0c6e 
…ndency

3ed5e68 refactor: Nuke coincontrol circular dependency (Hennadii Stepanov)

Pull request description:

  This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency.

ACKs for top commit:
  Sjors:
    re-ACK 3ed5e68
  meshcollider:
    utACK 3ed5e68

Tree-SHA512: 7fbceb74a9cd04157170df158d2deb979cf397df818376b478224d2423f1d8504a8688e3a9b8fc527da73e4a34ab6bc4a98be0cc2937e102a063ab2ac553e86d
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 29, 2019
@MarkLTZ
[wallet] Nuke coincontrol circular dependency (bitcoin#17518)
a93e7e1
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
@meshcollider @majcosta
[backport#17518] refactor, wallet: Nuke coincontrol circular dependency
532d589 
Summary:
3ed5e6819a50434449d92cb96b9d8d141e8c7d2b refactor: Nuke coincontrol circular dependency (Hennadii Stepanov)

Pull request description:

  This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency.

---

Backport of Core [[bitcoin/bitcoin#17518 | PR17518]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7694
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@meshcollider
Merge bitcoin#17518: refactor, wallet: Nuke coincontrol circular depe…
97a74a7 
…ndency

3ed5e68 refactor: Nuke coincontrol circular dependency (Hennadii Stepanov)

Pull request description:

  This PR gets rid of `wallet/coincontrol` -> `wallet/wallet` -> `wallet/coincontrol` circular dependency.

ACKs for top commit:
  Sjors:
    re-ACK 3ed5e68
  meshcollider:
    utACK 3ed5e68

Tree-SHA512: 7fbceb74a9cd04157170df158d2deb979cf397df818376b478224d2423f1d8504a8688e3a9b8fc527da73e4a34ab6bc4a98be0cc2937e102a063ab2ac553e86d
@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

@meshcollider meshcollider meshcollider left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hebasto @practicalswift @Sjors @DrahtBot @meshcollider @promag @fanquake