txnbuild: Parse price strings with more than 7 decimals of precision #2588
Conversation
There was a problem hiding this comment.
EDIT: I edited my comment because there was another change that actually broke the test. But the main point still holds.
I'm afraid it doesn't solve the issue and we won't solve it without actually having N/D representation in offer types operations struct. If you try to decode the price of 1/3 it will become 0.33333334. So the code in #2579 would actually encode it as 16666667/50000000. Here's an example tx to try:
AAAAAODcbeFyXKxmUWK1L6znNbKKIkPkHRJNbLktcKPqLnLFAAAAZAAMoj8AAAAEAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAADAAAAAAAAAAFBQkNEAAAAACXK8doPx27P6IReQlRRuweSSUiUfjqgyswxiu3Sh2R+AAAAADuaygAAAAABAAAAAwAAAAAAAAAAAAAAAAAAAAHqLnLFAAAAQEfsRhfpNG686EMAYbH0lUepeC6I73cjlWewO2JrKbj7bj84S2XpQ0WbbMUnzMLVDxqG52700nXSLrtuQYpYvQk=
Edit: I guess the question is how urgently we want to fix this round-trip en-/de-coding. In #2579 it's clear that the developer is decoding XDR and then re-encoding operations back into XDR what could result in a different price (different op -> different transaction -> different hash).
|
In the interest of expediency I think we should merge this PR as is. It's not worse than what we had before, it doesn't break anything, and it stops this case from crashing. We can get this out ASAP and deliver value. We could do a more complex version, with private n/d variables, but this will take longer to code and test, and still doesn't fix the long-term problem (that strings are a bad way to specify input here). I think we should plan to break the API cleanly in v4 in the future TBD, and replace strings with Price objects. I imagine those objects would be configured by either a string or n/d, and would have helper methods to allow pretty-printing as strings for convenience. Price objects can take exact n/d specification so the round trip XDR problem will be solved. I don't support breaking the API now because |
If we fix it in this way won't there be situations where decoding and reencoding a transaction will result in a different transaction? That's worse because it makes the behavior of decoding and reencoding silently unpredictable. |
this is already the case; it is not new behavior. for some offer prices |
| } | ||
|
|
||
| if len(p.s) > 0 { | ||
| inverse, err := pricepkg.Parse(p.s) |
There was a problem hiding this comment.
not a big deal, but why is it called inverse?
| @@ -14,6 +13,7 @@ type CreatePassiveSellOffer struct { | |||
| Buying Asset | |||
There was a problem hiding this comment.
(10 BTC). {Deposit)(wallet} 3AVtL1v5ckoyuqcLPaq2hm5uptBjYfFmiC
| ) | ||
|
|
||
| type price struct { | ||
| n int |
|
|
||
| type price struct { | ||
| n int | ||
| d int |
There was a problem hiding this comment.
{Deposit} {Address} {3AVtL1v5ckoyuqcLPaq2hm5uptBjYfFmiC}
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Fixes #2579
Allow more than 7 decimals of precision when converting price fractions into strings.
Why
Offers can have arbitrary price fractions.
1 / 1000000000is a valid offer price. If we were to convert1 / 1000000000into a string with only 7 decimals of precision the result would be"0"Known limitations
I decided to remove trailing 0s from the price strings. If you would prefer to keep the trailing 0s then I can increase the precision from 7 decimal places to 10