diff --git a/Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj b/Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj index 3f3af843..e6711263 100644 --- a/Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj +++ b/Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj @@ -13,7 +13,7 @@ Apache-2.0 https://github.com/StaticBit-io/XrplCSharp XrplCSharp - 11.0.0.0 + 11.0.1.0 diff --git a/CHANGES.md b/CHANGES.md index c5cf7a5a..f20e80ee 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,26 @@ # Changes +## Unreleased + +* **The signing path builds its `JsonSerializerOptions` once** (#147). `XrplBinaryCodec.ObjectToJsonNode` constructed a fresh instance on every call, and every signing operation goes through it - `Encode`, `EncodeForSigning`, `EncodeForSigningClaim` and `EncodeForMultiSigning` all route there. Measured end to end on `EncodeForSigning`, 50 000 calls: **1075.8 ms and 14458 B/op before, 621.8 ms and 13601 B/op after** - 1.73x, and 857 fewer bytes each call. The encoded blob is unchanged, hashing identically either way. + * not the catastrophe this bug is usually described as: since .NET 7 System.Text.Json shares a caching context between structurally equal options instances, so type metadata was not being rebuilt per call - had it been, the gap would be orders of magnitude rather than 1.7x. What was paid is an allocation and a structural-equality lookup in a pool capped at 64 contexts + * `LOVault.ToHex` had the same pattern on a colder path + * `Xrpl.BinaryCodec` moves to **11.0.1.0** for it. The other base packages are untouched and stay where they are - they are consumed by `ProjectReference`, so a package built at a newer version keeps depending on the published ones + +* **An amount the ledger allows but `decimal` cannot hold is refused, not guessed at** (#148). `Currency.ValueAsNumber` answered such values three different ways: a positive one clamped to `decimal.MaxValue`, a negative one threw `FormatException`, and a very small one quietly became zero. XRPL issued currency runs from `1e-81` to roughly `1e96` - a 16-digit mantissa with an exponent in `[-96, 80]`, per rippled's `STAmount` - while `decimal` stops near `7.9e28`, so this cannot be parsed away; the only choice is how to fail. + * the clamp is gone. An amount above the range now throws `AmountOutOfRangeException`, which carries the value as the node sent it. Returning `7.9e28` for `1e96` is wrong by 67 orders of magnitude, and it did not stay contained: `GetBalanceChanges` subtracts two balances, so the clamped value went on to throw `OverflowException` from arithmetic instead + * the negative case was a parse bug. The fallback's `NumberStyles` expression came to `AllowExponent | AllowDecimalPoint` - `AllowLeadingSign` was missing, so no negative value could reach the branch meant to handle it. The primary parse was correct all along, despite six `&` terms that all evaluate to zero + * **an amount below `1e-28` still returns zero, and the asymmetry is deliberate.** A balance of `1e-81` rounded to zero is zero at any scale a caller can act on; failing over it would cost more than it protects. An amount of `1e96` reported as `7.9e28` is not in that category + * the threshold is nowhere near the protocol's ceiling: `1e29` is barely above `decimal.MaxValue` and was already unreachable. A token with a large supply meets this without going anywhere near the ledger's limits + * `Offer.AmountEach` reads the same property on both sides of an order and divides them. Anyone may place an offer in their own token at any value the protocol allows, so it fails the same way - and used to return a plausible-looking exchange rate that was wrong by 67 orders of magnitude, without throwing. Both it and `GetBalanceChanges` now say so in their own documentation rather than leaving it to be discovered + * `Currency.ToString()` falls back to the raw value rather than letting the getter throw through it. By convention `ToString` does not throw, and logging, string interpolation and a debugger's watch window are exactly where someone would be while working out why an amount is unusual - failing there hides the value at the moment it is most wanted + * the setter no longer writes a string it cannot read back. `G16` keeps the ledger's sixteen significant digits and rounds to nearest - which is what rippled does, so it stays - but at the top of `decimal`'s own range rounding to nearest rounds **up**, past what the type holds. Only there is the sixteenth digit truncated instead, which cannot overflow because dropping digits only moves a number toward zero + * **dust survives.** Balances like `0.000000000000000001` do arrive from the network and the SDK must be able to send them back; they are safe because the ledger's limit is sixteen *significant* digits and dust carries one. Pinned by a test, because the obvious way to bound precision - truncating to sixteen decimal *places* - turns `1e-18` into zero and would make a remainder disappear silently + * a test that claimed a round trip can never increase a value was asserting something the protocol does not promise, on a value where it could not fail. rippled's `Number` defaults to `ToNearest`, so an amount beyond sixteen digits can legitimately come back larger. Replaced with the property that does hold - an amount already at ledger precision goes out unchanged - and with one stating the rounding, so the next reader does not reach for truncation and move the SDK away from rippled + * why the setter rounds while the binary codec refuses more than sixteen digits outright is now written down. The two see different inputs: seventeen digits cannot arrive from the network, so the codec only ever meets a hand-written string, while the setter meets computed `decimal`s that routinely carry 28 - `AmmMath` returns them + * `Console.WriteLine(exception)` is out of the parse path. A library does not write to the console + * **breaking in effect, if not in signature**: code that read an out-of-range amount used to get a number and now gets an exception. Representing the full range instead of refusing it is #150 + ## 11.0.0.0 08/26/2026 ### Migration at a glance diff --git a/Tests/Xrpl.Tests/Models/TestCurrency.cs b/Tests/Xrpl.Tests/Models/TestCurrency.cs index c0aaabe0..4d8b9d06 100644 --- a/Tests/Xrpl.Tests/Models/TestCurrency.cs +++ b/Tests/Xrpl.Tests/Models/TestCurrency.cs @@ -2,13 +2,291 @@ using System.Globalization; +using System; + +using Xrpl.Client.Exceptions; using Xrpl.Models.Common; +using Xrpl.Models.Transactions; namespace XrplTests.Xrpl.Models { [TestClass] public class TestUCurrency { + #region Amounts outside decimal's range - issue #148 + + private static Currency Iou(string value) => + new Currency { CurrencyCode = "USD", Issuer = "rIssuer", Value = value }; + + /// + /// An amount too large for decimal is refused, whichever sign it carries. + /// + /// + /// + /// These used to do two different things. A positive one clamped to decimal.MaxValue + /// and said nothing, so a balance of 1e96 was answered with 7.9e28 - wrong by 67 orders of + /// magnitude, and wrong in a way that flowed onward into the caller's arithmetic. A + /// negative one threw FormatException, because the fallback parse was missing + /// AllowLeadingSign and could not read the minus. + /// + /// + /// The threshold is not near the ledger's ceiling: 1e29 is barely above + /// decimal.MaxValue and already unreachable. + /// + /// + [TestMethod] + public void ValueAsNumber_OutsideDecimalRange_Throws() + { + foreach (string value in new[] + { + "1e29", "-1e29", + "9e80", "-9e80", + "9999999999999999e80", "-9999999999999999e80", + }) + { + Assert.ThrowsExactly( + () => _ = Iou(value).ValueAsNumber, + $"'{value}' is a legitimate ledger amount that decimal cannot hold."); + } + } + + /// + /// The exception carries the amount as the node sent it. + /// + /// + /// The point of refusing rather than clamping is that the real figure is still available; + /// an exception that only said "too big" would trade one lost value for another. + /// + [TestMethod] + public void ValueAsNumber_OutOfRangeException_CarriesTheOriginalValue() + { + AmountOutOfRangeException error = Assert.ThrowsExactly( + () => _ = Iou("-9999999999999999e80").ValueAsNumber); + + Assert.AreEqual("-9999999999999999e80", error.Value); + Assert.Contains("-9999999999999999e80", error.Message, StringComparison.Ordinal); + } + + /// + /// Negative amounts inside the range still parse, which is what makes the bound the bug. + /// + /// + /// Without this the test above would pass on an implementation that simply refused every + /// negative amount - and negative balances are ordinary: a RippleState balance is + /// negative from the low account's side. + /// + [TestMethod] + public void ValueAsNumber_NegativeInsideRange_StillParses() + { + Assert.AreEqual(-100m, Iou("-100").ValueAsNumber); + Assert.AreEqual(-0.00000000015m, Iou("-1.5e-10").ValueAsNumber); + Assert.AreEqual(-79228162514264337593543950335m, Iou("-79228162514264337593543950335").ValueAsNumber); + } + + /// + /// An amount too small for decimal becomes zero rather than throwing. + /// + /// + /// The asymmetry with overflow is deliberate. The ledger goes down to 1e-81 and decimal + /// stops near 1e-28, but a balance that small is zero at any scale a caller can act on, so + /// failing over it would cost more than it protects. Overflow is the opposite: the number + /// that would be returned is wrong by orders of magnitude and unsafe to use. + /// + [TestMethod] + public void ValueAsNumber_BelowDecimalPrecision_IsZeroNotAnError() + { + Assert.AreEqual(0m, Iou("1e-96").ValueAsNumber); + Assert.AreEqual(0m, Iou("-9999999999999999e-96").ValueAsNumber); + } + + /// + /// Something that is not a number is still a format error, not an out-of-range one. + /// + /// + /// The two are told apart by whether double can read the string: it spans the whole + /// ledger range, so it succeeds exactly when the value is real and decimal merely cannot + /// hold it. Reporting both the same way would hide a malformed response behind a message + /// about magnitude. + /// + [TestMethod] + public void ValueAsNumber_NotANumber_IsAFormatError() + { + Assert.ThrowsExactly(() => _ = Iou("abc").ValueAsNumber); + Assert.ThrowsExactly(() => _ = Iou("1.2.3").ValueAsNumber); + Assert.ThrowsExactly(() => _ = Iou(" 100 ").ValueAsNumber); + } + + /// + /// A non-finite string is not a quantity too large, and must not be reported as one. + /// + /// + /// double.TryParse accepts NaN, Infinity and -Infinity whatever + /// NumberStyles it is handed, because those symbols are matched separately from the + /// numeric ones. Since the check that separates "will not fit" from "is not a number" runs + /// through double, without a finiteness test these would come back as + /// - a confident answer about magnitude for a + /// string that has none. + /// + [TestMethod] + public void ValueAsNumber_NonFiniteStrings_AreFormatErrorsNotRangeErrors() + { + foreach (string value in new[] { "NaN", "Infinity", "-Infinity" }) + { + Assert.ThrowsExactly( + () => _ = Iou(value).ValueAsNumber, + $"'{value}' is not a quantity at all, let alone one that is too large."); + } + } + + /// + /// An out-of-range numerator is refused even when the denominator is zero. + /// + /// + /// Offer.AmountEach returns zero when TakerPays is zero. While it read the + /// two sides lazily, that early return meant an unrepresentable TakerGets slipped + /// through unnoticed - so whether the documented exception appeared depended on the value + /// of an unrelated field, which is not a contract anyone can hold you to. + /// + [TestMethod] + public void AmountEach_OutOfRangeNumerator_ThrowsEvenWithAZeroDenominator() + { + Offer offer = new Offer { TakerGets = Iou("9e80"), TakerPays = Iou("0") }; + + Assert.ThrowsExactly(() => _ = offer.AmountEach); + } + + /// + /// A zero denominator on its own still yields zero rather than dividing. + /// + [TestMethod] + public void AmountEach_ZeroDenominator_IsZero() + { + Offer offer = new Offer { TakerGets = Iou("100"), TakerPays = Iou("0") }; + + Assert.AreEqual(0m, offer.AmountEach); + } + + /// + /// The two properties that compute rather than read fail the same single way. + /// + /// + /// GetBalanceChanges subtracts two balances and Offer.AmountEach divides two + /// amounts, and both used to have a second failure behind the first: a clamped + /// decimal.MaxValue would go on to throw OverflowException from the + /// arithmetic, or - worse for the order book - return a plausible exchange rate that was + /// wrong by 67 orders of magnitude without throwing at all. + /// + [TestMethod] + public void ValueAsNumber_ArithmeticOnOutOfRangeAmounts_FailsAtTheSource() + { + Assert.ThrowsExactly( + () => _ = Iou("9e80").ValueAsNumber - Iou("-100").ValueAsNumber, + "This used to be an OverflowException from subtracting a clamped MaxValue."); + + Offer offer = new Offer { TakerGets = Iou("9e80"), TakerPays = Iou("1") }; + + Assert.ThrowsExactly( + () => _ = offer.AmountEach, + "This used to return decimal.MaxValue as an exchange rate, silently."); + } + + /// + /// ToString shows the amount instead of failing on it. + /// + /// + /// By convention does not throw, and the places it is reached + /// from - logging, string interpolation, a debugger's watch window - are exactly where + /// someone would be while working out why an amount is unusual. Letting the getter throw + /// through it would hide the value at the moment it is most wanted. + /// + [TestMethod] + public void ToString_OutOfRangeAmount_ShowsTheRawValue() + { + Assert.AreEqual("USD: 9e80", Iou("9e80").ToString()); + Assert.AreEqual("USD: -9e80", Iou("-9e80").ToString()); + Assert.AreEqual("USD: NaN", Iou("NaN").ToString()); + + // Anything it can render, it still renders the same way. + Assert.AreEqual("USD: 100", Iou("100").ToString()); + } + + /// + /// Writing the largest produces an amount that can be read back. + /// + /// + /// + /// The setter formats with G16 to keep the ledger's sixteen significant digits, + /// rounding to nearest - which is what rippled does, so it is not a place to be clever. + /// The one input that could not serve is the top of 's own range, + /// where rounding to nearest rounds up, past what the type holds: the SDK wrote a + /// string it then refused to read. + /// + /// + /// There the sixteenth digit is truncated instead. Truncating cannot overflow, because + /// dropping digits only ever moves a number toward zero. + /// + /// + [TestMethod] + public void ValueAsNumber_WritingTheLargestDecimal_CanBeReadBack() + { + foreach (decimal edge in new[] { decimal.MaxValue, decimal.MinValue }) + { + Currency currency = new Currency { CurrencyCode = "USD", Issuer = "rIssuer" }; + currency.ValueAsNumber = edge; + + decimal readBack = currency.ValueAsNumber; + + Assert.IsTrue( + Math.Abs(readBack) <= Math.Abs(edge), + $"Truncation moves toward zero; {currency.Value} came back as {readBack}."); + } + } + + /// + /// A dust remainder survives being read, written and encoded for the ledger. + /// + /// + /// + /// Balances like 0.000000000000000001 do arrive from the network, and the SDK has to + /// be able to send one back. They are safe here because smallness is not the constraint - + /// the ledger's limit is sixteen significant digits, and dust carries one. + /// + /// + /// Pinned because the obvious way to bound precision destroys exactly these. Truncating to + /// sixteen decimal places rather than significant digits turns 1e-18 into + /// zero: a remainder would silently disappear, and the SDK would send nothing where a + /// balance stood. This test fails if anyone reaches for that. + /// + /// + [TestMethod] + public void ValueAsNumber_DustRemainders_SurviveTheWholeRoundTrip() + { + foreach (string fromTheWire in new[] + { + "0.000000000000000001", // 1e-18 + "1e-18", + "0.0000000000000000000000001", // 1e-25 + "1e-28", // the smallest decimal holds + "-0.000000000000000001", + }) + { + Currency incoming = new Currency { CurrencyCode = "USD", Issuer = "rIssuer", Value = fromTheWire }; + decimal amount = incoming.ValueAsNumber; + + Assert.AreNotEqual(0m, amount, $"'{fromTheWire}' must not read as nothing."); + + Currency outgoing = new Currency { CurrencyCode = "USD", Issuer = "rIssuer" }; + outgoing.ValueAsNumber = amount; + + Assert.AreEqual( + amount, + outgoing.ValueAsNumber, + $"'{fromTheWire}' must survive being written back; it became '{outgoing.Value}'."); + } + } + + #endregion + #region Round-trip ValueAsNumber (G16 fix verification) [TestMethod] @@ -111,15 +389,68 @@ public void ValueAsNumber_Setter_UsesG0ForXrp() Assert.AreEqual("1500000", currency.Value); } + /// + /// An amount already at the ledger's precision survives a round trip untouched. + /// + /// + /// + /// This was called NeverRoundsUp and asserted that a round trip must not increase a + /// value. That is not a property of the ledger: G16 rounds to nearest, and so does + /// rippled - Number's default mode is ToNearest - so an amount carrying more + /// than sixteen significant digits can legitimately come back larger. The old name + /// promised an invariant the protocol does not have. + /// + /// + /// It also could not have caught a violation. The value it used has exactly sixteen + /// significant digits, so there is nothing for G16 to round in either direction; the + /// assertion passed for a value where it could not fail. + /// + /// + /// What is worth pinning is the property that does hold: an amount the ledger could have + /// sent goes out again unchanged. + /// + /// + [TestMethod] + public void ValueAsNumber_AtLedgerPrecision_RoundTripsUnchanged() + { + foreach (string atPrecision in new[] + { + "316227.7660168379", // an AMM LP token amount + "9999999999999999", // the largest mantissa + "1000000000000000", // the smallest + "0.1234567890123457", + }) + { + Currency currency = new Currency { CurrencyCode = "USD", Issuer = "rIssuer", Value = atPrecision }; + decimal original = currency.ValueAsNumber; + + currency.ValueAsNumber = original; + + Assert.AreEqual( + original, + currency.ValueAsNumber, + $"'{atPrecision}' is already at ledger precision and must survive intact."); + } + } + + /// + /// Beyond sixteen digits the value rounds to nearest, and may grow. That is the ledger's + /// own behaviour, not a defect. + /// + /// + /// Stated as a test because the opposite was previously asserted, and because a reader who + /// finds a value that grew will otherwise reach for the same wrong fix: rippled's + /// Number defaults to RoundingMode::ToNearest, so truncating here would move + /// the SDK away from the protocol rather than toward it. + /// [TestMethod] - public void ValueAsNumber_16Digits_NeverRoundsUp() + public void ValueAsNumber_BeyondLedgerPrecision_RoundsToNearestAsTheLedgerDoes() { - Currency currency = new Currency { CurrencyCode = "USD", Issuer = "rIssuer", Value = "316227.7660168379" }; - decimal original = currency.ValueAsNumber; - currency.ValueAsNumber = original; - decimal afterRoundTrip = decimal.Parse(currency.Value, CultureInfo.InvariantCulture); - Assert.IsTrue(afterRoundTrip <= original, - $"Round-trip must not increase value: original={original}, afterRoundTrip={afterRoundTrip}"); + Currency currency = new Currency { CurrencyCode = "USD", Issuer = "rIssuer" }; + currency.ValueAsNumber = 1.2345678901234565m; + + Assert.AreEqual("1.234567890123457", currency.Value); + Assert.IsTrue(currency.ValueAsNumber > 1.2345678901234565m, "Rounding to nearest went up here, as it should."); } #endregion diff --git a/Tests/Xrpl.Tests/Utils/GetBalanceChangesTests.cs b/Tests/Xrpl.Tests/Utils/GetBalanceChangesTests.cs index 0ca19e49..d6aebacf 100644 --- a/Tests/Xrpl.Tests/Utils/GetBalanceChangesTests.cs +++ b/Tests/Xrpl.Tests/Utils/GetBalanceChangesTests.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Text.Json; +using Xrpl.Client.Exceptions; using Xrpl.Client.Json; using Xrpl.Models.Transactions; @@ -66,6 +67,73 @@ public void TestBalanceChanges_Metadata1() Assert.AreEqual("-639.11146416", issuerTokenBuyerChanges.Value); Assert.AreEqual(buyer, issuerTokenBuyerChanges.Issuer); } + /// + /// An amount the ledger allows but decimal cannot hold stops this, and says so. + /// + /// + /// + /// The exception is documented on GetBalanceChanges itself, so it is exercised through + /// GetBalanceChanges rather than by imitating the subtraction it performs. A test that + /// mimics the arithmetic proves the arithmetic; it does not prove that this method reaches it, + /// which is what the documentation promises a caller. + /// + /// + /// The balance below is negative, which is the ordinary shape for a RippleState node + /// from the low account's side, and it is out of range - so it exercises the case that used to + /// fail with FormatException before the parse fix, and now names the real problem. + /// + /// + [TestMethod] + public void TestUGetBalanceChanges_AmountBeyondDecimal_ThrowsRatherThanReportingSomethingElse() + { + Meta metadata = JsonSerializer.Deserialize(metaDataOutOfRange, XrplJsonOptions.Default); + + AmountOutOfRangeException error = Assert.ThrowsExactly( + () => BalanceChanges.GetBalanceChanges(metadata)); + + Assert.AreEqual("-9999999999999999e80", error.Value); + } + + private const string metaDataOutOfRange = @"{ + ""AffectedNodes"": [ + { + ""ModifiedNode"": { + ""FinalFields"": { + ""Balance"": { + ""currency"": ""USD"", + ""issuer"": ""rrrrrrrrrrrrrrrrrrrrBZbvji"", + ""value"": ""-9999999999999999e80"" + }, + ""Flags"": 1114112, + ""HighLimit"": { + ""currency"": ""USD"", + ""issuer"": ""rXPMxBeefHGxx2K7g5qmmWq3gFsgawkoa"", + ""value"": ""0"" + }, + ""HighNode"": ""0"", + ""LowLimit"": { + ""currency"": ""USD"", + ""issuer"": ""rLiooJRSKeiNfRJcDBUhu4rcjQjGLWqa4p"", + ""value"": ""1000000000"" + }, + ""LowNode"": ""0"" + }, + ""LedgerEntryType"": ""RippleState"", + ""LedgerIndex"": ""1BC0B4F0B0C0B0F0B0C0B0F0B0C0B0F0B0C0B0F0B0C0B0F0B0C0B0F0B0C0B0F0"", + ""PreviousFields"": { + ""Balance"": { + ""currency"": ""USD"", + ""issuer"": ""rrrrrrrrrrrrrrrrrrrrBZbvji"", + ""value"": ""-100"" + } + } + } + } + ], + ""TransactionIndex"": 0, + ""TransactionResult"": ""tesSUCCESS"" + }"; + private const string metaData_1 = @"{ ""AffectedNodes"": [ { diff --git a/Xrpl/Client/Exceptions/AmountOutOfRangeException.cs b/Xrpl/Client/Exceptions/AmountOutOfRangeException.cs new file mode 100644 index 00000000..824c8620 --- /dev/null +++ b/Xrpl/Client/Exceptions/AmountOutOfRangeException.cs @@ -0,0 +1,48 @@ +using System; +using System.Globalization; + +namespace Xrpl.Client.Exceptions +{ + /// + /// An issued-currency amount the node sent is outside the range can hold. + /// + /// + /// + /// XRPL issued currency runs from 1e-81 to roughly 1e96 - rippled's + /// STAmount allows a 16-digit mantissa with an exponent in [-96, 80] - while + /// stops at about 7.9e28. The two do not fit inside one another, + /// and no parsing of the string can change that. + /// + /// + /// This used to be answered by clamping to , which is a number + /// that is wrong by up to 67 orders of magnitude and does not say so - and by a bare + /// for negative amounts, which named the string rather than the + /// problem. Both are gone; the amount that does not fit is reported as such. + /// + /// + /// carries what the node actually sent, so a caller who needs the real + /// figure still has it. Representing it rather than reporting it is issue #150. + /// + /// + public class AmountOutOfRangeException : RippleException + { + /// + /// The amount as the node sent it, in the ledger's own string form. + /// + public string Value { get; } + + /// + public AmountOutOfRangeException(string value) + : base(BuildMessage(value)) + { + Value = value; + } + + private static string BuildMessage(string value) => string.Format( + CultureInfo.InvariantCulture, + "The amount '{0}' is outside the range System.Decimal can represent (about ±7.9e28). " + + "XRPL issued currency reaches roughly 1e96, so this is a legitimate ledger value that " + + "this property cannot return. Read Currency.Value for the amount as sent.", + value); + } +} diff --git a/Xrpl/Models/Common/Currency.cs b/Xrpl/Models/Common/Currency.cs index 2ddc5d1f..cc889636 100644 --- a/Xrpl/Models/Common/Currency.cs +++ b/Xrpl/Models/Common/Currency.cs @@ -2,6 +2,7 @@ using System; using System.Globalization; +using Xrpl.Client.Exceptions; using System.Linq; using System.Text; using System.Text.RegularExpressions; @@ -78,64 +79,136 @@ public string MPTokenIssuanceID [JsonIgnore] public string CurrencyValidName => CurrencyCode.CurrencyReadableName(); + /// + /// What the ledger's amount string allows: a sign, a decimal point and an exponent, and + /// nothing else. Surrounding whitespace is not accepted, because the node never sends it. + /// + private const NumberStyles AmountStyles = + NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent; + /// /// decimal currency amount (drops for XRP) /// + /// + /// + /// does not cover the range XRPL allows an issued currency: the ledger + /// reaches roughly 1e96 and down to 1e-81, this type stops at about + /// 7.9e28. Amounts above that throw rather than + /// being answered with a number that is not the one the node sent. + /// + /// + /// Amounts below 1e-28 return zero instead of throwing, and the asymmetry is deliberate. + /// A balance of 1e-81 rounded to zero is zero at any scale a caller can act on, so + /// failing over it would cost more than it protects; an amount of 1e96 reported as + /// 7.9e28 is wrong by 67 orders of magnitude and is worth stopping for. + /// + /// + /// Writing rounds to the ledger's sixteen significant digits; the binary codec, given a string + /// with more than sixteen, refuses it outright. That looks inconsistent and is not: the two see + /// different inputs. A string with seventeen digits cannot arrive from the network - rippled + /// normalises the mantissa into [1e15, 1e16) before serialising - so the codec only ever + /// meets one a caller wrote by hand, where refusing is right. This setter meets a computed + /// , which routinely carries 28 digits: + /// returns such values. Rounding them is the service, not a loss. + /// + /// + /// The amount exceeds what can hold. + /// The amount is not a number at all. [JsonIgnore] public decimal ValueAsNumber { get { - try + if (string.IsNullOrWhiteSpace(Value)) { - return string.IsNullOrWhiteSpace(Value) - ? 0 - : decimal.Parse( - Value, - NumberStyles.AllowLeadingSign - | (NumberStyles.AllowLeadingSign & NumberStyles.AllowDecimalPoint) - | (NumberStyles.AllowLeadingSign & NumberStyles.AllowExponent) - | (NumberStyles.AllowLeadingSign & NumberStyles.AllowExponent & NumberStyles.AllowDecimalPoint) - | (NumberStyles.AllowExponent & NumberStyles.AllowDecimalPoint) - | NumberStyles.AllowExponent - | NumberStyles.AllowDecimalPoint, - CultureInfo.InvariantCulture); + return 0; } - catch (Exception e) + + if (decimal.TryParse(Value, AmountStyles, CultureInfo.InvariantCulture, out decimal amount)) { - try - { - var num = double.Parse( - Value, - (NumberStyles.Float & NumberStyles.AllowExponent) | NumberStyles.AllowExponent | NumberStyles.AllowDecimalPoint, - CultureInfo.InvariantCulture); - var valid = $"{num:#########e00}"; - if (valid.Contains(value: "e-")) - { - return 0; - } - - if (valid.Contains(value: '-')) - { - return decimal.MinValue; - } - - return decimal.MaxValue; - } - catch (Exception exception) - { - Console.WriteLine(exception); - throw; - } + return amount; } + + // Tell the two failures apart rather than reporting both as a bad format. double + // spans the whole ledger range, so parsing there succeeds exactly when the string is + // a real number that decimal simply cannot hold. + // + // IsFinite matters: double.TryParse accepts "NaN", "Infinity" and "-Infinity" whatever + // NumberStyles it is given, because those symbols are matched separately from the + // numeric ones. Without the check, a string that is not a quantity at all would be + // reported as a quantity too large - which is the sort of confident wrong answer this + // property is being changed to stop giving. + if (double.TryParse(Value, AmountStyles, CultureInfo.InvariantCulture, out double asDouble) + && double.IsFinite(asDouble)) + { + throw new AmountOutOfRangeException(Value); + } + + throw new FormatException( + $"The amount '{Value}' is not a number in the form the XRP Ledger uses."); + } + + set + { + if (CurrencyCode == "XRP") + { + Value = value.ToString("G0", CultureInfo.InvariantCulture); + return; + } + + // G16 keeps the sixteen significant digits the ledger allows, rounding to nearest - + // which is what rippled does too, so this is not a place to be clever. The one input + // it cannot serve is the top of decimal's own range: there, rounding to nearest rounds + // *up*, past what decimal holds, and the SDK would write a string it could not read + // back. Only then is the sixteenth digit truncated instead. + string formatted = value.ToString("G16", CultureInfo.InvariantCulture); + + if (!decimal.TryParse(formatted, AmountStyles, CultureInfo.InvariantCulture, out _)) + { + formatted = TruncateToLedgerPrecision(value).ToString("G16", CultureInfo.InvariantCulture); + } + + Value = formatted; } - set => Value = value.ToString( - CurrencyCode == "XRP" - ? "G0" - : "G16", - CultureInfo.InvariantCulture); } + /// + /// The same number with its digits beyond the ledger's sixteen dropped rather than rounded. + /// + /// + /// Reached only when rounding would carry the value past . + /// Truncating cannot: dropping digits only ever moves a number toward zero. + /// + private static decimal TruncateToLedgerPrecision(decimal value) + { + if (value == 0m) + { + return 0m; + } + + int magnitude = (int)Math.Floor(Math.Log10((double)Math.Abs(value))); + int digitsToDrop = magnitude - (LedgerSignificantDigits - 1); + + if (digitsToDrop <= 0) + { + return value; + } + + decimal scale = 1m; + for (int i = 0; i < digitsToDrop; i++) + { + scale *= 10m; + } + + return Math.Truncate(value / scale) * scale; + } + + /// + /// How many significant digits an issued-currency amount carries on the ledger. + /// + /// rippled's STAmount normalises the mantissa into [1e15, 1e16). + private const int LedgerSignificantDigits = 16; + /// /// XRP token amount (non drops value) /// @@ -168,9 +241,28 @@ public decimal? ValueAsXrp #region Overrides of Object + /// + /// A readable form of the amount. + /// + /// + /// Falls back to the raw for an amount outside what + /// can hold, rather than letting throw through it. By convention + /// does not throw, and the places it is called from - logging, + /// string interpolation, a debugger's watch window - are exactly where someone would be while + /// working out why an amount is unusual. Failing there hides the value instead of showing it. + /// public override string ToString() { - return CurrencyValidName == "XRP" ? $"XRP: {ValueAsXrp:0.######}" : $"{CurrencyValidName}: {ValueAsNumber:0.###############}"; + try + { + return CurrencyValidName == "XRP" + ? $"XRP: {ValueAsXrp:0.######}" + : $"{CurrencyValidName}: {ValueAsNumber:0.###############}"; + } + catch (Exception exception) when (exception is AmountOutOfRangeException or FormatException) + { + return $"{CurrencyValidName}: {Value}"; + } } public override bool Equals(object o) { return o is Currency model && model.Issuer == Issuer && model.CurrencyCode == CurrencyCode; } diff --git a/Xrpl/Models/Transactions/BookOffers.cs b/Xrpl/Models/Transactions/BookOffers.cs index f05645da..4add340e 100644 --- a/Xrpl/Models/Transactions/BookOffers.cs +++ b/Xrpl/Models/Transactions/BookOffers.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using Xrpl.Client.Json.Converters; +using Xrpl.Client.Exceptions; using Xrpl.Models.Common; using Xrpl.Models.Enums; using Xrpl.Models.Methods; @@ -98,16 +99,27 @@ public Offer() /// /// The exchange rate, as the ratio taker_gets divided by taker_pays. /// + /// + /// Anyone may place an offer in their own token at any value the protocol allows, so both + /// sides of this ratio are untrusted input. An amount beyond what + /// holds throws rather than yielding a rate that + /// looks usable and is not - see issue #148. Guard this when walking an order book that is + /// not your own. + /// + /// Either side exceeds what can hold. public decimal AmountEach { get { - if ((TakerPays.ValueAsXrp ?? TakerPays.ValueAsNumber) != 0) - { - return (TakerGets.ValueAsXrp ?? TakerGets.ValueAsNumber) / - (TakerPays.ValueAsXrp ?? TakerPays.ValueAsNumber); - } - return 0; + // Both sides are read before the denominator is tested. Reading them lazily made + // whether this threw depend on an unrelated field: an out-of-range TakerGets went + // unnoticed whenever TakerPays happened to be zero, so the exception documented + // above was not one a caller could rely on. It also parsed TakerPays twice, and + // ValueAsNumber parses the string on every read. + decimal takerPays = TakerPays.ValueAsXrp ?? TakerPays.ValueAsNumber; + decimal takerGets = TakerGets.ValueAsXrp ?? TakerGets.ValueAsNumber; + + return takerPays != 0 ? takerGets / takerPays : 0; } } /// diff --git a/Xrpl/Utils/GetBalanceChanges.cs b/Xrpl/Utils/GetBalanceChanges.cs index bd91c94b..ebac24ea 100644 --- a/Xrpl/Utils/GetBalanceChanges.cs +++ b/Xrpl/Utils/GetBalanceChanges.cs @@ -2,6 +2,7 @@ using System.Linq; using Xrpl.Models; +using Xrpl.Client.Exceptions; using Xrpl.Models.Common; using Xrpl.Models.Ledger; using Xrpl.Models.Transactions; @@ -153,8 +154,24 @@ private static Dictionary> GroupByAccount(IEnumerable /// Computes balance changes per account from transaction metadata. /// + /// + /// + /// This walks every affected node and computes a delta for every account on the payment path, + /// not only the one the caller has in mind. The amounts it reads are therefore untrusted: it + /// is enough for a payment to route through an offer in somebody's own token for a value + /// beyond to reach this code, and then + /// comes out - see issue #148. + /// + /// + /// That matters most to anything that re-reads history. A monitor catching up over a ledger + /// range, an indexer or a reconciler meets the same transaction on every pass, so an unguarded + /// call does not fail once, it stops there permanently. Catch it and decide what an + /// unrepresentable balance means for you; issue #150 tracks representing it instead. + /// + /// /// Transaction metadata including affected nodes. /// Dictionary mapping account addresses to balance changes (XRP string or IssuedCurrencyAmount). + /// An amount in the metadata exceeds what can hold. public static Dictionary> GetBalanceChanges(ITransactionMetadata metadata) { var list = new List(); diff --git a/Xrpl/Xrpl.csproj b/Xrpl/Xrpl.csproj index cf08d66b..6ac2e1d8 100644 --- a/Xrpl/Xrpl.csproj +++ b/Xrpl/Xrpl.csproj @@ -14,7 +14,7 @@ Apache-2.0 https://github.com/StaticBit-io/XrplCSharp XrplCSharp - 11.0.0.0 + 11.1.0.0