Skip to content

Conversation

kiminuo
Copy link
Contributor

@kiminuo kiminuo commented Feb 14, 2022

This PR attempts to replicate

{RPCResult::Type::STR, "type", "The output type (e.g. "+GetAllOutputTypes()+")"},
to one other place (at the moment) so that users have better idea what RPC methods can actually return.

I created this PR as a follow-up to the idea mentioned here #23320 (comment) (resolved).

@kiminuo kiminuo changed the title Improve RPC help by explicitly mentioning output types rpc: Improve RPC help by explicitly mentioning output types Feb 14, 2022
@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 14, 2022

cc @laanwj given he said #23320 (comment).

@kiminuo kiminuo marked this pull request as ready for review February 15, 2022 11:19
@laanwj
Copy link
Member

laanwj commented Feb 15, 2022

Concept ACK

Edit: diff:

--- /tmp/getblock.1     2022-02-15 13:33:21.436527762 +0100
+++ /tmp/getblock.2     2022-02-15 13:48:38.017920912 +0100
@@ -66,7 +66,7 @@
               "asm" : "str",             (string) The asm
               "hex" : "str",             (string) The hex
               "address" : "str",         (string, optional) The Bitcoin address (only if a well-defined address exists)
-              "type" : "str"             (string) The type, eg 'pubkeyhash'
+              "type" : "str"             (string) The type (nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_scripthash, witness_v0_keyhash, witness_v1_taproot, witness_unknown)
             }
           }
         },

@michaelfolkson
Copy link

Concept ACK

Seems like an improvement to display all the output types in the RPC help rather than just the pubkeyhash example.

@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 15, 2022

Does it make sense to you to use GetAllOutputTypes where possible. I mean in RPC help sections?

Is the output OK? Or are possibly any output types that you would NOT show?

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK 78d9227

@theStack
Copy link
Contributor

Concept ACK

@kiminuo kiminuo force-pushed the feature/2022-02-output-types branch from 78d9227 to c9ca3e8 Compare February 20, 2022 20:40
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK c9ca3e8

Copy link
Contributor

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

Approach ACK c9ca3e8

@kiminuo kiminuo force-pushed the feature/2022-02-output-types branch from c9ca3e8 to c821ab8 Compare February 21, 2022 11:57
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

re-ACK c821ab8

@maflcko maflcko merged commit 1337b93 into bitcoin:master Feb 21, 2022
@kiminuo kiminuo deleted the feature/2022-02-output-types branch February 21, 2022 13:05
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants