Skip to content
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

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Aug 5, 2024

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

@ffranr
Copy link
Contributor

ffranr commented Aug 5, 2024

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 docs directory. I think it would be wise not to repeat ourselves, one copy is bound to go stale!

@Liongrass
Copy link
Contributor

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

@MegalithicBTC
Copy link

MegalithicBTC commented Aug 7, 2024

useful: The rule of thumb is: The closer to representing 1 satoshi with one asset unit, the better (the smaller the potential overpayment due to integer division).

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.

@guggero
Copy link
Member Author

guggero commented Aug 7, 2024

useful: The rule of thumb is: The closer to representing 1 satoshi with one asset unit, the better (the smaller the potential overpayment due to integer division).

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.
The reason I didn't include any explicit recommendations is that the actual number chosen depends on the exchange rate of the stablecoin to BTC. For example if BTC is at $1,000,000 the recommendation would be different than for BTC at $10,000.

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 decimal_display should be one (or two?) more than the smallest unit that exists for the fiat currency? So for example USD has cents as the smallest unit, which is already decimal_display = 2, so the recommendation for the asset would be decimal_display = 3.
And JPY has 1 ¥ as the smallest unit (decimal_display = 0), so the recommendation would be decimal_display = 1.

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.

@MegalithicBTC
Copy link

The standard in the ETH and USDT world seems to be 6 decimal places: https://ethereum.stackexchange.com/questions/134562/why-stables-coins-erc-20-have-6-decimals-while-other-have-18-decimals -- therefore, barring new information, I would suggest that for new stablecoins...

decimal display of 6
where one asset is worth is worth $0.000001
Assuming a current BTC price of $60,000, taproot-assets.experimental.rfq.mockoracleassetsperbtc should be set to 60000000000 (60,000,000,000)

@guggero
Copy link
Member Author

guggero commented Aug 7, 2024

Okay, that's useful to know that 6 decimal units is the default for ERC-20.

Unfortunately I think using 6 decimal places by default doesn't work in the BTC ecosystem, at least not with how we currently use the price rate .
Because the underlying asset of the Lightning Network is the milli-satoshi, so only three more decimal places than the satoshi itself.

For example, let's assume the BTC price is USD 53,000.00 and decimal_display = 6:

Satoshi per USD         = 1886.79245283 (1 BTC = 53,000 USD)
Decimal Display         = 6
Satoshi per Asset Unit  = 0.00188679245283 (1.866 milli-satoshi per Asset Unit)
Integer price_rate      = 1

So the price rate returned by the oracle would be 1 milli-satoshi per asset unit, which is off by a factor of 0.866.

To avoid that we'd either want fewer decimal places to represent the asset or need to use another unit than milli-satoshi per asset unit. Perhaps the tick rate mentioned in the original bLIP needs to be used after all.

@ffranr any opinions here?

@MegalithicBTC
Copy link

Putting this here because it is closely related, but let me know if you think I should raise a separate issue.

As above, the decimal_display is critical to interacting with any kind of stablecoin. So I would guess, that as the recipient of a newly-opened asset channel, at the time that the channel is opened, I should also know immediately the decimal_display of the asset.

It seems so far that lncli listchannels does not include the decimal_display information. Maybe the recipient of the channel is expected to call another API to find out the decimal_display value?

@guggero
Copy link
Member Author

guggero commented Aug 7, 2024

It seems so far that lncli listchannels does not include the decimal_display information. Maybe the recipient of the channel is expected to call another API to find out the decimal_display value?

The decimal display is returned from tapcli assets meta --asset_id xyz. But I guess it would make sense to add that information to the different calls that show a balance. Can you create a separate issue for just that please?

@MegalithicBTC
Copy link

To avoid that we'd either want fewer decimal places to represent the asset or need to use another unit than milli-satoshi per asset unit. Perhaps the tick rate mentioned in the original bLIP needs to be used after all.

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 4 ? or 3 ?

@guggero
Copy link
Member Author

guggero commented Aug 7, 2024

so is that 4 ? or 3 ?

It depends. As things are right now, 3 would be better for the accuracy of the price_rate. But it would mean the potential loss/gain due to overpayment/underpayment because of the integer division when doing multi-part payments would be more significant than with a value of 4.

As mentioned above, I think we need to change how the price_rate works, so we don't have any problems with accuracy when decimal_display = 6 is used.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 9, 2024

To avoid that we'd either want fewer decimal places to represent the asset or need to use another unit than milli-satoshi per asset unit. Perhaps the tick rate mentioned in the original bLIP needs to be used after all.

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)?

@guggero
Copy link
Member Author

guggero commented Aug 9, 2024

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 asset_units per msat representation also gets more inaccurate the smaller the asset unit gets (decimal_display of 6 for example).

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 decimal_display to choose (IMO there shouldn't be an accuracy tradeoff between different values).

My current thinking is to bring the rate tick back, which would mean we'd represent units as asset_units per 10_000 msat or asset_units per 10 sat. That would give us more decimal places to work with.
Or perhaps go two decimal steps further and make things a bit easier to process for human brains as well and choose asset_units per 1000 sat or asset_units per kilo-sat.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 10, 2024

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 (-msat and -usd). Those values then get scaled up internally for all the operations. Fixed point arithmetic is used everywhere. The base demo uses a scale value of 7, which is equivalent to a decimal display value of 7. It then also seeks to scale up mSATs by that same amount. So if you had a decimal display of 2 (cents), we'd map it megasat (lol, so 1 megasat = 1_000_000).

Here's a sample run of the CLI:

⛰   ./rfqmath -usd=1 -msat=1
1.0000000 mSAT is equal to 0.0000005 USD
1.0000000 USD is equal to 2000000.0000000 mSAT

The example chooses decimal display of 7 as:

  • BTC/USD price is 50,000 USD
  • 1 BTC = $50,000
  • 1 sat - 50,000 / 100_000_000 = $0.0005
  • 1 msat = 0.0005 / 1000 = $0.0000005

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 0.0000005. Instead you'd set that value to effectively zero, then multiply by 5 and divide by 10^7. The trade off is loss of precision below $0.0001.

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.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 13, 2024

Offline there was a comment that it's still the case that a higher scale value (term used in the gist above, decimal_display) does always give more precision up until uint64 overflow. This is certainly the case, to get around this we can move to 128-bit arithmetic as mentioned above, or even just have things operate at even higher precision and use a constrained big int internally.

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 1 BTC = 50,702.12:

scale: 5 
1 mSAT = 0.000.01 USD

scale: 7
1 mSAT = 0.0000197 USD

scale: 9
1 mSAT = 0.000019723 USD

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 BTC for USD-BEEF, there's no actual way for me get 0.00001 USD of value into "meatspace". UI wise, if we assume that the smallest unit it'll display is 0.01 USD, then if I try to send 1 mSAT into the wallet, will it even show up?

I think the answer to the above is to introduce a min_rfq_unit value into the protocol. This determines what the smallest unit is that can be sent/recv'd using the RFQ flow. Practically, I think all that matters is being able to send 1 SAT and/or 0.01 USD. Snapshotting today, 1 USD = 1,676 SATS, 0.01 USD = 16.76 SATS and 1 SAT = 0.000596 USD.

From the other angle, assuming we're using uint64 or uint128 as our internal unit for the calculations, then we also need to put some constraints on the values of decimal_display. Potentially the RFQ logic just rejects any orders for something with a ridiculous dec_display value like 100. On the issuer side, it's possible to burn the mint in a single transaction, effectively reminting with a distinct decimal display value.

@coveralls
Copy link

coveralls commented Aug 13, 2024

Pull Request Test Coverage Report for Build 10847808768

Details

  • 223 of 223 (100.0%) changed or added relevant lines in 3 files are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 40.376%

Files with Coverage Reduction New Missed Lines %
tapdb/addrs.go 2 79.04%
tappsbt/create.go 2 53.22%
tapgarden/caretaker.go 4 68.5%
Totals Coverage Status
Change from base Build 10839632557: 0.2%
Covered Lines: 24210
Relevant Lines: 59962

💛 - Coveralls

@guggero
Copy link
Member Author

guggero commented Aug 13, 2024

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:

  • We use the FixedPoint struct to communicate asset prices (with the unit/precision always being number of asset units per BTC, scaled to decimal_display of asset), thanks @Roasbeef for the idea. The FixedPoint also maps nicely to gRPC.
  • We don't use the proposed FixedPoint muldiply/divide functions but big.Float under the hood to avoid overflows and loss of precision in integer space.
  • The TestFindDecimalDisplayBoundaries test prints boundary values for different decimal display units at different BTC/USD prices. Given the different values (max BTC that can be represented, smallest payable HTLC amount, max MPP rounding error), we can now confidently recommend a value for decimal_display of 6 for currencies with subunits (e.g. cents/pennies) and a value of 4 for currencies with no subunits (e.g. JPY).
  • The price_rate field in the RFQ RPC calls will be replaced with:
    • price_out_asset: out_asset_units_per_btc (e.g. 50702120000 for 50'702.12 USD/BTC with decimal_display=6)
    • price_in_asset: in_asset_units_per_btc (msat per BTC, always 100000000000 for BTC/mSAT)

rfq/convert.go Outdated Show resolved Hide resolved
@ffranr
Copy link
Contributor

ffranr commented Aug 20, 2024

This commit by roas is relevant to this PR: Roasbeef@c2da7c6

Avoids the use of floating-point arithmetic.

@Roasbeef
Copy link
Member

@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.

@dstadulis dstadulis added this to the v0.4.2 milestone Aug 24, 2024
docs/rfq-and-price-oracle.md Outdated Show resolved Hide resolved
# 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.
Copy link
Member

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.

Copy link
Member Author

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 Show resolved Hide resolved

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.
Copy link
Member

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).

Copy link
Member Author

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 Show resolved Hide resolved
docs/rfq-and-price-oracle.md Outdated Show resolved Hide resolved
docs/rfq-and-price-oracle.md Outdated Show resolved Hide resolved
docs/rfq-and-price-oracle.md Outdated Show resolved Hide resolved
- 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
Copy link
Member

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.

Copy link
Member Author

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.

docs/rfq-and-price-oracle.md Outdated Show resolved Hide resolved
@dstadulis
Copy link
Collaborator

dstadulis commented Aug 27, 2024

@ffranr will pivot review focus to this shortly

@guggero
Copy link
Member Author

guggero commented Sep 4, 2024

Addressed all comments and integrated your latest changes, @Roasbeef. Thanks a lot for all the suggestions and the arithmetic implementation!
PR should now be ready for full review.

@guggero guggero changed the title README+docs: create document explaining decimal display and RFQ README+docs: create document explaining decimal display and RFQ, add reference implementation Sep 4, 2024
@guggero guggero force-pushed the rfq-doc branch 2 times, most recently from bc28c37 to b4a04b3 Compare September 5, 2024 11:44
@guggero guggero requested a review from ffranr September 5, 2024 15:46
@lightninglabs-deploy
Copy link

@Roasbeef: review reminder
@Liongrass: review reminder
@ffranr: review reminder

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏸

Comment on lines 9 to 21
// 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
}
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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.

@ffranr ffranr added this pull request to the merge queue Sep 16, 2024
Merged via the queue into lightninglabs:main with commit 7496ab3 Sep 16, 2024
18 checks passed
@guggero guggero deleted the rfq-doc branch September 16, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]: Decimals cause the Price Oracle to have significant price deviations or failures.
8 participants