doc: Include wallet path to relevant RPC calls

D4nte commented at 7:46 am on June 22, 2020: none

For wallet-related RPC calls, the url must have the wallet name in the path: http://localhost:8332/wallet/testwallet.

This patch corrects the documentation.

Note that for unloadwallet 2 forms are accepted: wallet name in the path and wallet name as an argument. Hence, I added an example to show both ways.

fanquake added the label RPC/REST/ZMQ on Jun 22, 2020

fanquake added the label Docs on Jun 22, 2020

promag commented at 9:20 am on June 22, 2020: member

the url must have the wallet name in the path

If only one wallet is loaded then this is not true - to not break clients pre multiwallet - see GetWalletForJSONRPCRequest.

DrahtBot commented at 10:34 am on June 22, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19994 (Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) by MarcoFalke)
#19443 (rpc: Add options argument to listtransactions, with paginatebypointer options by kristapsk)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

D4nte commented at 0:13 am on June 23, 2020: none

the url must have the wallet name in the path

If only one wallet is loaded then this is not true - to not break clients pre multiwallet - see GetWalletForJSONRPCRequest.

Oh thanks! I’ll correct the commit message. I did not realise I had several wallets loaded when I encountered the issue.

I think we should still update the doc as this is the way forward.

D4nte force-pushed on Jun 23, 2020

DrahtBot added the label Needs rebase on Jun 25, 2020

D4nte force-pushed on Jun 26, 2020

DrahtBot removed the label Needs rebase on Jun 26, 2020

D4nte commented at 0:48 am on June 30, 2020: none

I ran the tests locally and there was no issues contrary to the CI run. Not sure how to retrigger the CI so I’ll just rebase.

Ran locally:

0make check
1python3 ./test/functional/wallet_descriptor.py --descriptors

D4nte force-pushed on Jun 30, 2020

laanwj commented at 12:58 pm on July 15, 2020: member

I think we should still update the doc as this is the way forward.

Yes, I think so too. The examples only have to work against the current version anyway, there is no need for compatibility there.

Edit: Do we want to do a similar thing (-rpcwallet=<walletname>) for HelpExampleCli?

laanwj added the label Wallet on Jul 15, 2020

D4nte commented at 0:38 am on July 16, 2020: none

Do we want to do a similar thing (-rpcwallet=<walletname>) for HelpExampleCli?

Happy to give it a whack but I don’t think we should stop merging this PR because of this.

in src/rpc/util.cpp:121 in 1e7e92c99b outdated

117@@ -118,10 +118,20 @@ std::string HelpExampleCli(const std::string& methodname, const std::string& arg
118     return "> bitcoin-cli " + methodname + " " + args + "\n";
119 }
120 
121-std::string HelpExampleRpc(const std::string& methodname, const std::string& args)

promag commented at 10:42 pm on July 26, 2020:

Just add const std::string& path = {} argument here?

D4nte commented at 1:54 am on July 27, 2020:

Oh, thanks!

promag commented at 10:48 pm on July 26, 2020: member

@D4nte vice-versa, and considering the change @laanwj is suggesting, I think you should include it here. It’s pretty much the same change but for HelpExampleCli.

Either way, no strong opinion on this change. I even think we could drop these “usages” from the help output 🙄

doc: Include wallet path to relevant RPC calls

Since the addition of multiwallet support and if there are more than one
loaded wallet, the url should have the wallet name in the path:
`http://localhost:8332/wallet/testwallet`.

This patch corrects the documentation.

Note that for `unloadwallet` the wallet name can be passed in two
manners: as an argument or in the url path. Hence, I added an example
to show both ways.

eb18346649

D4nte commented at 6:12 am on July 27, 2020: none

Either way, no strong opinion on this change. I even think we could drop these “usages” from the help output 🙄

If we drop those examples, how should one guess that wallet/foo needs to be appended to the RPC calls?

D4nte force-pushed on Jul 27, 2020

MarcoFalke removed the label Docs on Aug 23, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Aug 23, 2020

MarcoFalke removed the label Wallet on Aug 23, 2020

DrahtBot added the label Docs on Aug 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 23, 2020

DrahtBot added the label Wallet on Aug 23, 2020

DrahtBot added the label Needs rebase on Sep 23, 2020

DrahtBot commented at 6:21 pm on September 23, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

D4nte closed this on Oct 5, 2021

DrahtBot locked this on Oct 30, 2022

doc: Include wallet path to relevant RPC calls #19349

Conflicts