doc: minor improvements in getutxos REST endpoint synopsis #24467

theStack commented at 11:40 AM on March 3, 2022: contributor

Describing an optional sub-path as <checkmempool> in the synopsis could be misleading as the angle brackets normally indicate that the field has to be replaced a custom value. Clarify that by showing two variants instead, similar to the block endpoint with the notxdetails option:

#### Blocks
`GET /rest/block/<BLOCK-HASH>.<bin|hex|json>`
`GET /rest/block/notxdetails/<BLOCK-HASH>.<bin|hex|json>`

Further improvements:

uppercase <TXID> and <N>, to match the description of the other endpoints
s/getutxo command/getutxos endpoint/
describe what the checkmempool option does
s/serialisation/serialization/ (the US spelling is more dominant than the UK spelling in the project, and there is indeed no other instance of the string "serialis*" in the source tree, except once in a release note)
link to BIP64 within the text instead of only showing bare URL
mention that BIP64 is only relevant for bin and hex output formats
show two endpoint formats of the block section as list

DrahtBot added the label Docs on Mar 3, 2022

w0xlt approved

w0xlt commented at 1:37 PM on March 3, 2022: contributor

ACK d589ac2

in doc/REST-interface.md:95 in d589ac294f outdated

  93 | +`GET /rest/getutxos/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>`
  94 | +`GET /rest/getutxos/checkmempool/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>`
  95 |  
  96 | -The getutxo command allows querying of the UTXO set given a set of outpoints.
  97 | -See BIP64 for input and output serialisation:
  98 | +The getutxos endpoint allows querying of the UTXO set given a set of outpoints.

brunoerg commented at 11:47 AM on March 4, 2022:

nit:

The getutxos endpoint allows querying the UTXO set given a set of outpoints.

theStack commented at 12:22 PM on March 4, 2022:

Thanks, done.

theStack force-pushed on Mar 4, 2022

shaavan commented at 3:58 PM on March 6, 2022: contributor

Concept ACK

I want to make some suggestions:

I would suggest adding a line break between the two variants. This would make it look good in the rendered format.
It would be helpful to add the reasoning of change and further improvements in the commit message itself. This would be helpful for someone referencing the commit to understand the changes done.

in doc/REST-interface.md:93 in 3fcbf81abd outdated

  88 | @@ -89,10 +89,12 @@ Only supports JSON as output format.
  89 |  * softforks : (array) status of softforks in progress
  90 |  
  91 |  #### Query UTXO set
  92 | -`GET /rest/getutxos/<checkmempool>/<txid>-<n>/<txid>-<n>/.../<txid>-<n>.<bin|hex|json>`
  93 | +`GET /rest/getutxos/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>`
  94 | +`GET /rest/getutxos/checkmempool/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>`

jonatack commented at 4:13 PM on March 6, 2022:

I agree with @shaavan on adding a line break here (and for https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md#blocks; may as well sneak it into the commit if you agree).

theStack commented at 5:37 PM on March 6, 2022:

Agree that this is a good opportunity to also fix this for the blocks section. Done.

in doc/REST-interface.md:98 in 3fcbf81abd outdated

  96 | -The getutxo command allows querying of the UTXO set given a set of outpoints.
  97 | -See BIP64 for input and output serialisation:
  98 | +The getutxos endpoint allows querying the UTXO set given a set of outpoints.
  99 | +With the /checkmempool/ option the mempool is also taken into account.
 100 | +See BIP64 for input and output serialization (relevant for `bin` and `hex` output formats):
 101 |  https://github.com/bitcoin/bips/blob/master/bip-0064.mediawiki

jonatack commented at 4:19 PM on March 6, 2022:

Optionally, could put the url into "BIP64" and replace the colon at the end of the sentence with a full stop.

(FWIW "serialisation" isn't incorrect; IIRC it's the English spelling except in North America. That said, the North American spelling seems dominant here in the project.)

theStack commented at 5:39 PM on March 6, 2022:

Good idea, done. Also add a motivation hint in PR description and the commit body on why "serialization" is preferred over "serialisation" (I agree that it is not incorrect from a language perspective and the reason for change is only consistency).

jonatack commented at 4:23 PM on March 6, 2022: contributor

ACK modulo a coupla nits

theStack force-pushed on Mar 6, 2022

theStack commented at 5:36 PM on March 6, 2022: contributor

Thanks for the thorough review and the suggestions @shaavan and @jonatack. I took all of them and updated the PR description accordingly. Force-pushed with the following changes:

add line breaks via <br> between the two variants both in the blocks and the getutxos endpoint sections
put a motivation/description of the changes into the commit body (mostly taken over from the PR description)
link to BIP64 within the text instead of only showing bare URL

in doc/REST-interface.md:92 in aea690ab8d outdated

  88 | @@ -89,11 +89,13 @@ Only supports JSON as output format.
  89 |  * softforks : (array) status of softforks in progress
  90 |  
  91 |  #### Query UTXO set
  92 | -`GET /rest/getutxos/<checkmempool>/<txid>-<n>/<txid>-<n>/.../<txid>-<n>.<bin|hex|json>`
  93 | +`GET /rest/getutxos/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>` <br>

jonatack commented at 7:03 PM on March 6, 2022:

Extra space before each of the two <br> tags

in doc/REST-interface.md:95 in aea690ab8d outdated

  94 | +`GET /rest/getutxos/checkmempool/<TXID>-<N>/<TXID>-<N>/.../<TXID>-<N>.<bin|hex|json>`
  95 |  
  96 | -The getutxo command allows querying of the UTXO set given a set of outpoints.
  97 | -See BIP64 for input and output serialisation:
  98 | -https://github.com/bitcoin/bips/blob/master/bip-0064.mediawiki
  99 | +The getutxos endpoint allows querying the UTXO set given a set of outpoints.

jonatack commented at 7:04 PM on March 6, 2022:

if you retouch

The getutxos endpoint allows querying the UTXO set, given a set of outpoints.

and also s/option/option,/

theStack commented at 9:56 AM on March 8, 2022:

Thanks, done.

shaavan approved

shaavan commented at 3:36 AM on March 7, 2022: contributor

ACK aea690ab8d09ce3af841862688e9cdaedaba2658

All the changes are finely documented in the commit message, making the changes very clear to understand.
The added line break makes the rendered output cleaner and easier to understand.

in doc/REST-interface.md:39 in aea690ab8d outdated

  35 | @@ -36,7 +36,7 @@ By default, this endpoint will only search the mempool.
  36 |  To query for a confirmed transaction, enable the transaction index via "txindex=1" command line / configuration option.
  37 |  
  38 |  #### Blocks
  39 | -`GET /rest/block/<BLOCK-HASH>.<bin|hex|json>`
  40 | +`GET /rest/block/<BLOCK-HASH>.<bin|hex|json>` <br>

luke-jr commented at 1:43 AM on March 8, 2022:

Markdown is supposed to be readable as plain text.

Make this a list instead?

theStack commented at 9:58 AM on March 8, 2022:

Right, this also explains why there is no other instance of <br> in any of our Markdown docs. Changed it to a list (here an for the blocks endpoint), as you suggested.

luke-jr changes_requested

theStack force-pushed on Mar 8, 2022

theStack commented at 9:59 AM on March 8, 2022: contributor

Took the suggestions by jonatack (https://github.com/bitcoin/bitcoin/pull/24467#discussion_r820273828) and luke-jr (https://github.com/bitcoin/bitcoin/pull/24467#discussion_r821251526) and force-pushed.

in doc/REST-interface.md:96 in 9b9149b2f0 outdated

  95 |  
  96 | -The getutxo command allows querying of the UTXO set given a set of outpoints.
  97 | -See BIP64 for input and output serialisation:
  98 | -https://github.com/bitcoin/bips/blob/master/bip-0064.mediawiki
  99 | +The getutxos endpoint allows querying the UTXO set, given a set of outpoints.
 100 | +With the /checkmempool/ option, the mempool is also taken into account.

shaavan commented at 2:43 PM on March 27, 2022:

It would be better to write /checkmempool/ under the code tag, as this is part of a line of code and would help make it adequately visible with other text around.

With the `/checkmempool/` option, the mempool is also taken into account.

theStack commented at 3:29 PM on March 30, 2022:

Thanks, I agree that this improves the visibility. Done.

shaavan commented at 2:50 PM on March 27, 2022: contributor

Changes since my last review:

<br> tag is removed and is replaced by using the list.
, are added at appropriate positions.

I think both of these changes improve the file and the rendered output.

However, I would like to give another suggestion that would help make this PR better.

doc: minor improvements in getutxos REST endpoint synopsis

Describing an optional sub-path as <checkmempool> in the synopsis could
be misleading as the angle brackets normally indicate that the field has
to be replaced a custom value. Clarify that by showing two variants
instead, similar to the block endpoint with the notxdetails option.

Further improvements:

- uppercase <TXID> and <N>, to match the description of the other endpoints
- s/getutxo command/getutxos endpoint/
- describe what the checkmempool option does
- s/serialisation/serialization/ (the US spelling is more dominant than
  the UK spelling in the project, and there is indeed no other instance
  of the string "serialis*" in the source tree, except once in a release
  note)
- link to BIP64 within the text instead of only showing bare URL
- mention that BIP64 is only relevant for bin and hex output formats
- show two endpoint formats of the block section as list

c456302d42

theStack force-pushed on Mar 30, 2022

DrahtBot commented at 12:57 PM on September 23, 2022: contributor

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

Conflicts

No conflicts as of last run.

stickies-v approved

stickies-v commented at 7:20 PM on October 12, 2022: contributor

ACK c456302d4258e3abc4b8afde20fba808632771b2 - also checked that current master (cc12b8947) doesn't have any other lines changes that would require updates as per the outlined improvement points.

fanquake merged this on Oct 13, 2022

fanquake closed this on Oct 13, 2022

theStack deleted the branch on Oct 13, 2022

sidhujag referenced this in commit 5b26ae4319 on Oct 23, 2022

bitcoin locked this on Oct 13, 2023