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

Remove pyln-client json monkey patch for _msat fields to Millisatoshis #6865

Conversation

rustyrussell
Copy link
Contributor

This was interfering with other JSON usages (confusingly!) such as clnrest. We only did it because _msat fields used to be strings.

Leaves the generic fallback to ".to_json" to marshall into JSON though.

@rustyrussell rustyrussell added this to the v24.02 milestone Nov 13, 2023
Since the class interacts with int very well now, we don't have to
make explicit accesses, so it's easy to write code which works with
Millisatoshi or int.

Also, don't access rpc.decoder in one test, it's unnecessary.

Signed-off-by: Rusty Russell <[email protected]>
Now _msat fields are all integers (last conversion 23.08) we can simply
leave them alone, rather than trying to convert them.

And for turning Millisatoshi into JSON, we simply globally replace the
default encoding function to try ".to_json()" on items, which allows
anything to be marshalled.

The global replacement was interfering with other uses of JSON, such
as the clnrest plugin.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: pyln-client: no longer autoconverts _msat field to Millisatoshi class (leaves as ints).
@rustyrussell rustyrussell force-pushed the guilt/remove-json-monkey-patch branch from bc447b2 to 1123fee Compare December 16, 2023 02:01
@rustyrussell
Copy link
Contributor Author

Trivial rebase

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 1123fee

@vincenzopalazzo vincenzopalazzo merged commit 09c1cfd into ElementsProject:master Dec 16, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants