Release 11.1.0.0 - #153
Merged
Merged
Conversation
* test(mock): the mock server stops taking the test host down with it A run on #145 died with "Server error: OnClientDisconnected is not bound!" and "Test host process crashed". The chain, in order: Server's constructor called start(), so the socket accepted connections before the caller had bound a single handler. CreateMockRippled.Start() binds after constructing, OnClientDisconnected last of the four, leaving a window where the server was live and had no subscribers. MockClient.messageCallback is a socket callback, so it runs on a thread-pool thread. When the socket faults it enters its own catch, and that catch calls ClientDisconnect - which threw when nothing was subscribed. An exception raised inside a catch block on a pool thread has nowhere left to go, so the runtime ends the process. That last part is what makes this worth more than a rerun. An aborted run does not report the tests it never reached: CI shows one failed job where in truth an unknown number of tests did not execute. The failure hides its own size, and the only thing that saved it from being silent is the non-zero exit code. Three changes, each closing one link: Events no longer throw when unsubscribed - all four now use ?.Invoke. For an event, having no subscriber is a legitimate state, not a server fault, and these fire from threads where a throw is not a failed assertion. Handlers are bound before the socket accepts. The constructor no longer listens; StartListening() is explicit, and CreateMockRippled calls it inside the same lock that guards Stop(), so nothing slips between publishing the server and it beginning to accept. This closes the window rather than merely surviving it: a request arriving before OnMessageReceived was bound used to go unanswered, which a test sees as a timeout rather than as a race. _clients is guarded. It was mutated from accept and disconnect callbacks - both pool threads - and read from the test thread with no synchronisation. An add during an enumeration throws InvalidOperationException on a thread with no catch above it: the same fatal shape by a different route. Four tests pin the invariants. Restoring the throw in ClientDisconnect fails two of them. Six consecutive local runs of the unit suite are clean, though that is weak evidence about a rare race - the argument is the mechanism, not the sample. No CHANGES.md entry: this is test infrastructure with no consumer-visible effect, the same call made for the test-only #139. * test(mock): assert the socket is bound, not merely present Review catch. Asserting GetSocket() is non-null passes whether or not StartListening() does anything, which makes it no test of the split it was written to pin. IsBound is the actual invariant: false after construction, true after listening. * test(mock): make the concurrency test actually pin the lock it names Review findings. TestUTheClientListToleratesConcurrentUse pinned nothing. It churned ClientDisconnect(null) against readers, but the list then stays empty, and List<T>.Remove of an absent element returns without touching the version counter an enumerator checks - so no mutation ever raced an enumeration. The test passed with _clientsLock removed outright; verified by replacing it with a fresh object per access, which disables mutual exclusion entirely. It now drives real clients over loopback sockets: two threads add and remove while two more walk the list end to end. Same mutation now fails it, which is the whole point of writing it down. TrackClient() is extracted from the accept callback so the add side is reachable without a socket handshake. It is the same code, under the same lock. The handshake read in connectionCallback is now bounded at five seconds. BeginAccept can complete synchronously when a connection is already pending, in which case that blocking read runs on the thread that called StartListening - which holds _serverLock - so a client that connects and then says nothing would hold up Stop() indefinitely. Narrow, but this change exists to remove a hang. And TestUStoppingAServerThatNeverListenedIsQuiet said "and stopping twice is too" while stopping once. It stops twice now.
* perf(codec): build the signing path's JsonSerializerOptions once
ObjectToJsonNode constructed a fresh JsonSerializerOptions on every call, and
every signing operation goes through it - Encode, EncodeForSigning,
EncodeForSigningClaim and EncodeForMultiSigning all route there. LOVault.ToHex
did the same, on a much colder path.
Measured end to end on EncodeForSigning, 50 000 calls, best of five rounds:
before 1075.8 ms 14458 B/op
after 621.8 ms 13601 B/op
1.73x, and 857 fewer bytes per call. The output is unchanged: the encoded blob
hashes to E9DE857C47B2CDDD0845344D0A9C4892C91A1A41051FD7A692EFFA6C4A46B47D
before and after, and the 28 existing assertions against known-good hex across
six BinaryCodec test files still pass.
Worth stating what this is not, because the usual telling of this bug oversells
it. Since .NET 8 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
was an allocation and a structural-equality lookup in that shared pool, which
is capped at 64 contexts and no longer leaned on here.
The instances are only ever read, and System.Text.Json freezes an options
object on first use, so sharing them across threads is safe.
* docs(codec): fix an ambiguous cref and cite the measurement that fits
Self-review of the previous commit.
<see cref="Encode"/> matched two overloads, which the compiler reported as
CS0419 - a warning this change introduced. Pinned to Encode(object), the one
ObjectToJsonNode is actually reached from.
The remarks quoted the isolated benchmark - 1.61x on a serialize call in a
scratch project - where the number that characterises this code is the end-to-
end one on EncodeForSigning: 1.73x and 857 bytes. A comment outlives the pull
request it was written alongside, so it should carry the figure that describes
the path it sits on.
Also split a sentence that ran "since ... so ..." into two, and recorded why
the modest size of the gap is itself the evidence that metadata was not being
rebuilt.
* docs(json): the shared caching context dates from .NET 7, not 8
Review finding, checked against the source rather than taken on trust:
JsonSerializerOptions.Caching.cs on release/7.0 already carries
TrackedCachingContexts with MaxTrackedContexts = 64 and an EqualityComparer
over structural equality. The later PR rewrote that mechanism, it did not
introduce it.
Nothing about the argument changes - metadata still was not being rebuilt per
call, and the modest size of the measured gap remains the evidence for that.
Only the version is wrong, and it is wrong in a comment, which is where a wrong
fact does the most quiet damage.
Corrected in the remarks added by this branch and in JsonSerializerOptionsCache,
which has carried the same claim since it was written. Same sentence, same
error, no reason to leave one of them standing.
Documentation was already English throughout - CLAUDE.md, README.md, CHANGES.md, CONTRIBUTING.md and every XML doc comment carry no Cyrillic at all - but nothing said so, which leaves it to be inferred from the surroundings. A convention that is only ever inferred erodes at the first entry written in a hurry. Commit history is the case in point: 36 of the 562 subjects on dev are Russian. This repository is public and publishes to nuget.org, so its history is part of what a reader sees. Nothing is being rewritten - the existing subjects stay - but no more are to be added. Both lines go first in Development Notes: they govern everything else written there rather than sitting alongside it.
* fix(models): refuse an amount decimal cannot hold instead of guessing at it Currency.ValueAsNumber answered an out-of-range amount 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. No parsing changes that. The only thing available is how to fail. The clamp is gone. Above the range this now throws AmountOutOfRangeException, carrying the value as the node sent it. Answering 1e96 with 7.9e28 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 the arithmetic instead - one silent lie turning into a second, unrelated exception a caller could not diagnose. The negative case was a plain bug. The fallback's NumberStyles expression came to AllowExponent | AllowDecimalPoint, missing AllowLeadingSign, so no negative value could reach the branch written to handle it. The primary parse was correct throughout, despite six & terms that all evaluate to zero. An amount below 1e-28 still returns zero, and that 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. Offer.AmountEach reads the same property on both sides of an order and divides them, on values anyone may place in the book. It used to return a plausible-looking exchange rate that was wrong by 67 orders of magnitude without throwing at all. It and GetBalanceChanges now document what they do on untrusted amounts rather than leaving it to be found. Console.WriteLine(exception) is out of the parse path. Six tests, including that a negative amount inside the range still parses - without it the same tests would pass on an implementation that refused every negative value, and negative balances are ordinary. Restoring the clamp fails three of them. Minor rather than patch: code that read an out-of-range amount used to get a number and now gets an exception, which is a contract change even though no signature moved. Representing the full range rather than refusing it is #150. * fix(models): a non-finite string is not an amount that is too large Review findings, both accepted. double.TryParse accepts "NaN", "Infinity" and "-Infinity" whatever NumberStyles it is handed - those symbols are matched separately from the numeric ones. Since the test that separates "will not fit" from "is not a number" runs through double, all three came back as AmountOutOfRangeException: a confident statement about magnitude for a string that has none. Checked against the runtime rather than taken on the reviewer's word, then guarded with double.IsFinite. Offer.AmountEach read its two sides lazily, so the early return for a zero TakerPays skipped TakerGets entirely - an unrepresentable numerator went unnoticed whenever the denominator happened to be zero. The exception this branch documents was therefore not one a caller could rely on: whether it appeared depended on the value of an unrelated field. Both sides are read first. It also parsed TakerPays twice, and ValueAsNumber parses on every read. The second is the one worth noting. The documentation added in the previous commit claimed something the code did not do, in a change whose whole subject is not saying false things about values. Four tests. Removing IsFinite fails one, restoring the lazy read fails another. * fix(models): ToString shows an out-of-range amount instead of failing on it Review findings. ToString interpolates ValueAsNumber, so making the getter throw made ToString throw with it - for positives, which used to print a clamped number, as well as for negatives, which already threw. By convention ToString does not throw, and the places it is reached from are logging, string interpolation and a debugger's watch window: 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. It now falls back to the raw string, which is what the node sent. Two tests were missing behind claims already made. GetBalanceChanges documents that it throws on an out-of-range amount, and nothing exercised that through GetBalanceChanges - only a hand-written subtraction imitating what it does. Imitating the arithmetic proves the arithmetic; it does not prove the method reaches it, which is what the documentation promises. Now driven through the method, on a negative balance in a RippleState node - the ordinary shape from the low account's side, and the case that used to fail as FormatException. And one edge is documented rather than fixed: writing decimal.MaxValue through the setter formats with G16, which rounds the mantissa up past what decimal holds, so the SDK can write a string the ledger would accept and then refuse to read it. The window is the last ~7e12 below decimal.MaxValue, reachable only by assigning a number no token amount would be, and changing how the setter rounds would touch every round trip in the type to rescue a value nobody writes. The test states the decision so the next person meets one rather than a surprise. Restoring the clamp now fails seven tests. * fix(models): the setter stops writing an amount it cannot read back Checked against rippled first, which changed what this should be. I had proposed replacing G16 with truncation, on the belief that rippled truncates a mantissa when normalising. It does not: Number.cpp sets RoundingMode::ToNearest as the default, which is what G16 already does. Making the SDK truncate would have moved it away from the protocol, not toward it. The rounding stays. What is left is narrow. At the top of decimal's own range, rounding to nearest rounds up past what the type holds, so the setter wrote a string it then refused to read - a valid ledger amount the SDK produced and could not consume. There, and only there, the sixteenth digit is truncated instead; truncating cannot overflow, because dropping digits only moves a number toward zero. Dust is pinned by a test. Balances like 0.000000000000000001 arrive from the network and must go back out, and they are safe because the ledger's limit is sixteen significant digits while dust carries one. The test exists because the obvious way to bound precision - truncating to sixteen decimal places rather than significant digits - turns 1e-18 into zero, and a remainder would vanish in silence. That mutation fails it. ValueAsNumber_16Digits_NeverRoundsUp asserted that a round trip must not increase a value. The protocol makes no such promise, and the test could not have caught a violation anyway: its input has exactly sixteen significant digits, so there was nothing to round. Replaced by the property that does hold, and by one stating the rounding outright so the next reader does not repeat the mistake I nearly shipped. Also written down: why the setter rounds while the codec refuses more than sixteen digits. They see different inputs. Seventeen digits cannot arrive from the network - rippled normalises the mantissa into [1e15, 1e16) before serialising - so the codec only ever meets a hand-written string, while the setter meets computed decimals that routinely carry 28. AmmMath returns them. * chore(release): bump Xrpl.BinaryCodec, and record the change that moved it Release preparation for 27/08, found by checking what actually changed since 11.0.0.0 rather than by looking at this branch alone. Xrpl.BinaryCodec/XrplBinaryCodec.cs changed in #147 and the package version did not. Promoting that way publishes nothing: dotnet nuget push runs with --skip-duplicate, so a package whose version already exists on the feed is passed over in silence, and the fix reaches no consumer while the run stays green. Moved to 11.0.1.0 - a performance fix with no contract change, so patch. The same PR left no CHANGES.md entry. A 1.73x change on the path every signing operation takes is not a silent one, so it has one now, with the measurement and with why the usual telling of that bug oversells it. Xrpl stays at 11.1.0.0: this release carries a contract change, since code that read an out-of-range amount used to get a number and now gets an exception. AddressCodec, Keypairs and both X402 packages are untouched and keep their versions - they are consumed by ProjectReference, so a package built at a newer version keeps depending on the published ones. CHANGES.md still opens with "## Unreleased". Stamping it belongs to the promotion, when the date is known.
The section going out today was still titled "Unreleased". Promoting with it that way leaves the v11.1.0 tag pointing at a changelog whose top section names neither version nor date, and by then it is too late to fix for that tag. Dated 08/27/2026, the day of the rollout, following the convention of the headings above it - those track the commit date in UTC. Checked by running the release workflow's own extraction against it rather than by eye: version 11.1.0.0 resolves from Xrpl.csproj, the tag comes out v11.1.0, and the notes extract to 5586 characters over 20 lines with no stray heading caught in them. Both entries are present - the JsonSerializerOptions change and the out-of-range amounts. Nothing else changes. CHANGES.md is not packed into any NuGet package, so this affects the repository and the tag rather than what gets published.
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe change adds explicit handling for XRPL amounts outside the ChangesXRPL amount handling
Mock server lifecycle and concurrency
Serialization and package metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The proposed release is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 12 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotion of
devintoreleasefor 11.1.0.0. Merging this publishes to nuget.org, which cannot be undone.Versions
releaseXrplXrpl.BinaryCodecXrpl.AddressCodecXrpl.KeypairsXrpl.X402,Xrpl.X402.AspNetCoregit diff --name-only origin/release...origin/devover the package directories returns files in exactly those two packages and nothing else, so the versions that moved are the versions that had to.The
Xrpl.BinaryCodecbump is the one worth pausing on. Its source changed in #147 while its version did not, anddotnet nuget pushruns with--skip-duplicate: a package whose version already exists on the feed is passed over in silence. Promoting in that state would have published nothing for it, left consumers on the old build, and kept the run green throughout. Caught by comparingreleaseagainstdevrather than by reading this branch.What ships
Five merged PRs:
#148 — an amount the ledger allows but
decimalcannot hold is refused rather than guessed at.Currency.ValueAsNumberanswered such values three different ways: a positive one clamped todecimal.MaxValue, a negative one threwFormatExceptionbecause the fallback parse had lostAllowLeadingSign, and a very small one quietly became zero. XRPL issued currency runs from1e-81to roughly1e96whiledecimalstops near7.9e28, so this cannot be parsed away — the only choice is how to fail, and it is now one way.AmountOutOfRangeExceptioncarries the value as the node sent it.Also from that PR:
ToStringshows the raw value instead of throwing through it; the setter no longer writes a string it cannot read back;Offer.AmountEachreads both sides before testing the denominator, so the exception it documents is one a caller can rely on.#147 — the signing path builds its
JsonSerializerOptionsonce. 1.73x end to end onEncodeForSigning, 857 fewer bytes per call, with the encoded blob hashing identically either way.#146 — the mock server stops taking the test host down with it. Test infrastructure. An aborted run does not report the tests it never reached, so this failure hid its own size.
#149 — documentation and commit language stated in
CLAUDE.md. #152 — the changelog heading stamped for today.Verification
releaseare not behind the merge queue, so wait for it rather than for the local figure aboveNumber's default rounding mode isToNearest, so the setter's rounding matches the protocol and was left alone; and the throw-on-overflow, zero-on-underflow asymmetry turns out to be exactly whatIOUAmountdoesThe release run
Pushing to
releasestartsnuget.release.yml, which will:11.1.0.0fromXrpl.csprojand extract the## 11.1.0.0 08/27/2026section — 5586 characters, checked by running that extraction against this treeXrpl11.1.0 andXrpl.BinaryCodec11.0.1; skip the four unchanged packages as duplicatesv11.1.0from those notesStep 1 runs before the build on purpose: a changelog that was not stamped fails the run while nothing has been published yet.
Summary by CodeRabbit
decimalrange, includingAmountOutOfRangeExceptionwith the original ledger value.