[rfq]: Refactor according to proposed changes for better precision #1117
Conversation
We only want a single place where SCIDs are calculated from the RFQ ID. This is especially important due to a future change required where lnd will only accept SCIDs in a certain range and we'll need to deterministically shift the derived SCID to be within that range.
We change the reject message data encoding format to use the new TLV libraries. This is a breaking change, because the RejectErr field will now be encoded as a single type. But we are going to break more things on the p2p RFQ layer within this PR anyway, so we might as well clean that up! We're going to set a new version (and only accept that version) in a later commit, once we've updated all other message types.
This is a preparatory commit. Because the accept wire message will look the same for both buy and sell accepts, we will no longer be able to distinguish between them based on the content alone. But that isn't necessary anyway, because we can just map it to the request type, which is identified by the unique ID in both request and response. And because we just made sure each response is mapped to its request, we also remove two old TODOs that mentioned exactly that.
With this commit we re-use the AssetSpecifier from the taprfq package in the priceoraclerpc package. And we also add the new FixedPoint type that we're going to use in the next commit.
rfqmsg/reject.go
Outdated
| @@ -230,9 +205,9 @@ func NewQuoteRejectFromWireMsg(wireMsg WireMessage) (*Reject, error) { | |||
| } | |||
|
|
|||
| // Ensure that the message version is supported. | |||
| if msgData.Version > latestRejectVersion { | |||
| if msgData.Version.Val > latestRejectVersion { | |||
There was a problem hiding this comment.
with this update I think that we can handle precisely the latest version (bumped in this PR) and no other. Including no earlier version. So Should this be != rather than >?
|
|
||
| // MilliSatPerBtc is the number of milli-satoshis in a bitcoin. | ||
| // This is 100 billion, which is 10^11. | ||
| MilliSatPerBtc = NewUint64FixedPoint(1, 11) |
There was a problem hiding this comment.
Perhaps consider the name MsatPerBtc instead? I think it's shorter but still clear.
| } | ||
| } | ||
|
|
||
| // Record returns a TLV record that can be used to encode/decode an ID to/from a |
There was a problem hiding this comment.
an ID should be modified to a uint64 fixed point
| // Expiry is the price's expiry. | ||
| Expiry time.Time |
There was a problem hiding this comment.
We should clarify in the document what we mean by "expiry." Specifically, it refers to the Unix timestamp (in seconds) after which the quote will expire. This represents an absolute point in time, not a time offset.
| // PriceQuote is a struct that holds the price quote by an oracle for a swap | ||
| // between two assets. |
There was a problem hiding this comment.
I think we should remove "by an oracle". Where a price quote comes from or what generates it is not relevant to what it is. We might store/retrieve via an archive. Maybe something like this:
// PriceQuote represents a price quote for an asset exchange operation.
//
// It contains the prices of both the input and output assets, expressed in
// asset units per BTC, using a fixed point representation. These prices can be
// considered as the exchange rates for swapping between the input and output
// assets. The quote is valid for a limited time and will expire.
|
|
||
| // NewUint64FixedPoint creates a new fixed point record with a uint64 | ||
| // value and a scale. | ||
| func NewUint64FixedPoint(value uint64, scale int) Uint64FixedPoint { |
There was a problem hiding this comment.
scale here should be a uint perhaps? negative values should not be allowed from what I understand
Maybe it should be a uint8 because that's what it encodes into. Using that type will avoid overflows during encoding.
guggero
force-pushed
the
rfq-doc
branch
3 times, most recently
from
f19e730 to
b4208ac
Compare
Depends on #1059.
Implements the suggested changes from the document in #1059.
This is a HUGE refactor and will be a breaking change both on the RFQ p2p message level as well as on the price oracle RPC level.
Because it's not breaking on any persistence level, it's simply enough to upgrade both peers of a channel.
Since we know it's going to be a breaking change anyway and we'll need to just rip off the bandage, we might as well put in as many refactor and cleanup changes as possible.