Skip to content

Conversation

willcl-ark
Copy link
Member

fixes #20246

This documents the two JSON-RPC endpoints available, details when they are active, specifies when they can or must be used, and outlines some known behaviour quirks.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 8, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake
Concept ACK pinheadmz, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label Mar 8, 2023
@fanquake fanquake requested review from luke-jr and stickies-v March 8, 2023 10:53
@fanquake
Copy link
Member

fanquake commented Mar 8, 2023

Concept ACK

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

ACK 86cff0e

Error is also helpful if someone is unaware of this and tries to use / endpoint with 2 or more wallets for wallet based RPC commands.

{
    "result": null,
    "error": {
        "code": -19,
        "message": "Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path)."
    },
    "id": "test"
}

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

concept ACK

Comment on lines +12 to +13
1. `/`
2. `/wallet/<walletname>/`
Copy link
Member

Choose a reason for hiding this comment

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

A few ideas about these:

  • could include full path http://localhost:{port}/
  • could list relevant ports per network (i.e. main=8332, regtest=18443 etc)
  • not sure if this applies here but I'm used to seeing API endpoints with variables expressed like /wallet/:walletname, at least for REST APIs. I dunno if there is otherwise a convention for that

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I feel it's clearer just to use the endpoints themselves, rather than cluttering with hosts and ports.

Also, we have examples now!


### `/wallet/<walletname>/` endpoint

This endpoint is only activated when the wallet component has been compiled in
Copy link
Member

Choose a reason for hiding this comment

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

It is also always active for non-wallet requests before any wallets are loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

(but only when the wallet is compiled in).

Also I think i've covered that it handles both types of request when no wallet is loaded in the Quirks section?

@willcl-ark willcl-ark force-pushed the document-json-endpoint branch from 86cff0e to 36748e8 Compare March 8, 2023 16:56
Comment on lines +36 to +39
$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: text/plain;' localhost:38332/

# Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet
$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: text/plain;' localhost:38332/wallet/desc-wallet
Copy link
Member

Choose a reason for hiding this comment

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

If this gets merged before #27101, remind me to update to 2.0 ;-)

Technically I think jsonrpc:"1.0" isn't necessary and doesn't actually do anything.

@kristapsk
Copy link
Contributor

Concept ACK

@kristapsk
Copy link
Contributor

Nit - there is typo in commit message / PR title (s/docment/document/).

fixes bitcoin#20246

Document both JSON-RPC endpoints, when they are active and which types
of requests they are able to service.

Adds two example curl requests, one for each endpoint.
@willcl-ark
Copy link
Member Author

Nit - there is typo in commit message / PR title (s/docment/document/).

Thanks, i've updated this in 65e3abc

@willcl-ark willcl-ark force-pushed the document-json-endpoint branch from 36748e8 to 65e3abc Compare March 9, 2023 08:44
@kristapsk
Copy link
Contributor

Nit - there is typo in commit message / PR title (s/docment/document/).

Thanks, i've updated this in 65e3abc

You should also edit PR title. It is used by github-merge.py.

@willcl-ark willcl-ark changed the title doc: docment json rpc endpoints doc: document json rpc endpoints Mar 9, 2023
@willcl-ark
Copy link
Member Author

Thanks, I thought that updated from the force push, but apparently not. Now also done :)

Comment on lines +35 to +39
# Get block count from the / endpoint when rpcuser=alice and rpcport=38332
$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: text/plain;' localhost:38332/

# Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet
$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: text/plain;' localhost:38332/wallet/desc-wallet
Copy link

Choose a reason for hiding this comment

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

Why are the examples using signet port? I think we should use mainnet port (8332) same as all the RPC examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to be persuaded either way here, no particular reason for using signet (other than I was using it for testing). Although I personally don't think it matters much as the comment for each shows which variables are being used

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 65e3abc - Seems fine. Can be improved if need be.

@fanquake fanquake merged commit e43fdfd into bitcoin:master Jun 2, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 2, 2023
65e3abc doc: document json rpc endpoints (willcl-ark)

Pull request description:

  fixes bitcoin#20246

  This documents the two JSON-RPC endpoints available, details when they are active, specifies when they can or must be used, and outlines some known behaviour quirks.

ACKs for top commit:
  fanquake:
    ACK 65e3abc - Seems fine. Can be improved if need be.

Tree-SHA512: d557c2caf000a1bdd7b46c9da38afe63dc28446ba4a961526f1af3cec81d994a9da663e02c81ebdc4f609b794585349cfca77a582dc1e788c120de1d3b4c7db6
@bitcoin bitcoin locked and limited conversation to collaborators Jun 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document JSON-RPC wallet endpoints
5 participants