From fe2f41727123c3e4a0eec72a23ada7aae11886db Mon Sep 17 00:00:00 2001 From: Aleksandr Platonenkov Date: Wed, 12 Aug 2026 15:12:18 -0300 Subject: [PATCH] =?UTF-8?q?fix(sugar):=20=D0=BD=D0=B5=20=D0=B3=D0=BB=D0=BE?= =?UTF-8?q?=D1=82=D0=B0=D1=82=D1=8C=20=D0=BE=D1=82=D0=BC=D0=B5=D0=BD=D1=83?= =?UTF-8?q?=20=D0=B2=20fallback-=D0=B0=D1=85=20autofill=20+=20ValidationEx?= =?UTF-8?q?ception=20=D0=B4=D0=BB=D1=8F=20Flags?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Замечания CodeRabbit к релизному PR #76. - FetchCounterpartySignerCount и FetchLoan оборачивают вызов клиента широким catch — это верно для случая, ради которого он существует (аккаунт контрагента или объект Loan ещё не созданы, дальше отработает preclaim), но туда же уходил OperationCanceledException от токена вызывающего. Autofill продолжал работу и записывал комиссию, посчитанную по fallback — один подписант, отсутствующий заём, — для запроса, который вызывающий уже отменил. Добавлен фильтр when (!cancellationToken.IsCancellationRequested): отмена вызывающего всплывает, а таймаут внутри клиента, который этот токен не отменяет, по-прежнему уходит в fallback - MPTokenIssuanceSet: Flags разбирался через Convert.ToUInt32, бросающий FormatException/InvalidCastException на нечисловом значении, тогда как соседняя проверка ImmutableFlags и остальные валидаторы сообщают ValidationException — её и ловят вызывающие Тесты: отменённый токен обязан бросить и не оставить Fee (LoanSet и LoanPay), нечитаемый объект Loan обязан по-прежнему уходить в fallback, нечисловой Flags — давать ValidationException. Первые два падают, если убрать фильтры. --- CHANGES.md | 2 + .../Models/TestUProtocolCompleteness.cs | 8 +- Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs | 73 ++++++++++++++++++- .../Models/Transactions/MPTokenIssuanceSet.cs | 9 ++- Xrpl/Sugar/Autofill.cs | 10 ++- 5 files changed, 97 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1fb655ad..43133aa8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -24,6 +24,8 @@ * `.ci-config/bump-nightly-pin.sh` does the move: newest `xrpld` build from the nightly apt channel, `ARG XRPLD_VERSION` rewritten, `rippled.batchv11.cfg` regenerated from the develop commit **encoded in that version string** — config and binary cannot drift apart, which is the failure mode the old manual two-step invited. `--check` reports the pin, the newest build and the pin's age without touching anything. Both timestamp formats are compared by their common `YYYYMMDDHHMM` prefix * the workflow bumps only once the pin is older than `MAX_PIN_AGE_DAYS` (21) — nightly publishes several builds a day, and a weekly PR would be noise rather than signal; `workflow_dispatch` takes a `force` input for the exceptions. It then builds and starts the stand on the new pin and requires the AMM sentinel amendment to come up enabled at genesis, which is what proves the regenerated config was accepted rather than silently ignored, and attaches the definitions diff against the new build to the PR body — a `node-only` field there is the SDK being behind develop, reported instead of hidden * credentials, idempotency and the tracking-issue fallback follow release-watch exactly, including the one-notification-per-failure-streak rule +* **Cancellation no longer disappears into the autofill fee fallbacks** — `FetchCounterpartySignerCount` and `FetchLoan` wrap their client call in a broad `catch`, which is right for the case they exist for (the counterparty account or the Loan object is not there yet, and preclaim will report it) but also swallowed an `OperationCanceledException` raised from the caller's own token. Autofill then carried on and wrote a fee derived from the fallback — one signer, no loan — for a request the caller had already abandoned. Both catches now carry `when (!cancellationToken.IsCancellationRequested)`, which lets a caller's cancellation through while a client-side timeout, which does not cancel that token, still falls back as before. Covered in both directions: a cancelled token must throw and leave no `Fee` behind, an unreadable Loan object must still fall back +* **`MPTokenIssuanceSet` validation reports a malformed `Flags` as `ValidationException`** — it went through `Convert.ToUInt32`, which throws `FormatException` or `InvalidCastException` on a non-numeric value, while the `ImmutableFlags` check two lines below reports `ValidationException` like the rest of the validators. Callers catching `ValidationException` did not catch the other two * **The conformance fixtures are re-pinned to the 3.3.0 tag** — `transactions.macro` and `LedgerFormats.h` now come from the release commit (`00a178fb`) instead of a July `develop` sha and `3.3.0-rc1`; `ledger_entries.macro` stays on `develop` (`9859e5ce`) for the reason its `.ref` already gives — `sfLEVersion` exists only there. Both macro files are byte-identical to upstream and re-verifiable with the `curl … | diff` line in each `.ref`. This is what makes the guards test against the version CI actually runs: * `RippledLedgerFlags.Parse` learned to read the `lsif*` values. In 3.3.0 they are no longer a `LEDGER_OBJECT(MPTokenIssuanceMutable, …)` block but plain `inline constexpr std::uint32_t` constants next to the macro list, so the flag guard would have quietly lost that enum entirely. They are reported under a synthetic `MPTokenIssuanceImmutable` object, and a parse that finds none of them now throws instead of returning a thinner table * **Why the weekly Definitions Watch stayed green through all of this** — `definitions-watch.yml` raises a stand from `docker-compose.batchv11.yml`, i.e. the **pinned** nightly `XRPLD_VERSION`. While the pin is stale the monitor diffs `definitions.json` against a build older than the one CI runs, and reports "in sync" about the past. The pin needs to move with every stable bump, not only when a new amendment is wanted diff --git a/Tests/Xrpl.Tests/Models/TestUProtocolCompleteness.cs b/Tests/Xrpl.Tests/Models/TestUProtocolCompleteness.cs index 2195911f..5d62982c 100644 --- a/Tests/Xrpl.Tests/Models/TestUProtocolCompleteness.cs +++ b/Tests/Xrpl.Tests/Models/TestUProtocolCompleteness.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using System.Linq; using System.Text.Json; using System.Text.Json.Nodes; @@ -205,6 +205,12 @@ public async Task TestUMPTokenIssuanceSet_PreflightRules() ["MPTokenIssuanceID"] = "00000001A407AF5856CCF3C42619DAA925813FC955C72983", }; + // A non-numeric Flags value must report as ValidationException like every other + // malformed field here, not as a raw conversion exception callers do not catch. + tx["Flags"] = "not-a-number"; + await Assert.ThrowsExactlyAsync(() => Validation.ValidateMPTokenIssuanceSet(tx)); + tx.Remove("Flags"); + // ImmutableFlags: zero and out-of-mask values are temINVALID_FLAG tx["ImmutableFlags"] = 0u; await Assert.ThrowsExactlyAsync(() => Validation.ValidateMPTokenIssuanceSet(tx)); diff --git a/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs b/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs index d6f9926f..88391e29 100644 --- a/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs +++ b/Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs @@ -1,4 +1,4 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.VisualStudio.TestTools.UnitTesting; using System.Text.Json.Nodes; @@ -341,6 +341,73 @@ public async Task TestUCalculateFee_LoanSet_ExistingCounterpartySignature_Counts #endregion + #region Cancellation + + /// + /// A cancelled token must stop fee calculation rather than be absorbed by the fallback that + /// exists for a counterparty account which does not exist yet. + /// + /// + /// Both lookups sit behind a broad catch, so without an exception filter the + /// OperationCanceledException became a silent "assume one signer" and autofill carried on + /// writing a fee the caller never asked for. + /// + [TestMethod] + public async Task TestUCalculateFee_LoanSet_CancellationIsNotSwallowedByTheSignerFallback() + { + var client = new FeeTestClient(MAINNET_BASE_FEE, RESERVE_INC) + { + CounterpartySignerLists = CreateSignerList(3) + }; + var tx = CreateLoanSetTx(); + + using CancellationTokenSource cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAsync( + () => client.CalculateFeePerTransactionType(tx, 0, cts.Token)); + + Assert.IsFalse(tx.ContainsKey("Fee"), "A cancelled autofill must not leave a fee behind."); + } + + /// The same for the Loan lookup, whose fallback is a null object. + [TestMethod] + public async Task TestUCalculateFee_LoanPay_CancellationIsNotSwallowedByTheLoanFallback() + { + var client = new FeeTestClient(MAINNET_BASE_FEE, RESERVE_INC) + { + LoanEntry = CreateLoan(paymentRemaining: 50) + }; + var tx = CreateLoanPayTx(amount: "10000"); + + using CancellationTokenSource cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAsync( + () => client.CalculateFeePerTransactionType(tx, 0, cts.Token)); + + Assert.IsFalse(tx.ContainsKey("Fee"), "A cancelled autofill must not leave a fee behind."); + } + + /// + /// The filter must not turn every failure into a hard error: a lookup that fails on its own — + /// the object is missing — still falls back while the caller's token is untouched. + /// + [TestMethod] + public async Task TestUCalculateFee_LoanPay_FailedLookupStillFallsBackWhenNotCancelled() + { + var client = new FeeTestClient(MAINNET_BASE_FEE, RESERVE_INC) { LedgerEntryThrows = true }; + var tx = CreateLoanPayTx(amount: "10000"); + + using CancellationTokenSource cts = new CancellationTokenSource(); + + await client.CalculateFeePerTransactionType(tx, 0, cts.Token); + + Assert.IsTrue(tx.ContainsKey("Fee"), "An unreadable Loan object is a fallback, not a failure."); + } + + #endregion + #region LoanPay Fee Tests [TestMethod] @@ -650,6 +717,9 @@ public Task ServerState(ServerStateRequest request, CancellationTok public Task Fee(CancellationToken cancellationToken = default) => throw new NotSupportedException(); public Task AccountInfo(AccountInfoRequest request, CancellationToken cancellationToken = default) { + // Honour the token the way a real client does, so tests can assert that a caller's + // cancellation reaches autofill instead of being turned into a fee fallback. + cancellationToken.ThrowIfCancellationRequested(); AccountInfoCalls++; LastAccountInfoRequest = request; return Task.FromResult(new AccountInfo { SignerLists = CounterpartySignerLists }); @@ -669,6 +739,7 @@ public Task AccountInfo(AccountInfoRequest request, CancellationTok public Task LedgerData(LedgerDataRequest request, CancellationToken cancellationToken = default) => throw new NotSupportedException(); public Task LedgerEntry(LedgerEntryRequest request, CancellationToken cancellationToken = default) { + cancellationToken.ThrowIfCancellationRequested(); LedgerEntryCalls++; LastLedgerEntryRequest = request; if (LedgerEntryThrows) diff --git a/Xrpl/Models/Transactions/MPTokenIssuanceSet.cs b/Xrpl/Models/Transactions/MPTokenIssuanceSet.cs index 8e0e11e5..b1c75a50 100644 --- a/Xrpl/Models/Transactions/MPTokenIssuanceSet.cs +++ b/Xrpl/Models/Transactions/MPTokenIssuanceSet.cs @@ -244,7 +244,14 @@ public static async Task ValidateMPTokenIssuanceSet(Dictionary t uint flagValue = 0; if (tx.TryGetValue("Flags", out var flags) && flags is not null) { - flagValue = Convert.ToUInt32(flags); + // Same reporting as the ImmutableFlags check below: a non-numeric value has to + // surface as ValidationException, which is what callers of this method catch — + // Convert.ToUInt32 would throw FormatException or InvalidCastException instead. + if (!Common.TryGetUInt32(flags, out flagValue)) + { + throw new ValidationException("MPTokenIssuanceSet: Flags must be a number"); + } + bool hasLock = (flagValue & (uint)MPTokenIssuanceSetFlags.tfMPTLock) != 0; bool hasUnlock = (flagValue & (uint)MPTokenIssuanceSetFlags.tfMPTUnlock) != 0; diff --git a/Xrpl/Sugar/Autofill.cs b/Xrpl/Sugar/Autofill.cs index 33d879a3..92d42be1 100644 --- a/Xrpl/Sugar/Autofill.cs +++ b/Xrpl/Sugar/Autofill.cs @@ -343,9 +343,13 @@ private static async Task FetchCounterpartySignerCount(IXrplClient client, int? entries = data?.SignerLists?.Length > 0 ? data.SignerLists[0].SignerEntries?.Count : null; return entries is > 0 ? entries.Value : 1; } - catch (Exception) + catch (Exception) when (!cancellationToken.IsCancellationRequested) { // The counterparty account may not exist yet; preclaim rejects the transaction anyway. + // The filter keeps a caller's cancellation out of that fallback: without it an + // OperationCanceledException would be swallowed and autofill would carry on with a + // guessed signer count instead of stopping. A timeout inside the client still falls + // back, since it does not cancel this token. return 1; } } @@ -424,8 +428,10 @@ private static async Task FetchLoan(IXrplClient client, string loanId, C LedgerEntryResponse response = await client.LedgerEntry(request, cancellationToken); return response?.Node as LOLoan; } - catch (Exception) + catch (Exception) when (!cancellationToken.IsCancellationRequested) { + // Same reasoning as FetchCounterpartySignerCount: a missing object is a fallback, + // a cancellation asked for by the caller is not. return null; } }