rfqmsg: request fields blip align #1157

ffranr · 2024-10-22T14:40:18Z

In this PR, we align the request RFQ wire message fields with the tap BLIP.

All TLV record values are already aligned with the BLIP due to previous PRs. This PR introduces internal changes to tapd to ensure the request message fields are handled, named, and interpreted correctly. These changes are non-interface and non-breaking.

This will be the last RFQ BLIP alignment PR.

coveralls · 2024-10-22T14:49:59Z

Pull Request Test Coverage Report for Build 11660754444

Details

103 of 412 (25.0%) changed or added relevant lines in 13 files are covered.
54 unchanged lines in 14 files lost coverage.
Overall coverage decreased (-0.04%) to 40.681%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfqmsg/records.go	14	17	82.35%
rfqmath/fixed_point.go	1	7	14.29%
tapchannel/aux_traffic_shaper.go	0	7	0.0%
rfq/manager.go	0	8	0.0%
asset/asset.go	16	41	39.02%
rfqmsg/buy_request.go	0	36	0.0%
rpcserver.go	0	36	0.0%
rfqmsg/sell_request.go	0	37	0.0%
rfq/order.go	0	39	0.0%
rfqmsg/request.go	41	80	51.25%

Files with Coverage Reduction	New Missed Lines	%
tapchannel/aux_traffic_shaper.go	1	0.0%
rpcserver.go	1	0.0%
rfq/order.go	2	0.0%
tapdb/addrs.go	2	79.04%
tapgarden/planter.go	2	74.87%
asset/asset.go	2	80.2%
rfqmsg/request.go	2	43.85%
rfqmsg/sell_request.go	3	0.0%
rfqmsg/buy_request.go	3	0.0%
tapdb/universe.go	4	80.91%

Totals
Change from base Build 11637218948:	-0.04%
Covered Lines:	24683
Relevant Lines:	60675

💛 - Coveralls

Roasbeef · 2024-10-22T18:32:40Z

We should also make sure the min+max fields are set properly, along with the transfer type.

guggero · 2024-10-22T18:34:31Z

We should probably also remove the --experimental.rfq.mockoraclesatsperasset= flag in this PR (or a related PR).

ffranr · 2024-10-23T15:44:30Z

We should probably also remove the --experimental.rfq.mockoraclesatsperasset= flag in this PR (or a related PR).

@guggero I would prefer to do that in a future PR. I want to prioritize BLIP alignment and address any bugs arising from misalignment issues.

Roasbeef · 2024-10-23T18:20:48Z

rfq/order.go

 	// Convert the inbound asset amount to millisatoshis and ensure that the
 	// outgoing HTLC amount is not more than the inbound asset amount.
-	assetAmt := rfqmath.NewBigIntFixedPoint(c.AssetAmount, 0)
+	assetAmtFp := rfqmath.BigIntFixedPoint{


I think the constructor can still be used here?

In the latest commits, I've introduced a new fixed-point method, SetIntValue, which enables constructing a fixed-point instance from any integer value of the same type as the fixed-point coefficient's type:

assetAmtFp := new(rfqmath.BigIntFixedPoint).SetIntValue(assetAmt)

guggero

Very nice improvements! Did a quick pass, will do a full review once ready.

rfqmsg/request.go

asset/asset.go

rfq/negotiator.go

rfqmath/convert_test.go

rfqmsg/records_test.go

ffranr · 2024-10-26T01:04:09Z

I think we should drop the TransferType field from the request message. As I was adding it I found that it was pulling in unnecessary context which I found made things more complicated. It relates different request messages to what there intended big picture uses might be. Which is not something the request/accept/reject RFQ messages need to be aware of IMO. It's a bit like a "note" or "label" but doesn't seem to add much when applied to the message level.

What do you think @guggero ?

ffranr · 2024-10-26T01:08:58Z

I've added the MinInAsset and MinOutAsset request message fields. I think we need to add reject message error types for these fields as the accept message doesn't propose other lower bounds.

And we need to reject based on the dust limit in relation to MinInAsset or MinOutAsset depending on whichever one is BTC. Not sure where the dust limit is defined so might add my own msats dust limit const.

Any other use of these fields will probably be rewritten by the MPP OKR.

ffranr · 2024-10-28T09:33:03Z

I think we should merge this PR as is, subject to reviews.

guggero

Thanks for all the updates. I think we are fairly close here, but I have a couple of things that we should try to get in with this PR, as IMO the changes are within the scope of the PR:

Rename the SellRequest's AssetAmount either to MaxInAsset with uint64 or to MaxInboundAmount with lnwire.MilliSatoshi (depending on the decision where to make the switch, see inline comment).
Enforce the above max amount in the asset purchase policy.
Since we don't have the "straightening out" of the buy/ask/purchase terminology I had in my WIP branch, and we still turn a buy request into an ask request for the oracle (which internally is then called a sale transaction), we should add that to the RFQ doc, best with a sequence diagram. Otherwise it will endlessly confuse me, and probably any authors of oracle logic as well.

rfq/order.go

guggero · 2024-10-28T13:50:44Z

I think we should drop the TransferType field from the request message.

We currently decide whether it's a buy or sell request based on which asset is BTC here:

taproot-assets/rfqmsg/request.go

Line 503 in e1e8268

isBuyRequest := false

Once we support asset-to-asset conversion directly, how would we distinguish between buy and sell? Just the direction of the in vs. out asset?

ffranr · 2024-10-31T13:07:51Z

I think we should drop the TransferType field from the request message.

We currently decide whether it's a buy or sell request based on which asset is BTC here:

taproot-assets/rfqmsg/request.go

Line 503 in e1e8268

isBuyRequest := false

Once we support asset-to-asset conversion directly, how would we distinguish between buy and sell? Just the direction of the in vs. out asset?

IMO we should also drop the buy/sell types when we move to support asset-to-asset. So the transfer type is not needed then either.

guggero

Almost there, thanks a lot for sticking to this huge undertaking 💯

rfq/negotiator.go

guggero · 2024-11-01T09:59:21Z

rfq/negotiator.go

@@ -106,7 +106,7 @@ func NewNegotiator(cfg NegotiatorCfg) (*Negotiator, error) {
 // queryBidFromPriceOracle queries the price oracle for a bid price. It returns
 // an appropriate outgoing response message which should be sent to the peer.
 func (n *Negotiator) queryBidFromPriceOracle(peer route.Vertex,
-	assetSpecifier asset.Specifier, assetAmount uint64,
+	assetSpecifier asset.Specifier, assetMaxAmt fn.Option[uint64],


Is the assetMaxAmount used anywhere anymore after this series of commits? Can't we just remove it, and make the paymentMaxAmount a non-option and just use that everywhere?

Is the assetMaxAmount used anywhere anymore after this series of commits?

assetMaxAmt is passed on to n.cfg.PriceOracle.QueryBidPrice below. It gets passed to the price oracle service.

Can't we just remove it, and make the paymentMaxAmount a non-option and just use that everywhere?

These two fields relate to two different assets.

Okay, I see. Confusion on my part, see comment below with a request for documenting this a bit more.

guggero · 2024-11-01T10:00:22Z

taprpc/priceoraclerpc/price_oracle.proto

+    // payment_asset_max_amount is the maximum amount of the payment asset that
+    // could be involved in the transaction. This field is optional. If set to
+    // zero, it is considered unset.
+    uint64 payment_asset_max_amount = 5;


Do we need this field for an actual use case today? I think we should just remove it to make things easier to understand... See my other comments as well.

MaxInAsset sometimes relates to BTC and sometimes to a tap asset.

The two different fields in the price oracle relate to the two different assets that we're tying to find an exchange rate for. Instead of a single field that is sometimes the upper bound on the tap asset and sometimes an upper bound on the BTC side, I've got two different fields.

Okay, I guess I see where my confusion is coming from. It's because we still do the "flip" of the transaction type for the oracle (which I eliminated in my WIP refactor PR).

What I mean is:

On the wallet end user side, a SellOrder goes into the oracle as an ask (transaction type sale with payment_assset_max_amount set)

On the edge node, that same SellOrder goes into the oracle as a bid (transaction type purchase with payment_max_amount set)

And then for a BuyOrder it would be kind of the other way around.
Can we please add all of this to the RFQ documentation?

And perhaps update the comments in this proto file to make it more clear to the developer of an oracle when what field is set? For example that never both _max_amount fields are set at the same time?

guggero · 2024-11-01T10:01:18Z

rfq/negotiator.go

@@ -177,9 +177,12 @@ func (n *Negotiator) HandleOutgoingBuyOrder(buyOrder BuyOrder) error {

 		if n.cfg.PriceOracle != nil && assetSpecifier.IsSome() {
 			// Query the price oracle for a bid price.
+			//
+			// TODO(ffranr): Add assetMaxAmt to BuyOrder and use as


This TODO remains unresolved and we don't pass any amount to the oracle.

Adding and populating that field in the BuyOrder would require an RPC change. I mean to do that in a future PR. I think this PR is getting too long.

But at this specific call site, we only pass in fn.None for all the values. So we're not passing any amount when requesting a rate for the hint. Is that an allowed way to call into the oracle, with none of the _max_amount fields set?

I think it should technically be a valid way of calling the price oracle. If upper bounds aren't given then the price would just be less accurate.

But we should complete the TODO. I'll add the correct field to BuyOrder in a separate PR.

rpcserver.go

Rearranged the fields in the requestWireMsgData struct to match the order of TLV record integers, improving code readability and clarity.

This change aligns the field name with the Tap BLIP.

In this context, "selling" refers to divesting a tap asset in exchange for BTC, meaning the tap asset is outgoing from the RFQ request initiator's perspective. The `SellRequest` struct now uses the OutAssetRateHint field as the suggested asset rate.

Renamed the field to MaxInAsset to better represent the maximum quantity of in-asset that the counterparty is expected to divest, whether it's BTC or another asset. This change aligns the field name with the TLV type specified in the BLIP.

This commit adds a new constructor, `NewSpecifier`, for creating an `asset.Specifier`. The constructor accepts an asset ID, a group public key, or a group key to identify the asset. It includes a `mustBeSpecified` parameter, which, when set to true, enforces that at least one of the asset ID, group public key, or group key must be provided. If none are specified, an error is returned.

Add a helper function to create a BigInt from a `uint64`.

Refactor the unit test to make it easier to extend. A subsequent commit will add tests for a new method.

Add the SumAssetBalance method to the Htlc record, which represents capacity changes for an in-flight HTLC. This method returns the total asset balance for the specified asset.

Introduce a general method for constructing a fixed-point value from an integer. For example: ``` assetAmtFp := new(rfqmath.BigIntFixedPoint).SetIntValue(assetAmt) ```

Update `AssetPurchasePolicy` to include an asset specifier, which is used to select and sum asset balances in an in-flight HTLC. This ensures the outgoing msat amount of the HTLC is not less than the incoming Tap asset amount in msat. This commit fixes an issue where the asset amount used was incorrectly set on the policy instead of being derived from the in-flight HTLC. The erroneous asset amount field in the policy has been removed.

Rename field to better reflect its purpose.

Replace the asset ID and group key fields in the BuyRequest structure with a single AssetSpecifier field. This change simplifies how asset specifier information is handled.

Add a new `payment_asset_max_amount` field to the `QueryAssetRatesRequest` RPC message in the RFQ price oracle. This field optionally allows callers to specify an upper limit for the payment asset amount in their requests. A future commit will populate and utilize this field in the RFQ package.

Rename the `assetAmount` argument to `assetMaxAmt` to better reflect its purpose as an upper bound on the asset amount. This change improves clarity in the codebase.

This commit introduces a new field, `SellRequest.PaymentMaxAmt`, replacing the previous `AssetAmount` field with a different type and purpose. This change allows us to populate `PaymentMaxAmt` with `MaxInAsset` from request wire messages, reflecting the maximum payment amount more accurately.

This commit removes the `max_asset_amount` and `min_ask` fields from the `SellOrder` type and introduces a new `payment_max_amt` field. The removed fields were not used correctly. The new `payment_max_amt` field allows us to leverage the `MaxInAmount` from the RFQ request wire message.

This commit adds a check to ensure that the outgoing HTLC `msat` amount does not exceed the `PaymentMaxAmt` specified in the RFQ quote.

guggero

Okay, I think I finally understand all the changes and aligned my own mental model to them. Would love to have a bit more documentation around that, but not blocking this PR on it. So LGTM, nice work! 🎉

guggero · 2024-11-04T10:07:15Z

taprpc/priceoraclerpc/price_oracle.proto

+    // payment_asset_max_amount is the maximum amount of the payment asset that
+    // could be involved in the transaction. This field is optional. If set to
+    // zero, it is considered unset.
+    uint64 payment_asset_max_amount = 5;


Okay, I guess I see where my confusion is coming from. It's because we still do the "flip" of the transaction type for the oracle (which I eliminated in my WIP refactor PR).

What I mean is:

On the wallet end user side, a SellOrder goes into the oracle as an ask (transaction type sale with payment_assset_max_amount set)

On the edge node, that same SellOrder goes into the oracle as a bid (transaction type purchase with payment_max_amount set)

And then for a BuyOrder it would be kind of the other way around.
Can we please add all of this to the RFQ documentation?

And perhaps update the comments in this proto file to make it more clear to the developer of an oracle when what field is set? For example that never both _max_amount fields are set at the same time?

guggero · 2024-11-04T10:08:01Z

rfq/negotiator.go

@@ -106,7 +106,7 @@ func NewNegotiator(cfg NegotiatorCfg) (*Negotiator, error) {
 // queryBidFromPriceOracle queries the price oracle for a bid price. It returns
 // an appropriate outgoing response message which should be sent to the peer.
 func (n *Negotiator) queryBidFromPriceOracle(peer route.Vertex,
-	assetSpecifier asset.Specifier, assetAmount uint64,
+	assetSpecifier asset.Specifier, assetMaxAmt fn.Option[uint64],


Okay, I see. Confusion on my part, see comment below with a request for documenting this a bit more.

guggero · 2024-11-04T10:10:24Z

rfq/negotiator.go

@@ -177,9 +177,12 @@ func (n *Negotiator) HandleOutgoingBuyOrder(buyOrder BuyOrder) error {

 		if n.cfg.PriceOracle != nil && assetSpecifier.IsSome() {
 			// Query the price oracle for a bid price.
+			//
+			// TODO(ffranr): Add assetMaxAmt to BuyOrder and use as


But at this specific call site, we only pass in fn.None for all the values. So we're not passing any amount when requesting a rate for the hint. Is that an allowed way to call into the oracle, with none of the _max_amount fields set?

ffranr · 2024-11-04T14:12:28Z

Further work to be included in a separate PR:

Better documentation for SellRequest, BuyRequest, BuyOrder, SellOrder.
Add missing field to BuyOrder and populate in RPC endpoint.

@guggero are you also ok with the following:

Remove transfer type field from request RFQ message in BLIP and revise TLV integers in BLIP and tapd.

guggero · 2024-11-04T15:06:39Z

Further work to be included in a separate PR:

* Better documentation for `SellRequest`, `BuyRequest`, `BuyOrder`, `SellOrder`.

* Add missing field to `BuyOrder` and populate in RPC endpoint.

Sure, let's do all of that in a follow-up PR, then we can discuss there.

@guggero are you also ok with the following:

* Remove transfer type field from `request` RFQ message in BLIP and revise TLV integers in BLIP and tapd.

I still don't fully see how that would work, given that we'd still need to know whether to ask the oracle for an ask or bid. Or would we also remove that directionality there and only have a single RPC method?

ffranr · 2024-11-04T15:58:32Z

I still don't fully see how that would work, given that we'd still need to know whether to ask the oracle for an ask or bid. Or would we also remove that directionality there and only have a single RPC method?

@guggero Yes, I think we only need a single oracle interface method. The oracle needs to know what asset is inbound and which is outbound and then it can return the ask/bid based on that. We can do that now with the oracle RPC endpoint by setting payment asset, subject asset, and the transfer type. But we can simplify the RPC endpoint's messages further also.

GeorgeTsagk

Very clean commits 💯

LGTM, straight forward changes ✅

have some minor Qs/nits, non-blocking

GeorgeTsagk · 2024-11-05T11:36:08Z

asset/asset.go

@@ -417,6 +417,12 @@ func (s *Specifier) UnwrapGroupKeyToPtr() *btcec.PublicKey {
 	return s.groupKey.UnwrapToPtr()
 }

+// UnwrapToPtr unwraps the asset ID and asset group public key fields,


nit: could squash this and 2 previous commits to something like "asset: add helper methods"

taprpc/priceoraclerpc/price_oracle.proto

rpcserver.go

ffranr self-assigned this Oct 22, 2024

Roasbeef added this to the v0.4.2 milestone Oct 22, 2024

dstadulis added the rfq label Oct 23, 2024

ffranr force-pushed the rfqmsg-request-fields-blip-align branch from 2507bcd to 3fe960a Compare October 23, 2024 15:45

guggero self-requested a review October 23, 2024 16:13

Roasbeef reviewed Oct 23, 2024

View reviewed changes

guggero reviewed Oct 24, 2024

View reviewed changes

ffranr force-pushed the rfqmsg-request-fields-blip-align branch 2 times, most recently from 0a96c9a to e1e8268 Compare October 26, 2024 00:58

ffranr marked this pull request as ready for review October 26, 2024 00:58

ffranr requested a review from guggero October 28, 2024 09:28

guggero reviewed Oct 28, 2024

View reviewed changes

rfq/order.go Show resolved Hide resolved

ffranr force-pushed the rfqmsg-request-fields-blip-align branch 2 times, most recently from 744f051 to 35f89bd Compare October 31, 2024 13:05

ffranr requested a review from guggero October 31, 2024 13:41

guggero reviewed Nov 1, 2024

View reviewed changes

ffranr added 5 commits November 4, 2024 08:32

rfqmsg: reorder requestWireMsgData fields for clarity

640ea7b

Rearranged the fields in the requestWireMsgData struct to match the order of TLV record integers, improving code readability and clarity.

rfqmsg: re-name request msg field SuggestedAssetRate to InAssetRateHint

a907cad

This change aligns the field name with the Tap BLIP.

ffranr added 11 commits November 4, 2024 08:32

rfqmath: add NewBigIntFromUint64 constructor

39d4024

Add a helper function to create a BigInt from a `uint64`.

rfqmsg: refactor unit test for easier extensibility

1e20488

Refactor the unit test to make it easier to extend. A subsequent commit will add tests for a new method.

rfqmsg: add Htlc.SumAssetBalance method

54200c1

Add the SumAssetBalance method to the Htlc record, which represents capacity changes for an in-flight HTLC. This method returns the total asset balance for the specified asset.

rfqmath: add FixedPoint.SetIntValue method

163a0b0

Introduce a general method for constructing a fixed-point value from an integer. For example: ``` assetAmtFp := new(rfqmath.BigIntFixedPoint).SetIntValue(assetAmt) ```

rfq: improve AssetSalePolicy.CheckHtlcCompliance comments

4ac78fb

rfq: refactor AssetSalePolicy field names for clarity

26aa3bf

multi: rename field BuyRequest.AssetAmount to AssetMaxAmt

a33a027

Rename field to better reflect its purpose.

rfq+rfqmsg: add AssetSpecifier field to BuyRequest

8f5fb5f

Replace the asset ID and group key fields in the BuyRequest structure with a single AssetSpecifier field. This change simplifies how asset specifier information is handled.

rfqmsg: add request fields MinInAsset and MinOutAsset

f922c40

rfq+asset: carry asset specifier through to price oracle query

d755237

ffranr force-pushed the rfqmsg-request-fields-blip-align branch from 35f89bd to 98080aa Compare November 4, 2024 08:34

ffranr added 5 commits November 4, 2024 08:40

rfq: reformulate oracle assetAmount to assetMaxAmt with Option type

ecc8450

Rename the `assetAmount` argument to `assetMaxAmt` to better reflect its purpose as an upper bound on the asset amount. This change improves clarity in the codebase.

rfq: add PaymentMaxAmt check to AssetPurchasePolicy

c5d356e

This commit adds a check to ensure that the outgoing HTLC `msat` amount does not exceed the `PaymentMaxAmt` specified in the RFQ quote.

ffranr force-pushed the rfqmsg-request-fields-blip-align branch from 98080aa to c5d356e Compare November 4, 2024 08:42

ffranr requested review from guggero and Roasbeef November 4, 2024 08:43

guggero approved these changes Nov 4, 2024

View reviewed changes

GeorgeTsagk self-requested a review November 5, 2024 09:26

GeorgeTsagk approved these changes Nov 5, 2024

View reviewed changes

ffranr added this pull request to the merge queue Nov 5, 2024

Merged via the queue into main with commit ab90081 Nov 5, 2024
18 checks passed

guggero deleted the rfqmsg-request-fields-blip-align branch November 5, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfqmsg: request fields blip align #1157

rfqmsg: request fields blip align #1157

ffranr commented Oct 22, 2024 •

edited

Loading

coveralls commented Oct 22, 2024 •

edited

Loading

Roasbeef commented Oct 22, 2024

guggero commented Oct 22, 2024

ffranr commented Oct 23, 2024

Roasbeef Oct 23, 2024

ffranr Oct 25, 2024

guggero left a comment

ffranr commented Oct 26, 2024

ffranr commented Oct 26, 2024 •

edited

Loading

ffranr commented Oct 28, 2024

guggero left a comment

guggero commented Oct 28, 2024

ffranr commented Oct 31, 2024

guggero left a comment

guggero Nov 1, 2024

ffranr Nov 4, 2024

guggero Nov 4, 2024

guggero Nov 1, 2024

ffranr Nov 4, 2024

guggero Nov 4, 2024

guggero Nov 1, 2024

ffranr Nov 4, 2024

guggero Nov 4, 2024

ffranr Nov 4, 2024 •

edited

Loading

guggero left a comment

guggero Nov 4, 2024

guggero Nov 4, 2024

guggero Nov 4, 2024

ffranr commented Nov 4, 2024 •

edited

Loading

guggero commented Nov 4, 2024

ffranr commented Nov 4, 2024 •

edited

Loading

GeorgeTsagk left a comment

GeorgeTsagk Nov 5, 2024

rfqmsg: request fields blip align #1157

rfqmsg: request fields blip align #1157

Conversation

ffranr commented Oct 22, 2024 • edited Loading

coveralls commented Oct 22, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11660754444

Details

💛 - Coveralls

Roasbeef commented Oct 22, 2024

guggero commented Oct 22, 2024

ffranr commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

ffranr commented Oct 26, 2024

ffranr commented Oct 26, 2024 • edited Loading

ffranr commented Oct 28, 2024

guggero left a comment

Choose a reason for hiding this comment

guggero commented Oct 28, 2024

ffranr commented Oct 31, 2024

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

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

ffranr Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

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 commented Nov 4, 2024 • edited Loading

guggero commented Nov 4, 2024

ffranr commented Nov 4, 2024 • edited Loading

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr commented Oct 22, 2024 •

edited

Loading

coveralls commented Oct 22, 2024 •

edited

Loading

ffranr commented Oct 26, 2024 •

edited

Loading

ffranr Nov 4, 2024 •

edited

Loading

ffranr commented Nov 4, 2024 •

edited

Loading

ffranr commented Nov 4, 2024 •

edited

Loading