rfq: fix precision issue in HTLC compliance check #1194
Conversation
Pull Request Test Coverage Report for Build 11888540506Details
💛 - Coveralls |
| // value of the asset. So we increase it by exactly one asset unit to | ||
| // ensure that the fee logic in lnd does not reject the HTLC. | ||
| roundingCorrection := rfqmath.NewBigIntFromUint64(1) | ||
| assetAmt = assetAmt.Add(roundingCorrection) |
There was a problem hiding this comment.
Would adding an entire asset unit be acceptable in the scenario where where one asset unit is a considerable amount / value to the user? e.g. the amount represented an 1e8 sats. Or are the rounding errors only expected when the values are de minis?
What other methods might be available to address the source of the rounding errors?
There was a problem hiding this comment.
just to clarify: this PR doesn't "fix" any rounding related code. It just adjusts a check to allow an HTLC to be forwarded that is affected by the rounding down. The rounding down will always happen, due to the integer nature of taproot assets (see RFQ/decimal display doc for more info).
The decision we can make is: do we force the sender to over pay by one unit so that the ultimate receiver always sees the expected (or even one unit more) balance? Or do we allow under payment of one unit, leading to the scenario where the ultimate receiver sees a final balance lower than expected.
But since this only fixes a check, I don't think we should block this PR and look at the decision holistically.
rfq/order.go
Outdated
| @@ -297,6 +297,12 @@ func (c *AssetPurchasePolicy) CheckHtlcCompliance( | |||
| return fmt.Errorf("error summing asset balance: %w", err) | |||
| } | |||
|
|
|||
| // Due to rounding errors, we may slightly underreport the incoming | |||
| // value of the asset. So we increase it by exactly one asset unit to | |||
| // ensure that the fee logic in lnd does not reject the HTLC. | |||
There was a problem hiding this comment.
fee logic in lnd
This is a somewhat confusing comment to me. Lnd fee logic doesn't come into play in this compliance check, does it? We check the inboundAmount against the outbound amount a few lines down and proceed accordingly.
There was a problem hiding this comment.
You're right, this was Copilot suggesting a comment based on other instances in this file 😅 In some places we report the converted value back to lnd and then the fee logic there decides if an HTLC pays enough fees. But not in this case. Updated the comment.
| // Due to rounding errors, we may slightly underreport the incoming | ||
| // value of the asset. So we increase it by exactly one asset unit to | ||
| // ensure that the fee logic in lnd does not reject the HTLC. | ||
| roundingCorrection := rfqmath.NewBigIntFromUint64(1) |
There was a problem hiding this comment.
Can we always assume the rounding error is less then 1 asset unit?
There was a problem hiding this comment.
Yes, the imprecision of an integer representation of a division can at most be 0.99999......, since we're strictly cutting off any fractional part after the division.
Due to the integer nature of asset units, when converting to milli-satoshi and back, we might be off by a fraction of an asset unit when comparing to the milli-satoshi amount of an HTLC. We'll add a single asset unit to the reported inbound amount when checking in-out amount compliance to allow for those asset rounding imprecisions. In a real-world scenario where an asset has a decimal display value of 6, this would result in an imprecision of fractions of cents and be earned back by the spread on the actual conversion itself.
guggero
force-pushed
the
rfq-rounding
branch
from
a644655 to
dafd55e
Compare
rfq: fix precision issue in HTLC compliance check
Due to the integer nature of asset units, when converting to milli-satoshi and back, we might be off by a fraction of an asset unit when comparing to the milli-satoshi amount of an HTLC. We'll add a single asset unit to the reported inbound amount when checking in-out amount compliance to allow for those asset rounding imprecisions. In a real-world scenario where an asset has a decimal display value of 6, this would result in an imprecision of fractions of cents and be earned back by the spread on the actual conversion itself.