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/Base/Xrpl.BinaryCodec/XrplBinaryCodec.cs b/Base/Xrpl.BinaryCodec/XrplBinaryCodec.cs index e7e9253f..aa7abd4b 100644 --- a/Base/Xrpl.BinaryCodec/XrplBinaryCodec.cs +++ b/Base/Xrpl.BinaryCodec/XrplBinaryCodec.cs @@ -20,6 +20,33 @@ public class XrplBinaryCodec { static uint PAYMENT_CHANNEL_CLAIM_PREFIX = 0x434C4D00u; + /// + /// The two option sets needs, built once. + /// + /// + /// These were constructed per call, on the path every signing operation takes - + /// , , + /// and all route through it. + /// + /// + /// The cost was smaller than the usual telling of this bug suggests: since .NET 7 + /// System.Text.Json shares a caching context between structurally equal options instances, + /// so type metadata was not rebuilt per call - had it been, the gap below would be orders of + /// magnitude rather than 1.7x. What was paid is an allocation and a structural-equality + /// lookup in that shared pool, which is capped at 64 contexts and no longer leaned on here. + /// + /// + /// Measured end to end on , 50 000 calls, best of five rounds: + /// 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. + /// + private static readonly JsonSerializerOptions IgnoreNullOptions = new JsonSerializerOptions + { + DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull + }; + + private static readonly JsonSerializerOptions KeepNullOptions = new JsonSerializerOptions(); + /// /// Decode a hex string into a JsonNode representing the transaction/object. /// @@ -101,9 +128,7 @@ private static JsonNode ObjectToJsonNode(object obj, bool ignoreNull = false) { if (obj is JsonNode node) return node; - JsonSerializerOptions options = ignoreNull - ? new JsonSerializerOptions { DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull } - : new JsonSerializerOptions(); + JsonSerializerOptions options = ignoreNull ? IgnoreNullOptions : KeepNullOptions; string jsonString = JsonSerializer.Serialize(obj, options); return JsonNode.Parse(jsonString); diff --git a/CHANGES.md b/CHANGES.md index c5cf7a5a..7f93b79a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,26 @@ # Changes +## 11.1.0.0 08/27/2026 + +* **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/CLAUDE.md b/CLAUDE.md index c14ddf33..d3b0b39c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -230,6 +230,8 @@ Reference: [xrpl-codec-gen](https://github.com/RichardAH/xrpl-codec-gen) ## Development Notes +- Documentation language: English. Prose, headings, and examples in English; do not mix languages +- Commit messages are English too — subject and body. This repository is public, and its history is part of what a reader sees; 36 of the 562 subjects on `dev` are Russian, and no more are to be added - No `Directory.Build.props` or `global.json` — versions are managed per `.csproj` - No centralized package management — each project specifies its own NuGet versions - `test.runsettings` configures MSTest parallel execution at class level diff --git a/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs new file mode 100644 index 00000000..5d7fba23 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs @@ -0,0 +1,216 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Sockets; +using System.Threading.Tasks; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Xrpl.Tests.MockRippled; + +namespace XrplTests.Xrpl.ClientLib; + +/// +/// The mock rippled server's own invariants - the ones whose absence took the whole test host +/// down rather than failing a test. +/// +/// +/// +/// A run on PR #145 aborted with Server error: OnClientDisconnected is not bound! and +/// Test host process crashed. The chain: MockClient.messageCallback is a socket +/// callback, so it runs on a thread-pool thread; when the socket faults it enters its own +/// catch, and from inside that catch it calls ClientDisconnect, which threw when +/// nothing was subscribed. An exception raised inside a catch block on a pool thread has nowhere +/// left to go, and .NET ends the process. +/// +/// +/// Worth testing rather than just fixing, because of how the failure presents: the run is +/// aborted, so the tests that had not been reached yet never run, and CI reports one failed job +/// rather than a few hundred unexecuted tests. It is a failure that hides its own size, and the +/// only reason it was not worse is that the process exits non-zero. +/// +/// +[TestClass] +public class TestUMockRippledServer +{ + private static IPEndPoint AnyLoopbackPort() => new IPEndPoint(IPAddress.Loopback, 0); + + /// + /// Raising an event nobody subscribed to is not an error. + /// + /// + /// All four events used to throw when unbound. For an event, no subscriber is a legitimate + /// state - and these fire from socket callbacks, where the difference between throwing and + /// not is the difference between a failed test and no test results at all. + /// + [TestMethod] + public void TestUAnUnsubscribedEventIsNotAnError() + { + Server server = new Server(AnyLoopbackPort()); + + try + { + // The one that actually crashed the host, called exactly as the catch block calls it. + server.ClientDisconnect(null); + + // And the others, which sit on the same kind of thread. + server.ReceiveMessage(null, "{}"); + } + finally + { + server.Stop(); + } + } + + /// + /// The client list survives being added to, removed from and read at once. + /// + /// + /// + /// _clients is added to from the accept callback and removed from on disconnect - both + /// thread-pool threads - while the test thread reads it through GetConnectedClient. + /// Unsynchronised, a mutation during an enumeration throws + /// on a thread with no catch above it: the same fatal + /// shape as the crash above, by a different route. + /// + /// + /// The clients here are real, and that is the point. An earlier version of this test spun + /// ClientDisconnect(null) against readers, which pins nothing: the list stays empty, and + /// List<T>.Remove of an absent element returns without touching the version counter + /// the enumerator checks. That test passed with _clientsLock removed outright. This one + /// does not. + /// + /// + [TestMethod] + public async Task TestUTheClientListToleratesConcurrentUse() + { + Server server = new Server(AnyLoopbackPort()); + List sockets = new List(); + + try + { + MockClient[] clients = new MockClient[8]; + for (int i = 0; i < clients.Length; i++) + { + clients[i] = new MockClient(server, ConnectedSocket(sockets)); + } + + List workers = new List(); + + // Two threads churn the list while two more walk it end to end. + for (int worker = 0; worker < 2; worker++) + { + int offset = worker * 4; + + workers.Add(Task.Run(() => + { + for (int n = 0; n < 20_000; n++) + { + MockClient client = clients[offset + (n % 4)]; + server.TrackClient(client); + server.ClientDisconnect(client); + } + })); + + workers.Add(Task.Run(() => + { + for (int n = 0; n < 20_000; n++) + { + // Enumerates to the end, because no client carries this guid. + server.GetConnectedClient("no-such-guid"); + server.GetConnectedClientCount(); + } + })); + } + + await Task.WhenAll(workers); + } + finally + { + foreach (Socket socket in sockets) + { + try { socket.Close(); } catch { } + } + + server.Stop(); + } + } + + /// + /// A connected loopback socket, so a can be built without a handshake. + /// + private static Socket ConnectedSocket(List toClose) + { + Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + listener.Bind(new IPEndPoint(IPAddress.Loopback, 0)); + listener.Listen(1); + + Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + client.Connect((IPEndPoint)listener.LocalEndPoint); + + Socket accepted = listener.Accept(); + listener.Close(); + + toClose.Add(client); + toClose.Add(accepted); + return accepted; + } + + /// + /// A server that has not been told to listen is not listening. + /// + /// + /// The constructor used to bind and accept on its own, so a caller could not subscribe before + /// clients arrived. Nothing here asserts about sockets: the point is only that constructing + /// is now separable from accepting, which is what lets handlers be bound first. + /// + [TestMethod] + public void TestUConstructingAServerDoesNotStartAccepting() + { + Server server = new Server(AnyLoopbackPort()); + + try + { + Assert.AreEqual( + 0, + server.GetConnectedClientCount(), + "A server that was never told to listen cannot have accepted anyone."); + + Assert.IsFalse( + server.GetSocket().IsBound, + "The constructor must not bind - that is the whole point of the split."); + + // Binding happens here, not in the constructor - and doing it explicitly must work. + server.StartListening(); + + Assert.IsTrue( + server.GetSocket().IsBound, + "StartListening must actually bind; asserting the socket is merely non-null would pass even if it did nothing."); + } + finally + { + server.Stop(); + } + } + + /// + /// Stopping a server that never listened is quiet, and stopping twice is too. + /// + /// + /// CreateMockRippled.Start() races its own Stop(): a mock stopped before startup + /// finishes has its server closed without ever having accepted. That path has to be silent, or + /// the teardown of a fast test becomes a failure of its own. + /// + [TestMethod] + public void TestUStoppingAServerThatNeverListenedIsQuiet() + { + Server server = new Server(AnyLoopbackPort()); + + server.Stop(); + server.Stop(); + + Assert.ThrowsExactly( + () => server.StartListening(), + "Listening on a socket that Stop() disposed should say so plainly, not carry on half-alive."); + } +} diff --git a/Tests/Xrpl.Tests/CreateMockRippled.cs b/Tests/Xrpl.Tests/CreateMockRippled.cs index 3b7653cb..d6b4c3a0 100644 --- a/Tests/Xrpl.Tests/CreateMockRippled.cs +++ b/Tests/Xrpl.Tests/CreateMockRippled.cs @@ -277,17 +277,10 @@ public void Start() Server server = new Server(new IPEndPoint(IPAddress.Parse("127.0.0.1"), this._port)); - lock (_serverLock) - { - if (_stopped) - { - // Stop() already ran - do not leave this listener accepting behind the test's back. - StopServer(server); - return; - } - - _server = server; - } + // Handlers first, listening afterwards. The server used to start accepting from its + // own constructor, which left a window where a client could connect - and disconnect, + // or send a request - before anything was subscribed. That window is what crashed the + // test host, and it is closed here rather than only survived. // Bind the event for when a client connected server.OnClientConnected += (object sender, OnClientConnectedHandler e) => @@ -392,6 +385,23 @@ public void Start() //e.GetClient().GetServer().ClientDisconnect(e.GetClient()); string clientGuid = e.GetClient().GetGuid(); }; + + lock (_serverLock) + { + if (_stopped) + { + // Stop() already ran - do not leave this listener accepting behind the test's back. + StopServer(server); + return; + } + + _server = server; + + // Inside the lock, so Stop() cannot slip between publishing the server and it + // beginning to accept: whichever takes the lock first wins outright, and a Stop() + // that follows closes a socket that is genuinely listening. + server.StartListening(); + } } } } \ No newline at end of file diff --git a/Tests/Xrpl.Tests/MockRippled/Server.cs b/Tests/Xrpl.Tests/MockRippled/Server.cs index 9af307f5..3a0a5b17 100644 --- a/Tests/Xrpl.Tests/MockRippled/Server.cs +++ b/Tests/Xrpl.Tests/MockRippled/Server.cs @@ -142,7 +142,16 @@ public partial class Server private IPEndPoint _endPoint; /// The connected clients to the server - private List _clients = new List(); + /// + /// Mutated from socket callbacks, which run on thread-pool threads, and read from the + /// test thread - so every touch goes through . Without it a + /// client connecting while another disconnects can corrupt the list, and enumerating it + /// during either throws on a thread where nothing + /// is left to catch it. + /// + private readonly List _clients = new List(); + + private readonly object _clientsLock = new object(); #endregion @@ -161,9 +170,6 @@ public Server(IPEndPoint EndPoint) //Console.WriteLine("Copyright © 2017 - MazyModz. Created by Dennis Andersson. All rights reserved.\n\n"); //Console.WriteLine("WebSocket Server Started\nListening on {0}:{1}\n", GetEndPoint().Address.ToString(), GetEndPoint().Port); - - // Start the server - start(); } #endregion @@ -189,8 +195,11 @@ public IPEndPoint GetEndPoint() /// The connected client at the index, returns null if the index is out of bounds public MockClient GetConnectedClient(int Index) { - if (Index < 0 || Index >= _clients.Count) return null; - return _clients[Index]; + lock (_clientsLock) + { + if (Index < 0 || Index >= _clients.Count) return null; + return _clients[Index]; + } } /// Gets a connected client with the given guid @@ -198,9 +207,12 @@ public MockClient GetConnectedClient(int Index) /// The client with the given id, return null if no client with the guid could be found public MockClient GetConnectedClient(string Guid) { - foreach (MockClient client in _clients) + lock (_clientsLock) { - if (client.GetGuid() == Guid) return client; + foreach (MockClient client in _clients) + { + if (client.GetGuid() == Guid) return client; + } } return null; } @@ -210,9 +222,12 @@ public MockClient GetConnectedClient(string Guid) /// The connected client with the given socket, returns null if no client with the socket was found public MockClient GetConnectedClient(Socket Socket) { - foreach (MockClient client in _clients) + lock (_clientsLock) { - if (client.GetSocket() == Socket) return client; + foreach (MockClient client in _clients) + { + if (client.GetSocket() == Socket) return client; + } } return null; } @@ -221,7 +236,10 @@ public MockClient GetConnectedClient(Socket Socket) /// The number of connected clients public int GetConnectedClientCount() { - return _clients.Count; + lock (_clientsLock) + { + return _clients.Count; + } } #endregion @@ -229,9 +247,16 @@ public int GetConnectedClientCount() #region Methods /// - /// Starts the listen server when a server object is created + /// Binds the listen socket and starts accepting connections. /// - private void start() + /// + /// Separate from the constructor on purpose. While this ran from there, the socket was + /// accepting before the caller had bound a single handler, so a client that connected in + /// that window raised its events into nothing: the connect was invisible, and a request + /// arriving before OnMessageReceived was bound went unanswered, which a test sees as a + /// timeout rather than as a race. Bind first, then call this. + /// + public void StartListening() { // Bind the socket and start listending GetSocket().Bind(GetEndPoint()); @@ -263,7 +288,14 @@ private void connectionCallback(IAsyncResult AsyncResult) // Gets the client thats trying to connect to the server clientSocket = GetSocket().EndAccept(AsyncResult); - // Read the handshake updgrade request + // Read the handshake updgrade request. + // Bounded on purpose: BeginAccept can complete synchronously when a connection is + // already pending, in which case this callback - and this blocking read - runs on + // the thread that called StartListening, which holds _serverLock. A client that + // connects and then says nothing would hold up Stop() for as long as it stayed + // quiet. Five seconds is far longer than a real handshake and far shorter than + // forever. + clientSocket.ReceiveTimeout = 5000; byte[] handshakeBuffer = new byte[1024]; int handshakeReceived = clientSocket.Receive(handshakeBuffer); @@ -278,11 +310,12 @@ private void connectionCallback(IAsyncResult AsyncResult) // it to the list of connected clients MockClient client = new MockClient(this, clientSocket); clientSocket = null; - _clients.Add(client); + TrackClient(client); - // Call the event when a client has connected to the listen server - if (OnClientConnected == null) throw new Exception("Server error: event OnClientConnected is not bound!"); - OnClientConnected(this, new OnClientConnectedHandler(client)); + // No subscriber is a legitimate state for an event, not a server fault. These + // fire from socket callbacks on thread-pool threads, where a throw is not a + // failed assertion but a dead process - see ClientDisconnect below. + OnClientConnected?.Invoke(this, new OnClientConnectedHandler(client)); } catch (ObjectDisposedException) { @@ -328,8 +361,23 @@ private void connectionCallback(IAsyncResult AsyncResult) /// The message that the client sent public void ReceiveMessage(MockClient Client, string Message) { - if (OnMessageReceived == null) throw new Exception("Server error: event OnMessageReceived is not bound!"); - OnMessageReceived(this, new OnMessageReceivedHandler(Client, Message)); + OnMessageReceived?.Invoke(this, new OnMessageReceivedHandler(Client, Message)); + } + + /// Records a connected client. Paired with . + /// + /// Extracted from the accept callback so a test can drive the add side of the list without a + /// socket handshake. Without it the only reachable mutation is removing a client that is not + /// there, and List<T>.Remove leaves its version counter alone in that case - so a + /// concurrency test built on it exercises no mutual exclusion at all and passes with the lock + /// removed entirely. + /// + internal void TrackClient(MockClient Client) + { + lock (_clientsLock) + { + _clients.Add(Client); + } } /// Called when a client disconnectes, calls event OnClientDisconnected @@ -337,11 +385,17 @@ public void ReceiveMessage(MockClient Client, string Message) public void ClientDisconnect(MockClient Client) { // Remove the client from the connected clients list - _clients.Remove(Client); + lock (_clientsLock) + { + _clients.Remove(Client); + } - // Call the OnClientDisconnected event - if (OnClientDisconnected == null) throw new Exception("Server error: OnClientDisconnected is not bound!"); - OnClientDisconnected(this, new OnClientDisconnectedHandler(Client)); + // This used to throw when nothing was subscribed, and it is the reason the whole test + // host died: MockClient.messageCallback calls this from inside its own catch block, on + // a thread-pool thread, so the exception escaped a catch and had nowhere left to go. + // A run aborted that way reports one failed job while an unknown number of tests never + // ran at all - a failure that hides its own size. + OnClientDisconnected?.Invoke(this, new OnClientDisconnectedHandler(Client)); } #endregion @@ -360,8 +414,7 @@ public void SendMessage(MockClient Client, string Data) Client.GetSocket().Send(frameMessage); // Call the on send message callback event - if (OnSendMessage == null) throw new Exception("Server error: event OnSendMessage is not bound!"); - OnSendMessage(this, new OnSendMessageHandler(Client, Data)); + OnSendMessage?.Invoke(this, new OnSendMessageHandler(Client, Data)); } /// Called after a message was sent 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/Client/Json/JsonSerializerOptionsCache.cs b/Xrpl/Client/Json/JsonSerializerOptionsCache.cs index 3128ce50..9d2518e8 100644 --- a/Xrpl/Client/Json/JsonSerializerOptionsCache.cs +++ b/Xrpl/Client/Json/JsonSerializerOptionsCache.cs @@ -13,7 +13,7 @@ namespace Xrpl.Client.Json /// /// Building those options inside Read/Write cost an allocation, a copy of the whole converter list and a /// structural-equality lookup in System.Text.Json's caching-context pool — once per converted value, so - /// once per element of a collection. Type metadata itself was not rebuilt: since .NET 8 System.Text.Json + /// once per element of a collection. Type metadata itself was not rebuilt: since .NET 7 System.Text.Json /// shares a caching context between structurally equal options instances, which is what kept the per-call /// copy from being far worse than it was. That pool is capped (64 contexts); caching here removes the /// dependency on it as well.
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/Ledger/LOVault.cs b/Xrpl/Models/Ledger/LOVault.cs index 32d67eb3..3742df2d 100644 --- a/Xrpl/Models/Ledger/LOVault.cs +++ b/Xrpl/Models/Ledger/LOVault.cs @@ -73,12 +73,21 @@ public class VaultDataFormat [JsonPropertyName("w")] public string Website { get; set; } + /// + /// Built once rather than per call. Colder than the codec's signing path, but the same + /// mistake, and an options instance is not the place to express "compact" one call at a time. + /// + private static readonly JsonSerializerOptions CompactOptions = new JsonSerializerOptions + { + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull + }; + /// /// Serializes to compact JSON, then hex-encodes for the Data field. /// public string ToHex() { - string json = JsonSerializer.Serialize(this, new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull }); + string json = JsonSerializer.Serialize(this, CompactOptions); return Convert.ToHexString(Encoding.UTF8.GetBytes(json)); } 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