test(sponsor): the Sponsor field on Vault, Loan and bridge attestations - #167
Conversation
TestIXChainAttestation and TestILoanMultisig were written after this workflow and never added to it. Between them they are the only traffic for both attestation transaction types, XChainClaim, and LoanSet.CounterpartySignature.Signers, so a default run covered none of that. Verified by dispatching with the corrected filter: 101 tests, 92 passed, 9 skipped for MPTokensV2 being absent from devnet, none failed.
These are the types rippled forbids inside a Batch (Batch::preflight kDisabledTxTypes), so whether they take a sponsor at all was worth establishing rather than assuming. They do: preflight1Sponsor in Transactor.cpp constrains only spfSponsorReserve, through the allow-list in isReserveSponsorAllowed, and no Vault or Loan type is on it. Fee sponsorship is unconstrained. A sponsored LoanSet carries three signatures at once - the broker's own, the borrower's CounterpartySignature and the sponsor's SponsorSignature - and this is the first time the composer has had to place all three in one transaction. LoanBrokerCoverWithdraw, LoanBrokerCoverClawback and LoanManage had never been submitted to a node by any test. Clawing broker cover back is the asset issuer's move and rippled refuses it on a native asset, so that case needs an IOU-backed vault whose issuer is a third account. Two rules from LoanBrokerSet::preflight that are easy to trip are written down where the test lives: VaultID is required even when the transaction updates a broker that already exists, and a transaction naming a LoanBrokerID may not carry ManagementFeeRate, CoverRateMinimum or CoverRateLiquidation - those are set once, at creation, and an update carrying one is temINVALID. VaultDelete.MemoData is left out: the field is optional on rippled's develop branch and the release build the CI stand runs answers temDISABLED for it, so a test carrying it would report the stand's version rather than the SDK. Verified: 337 of 337 on the standalone stand, 12 of 12 against devnet. The Sponsor amendment reads 109 of 109 on the coverage dashboard afterwards.
|
@coderabbitai review |
✅ Action performedReview 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 (1)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughAdded integration coverage for XLS-68 fee sponsorship across Vault, Loan, LoanBroker, and XChain attestation transactions. The changes also update devnet test selection and document covered transaction types, signature composition, validation scenarios, and version-specific exclusions. ChangesVault and Loan sponsorship
XChain attestation sponsorship
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The workflow now includes the intended attestation and loan multisignature coverage classes, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 77.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The review's docstring-coverage check counted 52% across the functions this branch touches. Documented the ones where a sentence says something a reader would not get from the name: the four bridge attestation cases, both sponsored ones, and the helpers whose behaviour is not obvious - what the witness signer list is for, what a null destination on an attestation means, why an object count is being taken. Left alone: the MSTest lifecycle members and the one-line amount converters, where a docstring would only restate the name.
|
@coderabbitai review |
✅ Action performedReview finished.
|
The class arrives with this branch, so the filter entry belongs here rather than in a follow-up: the sponsored Vault and Loan cases are exactly the transactions the dashboard scores for XLS-68, and a class the default run omits contributes nothing to it.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Review triage: no actionable findings. The one pre-merge warning is Docstring Coverage 77.42% against an 80.00% threshold, counted over 31 test methods in two files. Not acted on: test methods carry no XML documentation anywhere in this suite, and |
EasyTimer wrapped System.Timers.Timer in a pair of methods named after JavaScript's setInterval and setTimeout, because FundWallet.cs is a port of xrpl.js's fundWallet.ts. Nothing ever called it: across every revision of that file back to October 2022 there is not one use. The faucet poll it was presumably written for drove a System.Timers.Timer through static fields instead - the design behind the once-per-process poll budget fixed earlier in this version. Removed rather than deprecated: there is nothing to migrate to that is not already better. System.Timers.Timer is public, documented and one line away, while these wrappers could not report a failure - the callback ran on a timer thread with nothing to observe it - let SetInterval callbacks overlap when the work outlasted the interval, and left stopping to whoever remembered the returned handle. Removing a public type is a contract change and the changelog says so, but the version stays at 11.3.0.0 by the maintainer's call: the type had no callers to break. The Unreleased heading added in #167 is folded into the 11.3.0.0 section so unreleased work sits under one heading again, which is how this repository has kept the changelog - a section is created with its version and date when the version is bumped.
1 participant
The last 23 red cells of the
Sponsoramendment on the coverage dashboard were the Vault types, the Loan types, and the two bridge attestations withXChainClaim. This covers all of them; the amendment now reads 109 of 109.Whether these types take a sponsor at all
Worth establishing rather than assuming, because they are exactly the types rippled forbids inside a Batch (
Batch::preflightkDisabledTxTypes). They do take one:preflight1SponsorinTransactor.cppconstrains onlyspfSponsorReserve, through the allow-list inisReserveSponsorAllowed, and no Vault or Loan type is on it. Fee sponsorship is unconstrained, which is what these tests use.What the SDK had never done
A sponsored
LoanSetcarries three signatures at once — the broker's own, the borrower'sCounterpartySignatureand the sponsor'sSponsorSignature, all over one preimage. This is the first time the composer has had to place all three in a single transaction; the multisig-counterparty work in #165 built the third section but nothing exercised it alongside a sponsor.LoanBrokerCoverWithdraw,LoanBrokerCoverClawbackandLoanManagehad never been submitted to a node by any test. Cover clawback is the asset issuer's move and rippled refuses it on a native asset (LoanBrokerCoverClawback::preclaim: "Cannot clawback native asset"), so that case needs an IOU-backed vault whose issuer is a third account.Protocol rules the runs surfaced
Two from
LoanBrokerSet::preflight, both easy to trip and now written down where the test lives:VaultIDis required even when the transaction updates a broker that already exists.LoanBrokerIDmay not carryManagementFeeRate,CoverRateMinimumorCoverRateLiquidation. Those are set once, at creation; an update carrying one istemINVALID.VaultDelete.MemoDatais deliberately left out. The field is optional on rippled's develop branch and the release build the CI stand runs answerstemDISABLEDfor it, so a test carrying it would report the stand's version rather than anything about the SDK. That leaves oneSingleAssetVaultcell red until the stand moves.Verification
TestISponsoron the dashboardXChainBridgeon the dashboardBoth totals grew between the two readings: the watched surface is derived live from the node, so a node upgrade adds cells.
Absorbs #166
#166 put
TestIXChainAttestationandTestILoanMultisiginto the default filter ofdevnet-coverage.yml. It is merged in here, withTestISponsoredVaultLoanadded alongside them, because that class arrives with this branch and the filter entry has nowhere else to live: split across two pull requests, whichever landed second left the other's classes out of the default run, and a class the default run omits feeds nothing to the dashboard. #166 is closed in favour of this one.The default filter now names ten classes, each resolving to exactly one test class in the suite.
Summary by CodeRabbit
Tests
Documentation