-
Notifications
You must be signed in to change notification settings - Fork 901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove excessive wrapping in the rpc_command
hook call
#3560
Conversation
rpc_command
hook call
@@ -611,8 +611,21 @@ struct rpc_command_hook_payload { | |||
static void rpc_command_hook_serialize(struct rpc_command_hook_payload *p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this commit needs a Changelog-Deprecated line ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated libwally failure
ACK 206e197
|
||
#ifdef COMPAT_V081 | ||
if (deprecated_apis) | ||
json_add_tok(s, "rpc_command", p->request, p->buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh i came across this yesterday. can you remove the outer object and instead do this line? it should achieve the same result without needing to iterate through the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do that, however if I use json_add_tok
to fill in {"rpc_command": <copy>}
I cannot re-enter the rpc_command
key to then add the compat version, i.e., {"rpc_command": {"rpc_command": <copy>, <copied-fields}}
. Once we remove the COMPAT_V081
flag we can actually do this and avoid iterating through, but until then I'm afraid we need to keep this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack. i understand what this outputs now. thanks.
e0b9ecd
to
81ad2a5
Compare
We were nesting like the following: ```json {"params": { "rpc_command": { "rpc_command": { } } } ``` This is really excessive, so we unwrap once, and now have the following: ```json {"params": { "rpc_command": { } } ``` Still more wrapping than necessary (the method is repeated in the `params` object), but it's getting closer. Changelog-Deprecated: JSON-RPC: Removed double wrapping of `rpc_command` payload in `rpc_command` JSON field. Suggested-by: @fiatjaf Signed-off-by: Christian Decker <@cdecker>
Rebased and squashed :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack 6f78c31
We were nesting like the following:
This is really excessive, so we unwrap once, and now have the following:
Still more wrapping than necessary (the method is repeated in the
params
object), but it's getting closer.
Suggested-by: @fiatjaf
Signed-off-by: Christian Decker <@cdecker>
Fixes #3558