fusion44 / blitz_api

A management backend for the RaspiBlitz project written in Python / FastAPI
MIT License
19 stars 18 forks source link

RFC: Improve amount handling #200

Open fusion44 opened 1 year ago

fusion44 commented 1 year ago

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.

rootzoll commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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