Skip to content
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

jsonrpc: Remove the old "_msat" prefix in the listpeerchannels command #6245

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented May 9, 2023

This is a regression that we introduced in this release
due to some dirty parts of our codebase.

For historical reasons (I think), we were using a json_add_sat_only
procedure defined in peer_control.c. So when @rustyrussell removed the _msat,
we thought that all the fields were reflecting the new behavior, but
we were wrong.

This PR fixes this bug and also removes the additional function
from peer_control.c. This way, we can be sure that there is no other part
of our codebase that uses this method (except for other json_add methods).

Fixes: #6244
Reported-by: @hMsats
Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com

This is a regression that we introduced in this release
due to some dirty parts of our codebase.

For historical reasons (I think), we were using a `json_add_sat_only`
procedure defined in `peer_control.c`. So when @rustyrussell removed the _msat,
we thought that all the fields were reflecting the new behavior, but
we were wrong.

This PR fixes this bug and also removes the additional function
from `peer_control.c`. This way, we can be sure that there is no other part
of our codebase that uses this method (except for other `json_add` methods).

Link: ElementsProject#6244
Reported-by: @hMsats
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@endothermicdev
Copy link
Collaborator

Does this upset the schema deprecation logic? It might be useful to rebase on #6242 to check. LGTM otherwise.

@vincenzopalazzo vincenzopalazzo changed the title Macros/other msat purge jsonrpc: Remove the old "_msat" prefix in the listpeerchannels command May 9, 2023
@vincenzopalazzo
Copy link
Collaborator Author

I do not know if the #6242 will be go in for this release.

For sure this is a bug fix and will go in, but the other one i do not know.

Does this upset the schema deprecation logic?

Mh what do you mean? the _msat should be gone in this release right?

@ShahanaFarooqui
Copy link
Collaborator

I do not know if the #6242 will be go in for this release.

I am leaning towards not adding this one for May release.

For sure this is a bug fix and will go in, but the other one i do not know.

Does this upset the schema deprecation logic?

Mh what do you mean? the _msat should be gone in this release right?

I believe this one is a good candidate for May release to save developers from bumping into msat removal issues in future.

@endothermicdev
Copy link
Collaborator

Disregard my previous comment. Just wanted to make sure it would pass make check-source but the test I was thinking of only applies if the schema itself is changed, so no need to worry about that here.

the _msat should be gone in this release right?

Definitely. ACK 6f9d0ce

@hMsats
Copy link
Contributor

hMsats commented May 9, 2023

@ShahanaFarooqui maybe also add to CHANGELOG.md that value (unit: sats) has been removed from listfunds (and now amount_msat should be used (unit: msats)).

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 6f9d0ce

@ShahanaFarooqui
Copy link
Collaborator

Changelog-Removed: JSON-RPC: the "msat" suffix on millisatoshi fields, as deprecated in v0.12.0.

@ShahanaFarooqui ShahanaFarooqui merged commit 3a2b703 into ElementsProject:master May 9, 2023
9 of 12 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/other_msat_purge branch May 10, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Still some msat suffixes
5 participants