Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<PackageLicenseExpression>Apache-2.0</PackageLicenseExpression>
<PackageProjectUrl>https://github.com/StaticBit-io/XrplCSharp</PackageProjectUrl>
<Title>XrplCSharp</Title>
<PackageVersion>11.0.0.0</PackageVersion>
<PackageVersion>11.0.1.0</PackageVersion>
</PropertyGroup>

<PropertyGroup>
Expand Down
31 changes: 28 additions & 3 deletions Base/Xrpl.BinaryCodec/XrplBinaryCodec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,33 @@ public class XrplBinaryCodec
{
static uint PAYMENT_CHANNEL_CLAIM_PREFIX = 0x434C4D00u;

/// <summary>
/// The two option sets <see cref="ObjectToJsonNode"/> needs, built once.
/// </summary>
/// <remarks>
/// These were constructed per call, on the path every signing operation takes -
/// <see cref="Encode(object)"/>, <see cref="EncodeForSigning"/>, <see cref="EncodeForSigningClaim"/>
/// and <see cref="EncodeForMultiSigning"/> all route through it.
/// </remarks>
/// <remarks>
/// 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.
/// </remarks>
/// <remarks>
/// Measured end to end on <see cref="EncodeForSigning"/>, 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.
/// </remarks>
private static readonly JsonSerializerOptions IgnoreNullOptions = new JsonSerializerOptions
{
DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull
};

private static readonly JsonSerializerOptions KeepNullOptions = new JsonSerializerOptions();

/// <summary>
/// Decode a hex string into a JsonNode representing the transaction/object.
/// </summary>
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 21 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 2 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
216 changes: 216 additions & 0 deletions Tests/Xrpl.Tests/Client/TestUMockRippledServer.cs
Original file line number Diff line number Diff line change
@@ -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;

/// <summary>
/// The mock rippled server's own invariants - the ones whose absence took the whole test host
/// down rather than failing a test.
/// </summary>
/// <remarks>
/// <para>
/// A run on PR #145 aborted with <c>Server error: OnClientDisconnected is not bound!</c> and
/// <c>Test host process crashed</c>. The chain: <c>MockClient.messageCallback</c> is a socket
/// callback, so it runs on a thread-pool thread; when the socket faults it enters its own
/// <c>catch</c>, and from inside that catch it calls <c>ClientDisconnect</c>, 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
[TestClass]
public class TestUMockRippledServer
{
private static IPEndPoint AnyLoopbackPort() => new IPEndPoint(IPAddress.Loopback, 0);

/// <summary>
/// Raising an event nobody subscribed to is not an error.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
[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();
}
}

/// <summary>
/// The client list survives being added to, removed from and read at once.
/// </summary>
/// <remarks>
/// <para>
/// <c>_clients</c> is added to from the accept callback and removed from on disconnect - both
/// thread-pool threads - while the test thread reads it through <c>GetConnectedClient</c>.
/// Unsynchronised, a mutation during an enumeration throws
/// <see cref="InvalidOperationException"/> on a thread with no catch above it: the same fatal
/// shape as the crash above, by a different route.
/// </para>
/// <para>
/// The clients here are real, and that is the point. An earlier version of this test spun
/// <c>ClientDisconnect(null)</c> against readers, which pins nothing: the list stays empty, and
/// <c>List&lt;T&gt;.Remove</c> of an absent element returns without touching the version counter
/// the enumerator checks. That test passed with <c>_clientsLock</c> removed outright. This one
/// does not.
/// </para>
/// </remarks>
[TestMethod]
public async Task TestUTheClientListToleratesConcurrentUse()
{
Server server = new Server(AnyLoopbackPort());
List<Socket> sockets = new List<Socket>();

try
{
MockClient[] clients = new MockClient[8];
for (int i = 0; i < clients.Length; i++)
{
clients[i] = new MockClient(server, ConnectedSocket(sockets));
}

List<Task> workers = new List<Task>();

// 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();
}
}

/// <summary>
/// A connected loopback socket, so a <see cref="MockClient"/> can be built without a handshake.
/// </summary>
private static Socket ConnectedSocket(List<Socket> 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;
}

/// <summary>
/// A server that has not been told to listen is not listening.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
[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();
}
}

/// <summary>
/// Stopping a server that never listened is quiet, and stopping twice is too.
/// </summary>
/// <remarks>
/// <c>CreateMockRippled.Start()</c> races its own <c>Stop()</c>: 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.
/// </remarks>
[TestMethod]
public void TestUStoppingAServerThatNeverListenedIsQuiet()
{
Server server = new Server(AnyLoopbackPort());

server.Stop();
server.Stop();

Assert.ThrowsExactly<ObjectDisposedException>(
() => server.StartListening(),
"Listening on a socket that Stop() disposed should say so plainly, not carry on half-alive.");
}
}
32 changes: 21 additions & 11 deletions Tests/Xrpl.Tests/CreateMockRippled.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down Expand Up @@ -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();
}
}
}
}
Loading
Loading