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

RFC: Improve amount handling #200

Open
fusion44 opened this issue Apr 23, 2023 · 3 comments
Open

RFC: Improve amount handling #200

fusion44 opened this issue Apr 23, 2023 · 3 comments
Labels
enhancement New feature or request investigate Every issue that requires further investigation REST An issue only appearing when using the REST interface v0.6.0-beta
Milestone

Comments

@fusion44
Copy link
Owner

fusion44 commented Apr 23, 2023

Currently the handling of sats amounts is not clear. Sometimes the value has to be given as msat and sometimes as whole sats.

My proposal would be that each endpoint taking an amount as parameter requires an Amount object instead of an integer:

class Amount(BaseModel):
    btc: float = Query(None, description="The amount in BTC.")
    sat: int = Query(None, description="The amount in sat.")
    msat: int = Query(None, description="The amount in msat.")

When giving this parameter, only one of the fields must be set. When the user provides an amount in sat or btc, the endpoint converts it to to the appropriate and passes it to the implementation. The parameter would be passed as {"msat": 345678000}

Alternative approach would be to pass it as a string:

msat without the appendix would be default
as msat:   345678000
as btc:    0.00345678btc
as sat:    345678sat
as msat:   345678000msat

Of course this means that clients must be changed to support this, but currently we only have the WebUI as a user, so it should be fine.

A similar model will be used when the API returns an amount value.

@fusion44 fusion44 added enhancement New feature or request v0.6.0-beta REST An issue only appearing when using the REST interface investigate Every issue that requires further investigation labels Apr 23, 2023
@fusion44 fusion44 added this to the V0.6.0-beta milestone Apr 23, 2023
@rootzoll
Copy link
Collaborator

I like the idea of an object that kind of autoconverts - that way also in the code its much clearer what we are handleing.

@fusion44
Copy link
Owner Author

One unfortunate thing about this would be the increased amount of data that would be pushed through the wires.

I'm currently leaning towards the following:

Amount input:

class AmountInput(BaseModel):
    btc: float = Query(None, description="The amount in BTC.")
    sat: int = Query(None, description="The amount in sat.")
    msat: int = Query(None, description="The amount in msat.)

Only one of the parameters can be non-null.

Amount result:

Class AmountResult(BaseModel):
    sat: int = Query(None, description="The amount in sat.")
    msat: int = Query(None, description="The amount in msat.)

This would mean that the client would always get back an object with sat and msat. Implementations handle this in a very inconsistent way, but this covers all cases I have seen. The BTC value can be calculated on the client side if desired.

Of course, none of this relieves us of the need to actually do the conversion of the correct values from what we get from the implementation...

@cstenglein what do you think? It probably means a lot of work on both frontend and backend, but it would make the system much more intentional and less prone to errors. I don't want to introduce a new API version just for this, but on the other hand, this is a breaking change.

@cstenglein
Copy link
Collaborator

One unfortunate thing about this would be the increased amount of data that would be pushed through the wires.

It isn't much data, so I don't think that's an issue

what do you think?

Let's do it. But would push that to 1.10 earliest, not 1.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request investigate Every issue that requires further investigation REST An issue only appearing when using the REST interface v0.6.0-beta
Projects
None yet
Development

No branches or pull requests

3 participants