fix(review): замечания CodeRabbit к релизному PR #68

.ci-config/docker-compose.ci.yml

@@ -7,8 +7,10 @@ services:
       - "5005:5005"
       - "5006:5006"
       - "6006:6006"
-      # credential-protected admin ws port, see [port_ws_admin_auth] in rippled.cfg
-      - "6007:6007"
+      # credential-protected admin ws port, see [port_ws_admin_auth] in rippled.cfg.
+      # Loopback-only: rippled.cfg carries these credentials in plain text, so the port
+      # must not be reachable from other hosts. Tests connect over localhost.
+      - "127.0.0.1:6007:6007"
     volumes:
       - ./rippled.cfg:/config/rippled.cfg:ro
       - ./validators.txt:/config/validators.txt:ro

CHANGES.md

@@ -34,6 +34,13 @@
   * `TestIConnectionStates` (7 tests) pointed at the public testnet and devnet. Nothing in them is specific to a public network — every assertion is about the client's own state machine — so they now run against the local node. The bogus-hostname case that tested reconnect exhaustion used a DNS lookup; it now uses a closed loopback port, which refuses immediately and involves no resolver. Fixed `Task.Delay` sleeps were the other half of the flakiness (one of these tests failed a full run and passed on retry) and are replaced by waiting for the expected state with a timeout: the class went from ~40 s of sleeping to sub-second assertions
   * the x402 live t54 interop tests need the public testnet faucet *and* a hosted third-party facilitator. They are now `[TestCategory("Live")]` and excluded from CI (`--filter "TestI&TestCategory!=Live"`), leaving the six hermetic x402 E2E tests in the run. Invoke them deliberately with `--filter "TestCategory=Live"`
 
+* Review pass (PR [#68](https://github.com/StaticBit-io/XrplCSharp/pull/68)):
+  * **`ValidateCheckCreate` now enforces `InvoiceID` as a Hash256**, not merely as a string. `sfInvoiceID` is a 256-bit hash and this same release added exactly that rule for `WalletLocator` in `AccountSet` and `SignerListSet`, so `CheckCreate` was the odd one out: a malformed value passed validation and only blew up later inside the codec, reporting an encoding error instead of a `ValidationException`. The exception message is unchanged (`CheckCreate: invalid InvoiceID`)
+  * the standalone stand's credential-protected ws port is bound to `127.0.0.1` instead of every interface — `rippled.cfg` carries those credentials in plain text, and the nightly stand already published it loopback-only
+  * `TestIConnectionStates` — both reconnect-exhaustion tests discarded the `Task.WhenAny` winner, so a run where the terminal event never arrived proceeded after the 30 s timeout and could still pass. They now assert the event task won, and that at least one reconnect was attempted
+  * `TestIProtocolFieldSets` sets `Expiration` on the mint-time NFT offer but never checked it read back; asserted now, closing the last unverified field of the corrected `NFTokenMint` set
+  * test-only tidying: the parse-floor literal is shared instead of duplicated (`RippledTransactionFormats.MinimumExpectedTransactions`), the common-field set both conformance surfaces subtract now comes from one helper (`RippledTransactionFormats.CommonFields`), and a redundant `Link` on the vendored fixture is dropped
+
 ## 10.9.1.0 07/27/2026
 * **Fix `account_tx` losing the payment amount and, on API v1, the whole transaction** — a silent regression introduced by the 10.3.0.0 `Newtonsoft.Json` → `System.Text.Json` migration; affects every release from 10.3.0.0 on:
   * `Payment`/`PaymentResponse.DeliverMax` — the private set-only alias that maps API v2's `DeliverMax` onto `Amount` was carried over from Newtonsoft (which deserializes attributed non-public members) but `System.Text.Json` skips non-public members without `[JsonInclude]`. Every Payment read through `AccountTransactions`, `TxV2` or the transaction streams came back with `Amount = null` — no exception, no diagnostic. `Tx()` was unaffected because it pins `ApiVersion = 1`, and `meta.delivered_amount` kept parsing correctly, which is why the loss went unnoticed. The alias stays set-only, so `DeliverMax` is still never serialized back out

CLAUDE.md

@@ -204,11 +204,12 @@ Integration tests do **not** run on PRs into `dev` — `dev` uses a GitHub merge
 ### Release Process
 
 1. Ensure all tests pass on `dev`
-2. Update version in all `.csproj` files (`Xrpl`, `Xrpl.AddressCodec`, `Xrpl.BinaryCodec`, `Xrpl.Keypairs`)
-3. Update `CHANGES.md`
-4. Merge `dev` → `release`
-5. NuGet publish triggers automatically on push to `release`
-6. Create GitHub release with tag
+2. Bump `<PackageVersion>` **only in the packages that actually changed** — not in all four. The base packages (`Xrpl.AddressCodec`, `Xrpl.BinaryCodec`, `Xrpl.Keypairs`) are consumed via `ProjectReference`, so a `Xrpl` package built at a newer version keeps depending on the already published base packages at their existing version. Leaving an untouched package behind is correct, not an oversight. Check with `git diff --stat origin/release...origin/dev -- Base/` before deciding
+3. Choose the bump from the nature of the change: patch for a bugfix with no contract change, minor otherwise
+4. Update `CHANGES.md`
+5. Merge `dev` → `release`
+6. NuGet publish triggers automatically on push to `release`
+7. Create GitHub release with tag
 
 ### NuGet Packages Published
 

Tests/Xrpl.Tests/Integration/TestIConnectionStates.cs

@@ -140,10 +140,12 @@ public async Task TestConnectionStateReconnect_InvalidServer()
         {
         }
 
-        await Task.WhenAny(tcs.Task, Task.Delay(TimeSpan.FromSeconds(30)));
+        Task terminal = await Task.WhenAny(tcs.Task, Task.Delay(TimeSpan.FromSeconds(30)));
+        Assert.AreSame(tcs.Task, terminal, "Reconnect exhaustion was not observed before the 30s timeout");
+        await tcs.Task;
 
         Assert.IsTrue(stateChanges.Contains(XrpConnectionState.Connecting), "Should have Connecting state");
-        Console.WriteLine($"Reconnect attempts: {reconnectAttempts}");
+        Assert.IsTrue(reconnectAttempts >= 1, $"Expected at least one reconnect attempt, got {reconnectAttempts}");
 
         await client.Disconnect();
     }
@@ -270,7 +272,10 @@ public async Task TestChangeServer_AfterMaxReconnectAttempts_NoNotConnectedExcep
         {
         }
 
-        await Task.WhenAny(disconnectedPermanently.Task, Task.Delay(TimeSpan.FromSeconds(30)));
+        Task terminal = await Task.WhenAny(disconnectedPermanently.Task, Task.Delay(TimeSpan.FromSeconds(30)));
+        Assert.AreSame(disconnectedPermanently.Task, terminal,
+            "Permanent disconnect was not observed before the 30s timeout");
+        await disconnectedPermanently.Task;
 
         Assert.AreEqual(XrpConnectionState.Disconnected, client.connection.CurrentConnectionState,
             "Should be Disconnected after max reconnect attempts");

Tests/Xrpl.Tests/Integration/transactions/TestIProtocolFieldSets.cs

@@ -166,6 +166,8 @@ public async Task TestINFTokenMint_OfferFieldsLandOnTheLedger()
 
                 Assert.AreEqual("5000000", offer.Amount.Value, "the mint-time sell offer must carry Amount");
                 Assert.AreEqual(buyer.ClassicAddress, offer.Destination);
+                Assert.IsNotNull(offer.Expiration, "the mint-time sell offer must carry Expiration");
+                Assert.AreEqual(expiration, offer.Expiration.Value);
             }
             finally
             {

Tests/Xrpl.Tests/Models/RippledTransactionFormats.cs

@@ -1,8 +1,11 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Text.RegularExpressions;
 
+using Xrpl.BinaryCodec.Enums;
+
 using TxFormat = Xrpl.Models.Transaction.TxFormat;
 
 namespace Xrpl.Tests.Models.Tests
@@ -38,7 +41,19 @@ internal static class RippledTransactionFormats
             @"\{\s*sf(?<field>\w+)\s*,\s*Soe(?<requirement>\w+)",
             RegexOptions.Compiled);
 
-        private const int MinimumExpectedTransactions = 60;
+        /// <summary>
+        /// Lower bound on how many formats a healthy parse yields. Exposed so the guard test
+        /// asserts against the same number the parser enforces, instead of a second literal
+        /// that would drift when the fixture is re-pinned.
+        /// </summary>
+        internal const int MinimumExpectedTransactions = 60;
+
+        /// <summary>
+        /// Fields shared by every transaction, declared once in the <see cref="TxFormat"/> constructor.
+        /// rippled keeps them in a separate <c>commonFields</c> list, so both conformance surfaces
+        /// exclude them — they read the set from here so the two cannot drift apart.
+        /// </summary>
+        internal static HashSet<Field> CommonFields() => new TxFormat().Keys.ToHashSet();
 
         internal static string FixturePath =>
             Path.Combine(AppContext.BaseDirectory, "Fixtures", "transactions.macro");

Tests/Xrpl.Tests/Models/TestCheckCreate.cs

@@ -141,6 +141,54 @@ public void TestUCheckCreate_InvoiceIDIsAHash256()
 
             Assert.AreEqual(invoiceId, decoded["InvoiceID"].GetValue<string>());
         }
+
+        [TestMethod]
+        public async Task TestUCheckCreate_RejectsInvoiceIDThatIsNotA256BitHexValue()
+        {
+            // sfInvoiceID is Hash256. A string of the wrong length or with non-hex characters is
+            // malformed and must fail validation rather than blow up later inside the codec —
+            // the same rule SignerListSet and AccountSet already apply to WalletLocator.
+            string[] malformed =
+            {
+                "6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59",   // 63 chars
+                "6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59BA", // 65 chars
+                "6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF5XZ",  // non-hex
+                "",
+            };
+
+            foreach (string invoiceId in malformed)
+            {
+                Dictionary<string, object> tx = new Dictionary<string, object>
+                {
+                    { "TransactionType", "CheckCreate" },
+                    { "Account", "rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo" },
+                    { "Destination", "rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy" },
+                    { "SendMax", "100000000" },
+                    { "InvoiceID", invoiceId },
+                    { "Fee", "12" },
+                };
+
+                await Helper.ThrowsExceptionAsync<ValidationException>(
+                    () => Validation.ValidateCheckCreate(tx),
+                    "CheckCreate: invalid InvoiceID");
+            }
+        }
+
+        [TestMethod]
+        public async Task TestUCheckCreate_AcceptsA256BitHexInvoiceID()
+        {
+            Dictionary<string, object> tx = new Dictionary<string, object>
+            {
+                { "TransactionType", "CheckCreate" },
+                { "Account", "rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo" },
+                { "Destination", "rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy" },
+                { "SendMax", "100000000" },
+                { "InvoiceID", "6f1dfd1d0fe8a32e40e1f2c05cf1c15545bab56b617f9c6c2d63a6b704bef59b" },
+                { "Fee", "12" },
+            };
+
+            await Validation.ValidateCheckCreate(tx);
+        }
     }
 
 }

Tests/Xrpl.Tests/Models/TestUProtocolCompleteness.cs

@@ -237,13 +237,15 @@ public async Task TestUMPTokenIssuanceCreate_MutableFlagsMask()
 
         /// <summary>
         /// The fields a format declares on top of the common set shared by every transaction.
+        /// The common set comes from <see cref="RippledTransactionFormats.CommonFields"/>, which
+        /// <c>TestUTxFormatConformance</c> also reads, so the two conformance surfaces stay in step.
         /// </summary>
         private static Dictionary<BinaryCodec.Enums.Field, TxFormat.Requirement> TypeSpecificFields(
             BinaryCodec.Types.TransactionType transactionType)
         {
-            TxFormat common = new TxFormat();
+            HashSet<BinaryCodec.Enums.Field> common = RippledTransactionFormats.CommonFields();
             return TxFormat.Formats[transactionType]
-                .Where(entry => !common.ContainsKey(entry.Key))
+                .Where(entry => !common.Contains(entry.Key))
                 .ToDictionary(entry => entry.Key, entry => entry.Value);
         }
 

Tests/Xrpl.Tests/Models/TestUTxFormatConformance.cs

@@ -28,11 +28,14 @@ namespace Xrpl.Tests.Models.Tests
     public class TestUTxFormatConformance
     {
         /// <summary>
-        /// Fields shared by every transaction, declared once in the TxFormat constructor.
-        /// rippled keeps them in a separate commonFields list, so they are excluded on both sides.
+        /// Names of the fields shared by every transaction; excluded on both sides of the diff.
+        /// Derived from <see cref="RippledTransactionFormats.CommonFields"/> so this test and
+        /// <c>TestUProtocolCompleteness</c> cannot disagree about what "common" means.
         /// </summary>
         private static HashSet<string> CommonFieldNames() =>
-            new TxFormat().Keys.Select(field => field.Name).ToHashSet(StringComparer.Ordinal);
+            RippledTransactionFormats.CommonFields()
+                .Select(field => field.Name)
+                .ToHashSet(StringComparer.Ordinal);
 
         [TestMethod]
         public void TestUTxFormat_MatchesRippledTransactionsMacro()
@@ -93,7 +96,10 @@ public void TestUTxFormatConformance_ParserFailsLoudlyOnAnEmptySource()
             // conformance test above pass against an empty table.
             Dictionary<string, Dictionary<string, TxFormat.Requirement>> parsed = RippledTransactionFormats.Parse();
 
-            Assert.IsGreaterThan(60, parsed.Count, "the vendored macro must yield a full format table");
+            Assert.IsGreaterThanOrEqualTo(
+                RippledTransactionFormats.MinimumExpectedTransactions,
+                parsed.Count,
+                "the vendored macro must yield a full format table");
             Assert.IsTrue(parsed.ContainsKey("Payment"), "Payment must be present — the parser matched nothing sane");
             Assert.IsTrue(parsed["Payment"].ContainsKey("Destination"));
             Assert.AreEqual(TxFormat.Requirement.Required, parsed["Payment"]["Destination"]);

Tests/Xrpl.Tests/Xrpl.Tests.csproj

@@ -30,7 +30,7 @@
                 <None Include="..\..\Base\Xrpl.BinaryCodec\Enums\definitions.json" Link="definitions.json">
                         <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
                 </None>
-                <None Include="Fixtures\transactions.macro" Link="Fixtures\transactions.macro">
+                <None Include="Fixtures\transactions.macro">
                         <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
                 </None>
         </ItemGroup>

Xrpl/Client/connection.cs

@@ -2091,6 +2091,12 @@ private async Task ExecutePingCheckAsync(CancellationTokenSource cts)
                 try
                 {
                     Debug.WriteLine($"{DateTime.Now}[PING-CHECK] Fire-and-forget keepalive ping (active connection)");
+
+                    // Raw send: this bypasses RequestManager, so AdminUser/AdminPassword are NOT attached.
+                    // Safe for ping specifically — rippled resolves the role per command, and a guest-level
+                    // command is answered normally even on a port that sets admin_user/admin_password
+                    // (only commands requiring Role::ADMIN get "forbidden / Bad credentials."). Anything
+                    // needing admin must go through Request/GRequest instead of being added here.
                     currentSocket?.SendMessage("{\"command\":\"ping\",\"id\":\"00000000-0000-0000-0000-000000000000\"}");
                     if (OnPing != null)
                     {

Xrpl/Models/Transactions/CheckCreate.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Text.RegularExpressions;
 using System.Threading.Tasks;
 
 using System.Text.Json.Serialization;
@@ -108,7 +109,11 @@ public static async Task ValidateCheckCreate(Dictionary<string, object> tx)
                 throw new ValidationException("CheckCreate: invalid DestinationTag");
             if (tx.TryGetValue("Expiration", out var Expiration) && !Common.IsUInt32(Expiration))
                 throw new ValidationException("CheckCreate: invalid Expiration");
-            if (tx.TryGetValue("InvoiceID", out var InvoiceID) && InvoiceID is not string { })
+            // sfInvoiceID is a Hash256 — the same 256-bit rule SignerListSet and AccountSet apply to
+            // WalletLocator. Without the shape check a malformed string only fails later inside the
+            // codec, which reports an encoding error instead of a ValidationException.
+            if (tx.TryGetValue("InvoiceID", out var InvoiceID) &&
+                (InvoiceID is not string invoiceId || !Regex.IsMatch(invoiceId, @"^[0-9A-Fa-f]{64}$")))
                 throw new ValidationException("CheckCreate: invalid InvoiceID");