fix(validation)!: validators throw on the calling thread, so BatchUtils.Build validates - #187
Conversation
…ls.Build validates Validation.Validate and the 86 validators behind it were declared async Task without ever awaiting anything, and BatchUtils.Build called Validate without awaiting the result. An async method captures every exception into the task it returns, including the ones thrown before the first await, so a discarded task is a discarded verdict: ValidateBatch ran, decided the batch was malformed and reported it to nobody. A Batch built around a single inner transaction - which rippled answers with temARRAY_EMPTY - came back from Build looking well formed, and so did one with more than eight inners, with a Vault/Loan inner, with an inner missing tfInnerBatchTxn, or with an inner carrying a non-zero Fee. The compiler had been saying so as CS4014 since the method was written. The validators now return void and throw on the calling thread. Conditions, exception types and messages are unchanged; `await Validation.Validate(tx)` no longer compiles - drop the await. Making the signature honest is the fix rather than adding the missing await: Build is synchronous and public, so awaiting would have meant Task<Batch> BuildAsync, and .GetAwaiter().GetResult() would have left the next caller the same trap. TestUCredentialsValidator wrapped the already-synchronous ValidateCredentialsList in Task.Run to fit the async assertion helper's Func<Task>. That worked while the helper awaited the task, and is exactly the shape that stops working once the assertion is synchronous, since Action accepts a lambda whose value is discarded. The nine tests call the validator directly now. Released as 11.5.1.0 rather than a major: the validators are opt-in, nothing inside the SDK calls them, and BatchUtils.Build - the one caller that did - is what this release fixes. CHANGES.md states the deviation for anyone calling Validation.* or Common.ValidateBaseTransaction directly.
|
@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: Essentials Run ID: 📒 Files selected for processing (132)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughTransaction validation changed from asynchronous ChangesSynchronous transaction validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains; the validator migration and compatibility impact are explicitly documented. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 246 functions across 50 files. (82 skipped: 2 unsupported, 80 over the file limit.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
CodeRabbit triageFull review returned no actionable comments — no inline comments, and no collapsed 🧹 Nitpick or One pre-merge check failed, and here is the disposition.
|
1 participant
What was wrong
BatchUtils.BuildcalledValidation.Validate(...)without awaiting it, andValidation.Validateis declaredasync Task. Anasyncmethod captures every exception into the task it returns — including the ones thrown before the firstawait— so a discarded task is a discarded verdict.ValidateBatchran, decided the batch was malformed, and reported it to nobody.A Batch built around a single inner transaction, which rippled answers with
temARRAY_EMPTY, came back fromBuildlooking well formed. So did one with more than eight inners, one with aVault/Loaninner, one with an inner missingtfInnerBatchTxn, and one with an inner carrying a non-zeroFee— every rule inValidateBatchwas lost, not just the inner count.The compiler had been reporting it as CS4014 since the method was written.
The fix
The 86 validators —
Validation.Validate, the 79Validation.ValidateX,Common.ValidateBaseTransactionand the handful of helpers — now returnvoidand throw on the calling thread. Not one of them ever awaited anything: they are straight-line field checking that wasasynconly in signature.Adding the missing
awaitwas the alternative and is worse here.Buildis synchronous and public, so awaiting would have meantTask<Batch> BuildAsync, and.GetAwaiter().GetResult()would have left the next caller the same trap. A validator that cannot be forgotten is a validator that has no task to forget.Conditions, exception types and messages are unchanged. The migration is one edit:
await Validation.Validate(tx)→Validation.Validate(tx). Atry/catcharound the call keeps working as it is.Version: 11.5.1.0, and why a patch
This is a breaking change released as a patch, deliberately. Semver would call it a major — the return type is part of a method's signature in IL, so an assembly built against 11.5.0.0 meets a
MissingMethodExceptionon 11.5.1.0 even where it never wroteawait. It is numbered a patch because the validators are opt-in: nothing inside the SDK calls them, andBatchUtils.Build, the one caller that did, is the method this release fixes.CHANGES.mdstates the deviation and tells anyone callingValidation.*directly to treat the upgrade as a major one.One thing found on the way
TestUCredentialsValidatorwrapped the already-synchronousCredentialsValidator.ValidateCredentialsListinTask.Runto fit the async assertion helper'sFunc<Task>. That worked while the helper awaited the task — and it is exactly the shape that stops working the moment the assertion becomes synchronous, sinceActionaccepts a lambda whose value is discarded. Nine tests went red on the first full run and now call the validator directly.Tests
Tests/Xrpl.Tests/Models/TestUBatchUtils.cspins the behaviour: a single inner is refused, more than eight are refused, a well-formed batch is built. Written first — the two rejection tests failed with "Expected exception of type System.ArgumentException" before the fix.docker-compose.ci.yml): 346 passed, 0 failedBatchUtils.csis gone. The one left,connection.cs:4638, is anInterlocked.Exchangediscarding the previous task on purpose and is untouched.Diff shape
131 files, but three mechanical classes: validator signatures and their call sites, the now-unused
using System.Threading.Tasks;, and the test-sideawaitremovals (Helper.ThrowsExceptionAsync→ the existingHelper.ThrowsException,Assert.ThrowsExactlyAsync→Assert.ThrowsExactly, and test methods that no longer await anything becomingvoid). The behavioural change isBatchUtils.csplus the new test file.Summary by CodeRabbit
Breaking Changes
Bug Fixes
Documentation
Release