Skip to content

Push down RPC reqWallet flag #5992

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 3 commits into from
Apr 15, 2015
Merged

Push down RPC reqWallet flag #5992

merged 3 commits into from
Apr 15, 2015

Conversation

jonasschnelli
Copy link
Contributor

No description provided.

@sipa
Copy link
Member

sipa commented Apr 11, 2015

Concept ACK.

@jgarzik
Copy link
Contributor

jgarzik commented Apr 12, 2015

ut ACK

Would prefer code movement in a first commit, separated out from the rest.

@jonasschnelli jonasschnelli force-pushed the 2015/04/remove_reqWallet branch from 451f282 to 71189e5 Compare April 12, 2015 15:56
@jonasschnelli
Copy link
Contributor Author

separated out the code moving

@jgarzik
Copy link
Contributor

jgarzik commented Apr 12, 2015

Looks good. This probably wants a release note, as it's a slight RPC API-visible change.

@sipa
Copy link
Member

sipa commented Apr 12, 2015

How is it visible?

@jgarzik
Copy link
Contributor

jgarzik commented Apr 12, 2015

An exception is no longer thrown in all cases. Value::null return value comes through.

@jonasschnelli jonasschnelli force-pushed the 2015/04/remove_reqWallet branch from 71189e5 to b9fb692 Compare April 12, 2015 17:37
@jonasschnelli
Copy link
Contributor Author

This should not be the case. Value::null should only come through for the help string generation (in -disablewallet mode the cmd should not be listed under help). I tried to avoid any, even slightly, API changes.

laanwj
laanwj reviewed Apr 13, 2015
View reviewed changes
src/wallet/rpcwallet.cpp
pwalletMain->AvailableCoins(vecOutputs, false);
BOOST_FOREACH(const COutput& out, vecOutputs) {
if (out.nDepth < nMinDepth || out.nDepth > nMaxDepth)
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Something weird happened to the indentation of this function, for example these continues are not indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDE issue! Will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

laanwj
laanwj reviewed Apr 13, 2015
View reviewed changes
src/rpcserver.cpp
@@ -193,11 +193,6 @@ string CRPCTable::help(string strCommand) const
continue;
if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
continue;
#ifdef ENABLE_WALLET
Copy link
Member

Choose a reason for hiding this comment

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

This means that the wallet commands will be listed in help, even if the wallet is disabled. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.
the !pwalletMain check (wallet disabled at runtime yes/no) will be done within the new function EnsureWalletIsAvailable(bool) :
https://github.com/bitcoin/bitcoin/pull/5992/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR40

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fair enough. Forget my other remark then.

@laanwj
Copy link
Member

laanwj commented Apr 13, 2015

Value::null should only come through for the help string generation (in -disablewallet mode the cmd should not be listed under help). I

The help flag parameter is set to true when a full help message is requested, in case of help <command>. In this case throwing an exception is fine.

@laanwj
Copy link
Member

laanwj commented Apr 15, 2015

Tested ACK, also verified that listunspent is move-only.

@laanwj laanwj merged commit ea9e82d into bitcoin:master Apr 15, 2015
laanwj added a commit that referenced this pull request Apr 15, 2015
@laanwj
Merge pull request #5992
4c4f1b4 
ea9e82d [squashme] fix listunspent code indentation (Jonas Schnelli)
b9fb692 Push down RPC reqWallet flag (Jonas Schnelli)
0b9dc9c [move] move listunspent to wallet/rpcwallet.cpp (Jonas Schnelli)
@furszy furszy mentioned this pull request Jun 23, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 1, 2020
@random-zebra
Merge #1702: [RPC] Table registration update and wallet table decoupled.
7a849ca 
9f4868a [move] move listunspent to wallet/rpcwallet.cpp (furszy)
e85e4f1 [RPC] Move RPC dispatch table registration to rpcwallet code (furszy)
c0bc348 [RPC] push down reqWallet [first step] (furszy)
cdb80a6 [RPC] Remove not used threadsafe in CRPCCommand (furszy)

Pull request description:

  Initial work over the RPC dispatch table registration update.

  Includes the following changes:

  1) Remove the unused threadSafe member from every CPRCCommand. (bitcoin#5711 , second commit)
  2) Remove reqWallet from every CPRCCommand (bitcoin#5992 , second commit partially).
  3) Move most of the wallet RPC commands registration to the rpcwallet file (bitcoin#7307 without the `EnsureWalletIsAvailable` on every RPC call --> mainly because the wallet RPC commands will only be registered if the wallet is available).

  to do: Move the remaining commands that were not moved from server to rpcwallet.

  ----------

  Next step would be bitcoin#7766 (If anyone want to tackle it, feel more than welcome to tackle it).

ACKs for top commit:
  Fuzzbawls:
    ACK 9f4868a
  random-zebra:
    All good. ACK 9f4868a and merging...

Tree-SHA512: cd83cdc2de81efb5cb84d39753a160b19d8ca2967a1b303d85c3279559b478352c5a4316942c7a654e9c667b5a8f5bfa18020c1b6c9e78f4c408028840d0fc86
@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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonasschnelli @sipa @jgarzik @laanwj