Skip to content

doc: Include wallet path to relevant RPC calls#19349

Closed
D4nte wants to merge 1 commit intobitcoin:masterfrom
D4nte:doc-wallet
Closed

doc: Include wallet path to relevant RPC calls#19349
D4nte wants to merge 1 commit intobitcoin:masterfrom
D4nte:doc-wallet

Conversation

@D4nte
Copy link

@D4nte D4nte commented Jun 22, 2020

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.

@promag
Copy link
Contributor

promag commented Jun 22, 2020

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
Copy link
Contributor

DrahtBot commented Jun 22, 2020 •
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:

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
Copy link
Author

D4nte commented Jun 23, 2020

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.

This was referenced Jun 25, 2020
Merged
Merged
Merged
@D4nte

This comment has been minimized.

Sign in to view
@DrahtBot DrahtBot mentioned this pull request Jul 4, 2020
Closed
@laanwj
Copy link
Member

laanwj commented Jul 15, 2020 •
edited
Loading

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 laanwj added the Wallet label Jul 15, 2020
@D4nte
Copy link
Author

D4nte commented Jul 16, 2020

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.

promag
promag reviewed Jul 26, 2020
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.

@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 🙄

src/rpc/util.cpp Outdated
Copy link
Contributor

@promag promag Jul 26, 2020

Choose a reason for hiding this comment

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

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

Copy link
Author

@D4nte D4nte Jul 27, 2020

Choose a reason for hiding this comment

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

Oh, thanks!

@D4nte
doc: Include wallet path to relevant RPC calls
eb18346 
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.
@D4nte
Copy link
Author

D4nte commented Jul 27, 2020 •
edited
Loading

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?

@DrahtBot DrahtBot mentioned this pull request Sep 22, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2020

🐙 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 D4nte closed this Oct 5, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Needs rebase RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@D4nte @promag @DrahtBot @laanwj @fanquake @maflcko