Remove remaining instances of "rate tick" from RFQ systems #1156

ffranr · 2024-10-21T16:32:46Z

This PR removes any remaining mention of "rate tick" from the Price Oracle RPC endpoints and the RFQ packages.

ffranr · 2024-10-21T16:36:21Z

The remaining RFQ cleanup work after this PR is merge:

Add request message fields: suggestedOutAssetRate and transferType.
Rearrange fields in request message to correspond with BLIP (refactor only, no logic changes).

coveralls · 2024-10-21T16:41:59Z

Pull Request Test Coverage Report for Build 11447101930

Details

19 of 35 (54.29%) changed or added relevant lines in 3 files are covered.
29 unchanged lines in 9 files lost coverage.
Overall coverage decreased (-0.01%) to 40.424%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfq/negotiator.go	0	4	0.0%
rfqmsg/request.go	0	4	0.0%
rfq/oracle.go	19	27	70.37%

Files with Coverage Reduction	New Missed Lines	%
rfq/oracle.go	2	59.85%
tappsbt/create.go	2	53.22%
tapgarden/planter.go	2	74.87%
tapdb/addrs.go	2	79.04%
commitment/tap.go	3	84.7%
universe/interface.go	4	50.22%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.5%
tapdb/assets_store.go	6	63.69%

Totals
Change from base Build 11446789576:	-0.01%
Covered Lines:	24370
Relevant Lines:	60286

💛 - Coveralls

dstadulis · 2024-10-21T16:42:04Z

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

 //
 // Example use case:
 //
 // Alice is trying to pay an invoice by spending an asset. Alice therefore
 // requests that Bob (her asset channel counterparty) purchase the asset from
 // her. Bob's payment, in BTC, will pay the invoice.
 //
-// Alice requests a bid quote from Bob. Her request includes a rate tick
-// hint (ask). Alice get the rate tick hint by calling this endpoint. She sets:
+// Alice requests a bid quote from Bob. Her request includes an asset rates hint


Suggested change

// Alice requests a bid quote from Bob. Her request includes an asset rates hint

// Alice requests a bid quote from Bob. Her request includes an asset-rate hint

Or are we referring to the name of the bundle, Asset Rates?

The request includes an in-asset to BTC rate hint and an out-asset to BTC rate hint.

dstadulis · 2024-10-21T16:44:20Z

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

-// Alice requests a bid quote from Bob. Her request includes a rate tick
-// hint (ask). Alice get the rate tick hint by calling this endpoint. She sets:
+// Alice requests a bid quote from Bob. Her request includes an asset rates hint
+// (ask). Alice get the asset rates hint by calling this endpoint. She sets:


Suggested change

// (ask). Alice get the asset rates hint by calling this endpoint. She sets:

// (ask). Alice receives the asset rates hint by calling this endpoint. She sets:

I've gone with "Alice obtains". Let me know if you think "receives" is better or something else.

obtains sgtm

rfqmsg/accept.go

dstadulis · 2024-10-21T16:54:07Z

taprpc/priceoraclerpc/price_oracle.pb.go

+// Deprecated: Use QueryAssetRatesResponse.ProtoReflect.Descriptor instead.
+func (*QueryAssetRatesResponse) Descriptor() ([]byte, []int) {


Should this be fully removed now? or are the motivations to preserve functionality?

This was auto-generated by our protos generator. It's probably in place for backwards compatibility. I think we can ignore this.

dstadulis · 2024-10-21T17:02:02Z

taprpc/priceoraclerpc/price_oracle.swagger.json

+      "properties": {
+        "subjectAssetRate": {
+          "$ref": "#/definitions/priceoraclerpcFixedPoint",
+          "description": "subjectAssetRate is the number of subject asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\nsubject asset to BTC (conversion) rate.  When the subject asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."


Suggested change

"description": "subjectAssetRate is the number of subject asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\nsubject asset to BTC (conversion) rate. When the subject asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."

"description": "subjectAssetRate is the number of subject asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\nsubject asset to BTC (conversion) rate. When the subject asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats per one bitcoin."

I've gone with:

When the subject asset is BTC, // this field should be set to 100 billion, as one BTC is equivalent to 100 // billion msats.

dstadulis · 2024-10-21T17:02:19Z

taprpc/priceoraclerpc/price_oracle.swagger.json

+        },
+        "paymentAssetRate": {
+          "$ref": "#/definitions/priceoraclerpcFixedPoint",
+          "description": "paymentAssetRate is the number of payment asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\npayment asset to BTC (conversion) rate. When the payment asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."


Suggested change

"description": "paymentAssetRate is the number of payment asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\npayment asset to BTC (conversion) rate. When the payment asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."

"description": "paymentAssetRate is the number of payment asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\npayment asset to BTC (conversion) rate. When the payment asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats per one bitcoin."

I've gone with:

When the payment asset is BTC, // this field should be set to 100 billion, as one BTC is equivalent to 100 // billion msats.

dstadulis · 2024-10-21T17:03:48Z

taprpc/priceoraclerpc/price_oracle.swagger.json

-      "description": "QueryRateTickErrResponse is the error response to a rate tick query."
-    },
-    "priceoraclerpcQueryRateTickResponse": {
-      "type": "object",
-      "properties": {
-        "success": {
-          "$ref": "#/definitions/priceoraclerpcQueryRateTickSuccessResponse",
-          "description": "success is the successful response to the rate tick query."
-        },
-        "error": {
-          "$ref": "#/definitions/priceoraclerpcQueryRateTickErrResponse",
-          "description": "error is the error response to the rate tick query."
-        }
-      },
-      "description": "QueryRateTickResponse is the response to a rate tick query."
+      "description": "QueryAssetRatesErrResponse is the error response to a QueryAssetRates call."


Is the range of responses reduced s.t. QueryAssetRatesErrResponse encapsulates everything?

QueryAssetRatesErrResponse encapsulates all error responses from a price oracle service QueryAssetRates endpoint call. QueryAssetRatesErrResponse does not encapsulate every kind of response. We also have QueryAssetRatesOkResponse.

This commit removes the "rate tick" concept from the price oracle RPC and replaces it with an asset units per BTC rate, using the new fixed-point type. This commit focuses on RPC changes. Other instances of "rate tick" will be removed in subsequent commits.

This commit removes the "rate tick" concept from the price oracle basic example.

Rename variables, constants, and uses in comments.

guggero

LGTM 🎉

dstadulis

Thanks for the modifications!

ffranr self-assigned this Oct 21, 2024

ffranr requested review from guggero and GeorgeTsagk October 21, 2024 16:32

ffranr force-pushed the rfq-rm-rate-tick branch from 3efc8a8 to 9442dc9 Compare October 21, 2024 19:00

ffranr changed the base branch from approved-rfq-fixedpoint to main October 21, 2024 19:01

ffranr marked this pull request as ready for review October 21, 2024 19:01

dstadulis requested changes Oct 21, 2024

View reviewed changes

ffranr added 10 commits October 21, 2024 20:14

examples: remove "rate tick" from price oracle basic example

996a283

This commit removes the "rate tick" concept from the price oracle basic example.

rfq: rename validateRateTickRequest to validateAssetRatesRequest

424b31c

rfq: remove "rate tick" from oracle unit tests

0f71f09

Rename variables, constants, and uses in comments.

rfq: rename InvalidRateTickQuoteRespStatus

29d44d5

rfq: rename minRateTickExpiryLifetime

71502c0

rfqmsg: remove use of "rate tick" in expiry field doc

d2d8941

rfqmsg: remove use of "rate tick" in unit tests

b40cf5a

rfqrpc: rename QuoteRespStatus.INVALID_RATE_TICK

85d43a9

rfqmsg: rename variable suggestedRateTick

07ac6a4

ffranr force-pushed the rfq-rm-rate-tick branch from 9442dc9 to 07ac6a4 Compare October 21, 2024 19:19

ffranr requested a review from dstadulis October 21, 2024 19:23

guggero approved these changes Oct 22, 2024

View reviewed changes

dstadulis approved these changes Oct 22, 2024

View reviewed changes

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

dstadulis added the rfq label Oct 22, 2024

ffranr added this pull request to the merge queue Oct 22, 2024

Merged via the queue into main with commit eeadc03 Oct 22, 2024
18 checks passed

guggero deleted the rfq-rm-rate-tick branch October 22, 2024 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove remaining instances of "rate tick" from RFQ systems #1156

Remove remaining instances of "rate tick" from RFQ systems #1156

ffranr commented Oct 21, 2024 •

edited

Loading

ffranr commented Oct 21, 2024 •

edited

Loading

coveralls commented Oct 21, 2024 •

edited

Loading

dstadulis Oct 21, 2024

ffranr Oct 21, 2024

dstadulis Oct 21, 2024

ffranr Oct 21, 2024

dstadulis Oct 22, 2024

dstadulis Oct 21, 2024

ffranr Oct 21, 2024

dstadulis Oct 21, 2024

ffranr Oct 21, 2024

dstadulis Oct 21, 2024

ffranr Oct 21, 2024

dstadulis Oct 21, 2024

ffranr Oct 21, 2024 •

edited

Loading

guggero left a comment

dstadulis left a comment

	// Alice requests a bid quote from Bob. Her request includes an asset rates hint
	// Alice requests a bid quote from Bob. Her request includes an asset-rate hint

	// (ask). Alice get the asset rates hint by calling this endpoint. She sets:
	// (ask). Alice receives the asset rates hint by calling this endpoint. She sets:

		// Deprecated: Use QueryAssetRatesResponse.ProtoReflect.Descriptor instead.
		func (*QueryAssetRatesResponse) Descriptor() ([]byte, []int) {

	"description": "subjectAssetRate is the number of subject asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\nsubject asset to BTC (conversion) rate. When the subject asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."
	"description": "subjectAssetRate is the number of subject asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\nsubject asset to BTC (conversion) rate. When the subject asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats per one bitcoin."

	"description": "paymentAssetRate is the number of payment asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\npayment asset to BTC (conversion) rate. When the payment asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats in a BTC."
	"description": "paymentAssetRate is the number of payment asset units per BTC represented\nas a fixed-point number. This field is also commonly referred to as the\npayment asset to BTC (conversion) rate. When the payment asset is BTC\nmilli-satoshis (msats), this field should be set to 100 billion since\nthere are 100 billion msats per one bitcoin."

Remove remaining instances of "rate tick" from RFQ systems #1156

Remove remaining instances of "rate tick" from RFQ systems #1156

Conversation

ffranr commented Oct 21, 2024 • edited Loading

ffranr commented Oct 21, 2024 • edited Loading

coveralls commented Oct 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11447101930

Details

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

dstadulis left a comment

Choose a reason for hiding this comment

ffranr commented Oct 21, 2024 •

edited

Loading

ffranr commented Oct 21, 2024 •

edited

Loading

coveralls commented Oct 21, 2024 •

edited

Loading

ffranr Oct 21, 2024 •

edited

Loading