diff --git a/CHANGES.md b/CHANGES.md index 6c2583ed..666eb1e5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -17,6 +17,8 @@ This release makes the SDK stop misrepresenting what a node sent. It is a delibe | 15 more response properties | non-nullable | nullable — `LOBaseLedger.LedgerIndex`, `HashOrTransaction.LedgerTransaction.Validated`, `BaseLedgerEntity.Closed`/`IBaseLedgerEntity.Closed`, `NoRippleCheck.LedgerCurrentIndex`, `ServerFeatures.LedgerIndex`/`Validated`, `LedgerStreamResponse.LedgerIndex`/`ReserveBase`/`ReserveInc`/`FeeBase`/`FeeRef`/`LedgerTime`/`TxnCount`, `LedgerStream.FeeRef`, `ValidationStream.LedgerIndex` | | Quorum helper | `PickWalletsForQuorum` returned `(List, uint)` | returns `(List, uint, uint)` and throws `ValidationException` when `SignerQuorum` is absent | | Untyped request | `Task> Request(...)` | `Task>> Request(...)` | +| Stream events | `client.connection.OnTransaction += …` | also `client.OnTransaction += …` — on `IXrplClient` now; the old form still works | +| `IXrplClient.connection` | `{ get; set; }` | `{ get; }` — assigning it would strand handlers on the old object | | Stream event type | `LedgerStream.Type` was a public field | inherited `BaseStream.Type` property — source-compatible, but an assembly built against the old package needs a rebuild | | Stream event type value | `ResponseStreamType Type` | `ResponseStreamType? Type` — an event that carried no `type` no longer reports `UNKNOWN` as though the node had said so | @@ -191,6 +193,11 @@ The entries below are grouped by what changed, not by the order the levels were * **Locating a member no longer allocates.** Making the name match case-insensitive (so a key that fills a typed property cannot read as absent) had been written with `reader.GetString()`, which materialized every top-level key of every frame — ~760 B per scan, twice per stream event, on a struct whose whole purpose is to record where a value sits *without* materializing it. `Utf8JsonReader.ValueTextEquals` now answers the ordinary case against the raw bytes and only a differently-cased or escaped key falls through to the allocating comparison, which keeps the result identical to what the serializer does under `PropertyNameCaseInsensitive` * Three `UnknownFields` declarations were removed from `LOLedger`, `LedgerEntity` and `LedgerBinaryEntity`: their bases had gained the same property, and a duplicate in one hierarchy compiles (CS0108) with System.Text.Json binding the derived one — so the base property stayed `null` forever while the data sat on the subclass. `LedgerClosed` hands callers an `LOBaseLedger`, which would have read empty * `ErrorResponse` remains the one stream-path type without a whole-event `Raw`: it descends from `BaseResponse`, not `BaseStream`, and gets `RawResult`/`RawId`/`RawRequest` instead. Named here so it is a known boundary rather than an unnoticed gap +* **Stream events reach `IXrplClient`, so the raw bytes on them are usable through the SDK's own contract** (#103). Everything above gives a stream event `Raw` and `RawTransaction` — the point being that a wallet can show a person the transaction a node actually sent before they sign it. Transactions arrive by stream, and the only way to receive one was `client.connection.OnTransaction`: a property of a concrete class, so code written against `IXrplClient` could neither subscribe nor be exercised against a substitute client. The feature existed without a contract to reach it through. + * all 16 events `Connection` raises are declared on `IXrplClient` and forwarded to it. `client.connection.OnX` keeps working unchanged — this adds a surface, it does not move one + * **forwarded, not relayed**, and the distinction is the whole design: `add`/`remove` go straight to the same `Connection`, so the client holds no delegates, no subscriber list of its own, and no subscription that nothing removes. A relaying version would add all three, plus a second place to keep in sync. `TestUUnsubscribingThroughTheInterfaceRemovesTheHandler` pins it by crossing surfaces — subscribe through the client, remove through the connection — because removing through the same surface passes either way + * this is safe because the `Connection` outlives the client: it is assigned once, in the constructor, and `ChangeServer` swaps the *session* inside it rather than the object, so subscriptions survive a server change + * **`IXrplClient.connection` lost its setter** (**breaking**, though nothing in the tree assigned it). With handlers attached through the events above, replacing the connection would strand every one of them on the old object and the stream would go quiet with nothing to show for it ## 10.12.0.0 08/16/2026 diff --git a/Tests/Xrpl.Tests/Client/TestUClientStreamEvents.cs b/Tests/Xrpl.Tests/Client/TestUClientStreamEvents.cs new file mode 100644 index 00000000..91f52501 --- /dev/null +++ b/Tests/Xrpl.Tests/Client/TestUClientStreamEvents.cs @@ -0,0 +1,147 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using System; +using System.Threading.Tasks; + +using Xrpl.Client; +using Xrpl.Models.Subscriptions; + +namespace XrplTests.Client; + +/// +/// Stream events are reachable through , not only through the concrete +/// connection field. +/// +/// +/// This is what makes the raw bytes on a stream event usable through the SDK's own contract: a +/// wallet renders transaction events for signing, and until now the only way to receive one +/// was client.connection.OnTransaction - a property of a concrete class, so code written +/// against the interface could neither subscribe nor be tested against a substitute client. +/// +[TestClass] +public class TestUClientStreamEvents +{ + private const string TransactionMessage = """ + { + "type": "transaction", + "status": "closed", + "validated": true, + "engine_result": "tesSUCCESS", + "tx_json": { + "TransactionType": "Payment", + "Account": "rP9jPyP5kyvFRb6ZiRghAGw5u8SGAmU4bd", + "Sequence": 13 + }, + "meta": { "AffectedNodes": [], "TransactionIndex": 0, "TransactionResult": "tesSUCCESS" } + } + """; + + /// + /// A handler registered through the interface receives the event, and the raw bytes come with + /// it - the whole point of reaching the stream through the contract. + /// + [TestMethod] + public async Task TestUSubscribingThroughTheInterfaceReceivesStreamEvents() + { + using XrplClient client = new XrplClient("wss://localhost:1/"); + IXrplClient contract = client; + + TaskCompletionSource received = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + contract.OnTransaction += r => + { + received.TrySetResult(r); + return Task.CompletedTask; + }; + + await client.connection.OnMessage(TransactionMessage); + + Task completed = await Task.WhenAny(received.Task, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.AreSame(received.Task, completed, "the handler registered through IXrplClient was never invoked"); + + TransactionStream result = await received.Task; + Assert.AreEqual(13u, result.Transaction.Sequence); + Assert.IsFalse(result.Raw.IsEmpty, "the event carries the bytes the node sent, which is why reaching it through the contract matters"); + } + + /// + /// Subscribing through the client and through its connection reach one list, because the + /// client forwards rather than relaying. + /// + /// + /// Pins the forwarding shape itself: a relaying implementation would keep its own subscriber + /// list, and removing through one surface would leave the other still subscribed. + /// + [TestMethod] + public async Task TestUHandlerAddedThroughTheClientCanBeRemovedThroughTheConnection() + { + using XrplClient client = new XrplClient("wss://localhost:1/"); + IXrplClient contract = client; + + int calls = 0; + OnTransaction handler = _ => + { + calls++; + return Task.CompletedTask; + }; + + // Subscribed through the client, removed through the connection. Only forwarding makes + // that work: a relaying client would keep its own subscriber list, the removal would miss + // it, and the handler would keep firing. Removing through the same surface it was added + // to cannot tell the two apart - both pass - which is why the test crosses surfaces. + contract.OnTransaction += handler; + await client.connection.OnMessage(TransactionMessage); + Assert.AreEqual(1, calls, "sanity: the handler is attached"); + + client.connection.OnTransaction -= handler; + await client.connection.OnMessage(TransactionMessage); + + Assert.AreEqual(1, calls, "removing through the connection left the handler attached - the client is relaying into its own list rather than forwarding"); + } + + /// + /// The mirror case: added through the connection, removed through the client. + /// + /// + /// Needed as its own test because the one above never executes the client's remove + /// accessor - it removes through the connection. Verified by mutation: turning the client's + /// remove into connection.OnTransaction += value left the whole suite green + /// until this existed. + /// + [TestMethod] + public async Task TestUHandlerAddedThroughTheConnectionCanBeRemovedThroughTheClient() + { + using XrplClient client = new XrplClient("wss://localhost:1/"); + IXrplClient contract = client; + + int calls = 0; + OnTransaction handler = _ => + { + calls++; + return Task.CompletedTask; + }; + + client.connection.OnTransaction += handler; + await client.connection.OnMessage(TransactionMessage); + Assert.AreEqual(1, calls, "sanity: the handler is attached"); + + contract.OnTransaction -= handler; + await client.connection.OnMessage(TransactionMessage); + + Assert.AreEqual(1, calls, "removing through the client left the handler attached - its remove accessor does not reach the connection"); + } + + /// + /// connection is read-only on the contract, so a caller cannot swap the object every + /// handler is attached to and leave the stream silently unreachable. + /// + [TestMethod] + public void TestUConnectionCannotBeReplacedThroughTheContract() + { + System.Reflection.PropertyInfo property = typeof(IXrplClient).GetProperty(nameof(IXrplClient.connection)); + + Assert.IsNotNull(property); + Assert.IsNull(property.SetMethod, "a settable connection would let a caller strand every handler registered through these events on the old object"); + } +} diff --git a/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs b/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs index 14a78992..6618e4c7 100644 --- a/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs +++ b/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs @@ -640,7 +640,29 @@ public FeeTestClient(string feeXrp, uint reserveInc, string maxFeeXRP = "5") this.feeCushion = 1.0; } - public Connection connection { get; set; } = null!; + public Connection connection => null!; + + // Declared, never raised: this client answers requests, it does not stream. Being able to + // declare them at all is the point of the events moving onto IXrplClient - a substitute + // client could not carry them while they lived only on Connection. +#pragma warning disable CS0067 + public event OnError OnError; + public event OnWarning OnWarning; + public event OnServerWarning OnServerWarning; + public event OnConnected OnConnected; + public event OnDisconnect OnDisconnect; + public event OnPing OnPing; + public event OnLedgerClosed OnLedgerClosed; + public event OnTransaction OnTransaction; + public event OnValidationReceived OnValidationReceived; + public event OnManifestReceived OnManifestReceived; + public event OnPeerStatusChange OnPeerStatusChange; + public event OnConsensusPhase OnConsensusPhase; + public event OnPathFind OnPathFind; + public event OnBookChanges OnBookChanges; + public event OnServerStatus OnServerStatus; + public event Action OnConnectionStatus; +#pragma warning restore CS0067 public double feeCushion { get; set; } public string maxFeeXRP { get; set; } public uint? networkID { get; set; } diff --git a/Xrpl/Client/IXrplClient.cs b/Xrpl/Client/IXrplClient.cs index 9cc802d9..0ed2c4ab 100644 --- a/Xrpl/Client/IXrplClient.cs +++ b/Xrpl/Client/IXrplClient.cs @@ -46,7 +46,64 @@ namespace Xrpl.Client public interface IXrplClient : IDisposable { - Connection connection { get; set; } + /// + /// The socket this client speaks over. + /// + /// + /// Read-only: replacing it would leave every handler registered through the events below + /// attached to the old object, and the stream would go quiet with nothing to show for it. + /// ChangeServer is how a caller moves to another server - it swaps the session + /// inside this object rather than the object itself, so subscriptions survive. + /// + Connection connection { get; } + + /// Node error reported over the socket. + event OnError OnError; + + /// Node warning reported over the socket. + event OnWarning OnWarning; + + /// Server warnings attached to a response envelope. + event OnServerWarning OnServerWarning; + + /// The socket finished connecting. + event OnConnected OnConnected; + + /// The socket closed. + event OnDisconnect OnDisconnect; + + /// Keep-alive round trip completed. + event OnPing OnPing; + + /// ledgerClosed stream event. + event OnLedgerClosed OnLedgerClosed; + + /// transaction stream event - the one a wallet renders for signing. + event OnTransaction OnTransaction; + + /// validationReceived stream event. + event OnValidationReceived OnValidationReceived; + + /// manifestReceived stream event. + event OnManifestReceived OnManifestReceived; + + /// peerStatusChange stream event. + event OnPeerStatusChange OnPeerStatusChange; + + /// consensusPhase stream event. + event OnConsensusPhase OnConsensusPhase; + + /// path_find follow-up. + event OnPathFind OnPathFind; + + /// bookChanges stream event. + event OnBookChanges OnBookChanges; + + /// serverStatus stream event. + event OnServerStatus OnServerStatus; + + /// Connection state transitions, for diagnostics. + event Action OnConnectionStatus; double feeCushion { get; set; } string maxFeeXRP { get; set; } uint? networkID { get; set; } @@ -481,7 +538,118 @@ public class ClientOptions : ConnectionOptions public uint? ApiVersion { get; set; } } - public Connection connection { get; set; } + // get-only, not `private set`: the one-assignment invariant the forwarding below depends on + // is then checked by the compiler rather than by everyone who edits this 1100-line class. + // A second assignment would strand every handler attached through these events on the old + // object. + public Connection connection { get; } + + // Forwarded, not relayed: add/remove reach the same Connection a caller would have used + // through the property, so this type holds no delegates and no subscription of its own. A + // relaying version - a local event plus a subscription to the connection that re-raises it + // - would add a second subscriber list, a subscription nothing removes, and a second place + // to keep in sync. There is nothing to keep in sync here. + // + // Safe because the Connection outlives the client: it is assigned once, in the + // constructor, and ChangeServer swaps the session inside it rather than the object. Were + // that to change, handlers would silently stay on the old object - which is why the + // property lost its public setter. + + public event OnError OnError + { + add => connection.OnError += value; + remove => connection.OnError -= value; + } + + public event OnWarning OnWarning + { + add => connection.OnWarning += value; + remove => connection.OnWarning -= value; + } + + public event OnServerWarning OnServerWarning + { + add => connection.OnServerWarning += value; + remove => connection.OnServerWarning -= value; + } + + public event OnConnected OnConnected + { + add => connection.OnConnected += value; + remove => connection.OnConnected -= value; + } + + public event OnDisconnect OnDisconnect + { + add => connection.OnDisconnect += value; + remove => connection.OnDisconnect -= value; + } + + public event OnPing OnPing + { + add => connection.OnPing += value; + remove => connection.OnPing -= value; + } + + public event OnLedgerClosed OnLedgerClosed + { + add => connection.OnLedgerClosed += value; + remove => connection.OnLedgerClosed -= value; + } + + public event OnTransaction OnTransaction + { + add => connection.OnTransaction += value; + remove => connection.OnTransaction -= value; + } + + public event OnValidationReceived OnValidationReceived + { + add => connection.OnValidationReceived += value; + remove => connection.OnValidationReceived -= value; + } + + public event OnManifestReceived OnManifestReceived + { + add => connection.OnManifestReceived += value; + remove => connection.OnManifestReceived -= value; + } + + public event OnPeerStatusChange OnPeerStatusChange + { + add => connection.OnPeerStatusChange += value; + remove => connection.OnPeerStatusChange -= value; + } + + public event OnConsensusPhase OnConsensusPhase + { + add => connection.OnConsensusPhase += value; + remove => connection.OnConsensusPhase -= value; + } + + public event OnPathFind OnPathFind + { + add => connection.OnPathFind += value; + remove => connection.OnPathFind -= value; + } + + public event OnBookChanges OnBookChanges + { + add => connection.OnBookChanges += value; + remove => connection.OnBookChanges -= value; + } + + public event OnServerStatus OnServerStatus + { + add => connection.OnServerStatus += value; + remove => connection.OnServerStatus -= value; + } + + public event Action OnConnectionStatus + { + add => connection.OnConnectionStatus += value; + remove => connection.OnConnectionStatus -= value; + } public double feeCushion { get; set; } public string maxFeeXRP { get; set; } public uint? networkID { get; set; } @@ -497,15 +665,6 @@ public class ClientOptions : ConnectionOptions /// The API version to use when making requests. /// public uint ApiVersion { get; set; } - //public event OnError OnError; - //public event OnConnected OnConnected; - //public event OnDisconnect OnDisconnect; - //public event OnLedgerClosed OnLedgerClosed; - //public event OnTransaction OnTransaction; - //public event OnManifestReceived OnManifestReceived; - //public event OnPeerStatusChange OnPeerStatusChange; - //public event OnConsensusPhase OnConsensusPhase; - //public event OnPathFind OnPathFind; ///// Current web socket client state //public WebSocketState SocketState => client.State;