doc: add guidance for RPC to developer notes #30142
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. |
|
|
Adds guidance statements to the RPC interface, including data type selection and usage of deprecatedrpc.
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
|
||
| A few guidelines for modifying and reviewing existing RPC interfaces: | ||
|
|
||
| - When modifying RPC interface JSON structure, implement associated `-deprecatedrpc=` option to retain previous RPC behavior, including (but not limited to) the following: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Include detailed instructions on feature deprecation and re-enabling in release notes. |
There was a problem hiding this comment.
nit: this sentence might be better structured starting with a verb:
Implement associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying RPC interface JSON structure, including (but not limited to) the following:
| @@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces: | |||
| - *Rationale*: JSON strings are Unicode strings, not byte strings, and | |||
| RFC8259 requires JSON to be encoded as UTF-8. | |||
|
|
|||
| - When choosing data types for RPC JSON, prefer data types that are flexible to expansion without change of data type. For example, `{"result":{"warnings":[]}}` rather than `{"result":{"warnings":""}}`, which allows for multiple warnings to be included as array elements. | |||
There was a problem hiding this comment.
nit: this sentence might be better structured starting with a verb:
Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.
|
ACK 61853a2. |
| @@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces: | |||
| - *Rationale*: JSON strings are Unicode strings, not byte strings, and | |||
| RFC8259 requires JSON to be encoded as UTF-8. | |||
|
|
|||
| - When choosing data types for RPC JSON, prefer data types that are flexible to expansion without change of data type. For example, `{"result":{"warnings":[]}}` rather than `{"result":{"warnings":""}}`, which allows for multiple warnings to be included as array elements. | |||
There was a problem hiding this comment.
how is this different from "- Try to make the RPC response a JSON object."?
The "warnings" example here doesn't make much sense, because both are equally flexible to expansion. (See the plural s.) The reason why array should be preferred over string is that array is the correct type to represent an array. (But that is obvious, isn't it?)
Adds guidance statements to the RPC interface section of the developer notes for data type selection and examples of when to implement
-deprecatedrpc=.Wanted to increase awareness of preferred RPC implementation approaches for newer contributors.
This implements some of what's discussed in #29912 (comment)
I'm sure opinions will differ, so please don't be shy. We want to make RPC as solid/safe as possible.
Example of a discussion where guidelines/context might have added value:
#29845 (comment)