The "data" field without outputs was marked as "required" in the help docs when using bitcoin-cli. This field when left off worked as an intended optional OP_RETURN. closes #27828.
Motivation: It is hard to understand that "data" is actually optional for commands like createpsbt and walletcreatefundedpsbt.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
I get that this is confusing, but this change isn't correct.
The "data" field itself is required when the object is present (as in [{"address": amount, ..., {}] would not be valid). What's optional is the object (the { ... } around it), which is at a higher level.
Maybe it's worth elaborating in the prose text above?
I get that this is confusing, but this change isn't correct.
The
"data"field itself is required when the object is present (as in[{"address": amount, ..., {}]would not be valid). What's optional is the object (the{ ... }around it), which is at a higher level.Maybe it's worth elaborating in the prose text above?
Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent output is required and some of it's children elements are optional. From my perspective what I would expect to see is a required "outputs" and within outputs there being required and optional fields.
For instance: "address": amount, (numeric or string, required makes sense as address and amount are necessary.
But "data": "hex", (string, required) seems like a bug because this is not required and can be left out. If you specify "data" then of course there will be a required value for it, but this is implied with how json works.
Full text of for outputs as reference:
2. outputs (json array, required) The outputs (key-value pairs), where none of the keys are duplicated.
That is, each address can only appear once and there can only be one 'data' object.
For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
accepted as second parameter.
[
{ (json object)
"address": amount, (numeric or string, required) A key-value pair. The key (string) is the bitcoin address,
the value (float or string) is the amount in BTC
...
},
{ (json object)
"data": "hex", (string, required) A key-value pair. The key must be "data", the value is hex-encoded data
},
...
]
The "data": ... part cannot be left out. It's the {"data": ...} that can be left out.
The
"data": ...part cannot be left out. It's the{"data": ...}that can be left out.
Let me rephrase my question:
How would someone know {"data" ...} is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?
Does there need to be a new idea of specifying whether the (json object) itself is optional?
Edit: Is this what "prose text" means?
@sipa, edited the commit. Something like this look good?
ACK 30acffa32023838d7b0c3f48d2a3688fae15490a
148 | @@ -149,6 +149,7 @@ static std::vector<RPCArg> CreateTxDoc() 149 | }, 150 | {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n" 151 | "That is, each address can only appear once and there can only be one 'data' object.\n" 152 | + "Either {\"data\"} or {\"address\"}, are required, but specifying both is optional.\n"
I find this wording confusing. Do you mean to say that the user can combine data and address entries? Or that you can provide data or an address directly without putting "data" or "address" in front of it? (but then how it does it know the difference?)
Examples can be helpful. Maybe say "see send RPC for examples" and then expand the send RPC docs with said examples.
Do you mean to say that the user can combine data and address entries?
The outputs array, outputs [], must contain at minimum either an obj containing {"address" ".."} or an obj containing {"data": ".."}. You can also specify both, but you must have at least one.
I'm reading over this block of help text again.
(json array, required) The outputs (key-value pairs), where none of the keys are duplicated.
That is, each address can only appear once and there can only be one 'data' object.
For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
accepted as second parameter.
just using separate objs
[
{
"bc1address1":"0.01",
},
{
"bc1address2":"0.02",
},
{
"data":"ff00ff00ff"
}
]
or...
Also note:
a dictionary, which holds the key-value pairs directly, is also accepted as second parameter.
I believe this is saying you can also do something like:
[
{
"bc1address1":"0.01",
"bc1address2":"0.02",
"data":"ff00ff00ff"
}
]
I think this style is for compatibility reasons per docs, so I'm guessing it's preferred/future proof to break everything out into individual objects. Please correct me if I'm wrong!
Current:
"Either {\"data\"} or {\"address\"}, are required, but specifying both is optional.\n"
Something like this be better?:
"At least one object containing the key \"data\" or \"address\" is required.\n"
How about:
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.
@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.
I think you have correctly identified that this text could be improved and we should move forward with the PR.
@miketwenty1 Needs rebase. Are you still working on this?
@jonatack thanks for the bump. Open for feedback. I took @murchandamus suggestions where it seemed appropriate.
fixing typo
I think the CI failure is unrelated to your change (see #28027).
tACK 27b168b81f3959f5376a4176673187a71c7b25e1
I didn't think about it very deeply, but the new text is an improvement in clarity.
I think the CI failure is unrelated to your change (see #28027, cc @achow101).
ACK 27b168b81f3959f5376a4176673187a71c7b25e1