-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
README+docs: create document explaining decimal display and RFQ, add reference implementation #1059
Conversation
I'm still parsing, thanks for putting this up, very useful. I think we should make sure we're on the same page with @Liongrass on this before merging. Specifically on the question: what goes in the GitBooks site and what goes in this repo's |
Great doc! I will link to it from the github pages. I do think this kind of content would be most useful there, but we don't really have a clear idea of how we separate docs that are part of the repo and docs that are part of gitbook |
useful: I wonder if you could go even further and make at least one or two explicit recommendations. At a most basic level, if we are minting a coin intended to represent stable US currency, it sounds like, given the above assertion, we would want one asset to equal $0.0001. If that's the case (or it's a different number...), you could say this explicitly in the docs. |
Thanks for your feedback. There are definitely a number of things to still figure out here. But maybe that doesn't really matter since an asset minted today should also work well in the future when/if the price of BTC is very high... So perhaps the recommendation should be that the The goal of the document (and that's why the PR is still in draft) is to play around with actual numbers and different currency representation examples and then come up with a recommendation. So I appreciate any opinions and/or insights from other people, especially (and current) future asset issuers. |
The standard in the ETH and USDT world seems to be decimal display of |
Okay, that's useful to know that 6 decimal units is the default for ERC-20. Unfortunately I think using For example, let's assume the BTC price is
So the price rate returned by the oracle would be To avoid that we'd either want fewer decimal places to represent the asset or need to use another unit than @ffranr any opinions here? |
Putting this here because it is closely related, but let me know if you think I should raise a separate issue. As above, the It seems so far that |
The decimal display is returned from |
Good to know. So, let's try to narrow down on the ideal value -- seems like the ideal value would be the maximum value allowed by BTC/Satoshis -- so is that |
It depends. As things are right now, As mentioned above, I think we need to change how the |
IIUC, didn't we move away from the rate tick during development as we can't ever send a value smaller than 1 mSAT, so being told that 1 unit is 0.10 mSAT isn't useful (can only ever send 1 mSAT)? |
Yes, that was the short term fix to get things working. But the I'm in the process of running some numbers to find out how to best represent things in a way that has the fewest inaccuracies and avoids us needing to tell issuers what value for My current thinking is to bring the rate tick back, which would mean we'd represent units as |
Here's my attempt at deriving everything with an empty beginner's mind: https://gist.github.com/Roasbeef/962b263ae5bb0c261dc526330f2223a0. You can play around with it by specifying values on the CLI ( Here's a sample run of the CLI:
The example chooses decimal display of 7 as:
If you go below that, then you can't express the amount of USD in an mSAT. One can use lower values, say 4, but then you can't express Larger scale values means more precision, but you overflow at a certain point. You also run into limits re the amount of BTC you can express as it's scaled from mSATs internally (we can represent up to about 184,467,440,737 mSAT so 1.84 BTC). To address that, we can switch to 128-bit arithmetic internally. |
Offline there was a comment that it's still the case that a higher On this topic, I think it's worth stepping back to ask what more precision gives us, and if it's always useful or not. @guggero brought up this example, with
In this case you do get more precision as you increase the scale value, but what does that mean in practice? The fundamental unit we care about for LN is 1 mSAT, but 1 mSAT is so small in today's dollar values that it ceases to be useful for all practical purposes (even with 1 SAT, BTC is the king when it comes to divisibility). Eg: Assuming there's a service that'll swap I think the answer to the above is to introduce a From the other angle, assuming we're using |
Pull Request Test Coverage Report for Build 10847808768Details
💛 - Coveralls |
I finally pushed up a version I'm at least happy with. The most recent version also shows code to demonstrate and clarify all the edge cases that were explored. The most notable changes compared to the previous push are:
|
This commit by roas is relevant to this PR: Roasbeef@c2da7c6 Avoids the use of floating-point arithmetic. |
@ffranr the latest version of that is now here: https://github.com/Roasbeef/taproot-assets/commits/rfq-doc/. Will turn to review of the main doc, and propagation into the protocol itself. |
docs/rfq-and-price-oracle.md
Outdated
# Asset Decimal Display | ||
|
||
Within the Taproot Assets Protocol, an asset's unit is an integer (`uint64`). | ||
That means, the protocol cannot represent fractions of an asset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, using fixed point arithmetic we can achieve similar levels of precision one would get using normal floating point operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree with the sentence. But at this point we're trying to explain why the decimal display solution was chosen. I'll add this sentence further down where we explain the current solution.
docs/rfq-and-price-oracle.md
Outdated
|
||
Due to the non-divisible (integer) nature of Taproot Asset units, the smallest | ||
asset amount that can be transferred with a Lightning Network HTLC is one asset | ||
unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually 1 mSAT though right?
1 asset can be transmitted using a normal point to point push payment or invoice (direct hop).
However, once we get into the broader LN network, the minimum value we can send is 1 mSAT. It would be possible to transmit 1 unit, but the sender would need to overpay via routing fees to the nodes along the route to transmit 1 asset unit to the receiver (depending on conversion rate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not wrong. But the point this sentence is trying to make is that we cannot send half an asset unit or 0 units. There always has to be at least a single unit within an HTLC, and if that unit's precision is bad, there's more overpayment.
docs/rfq-and-price-oracle.md
Outdated
- User query: `Q = how many out_asset units for in_asset amount?` (how many | ||
`buxx` do I need to seel to pay this payment denominated in `msat`?) | ||
- `out_asset`: `buxx` (user sells `buxx` asset to RFQ peer) | ||
- `in_asset`: `msat` (user "receives" `msat` from RFQ peer, which are then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the in/out asset delineation match the current RFQ spec/code? Thinking about it anew, perhaps we should frame it as relative to the edge node: the edge node?
In that case, it would be the reverse here:
- The edge node receives
buxx
(in_asset
) - The edge node sends out
msat
(`the out_asset)
A similar reframing might be useful in the section below re receiving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the current RFQ and RPC code in tapd
as well as the overhauled bLIP code in Roasbeef/blips#4 always define things from the point of view of the wallet end user and not the edge node.
I'd like to avoid flipping that around as that would require a lot of changes (and potentially lead to more confusion). So I made the direction very clear in the # RFQ
section.
@ffranr will pivot review focus to this shortly |
Addressed all comments and integrated your latest changes, @Roasbeef. Thanks a lot for all the suggestions and the arithmetic implementation! |
bc28c37
to
b4a04b3
Compare
@Roasbeef: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏸
rfqmath/fixed_point.go
Outdated
// FixedPoint is used to represent fixed point arithmetic for currency related | ||
// calculations. A fixed point consists of a value, and a scale. The value is | ||
// the integer representation of the number. The scale is used to represent the | ||
// fractional/decimal component. | ||
type FixedPoint[T Int[T]] struct { | ||
// Value is the value of the FixedPoint integer. | ||
Value T | ||
|
||
// Scale is used to represent the fractional component. This always | ||
// represents a power of 10. Eg: a scale value of 2 (two decimal | ||
// places) maps to a multiplication by 100. | ||
Scale int | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the field Value
should be called Coefficient
. See https://en.wikipedia.org/wiki/Significand for other terms that might fit. It's easier to describe a fixed-point in the BLIP if this field is not called "value". If we rename it we can use sentences like "the value of the coefficient".
And I think Scale
should have type uint8
. Because:
int
can vary across systems- we use
uint8
in wire messages - I don't think we ever want a negative value of
Scale
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, renamed Value
to Coefficient
and set the unit of Scale
to uint8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, calling it Coefficient
just obfuscates the meaning, and it's a long variable name to boot. The value is that, just a value, and scale helps you interpret it. If anything you could rename Scale
to Coefficient
as that's the actual multiplier value.
Value
is what ends up going on the wire, when I send you the amount of msats to send, am I sending you a value, or coefficient? Which will be easier for someone that doesn't have as much context as to understand: that the oracle sends a value, or a coefficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flipped a coin and the magic conch chose Coefficient
.
This is the current state of how we think the RFQ and price oracle RPCs should behave.
@ffranr I think you are right that we might want to represent the price oracle's price rate as two separate fields. If you look at the very last examples, USD<->JPY, things break down if
price_rate
is a single integer.Fixes #1059
Fixes #1098