Small fixes: dual-funding #4295
Conversation
| """ | ||
| return Decimal(self.millisatoshis) / 1000 | ||
| return (Decimal(self.millisatoshis) + 999) // 1000 |
There was a problem hiding this comment.
import math
Decimal(math.ceil(self.millisatoshi / 1000))There was a problem hiding this comment.
I'm not sure that we should just be rounding up or down on a whim, we need to keep the Millisatoshi as precise as possible, and then let the final step in the computation specify whether we want to round up or down.
Any attempt to do so implicitly will likely cause issues because what might seem intuitive now, may not be when looked at from another side.
There was a problem hiding this comment.
Imo it's a mistake to return a non-rounded response from to_satoshi -- a partial value satoshi isn't correct. Taking a millisatoshi to a satoshi should be the last step in any millisatoshi operations. If you wanted precision you'd have left it in Millisatoshi format.
There was a problem hiding this comment.
Maybe the better solution here is to change the interface to accept an optional whole argument.
e.g.
def to_satoshi(self, whole=False)
There was a problem hiding this comment.
Agreed, however you're also inverting the semantics of the rounding of an existing API, which may break callers. Adding the whole argument doesn't make it less breaking :-)
I was thinking more of deprecating to_satoshi as is and then add floor() and ceil() methods to the Millisatoshi class that returns the msat amount rounded in the desired direction.
There was a problem hiding this comment.
I think if to_satoshi returns a Decimal we should not round internally at all, and defer the rounding to the caller.
There was a problem hiding this comment.
You're right, modifying to_satoshi is the wrong approach, especially given that it does return the Decmial type.
Added a new method to_whole_satoshi, which returns an int type.
There was a problem hiding this comment.
Fwiw I wasn't a hugely into adding a call to import math, but if you think it's better than the inline math version I'd be happy to update/change it.
openingd/dualopend.c
Outdated
| @@ -585,7 +585,13 @@ static char *check_balances(const tal_t *ctx, | |||
| initiator_weight); | |||
|
|
|||
| if (!amount_sat_greater_eq(accepter_diff, accepter_fee)) { | |||
| return "accepter fee not covered"; | |||
| return tal_fmt(ctx, "accepter fee %s not covered %s", | |||
There was a problem hiding this comment.
Would be clearer by showing the comparison directly:
"accepter fee not covered (need %s > have %s)"
niftynei
force-pushed
the
nifty/df-rbf-tweaks
branch
from
d339e4b
to
0646ffa
Compare
niftynei
force-pushed
the
nifty/df-rbf-tweaks
branch
from
0646ffa
to
b9d93bc
Compare
We need to use it for the 'df_accepter' plugin, so we get the feerate correct. Changelog-Added: pyln-client: `fundpsbt`/`utxopsbt` now support `min_witness_weight` param
|
Updated. |
A fractional satoshi value isn't really useful; rounding up loses precision but that's why you called "whole satoshi", wasn't it? Changelog-Changed: pyln-client: Millisatoshi has new method, `to_whole_satoshi`; *rounds value up* to the nearest whole satoshi
Makes things a bit cleaner elsewhere
We're the accepter, so we look up which inputs we need to sign and only sign those.
niftynei
force-pushed
the
nifty/df-rbf-tweaks
branch
from
b9d93bc
to
5617305
Compare
|
ACK 5617305 |
A few small things found while testing the dual-funding pathways.