Skip to content

MOVE ONLY: move rpc* to rpc/ #7348

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
merged 2 commits into from
Jan 28, 2016
Merged

MOVE ONLY: move rpc* to rpc/ #7348

merged 2 commits into from
Jan 28, 2016

Conversation

dcousens
Copy link
Contributor

Was just playing around with the RPC, and found the constant prefixing of file names frustrating.

This also seems to be in line with the way other modules (zmq etc) have been grouped.

Should be easy to review.

Note: This won't negatively impact git blame

@maflcko
Copy link
Member

maflcko commented Jan 15, 2016

What's up with src/wallet/rpc*?

maflcko
maflcko reviewed Jan 15, 2016
View reviewed changes
src/rpc/server.h
@@ -198,7 +198,7 @@ extern UniValue estimatepriority(const UniValue& params, bool fHelp);
extern UniValue estimatesmartfee(const UniValue& params, bool fHelp);
extern UniValue estimatesmartpriority(const UniValue& params, bool fHelp);

extern UniValue getnewaddress(const UniValue& params, bool fHelp); // in rpcwallet.cpp
extern UniValue getnewaddress(const UniValue& params, bool fHelp); // in rpc/wallet.cpp
Copy link
Member

Choose a reason for hiding this comment

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

There is no rpc/wallet.cpp?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.
I wonder what the best course of action is here?
I'll probably just change that to wallet/rpcwallet.cpp

Copy link
Contributor

Choose a reason for hiding this comment

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

#7307 would solve that.
But I agree, changing to wallet/rpcwallet.cpp is probably fine for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my suggested fix too would b to just fix the comment

@jonasschnelli
Copy link
Contributor

Nice!
Concept ACK.

@laanwj
Copy link
Member

laanwj commented Jan 16, 2016

Concept ACK.

Not that you moved it, but for completeness' sake: please leave rpcwallet.cpp in src/wallet. e.g. it is the RPC interface to the wallet. This, imo, logically belongs to the wallet: if there was a different wallet it could have a different RPC interface.

@dcousens
Copy link
Contributor Author

@laanwj amended inline comment commit. Should be good to go.

This, imo, logically belongs to the wallet: if there was a different wallet it could have a different RPC interface.

Agreed.

@maflcko
Copy link
Member

maflcko commented Jan 17, 2016

@dcousens Could you make the commits atomic, so that each of them compiles?

@dcousens
Copy link
Contributor Author

@MarcoFalke rebased and fixed a bad path in /src/qt/ (I don't typically build with Qt).
Commits were already atomic.

Waiting on travis.

@maflcko
Copy link
Member

maflcko commented Jan 17, 2016

Concept ACK 611508c

@laanwj
Copy link
Member

laanwj commented Jan 20, 2016

Needs rebase after #7307

@dcousens
Copy link
Contributor Author

Rebased

@maflcko
Copy link
Member

maflcko commented Jan 20, 2016

utACK d13f65e

@paveljanik
Copy link
Contributor

ACK https://github.com/dcousens/bitcoin/commit/d13f65ebac13ec18b7eb55176c31f1404f185c0c

Header guards do not match now (_ missing), but not that important.

@jonasschnelli
Copy link
Contributor

utACK d13f65e.
Confirm move only.

@laanwj laanwj merged commit d13f65e into bitcoin:master Jan 28, 2016
laanwj added a commit that referenced this pull request Jan 28, 2016
@laanwj
Merge #7348: MOVE ONLY: move rpc* to rpc/
62f2d76 
d13f65e rpc: update inline comments to refer to new file paths (Daniel Cousens)
a0eaff8 move rpc* to rpc/ (Daniel Cousens)
@jtimon
Copy link
Contributor

jtimon commented Jan 28, 2016

nice

@OlegGirko OlegGirko mentioned this pull request Jul 2, 2017
Merged
@dagurval dagurval mentioned this pull request Jan 29, 2018
Merged
@str4d str4d mentioned this pull request Jun 20, 2018
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Jun 20, 2018
@zkbot
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
52f4abc 
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 18, 2018
@zkbot
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
5978dfe 
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 18, 2018
@zkbot
Auto merge of #3350 - str4d:rpc-reorg, r=bitcartel
cd0484e 
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Refactoring RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dcousens @maflcko @jonasschnelli @laanwj @paveljanik @jtimon