Part 1: RFQ uses fixed-points #1136

ffranr · 2024-09-24T18:19:10Z

The high-level goal of this change is to represent asset-to-BTC conversion rates using the new fixed-point number type. This PR updates several "price" related fields that were previously typed as milli-satoshis, which were used as a workaround in the Proof of Concept (PoC).

It is part of the ongoing effort to resolve: #871.

The ultimate goal is to align tapd with these BLIP changes: Roasbeef/blips#4

Note to Reviewers

This is the first phase of the fixed-point refactor, targeting the approved-rfq-fixedpoint branch. This branch will accumulate all approved changes required to close #871 before being merged into main.

There are several TODO(ffranr): Temp solution. comments in this PR, which will be fully addressed in subsequent PRs before merging approved-rfq-fixedpoint into main.

Please review the commits in sequence. Note that some commit diffs may overlap slightly.

This change is

coveralls · 2024-09-24T18:39:05Z

Pull Request Test Coverage Report for Build 11073777673

Details

66 of 281 (23.49%) changed or added relevant lines in 13 files are covered.
41 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.1%) to 40.299%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapcfg/server.go	0	1	0.0%
rfqmsg/accept.go	0	2	0.0%
tapchannel/aux_invoice_manager.go	0	4	0.0%
rfqmsg/request.go	0	5	0.0%
rfqmsg/records.go	21	29	72.41%
rfqmsg/messages.go	19	29	65.52%
rfqmsg/buy_accept.go	0	19	0.0%
rpcserver.go	0	20	0.0%
rfqmsg/buy_request.go	0	21	0.0%
rfq/order.go	0	22	0.0%

Files with Coverage Reduction	New Missed Lines	%
rfqmsg/buy_request.go	2	0.0%
rfqmsg/buy_accept.go	2	4.62%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.87%
commitment/tap.go	4	83.91%
rpcserver.go	4	0.0%
rfq/negotiator.go	6	3.19%
universe/interface.go	15	47.09%

Totals
Change from base Build 11038470643:	-0.1%
Covered Lines:	24294
Relevant Lines:	60284

💛 - Coveralls

dstadulis · 2024-09-24T18:39:17Z

Current progress of feeding fixedpoints through the code, and building the commit structure through

ffranr · 2024-09-27T15:55:09Z

I need to re-write this so that we use BigIntFixedPoint for all "internal" tapd fixed-points and then use Uint64FixedPoint when we export/import via RPC or wire messages.

We must use BigIntFixedPoint for all internal arithmetic calculations.

Roasbeef

Nice work! In particular, I found the commit structure very easy to follow.

I think my main comments are related to the usage of the new FixedPoint integer, namely:

The main constructor should always be used. We take a value, then scale that, to arrive at the coefficient. At times I see a value (which may have already been scaled) used directly to create the struct, by passing the constructor. I think we should make the fields private to force everything to use the constructor.
- If this isn't done properly, then we'll end up manipulating a doubly or even triply scaled value. The eventual unit tests should also serve to bind this to make sure conversion is happening properly everywhere.
We only want to use the big int version. The uint64 version was added mainly for completions sake.

rfqmsg/messages.go

rfqmsg/messages_test.go

docs/examples/basic-price-oracle/main.go

Roasbeef · 2024-10-01T23:49:22Z

rfqmath/arithmetic.go

+// integer is negative or exceeds the maximum value for `uint64`.
+func (b BigInt) ToUint64Safe() (uint64, error) {
+	if !b.value.IsUint64() {
+		return 0, fmt.Errorf("cannot convert %s to uint64",


We should make this into a concrete error so future tests can assert agains it.

Also does this need to be added to the main interface?

I'm not sure if it should be added to the interface. The long term solution as I see it is to replace ToUint64 with ToUint64Safe. So end up with ToUint64() (uint64, error) in the interface. But I didn't want to do that here because that would change quite a few lines.

rpcserver.go

Roasbeef · 2024-10-02T00:12:11Z

taprpc/marshal.go

-		// TODO(ffranr): Temp solution.
-		AskPrice: event.AssetRate.Coefficient.ToUint64(),
-		Expiry:   event.Expiry,
+		AskAssetRate: &rfqrpc.FixedPoint{


Same here, the constructors should always be used. This is how we make sure that Coefficient is the right size internally. We want to scale up with the constructor, not before.

Would you like me to create a marshal constructor for the fixed-point RPC type?

Yeah, maybe just a rfqrpc.MarshalFixedPoint that does exactly this. Just to make sure we don't accidentally forget the scale or coefficient (however unlikely that is).

Roasbeef · 2024-10-02T00:13:49Z

rfqmath/convert.go

@@ -62,6 +62,10 @@ func MilliSatoshiToUnits[N Int[N]](milliSat lnwire.MilliSatoshi,
 // compute the total amount of mSAT (X) as follows:
 //   - X = (U / Y) * M
 //   - where M is the number of mSAT in a BTC (100,000,000,000).
+//
+// TODO(ffranr): This function only works with BigInt as the underlying


We only want to use the big int version. The other was added just to have another implementation, and also demonstrate where the unit64 will lose precision.

With this TODO, in a later PR, I hope to make UnitsToMilliSatoshi specific to BigInt fixed-points by removing the generic type parameter.

rfqmsg/sell_request.go

GeorgeTsagk

Lgtm

rfqmsg/buy_request.go

lightninglabs-deploy · 2024-10-10T22:42:28Z

@Roasbeef: review reminder

Roasbeef · 2024-10-11T10:46:59Z

Ok, discussed offline, and for now re format, we'll:

Figure out a proper format for the byte serialization of the fixed point instance.
As a stop gap, we'll:
- Write a property based test for the new intermediate value, to ensure that we never lose scale.
- On the wire, just use the big.Int serialization.
- For the RPC interface, write the scale and coefficient values a strings.

Roasbeef

Reviewed 30 of 30 files at r1, 1 of 29 files at r4.
Reviewable status: 2 of 30 files reviewed, 27 unresolved discussions (waiting on @ffranr)

rpcserver.go line 7174 at r4 (raw file):

	// Convert the asset amount into a fixed-point.
	assetAmount := rfqmsg.NewBigIntFixedPoint(req.AssetAmount, 0)

This should be using the decimal display of the given asset.

rfq/order.go line 128 at r4 (raw file):

	// Check that the HTLC amount is not greater than the negotiated maximum
	// amount.
	maxAssetAmount := rfqmsg.NewBigIntFixedPoint(c.MaxAssetAmount, 0)

Same here, should be using the proper scale value (from the decimal display).

This commit introduces the `BigInt.String` method, which can be used to safely (without overflow) export a `BigInt` to RPC or JSON.

ffranr · 2024-10-15T12:15:52Z

Ok, discussed offline, and for now re format, we'll:

* Figure out a proper format for the byte serialization of the fixed point instance.

* As a stop gap, we'll:
  
  * Write a property based test for the new intermediate value, to ensure that we never lose scale.
  * On the wire, just use the `big.Int` serialization.
  * For the RPC interface, write the scale and coefficient values a strings.

  * Write a property based test for the new intermediate value, to ensure that we never lose scale.
  * On the wire, just use the `big.Int` serialization.

This two points are relevant to the part 2 PR: #1141
This PR doesn't include any any RFQ wire messages changes.

For the RPC interface, write the scale and coefficient values a strings.

This change is included in this PR.

ffranr · 2024-10-15T12:48:03Z

Reviewed 30 of 30 files at r1, 1 of 29 files at r4.
Reviewable status: 2 of 30 files reviewed, 27 unresolved discussions (waiting on @ffranr)

rpcserver.go line 7174 at r4 (raw file):
	// Convert the asset amount into a fixed-point.
	assetAmount := rfqmsg.NewBigIntFixedPoint(req.AssetAmount, 0)
This should be using the decimal display of the given asset.

rfq/order.go line 128 at r4 (raw file):
	// Check that the HTLC amount is not greater than the negotiated maximum
	// amount.
	maxAssetAmount := rfqmsg.NewBigIntFixedPoint(c.MaxAssetAmount, 0)
Same here, should be using the proper scale value (from the decimal display).

@Roasbeef

In both of these comments I think you're asking me to construct the fixed-points using the asset's decimal display (instead of just using a scale of 0 and setting the coefficient directly).

I don't see why we should care about the decimal display value here. I think the value of the fixed-points remains the same, and the calculation is unaffected. IMO looking up the decimal display introduces an unnecessary potential for error.

Why would we need the asset's decimal display here?

guggero

Nice work! I think this is pretty close, with the exception of two uses of 0 as the scale that aren't marked with // TODO: temp solution.

Perhaps those instances could be partially recreated in a unit test to show they do the right thing. Properly executing the specific logic within a unit or integration test is probably pretty hard. So just a demo of the conversion part in a unit test should give us some confidence with some real-world number examples that are easy to follow as mere humans.

docs/examples/basic-price-oracle/go.mod

rfq/oracle.go

taprpc/priceoraclerpc/price_oracle.proto

guggero · 2024-10-16T11:09:40Z

taprpc/marshal.go

-		// TODO(ffranr): Temp solution.
-		AskPrice: event.AssetRate.Coefficient.ToUint64(),
-		Expiry:   event.Expiry,
+		AskAssetRate: &rfqrpc.FixedPoint{


Yeah, maybe just a rfqrpc.MarshalFixedPoint that does exactly this. Just to make sure we don't accidentally forget the scale or coefficient (however unlikely that is).

guggero · 2024-10-16T11:12:49Z

rpcserver.go

+	}
+
+	// Convert the asset amount into a fixed-point.
+	assetAmount := rfqmsg.NewBigIntFixedPoint(req.AssetAmount, 0)


I thought about this and what @Roasbeef said a lot, and I think I agree with him that we can't use 0 here.
Because we are going to calculate a price for the asset, it is important that we know the inherent representation of the asset. So basically if we use 0, we would imply one asset unit being 1 dollar, even though the asset might have a decimal display of 3 or 6.

So we do need to query the decimal display value here and use it. That also makes sure we have synced the issuance proof for that asset when attempting to create an invoice.

This is how I see it:

We're calculating msat value from asset units and an asset unit to BTC rate. The decimal display is not relevant to the calculation. I've added this unit test to show that scale 0 works here: 875c0e2

Why is decimal display useful here for converting asset units to msats?

Are we saying that req.AssetAmount is not in asset units but in real world currency units here? I think it's in asset units because it's a uint64, right?

Ultimately it is in asset units, just unscaled. I think that's where they divergence is in our thought models here, and also where I want to make sure we're very careful (re use of constructors, etc). My fear was that if this was already a scaled value (output from some other operation that didn't scale down), then we'd compound an error by not carrying along what the true scale should be.

So far now, I think this seems to be fine, but ultimately the further unit and itests will be the true judge.

I think my scenario above can still happen, if we ever write a scaled value to wire/rpc w/o the scale itself. Will think about how we can add some sanity checks or assertions elsewhere to make sure we catch instances like this.

guggero · 2024-10-16T11:16:33Z

rfq/order.go

@@ -128,7 +125,12 @@ func (c *AssetSalePolicy) CheckHtlcCompliance(

 	// Check that the HTLC amount is not greater than the negotiated maximum
 	// amount.
-	maxOutboundAmount := lnwire.MilliSatoshi(c.MaxAssetAmount) * c.AskPrice
+	maxAssetAmount := rfqmsg.NewBigIntFixedPoint(c.MaxAssetAmount, 0)


See previous comment, we need to fetch the decimal display unit here and use it.

guggero · 2024-10-16T11:17:38Z

rfq/order.go

+	outgoingAssetAmount := rfqmath.MilliSatoshiToUnits(
+		htlc.AmountOutMsat, c.AskAssetRate,
+	)
+	amt := outgoingAssetAmount.ScaleTo(0).ToUint64()


Hmm, from looking at the Godoc of ToUint64() the ScaleTo(0) shouldn't be required, as it mentions "scaled down from the existing scale". But it looks like the implementation doesn't match that. Perhaps something to fix later on.

This commit introduces a type alias for a fixed-point type where the coefficient is a `big.Int`.

This commit introduces a `big.Int` fixed-point constant representing the number of milli-satoshis in one BTC.

This commit updates the RFQ price oracle's `QueryRateTick` endpoint to return asset-to-BTC conversion rates as fixed-point numbers.

This commit replaces the BuyRequest.BipPrice field with SuggestedAssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

This commit replaces the BuyAccept.AskPrice field with AssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

This commit changes the PeerAcceptedBuyQuote.AskPrice field from a `uint64` to a fixed-point representation for the asset-to-BTC rate. It also updates the invoice milli-sat calculation to use the newly introduced fixed-point field.

This commit modifies the AssetSalePolicy struct to carry the asset-to-BTC rate instead of a "price". It also updates the policy check equations to use the new rate field.

This commit replaces the SellRequest.AskPrice field with SuggestedAssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

This commit replaces the SellAccept.BidPrice field with AssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

This test ensures that converting an asset amount to fixed-point values with different scales produces the same result when used in `UnitsToMilliSatoshi`. The test confirms that the absence of a decimal display value (scale) does not affect the final result.

ffranr · 2024-10-16T13:15:33Z

I think our confusion might come down to how we define fields. Consider this part of the code which is contentious:

	// Check that the HTLC amount is not greater than the negotiated maximum
	// amount.
	maxAssetAmount := rfqmath.NewBigIntFixedPoint(c.MaxAssetAmount, 0)

	maxOutboundAmount := rfqmath.UnitsToMilliSatoshi(
		maxAssetAmount, c.AskAssetRate,
	)

From my POV:

maxAssetAmount is in asset units uint64. This value has not been scaled down for decimal display in the UI. This isn't a cent float value. It is a fixed-point with coefficient c.MaxAssetAmount and scale 0.
c.AskAssetRate this is the asset units to BTC rate. It is not the same as the cents to BTC rate. It is the number of asset units per BTC. Before displaying this in a UI it would need to be scaled with decimal display.
maxOutboundAmount is in msats.

Roasbeef

LGTM 🐒

Roasbeef · 2024-10-16T19:04:58Z

docs/examples/basic-price-oracle/go.mod

@@ -4,21 +4,185 @@ go 1.22

 toolchain go1.22.3

-replace github.com/lightninglabs/taproot-assets => ../../../
+replace (
+	github.com/lightninglabs/taproot-assets => ../../../


I think this should be removed before merge?

Add a unit test demonstrating how a price oracle returns a TAP asset-to-BTC rate and how that rate is used to convert an asset amount into msats.

ffranr self-assigned this Sep 24, 2024

ffranr changed the title ~~Rfq uses fixed point~~ RFQ + price oracle uses fixed point Sep 24, 2024

dstadulis added gRPC bLIP rfq labels Sep 24, 2024

dstadulis added this to the v0.4.2 milestone Sep 24, 2024

ffranr force-pushed the rfq-uses-fixed-point branch from 756bc82 to bceafd1 Compare September 24, 2024 18:30

ffranr mentioned this pull request Sep 25, 2024

Add fixed-point WithinTolerance method and greater-than operators #1135

Merged

ffranr force-pushed the rfq-uses-fixed-point branch from 63133bb to ffa577d Compare September 27, 2024 15:53

ffranr force-pushed the rfq-uses-fixed-point branch from ffa577d to e213d57 Compare September 30, 2024 16:48

ffranr changed the title ~~RFQ + price oracle uses fixed point~~ Part 1: RFQ + price oracle uses fixed point Sep 30, 2024

ffranr changed the base branch from main to approved-rfq-fixedpoint September 30, 2024 16:49

ffranr marked this pull request as ready for review September 30, 2024 16:56

ffranr requested review from GeorgeTsagk and jharveyb September 30, 2024 17:04

Roasbeef requested changes Oct 2, 2024

View reviewed changes

Roasbeef removed the request for review from jharveyb October 2, 2024 00:17

GeorgeTsagk approved these changes Oct 3, 2024

View reviewed changes

rfqmsg/buy_request.go Outdated Show resolved Hide resolved

ffranr force-pushed the rfq-uses-fixed-point branch 5 times, most recently from 480c411 to aa53ed4 Compare October 3, 2024 23:28

ffranr requested a review from Roasbeef October 3, 2024 23:30

ffranr changed the title ~~Part 1: RFQ + price oracle uses fixed point~~ Part 1: RFQ uses fixed-points Oct 7, 2024

ffranr mentioned this pull request Oct 7, 2024

Part 2: RFQ uses fixed-points #1141

Merged

Roasbeef requested changes Oct 11, 2024

View reviewed changes

rfqmath: add BigInt.String method

e85fa8d

This commit introduces the `BigInt.String` method, which can be used to safely (without overflow) export a `BigInt` to RPC or JSON.

ffranr force-pushed the rfq-uses-fixed-point branch from c5a94c3 to 8825621 Compare October 15, 2024 12:50

ffranr requested a review from Roasbeef October 15, 2024 12:50

guggero self-requested a review October 15, 2024 19:13

guggero reviewed Oct 16, 2024

View reviewed changes

ffranr added 2 commits October 16, 2024 12:39

rfqmath: add BigIntFixedPoint type alias

070d651

This commit introduces a type alias for a fixed-point type where the coefficient is a `big.Int`.

rfqmsg: add MilliSatPerBtc constant

74fd6f8

This commit introduces a `big.Int` fixed-point constant representing the number of milli-satoshis in one BTC.

ffranr force-pushed the rfq-uses-fixed-point branch 2 times, most recently from 875c0e2 to 3a37e87 Compare October 16, 2024 12:46

ffranr added 10 commits October 16, 2024 14:01

multi: RFQ price oracle returns asset rates as fixed-point numbers

4e291e5

This commit updates the RFQ price oracle's `QueryRateTick` endpoint to return asset-to-BTC conversion rates as fixed-point numbers.

rfq+rfqmsg: replace BuyRequest.BipPrice with SuggestedAssetRate

6aee68b

This commit replaces the BuyRequest.BipPrice field with SuggestedAssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

multi: replace BuyAccept.AskPrice with AssetRate

239d035

This commit replaces the BuyAccept.AskPrice field with AssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

rfq: add missing doc

55923ae

rfq: replace AssetSalePolicy.AskPrice with asset-to-BTC rate

e90b77d

This commit modifies the AssetSalePolicy struct to carry the asset-to-BTC rate instead of a "price". It also updates the policy check equations to use the new rate field.

rfq+rfqmsg: replace SellRequest.AskPrice with SuggestedAssetRate

23dee0a

This commit replaces the SellRequest.AskPrice field with SuggestedAssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

multi: replace SellAccept.BidPrice with AssetRate

6458e3b

This commit replaces the SellAccept.BidPrice field with AssetRate, changing a `uint64` price to an asset-to-BTC rate represented as a fixed-point number.

rfq: remove unused function pricesWithinBounds

7dbfc2c

ffranr force-pushed the rfq-uses-fixed-point branch from 3a37e87 to eeeb198 Compare October 16, 2024 13:05

Roasbeef approved these changes Oct 16, 2024

View reviewed changes

rfqmath: add example for price oracle rate and asset conversion to msat

615fc4d

Add a unit test demonstrating how a price oracle returns a TAP asset-to-BTC rate and how that rate is used to convert an asset amount into msats.

Roasbeef merged commit e5466e3 into approved-rfq-fixedpoint Oct 16, 2024
18 checks passed

ffranr mentioned this pull request Oct 19, 2024

Merge feature-branch RFQ fixedpoint commits into main #1155

Merged

guggero deleted the rfq-uses-fixed-point branch November 25, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 1: RFQ uses fixed-points #1136

Part 1: RFQ uses fixed-points #1136

ffranr commented Sep 24, 2024 •

edited by Roasbeef

Loading

coveralls commented Sep 24, 2024 •

edited

Loading

dstadulis commented Sep 24, 2024

ffranr commented Sep 27, 2024 •

edited

Loading

Roasbeef left a comment

Roasbeef Oct 1, 2024

Roasbeef Oct 1, 2024

ffranr Oct 3, 2024 •

edited

Loading

Roasbeef Oct 2, 2024

ffranr Oct 11, 2024

guggero Oct 16, 2024

Roasbeef Oct 2, 2024

ffranr Oct 3, 2024

GeorgeTsagk left a comment

lightninglabs-deploy commented Oct 10, 2024

Roasbeef commented Oct 11, 2024

Roasbeef left a comment

ffranr commented Oct 15, 2024

ffranr commented Oct 15, 2024 •

edited

Loading

guggero left a comment

guggero Oct 16, 2024

guggero Oct 16, 2024

ffranr Oct 16, 2024

ffranr Oct 16, 2024 •

edited

Loading

Roasbeef Oct 16, 2024

guggero Oct 16, 2024

guggero Oct 16, 2024

ffranr commented Oct 16, 2024 •

edited

Loading

Roasbeef left a comment

Roasbeef Oct 16, 2024

Part 1: RFQ uses fixed-points #1136

Part 1: RFQ uses fixed-points #1136

Conversation

ffranr commented Sep 24, 2024 • edited by Roasbeef Loading

Note to Reviewers

coveralls commented Sep 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11073777673

Details

💛 - Coveralls

dstadulis commented Sep 24, 2024

ffranr commented Sep 27, 2024 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Oct 10, 2024

Roasbeef commented Oct 11, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

ffranr commented Oct 15, 2024

ffranr commented Oct 15, 2024 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr commented Oct 16, 2024 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr commented Sep 24, 2024 •

edited by Roasbeef

Loading

coveralls commented Sep 24, 2024 •

edited

Loading

ffranr commented Sep 27, 2024 •

edited

Loading

ffranr Oct 3, 2024 •

edited

Loading

ffranr commented Oct 15, 2024 •

edited

Loading

ffranr Oct 16, 2024 •

edited

Loading

ffranr commented Oct 16, 2024 •

edited

Loading