From dd09ec2c2f5f7e0b7e3edd082c96016a20206ed9 Mon Sep 17 00:00:00 2001 From: TimothyBanks Date: Tue, 8 Sep 2026 21:34:23 -0400 Subject: [PATCH] fix: Port smart escrow tests to new design --- .../libxrpl/helpers/TestServiceRegistry.h | 18 ------- src/tests/libxrpl/tx/wasm/README.md | 51 ++++++++----------- .../tx/wasm/transactor/BytecodeRun.cpp | 27 +++------- .../tx/wasm/transactor/BytecodeSize.cpp | 40 +++++---------- .../tx/wasm/transactor/FinishFailures.cpp | 32 ++++-------- .../libxrpl/tx/wasm/transactor/GasFees.cpp | 20 +++----- 6 files changed, 59 insertions(+), 129 deletions(-) diff --git a/src/tests/libxrpl/helpers/TestServiceRegistry.h b/src/tests/libxrpl/helpers/TestServiceRegistry.h index 451e47bc24..2b27d2d4ca 100644 --- a/src/tests/libxrpl/helpers/TestServiceRegistry.h +++ b/src/tests/libxrpl/helpers/TestServiceRegistry.h @@ -182,13 +182,6 @@ class TestServiceRegistry : public ServiceRegistry public: /** * @brief The fee settings a test environment starts with. - * - * Public and static because `TxTest` seeds its **genesis ledger** from the same values. - * The two have to agree: a transactor reads its limits from the registry - * (`EscrowCreate` checks `bytecodeSizeLimit` via `ctx.registry.get().getFees()`) but - * `Transactor::calculateBaseFee` reads `view.fees()`. Seeding them separately once left - * `gasPrice` at 1'000'000 in the registry and 0 in the view, which silently collapsed - * `EscrowFinish`'s gas-allowance fee to a single drop. */ static Fees defaultFees() @@ -514,17 +507,6 @@ public: /** * @brief Override the fee settings the transactors see. - * - * For tests about a limit rather than about a transaction: `EscrowCreate` screens - * `sfBytecode` against `bytecodeSizeLimit` from here, so a size test sets it directly - * instead of standing up fee voting. - * - * @note This writes the `Fees` fields directly and so is **not** bounded by - * `kMaxBytecodeSizeLimit` / `kMaxGasLimit`, which only constrain config parsing - * (`Config.cpp`) and `FeeVoteImpl`. Do not use it to test behaviour above those - * ceilings: no ledger can reach such a configuration, so any expectation set there - * is unfalsifiable in production. Prefer `TxTest`'s constructor when the whole - * environment wants one fee set, so the view and the registry stay in step. */ void setFees(Fees const& fees) diff --git a/src/tests/libxrpl/tx/wasm/README.md b/src/tests/libxrpl/tx/wasm/README.md index 75a25acf45..46ef6379ff 100644 --- a/src/tests/libxrpl/tx/wasm/README.md +++ b/src/tests/libxrpl/tx/wasm/README.md @@ -1,7 +1,7 @@ # WASM host-function tests — layering -These tests are deliberately **layered**: each layer isolates one thing, so a failure points at -one place instead of "somewhere in the stack." If a folder looks thin, the breadth it seems to be +These tests are deliberately **layered**: each isolates one thing, so a failure points at one +place instead of "somewhere in the stack." If a folder looks thin, the breadth it seems to be missing lives in a sibling layer. ## The layers @@ -30,32 +30,31 @@ Run the C++ side with: | **No GTest** — the `xrpl.testkit.wasm` library | `WasmLedger` (real genesis ledger + the real host over it), `WasmRun` (WAT assembler), `NftSetup`, `FloatConstants` | | **GTest** → `xrpl_tests` | `RealHostFixture` (`: testing::Test, WasmLedger` + `expectValue`/`expectError`/`expectKeyletMatches`), `FloatFixture`, `NFTFixture`, `MockHostFunctions`, `WasmFixture`, `RealVmTest`, `HostContextFixture`, `EscrowWasm` (transactor contracts + fee arithmetic), `ModuleBuilder` | -A benchmark wants a ledger and a host, not GTest's lifecycle. Both binaries link the library; +The split exists because a benchmark wants a ledger and a host, not GTest's lifecycle: `xrpl.bench.wasm` links no GTest and no GMock at all. Setup steps in `WasmLedger` and `NftSetup` **throw** (`fixtureFailed`) rather than using `EXPECT_`. -Not stylistic: an `EXPECT_` outside a running test is recorded and discarded, so a benchmark whose -escrow was never created would still run its host call, take the not-found path, and report a -cheap, plausible, completely wrong price. **If you add a setup step that can fail, throw.** +An `EXPECT_` outside a running test is recorded and discarded, so a benchmark whose escrow was +never created would still run its host call, take the not-found path, and report a cheap, +plausible, completely wrong price. **If you add a setup step that can fail, throw.** ## Gas calibration -The benchmarks that price these host functions live in `src/benchmarks/libxrpl/wasm/`, mirroring -this tree one file per function, and have their own README. They link `xrpl.testkit.wasm` (above) -for the ledger and host, and no test framework. +The benchmarks pricing these host functions live in `src/benchmarks/libxrpl/wasm/`, mirroring this +tree one file per function, with their own README. ## What `e2e/` covers — the rule **`e2e/` covers every marshalling shape and cross-call convention exactly once. It does not cover every function.** That is a completeness claim on the axis e2e uniquely tests, not a sample. -`host_calls` pins what the bridge _asks_ with a _canned_ answer; `host_functions` pins what the -real impl _answers_. The type system guarantees they agree on signatures. Nothing guarantees they +`host_calls` pins what the bridge _asks_ with a canned answer; `host_functions` pins what the real +impl _answers_. The type system guarantees they agree on signatures, but nothing guarantees they agree on **conventions** — units, endianness, buffer layout — because in neither test does a real -guest write bytes a real host reads. That is exactly the `seq`-as-little-endian-region bug: every -internal test passed, and it was caught by cross-checking the guest SDK. +guest write bytes a real host reads. That is the `seq`-as-little-endian-region bug: every internal +test passed, and it was caught by cross-checking the guest SDK. -Convention mismatch is a property of a call's **shape**, not of the function. All 19 keylets share +Convention mismatch is a property of a call's **shape**, not of the function — all 19 keylets share one shape, so a 19th keylet e2e proves nothing the 1st did. The inventory is meant to be exhaustive: | Shape / convention | Covered by | Why it is its own row | @@ -76,25 +75,17 @@ breadth lives in `host_functions/` and `host_calls/`, one case each. ## Out of scope -**The guest SDK** (`xrpl-std` / `xrpl-escrow`, external `xrpl-wasm-stdlib` repo) is not exercised -here — that is the SDK repo's own suite. These tests hand-write the ABI in WAT (raw imports, -literal field codes, hand-built byte layouts), deliberately bypassing all SDK code. Agreement is -verified _transitively_: the SDK repo tests the SDK against the ABI spec, this repo tests the host -against the same spec. That would not catch a drift where both diverge on an ambiguous point; -closing it needs a **cross-repo integration test** (compiled guests against a real host) in CI -where the Rust→wasm toolchain exists. +**The guest SDK** (`xrpl-std` / `xrpl-escrow`, external `xrpl-wasm-stdlib` repo) is the SDK repo's +own suite. These tests hand-write the ABI in WAT (raw imports, literal field codes, hand-built byte +layouts), deliberately bypassing all SDK code. Agreement is verified _transitively_: the SDK repo +tests the SDK against the ABI spec, this repo tests the host against the same spec. That would not +catch a drift where both diverge on an ambiguous point; closing it needs a **cross-repo integration +test** (compiled guests against a real host) in CI, where the Rust→wasm toolchain exists. -**Transactor-level (L5) tests** now live in `transactor/`, over `TxTest` — which runs the real -pipeline (preflight → preclaim → doApply → invariants) without needing an `Application`. They -cover what the earlier `EscrowSmart_test.cpp` did on Beast: `set_data` persistence through a -`tecBYTECODE_REJECTED`, `sfGasUsed` / `sfVMReturnCode` in metadata, owner-reserve accounting for -a bytecode-bearing escrow, the `bytecodeSizeLimit` boundary, and the gas-allowance fee. +## Adding to `transactor/` -Two things to know before adding to that folder: +Things to know: -- **Metadata only exists after `close()`.** `ApplyStateTable::apply` builds it for a view that is - not open, so `TxResult::metadata` from `submit` is always `nullopt`. The idiom is submit → - `close()` → `TxTest::getMetadata(txId)`. - **Fees live in two places and must agree.** A transactor reads its limits from the service registry (`ctx.registry.get().getFees()`), while `calculateBaseFee` reads `view.fees()`. Pass a `Fees` to `TxTest`'s constructor to set both; reach for `getServiceRegistry().setFees` only when diff --git a/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp b/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp index 4589709b1d..31d2d87942 100644 --- a/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp +++ b/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp @@ -23,16 +23,12 @@ namespace xrpl::test { namespace { -// A contract deciding whether an escrow releases, end to end through the transactor. -// -// The other transactor files are about refusals. This one is about the feature working: a -// predicate over ledger state that is false, then true, with the escrow surviving the -// rejections and being destroyed on approval. +// A contract deciding whether an escrow releases, end to end through the transactor. The +// other transactor files are about refusals; this one is about the feature working. constexpr std::uint32_t kAllowance = 10'000; -// The preimage-sha256 condition/fulfillment pair the Beast suite used. Copied rather than -// shared because `src/test/jtx` is not linked here, and they are inert constants. +// A preimage-sha256 pair, copied from jtx because `src/test/jtx` is not linked here. constexpr auto kFulfillment = std::array{{0xA0, 0x02, 0x80, 0x00}}; constexpr auto kCondition = std::array{ {0xA0, 0x25, 0x80, 0x20, 0xE3, 0xB0, 0xC4, 0x42, 0x98, 0xFC, 0x1C, 0x14, 0x9A, @@ -120,15 +116,14 @@ struct BytecodeRun : testing::Test } }; -// The whole point of a programmable escrow: it refuses while its condition is false, and -// releases once the ledger makes it true — without anyone resubmitting anything different. +// The whole point: it refuses while its predicate is false and releases once the ledger +// makes it true, with nothing resubmitted differently. TEST_F(BytecodeRun, AContractRejectsUntilItsConditionHoldsThenReleases) { auto const threshold = currentSeq() + 3; auto const wasm = assembleWat(gatedOnLedgerSqn(threshold)); auto const created = createEscrow(wasm); - // Below the threshold: rejected, and the escrow survives to be tried again. ASSERT_LT(currentSeq(), threshold); auto const rejected = finish(created.seq); EXPECT_EQ(rejected.ter, tecBYTECODE_REJECTED); @@ -141,7 +136,6 @@ TEST_F(BytecodeRun, AContractRejectsUntilItsConditionHoldsThenReleases) while (currentSeq() < threshold) env.close(); - // At the threshold: approved, and the escrow is gone. auto const approved = finish(created.seq); EXPECT_EQ(approved.ter, tesSUCCESS); EXPECT_FALSE(escrowExists(created.seq)) << "an approved escrow must be destroyed"; @@ -153,7 +147,6 @@ TEST_F(BytecodeRun, AContractRejectsUntilItsConditionHoldsThenReleases) EXPECT_TRUE(meta.isFieldPresent(sfGasUsed)); } -// The reserve a contract costs is released with it. TEST_F(BytecodeRun, TheBytecodeReserveIsHeldWhileTheEscrowLivesAndReleasedWhenItGoes) { EXPECT_EQ(env.getOwnerCount(alice), 0U); @@ -162,8 +155,7 @@ TEST_F(BytecodeRun, TheBytecodeReserveIsHeldWhileTheEscrowLivesAndReleasedWhenIt auto const wasm = assembleWat(gatedOnLedgerSqn(threshold)); auto const created = createEscrow(wasm); - // One increment for the escrow, plus one per 500 bytes of contract - // (`calculateAdditionalReserve`). + // `calculateAdditionalReserve`: one increment for the escrow, plus one per 500 bytes. auto const expected = 1U + static_cast(wasm.size() / 500); EXPECT_EQ(env.getOwnerCount(alice), expected); @@ -174,8 +166,6 @@ TEST_F(BytecodeRun, TheBytecodeReserveIsHeldWhileTheEscrowLivesAndReleasedWhenIt EXPECT_EQ(env.getOwnerCount(alice), 0U); } -// Creating a contract-bearing escrow costs the escrowed amount plus the fee, and the -// destination is untouched until it releases. TEST_F(BytecodeRun, CreatingChargesTheAmountAndTheFee) { auto const before = env.getXrpBalance(alice); @@ -187,8 +177,8 @@ TEST_F(BytecodeRun, CreatingChargesTheAmountAndTheFee) EXPECT_EQ(env.getXrpBalance(carol), XRP(5'000)); } -// A condition and a contract are both gates, and the condition is the outer one: without a -// fulfillment the contract is never reached, even though it would have approved. +// The condition is the outer gate: without a fulfillment the contract is never reached, even +// though it would have approved. TEST_F(BytecodeRun, AConditionIsCheckedBeforeTheContractRuns) { auto const threshold = currentSeq() + 2; @@ -201,7 +191,6 @@ TEST_F(BytecodeRun, AConditionIsCheckedBeforeTheContractRuns) EXPECT_EQ(finish(created.seq).ter, tecCRYPTOCONDITION_ERROR); EXPECT_TRUE(escrowExists(created.seq)); - // With the fulfillment, both gates open. auto const approved = finish(created.seq, /*withFulfillment*/ true); EXPECT_EQ(approved.ter, tesSUCCESS); EXPECT_FALSE(escrowExists(created.seq)); diff --git a/src/tests/libxrpl/tx/wasm/transactor/BytecodeSize.cpp b/src/tests/libxrpl/tx/wasm/transactor/BytecodeSize.cpp index bfe05ca611..14bd94029e 100644 --- a/src/tests/libxrpl/tx/wasm/transactor/BytecodeSize.cpp +++ b/src/tests/libxrpl/tx/wasm/transactor/BytecodeSize.cpp @@ -21,9 +21,7 @@ namespace xrpl::test { namespace { -// What a contract of a given size costs to submit: ten base fees plus five drops a byte -// (`EscrowCreate::calculateBaseFee`). Paying it exactly keeps a size test failing on the -// size rather than on the fee. +// `EscrowCreate::calculateBaseFee`: ten base fees plus five drops a byte. XRPAmount createFee(TxTest const& env, Bytes const& bytecode) { @@ -44,9 +42,7 @@ createEscrowWith(TxTest& env, Account const& account, Bytes const& bytecode) return env.submit(builder, account, createFee(env, bytecode)).ter; } -// An account rich enough for the owner reserve a large contract demands: one increment per -// 500 bytes (`calculateAdditionalReserve`), so 200 KB costs 401 increments — 802 XRP at the -// default 2 XRP increment. +// Rich enough for the owner reserve a 200 KB contract demands: 401 increments, 802 XRP. Account fundedAccount(TxTest& env) { @@ -57,13 +53,10 @@ fundedAccount(TxTest& env) } // namespace -// The transactor screens `sfBytecode` against `bytecodeSizeLimit` before the module ever -// reaches the engine. These pin that boundary, which is the one limit standing between an -// attacker-chosen module size and the *unmetered* work of compiling it: nothing charges for -// compilation, so size is the only thing bounding it. +// The transactor screens `sfBytecode` against `bytecodeSizeLimit` before the module reaches +// the engine. Compilation is unmetered, so that limit is the only thing bounding it. -// The sweep's own footing: the builders have to produce something the engine accepts, or -// every "too big" result below would be indistinguishable from "malformed". +// Footing for the rest: without this, "too big" and "malformed" are indistinguishable. TEST(BytecodeSize, TheBuildersProduceAModuleTheEngineAccepts) { TxTest env; @@ -95,9 +88,8 @@ TEST(BytecodeSize, AModuleOverTheLimitIsRefused) EXPECT_EQ(createEscrowWith(env, alice, wasm), temMALFORMED); } -// The size that counts is the module's, not the code's: a module made large by a data -// segment is screened the same way, so the limit cannot be walked around by moving the -// bulk out of the code section. +// The limit is on the module, not the code section — moving the bulk into a data segment +// does not walk around it. TEST(BytecodeSize, ADataSegmentCountsTowardTheLimit) { TxTest env; @@ -109,8 +101,8 @@ TEST(BytecodeSize, ADataSegmentCountsTowardTheLimit) EXPECT_EQ(createEscrowWith(env, alice, wasm), temMALFORMED); } -// The limit is a fee setting, so it moves. Raising it has to actually admit the module it -// now covers — otherwise some *other* cap is really in charge and the setting is decorative. +// The limit is a fee setting, so it moves. If raising it admits nothing new, some other cap +// is really in charge. TEST(BytecodeSize, RaisingTheLimitAdmitsALargerModule) { auto fees = TestServiceRegistry::defaultFees(); @@ -125,17 +117,9 @@ TEST(BytecodeSize, RaisingTheLimitAdmitsALargerModule) EXPECT_EQ(createEscrowWith(env, alice, wasm), tesSUCCESS); } -// `bytecodeSizeLimit` is the **only** thing bounding how much there is to compile. -// -// wasmparser defines `MAX_WASM_FUNCTION_SIZE` = 128 KiB, so it would be reasonable to -// assume a single function body is separately capped and that the size limit is a -// belt-and-braces second line. It is not: nothing on this path enforces that constant, and -// a lone body of a million instructions is accepted. Since compilation is unmetered — no -// gas is charged for it, and it happens once to screen the `EscrowCreate` and again on -// every `EscrowFinish` — the size limit is load-bearing on its own. -// -// If this ever starts failing, a second cap has appeared: good news, but the sweep above -// stops being the whole story and this comment is wrong. +// wasmparser defines `MAX_WASM_FUNCTION_SIZE` = 128 KiB, but nothing on this path enforces +// it: a lone body of a million instructions is accepted. So `bytecodeSizeLimit` really is +// the only bound, with no second line behind it. A failure here means one has appeared. TEST(BytecodeSize, ASingleFunctionBodyIsNotSeparatelyCapped) { TxTest const env; diff --git a/src/tests/libxrpl/tx/wasm/transactor/FinishFailures.cpp b/src/tests/libxrpl/tx/wasm/transactor/FinishFailures.cpp index fe00a72f7f..c0c061d4d9 100644 --- a/src/tests/libxrpl/tx/wasm/transactor/FinishFailures.cpp +++ b/src/tests/libxrpl/tx/wasm/transactor/FinishFailures.cpp @@ -22,9 +22,9 @@ namespace xrpl::test { namespace { -// The ways an `EscrowFinish` against a contract-bearing escrow fails, and what each one -// reports. The distinctions matter to a client: a rejection, a fault, and running out of gas -// are three different outcomes, and only one of them carries a return code. +// The ways an `EscrowFinish` against a contract-bearing escrow fails, and what each reports. +// A rejection, a fault, and running out of gas are three outcomes; only one carries a return +// code. struct FinishFailures : testing::Test { TxTest env; @@ -54,7 +54,6 @@ struct FinishFailures : testing::Test return seq; } - // An escrow with no contract at all, for the "gas without bytecode" case. std::uint32_t createPlainEscrow() { @@ -63,8 +62,8 @@ struct FinishFailures : testing::Test auto const now = static_cast(env.getCloseTime().time_since_epoch().count()); auto builder = transactions::EscrowCreateBuilder{alice, carol, STAmount{XRP(500)}}; - // A contract-free escrow needs a `FinishAfter` or a condition — `CancelAfter` alone - // is `temMALFORMED`, because nothing would ever release it. + // A contract-free escrow needs a `FinishAfter` or a condition; `CancelAfter` alone is + // `temMALFORMED`. builder.setFinishAfter(now + 1); builder.setCancelAfter(now + 1'000); @@ -105,8 +104,7 @@ TEST_F(FinishFailures, FinishIsRefusedWhileSmartEscrowIsDisabled) EXPECT_EQ(disabled.submit(builder, carol, XRPAmount{100'000}).ter, temDISABLED); } -// The allowance is bounded by the voted gas limit, so a contract cannot buy unbounded -// execution by simply asking for it. +// Execution cannot be bought unbounded just by asking for it. TEST_F(FinishFailures, AnAllowancePastTheGasLimitIsRefused) { auto fees = TestServiceRegistry::defaultFees(); @@ -119,9 +117,7 @@ TEST_F(FinishFailures, AnAllowancePastTheGasLimitIsRefused) EXPECT_EQ(env.submit(builder, carol, XRPAmount{10'000'000}).ter, temBAD_LIMIT); } -// A zero gas limit turns the runtime off. The old Beast test had to hand-insert an escrow -// ledger entry to reach this, because jtx cannot change its config mid-test; here the escrow -// is created normally and the limit drops afterwards. +// A zero gas limit turns the runtime off. TEST_F(FinishFailures, AZeroGasLimitDisablesFinishing) { auto const seq = createEscrow(kReadsLedgerSqn); @@ -153,7 +149,7 @@ TEST_F(FinishFailures, AZeroAllowanceIsRefused) EXPECT_EQ(env.submit(builder, carol, XRPAmount{100'000}).ter, temBAD_LIMIT); } -// The allowance is paid for up front, so under-paying is caught before anything runs. +// The allowance is paid up front, so under-paying is caught before anything runs. TEST_F(FinishFailures, AFeeThatDoesNotCoverTheAllowanceIsRefused) { auto const seq = createEscrow(kReadsLedgerSqn); @@ -163,7 +159,6 @@ TEST_F(FinishFailures, AFeeThatDoesNotCoverTheAllowanceIsRefused) EXPECT_EQ(finish(seq, kAllowance, fee).ter, telINSUF_FEE_P); } -// Gas on an escrow that has no contract: the transaction is about a thing that isn't there. TEST_F(FinishFailures, GasAgainstAnEscrowWithoutBytecodeIsRefused) { auto const seq = createPlainEscrow(); @@ -172,12 +167,8 @@ TEST_F(FinishFailures, GasAgainstAnEscrowWithoutBytecodeIsRefused) EXPECT_EQ(finish(seq, kAllowance, escrowFinishFee(env, kAllowance)).ter, tefNO_BYTECODE); } -// Running out of gas: essentially the whole allowance is consumed, and there is no return -// code because the contract never reached a return. -// -// "Essentially" because the meter stops at the last instruction it could afford, which for -// this loop leaves a few units unspent — the reported figure is what was really burned, not -// the allowance rounded up. The band is what distinguishes this from a trap, which stops +// A band rather than an equality: the meter stops at the last instruction it could afford, +// leaving a few units unspent. That band is what separates this from a trap, which stops // early and reports a small fraction. TEST_F(FinishFailures, RunningOutOfGasConsumesEssentiallyTheWholeAllowanceAndReportsNoReturnCode) { @@ -198,8 +189,7 @@ TEST_F(FinishFailures, RunningOutOfGasConsumesEssentiallyTheWholeAllowanceAndRep EXPECT_FALSE(meta.isFieldPresent(sfVMReturnCode)); } -// A trap is a fault, not a rejection: it reports the gas actually burned — less than the -// whole allowance, which is what distinguishes it from running out — and no return code. +// A trap is a fault, not a rejection: gas actually burned, and no return code. TEST_F(FinishFailures, ATrapReportsPartialGasAndNoReturnCode) { auto const seq = createEscrow(kTraps); diff --git a/src/tests/libxrpl/tx/wasm/transactor/GasFees.cpp b/src/tests/libxrpl/tx/wasm/transactor/GasFees.cpp index b429bf5675..a275a601e5 100644 --- a/src/tests/libxrpl/tx/wasm/transactor/GasFees.cpp +++ b/src/tests/libxrpl/tx/wasm/transactor/GasFees.cpp @@ -17,13 +17,10 @@ namespace xrpl::test { namespace { -// The gas allowance is paid for in drops up front, and the conversion is the arithmetic most -// likely to go wrong: allowance × gasPrice is a product of two 32-bit values, so a narrow -// intermediate would wrap and let a large allowance be bought for almost nothing. These pin -// that a big allowance costs a big fee. +// allowance × gasPrice is a product of two 32-bit values, so a narrow intermediate would wrap +// and let a large allowance be bought for almost nothing. These pin that it costs a big fee. -// Close to the default gas limit of 1'000'000, so the product is as large as the transactor -// will ever be asked to compute. +// Near the default gas limit, so the product is as large as the transactor ever computes. constexpr std::uint32_t kBigAllowance = 996'433; struct GasFees : testing::Test @@ -60,9 +57,8 @@ struct GasFees : testing::Test } }; -// The fee owed dwarfs the allowance's own magnitude, so a token payment cannot cover it. If -// the product ever wrapped, this is the test that would notice: 30 drops would start -// looking sufficient. +// If the product ever wrapped, this is the test that notices: 30 drops would start looking +// sufficient. TEST_F(GasFees, ALargeAllowanceCannotBeBoughtForAFewDrops) { auto const owed = escrowFinishFee(env, kBigAllowance); @@ -76,15 +72,13 @@ TEST_F(GasFees, AFeeOneDropShortOfTheAllowanceIsRefused) EXPECT_EQ(finishPaying(escrowFinishFee(env, kBigAllowance) - XRPAmount{1}), telINSUF_FEE_P); } -// And the exact fee is sufficient — otherwise the two refusals above would prove nothing, -// since any fee at all might be being rejected. +// Otherwise the two refusals above prove nothing: any fee at all might be rejected. TEST_F(GasFees, TheExactFeeIsAccepted) { EXPECT_EQ(finishPaying(escrowFinishFee(env, kBigAllowance)), tesSUCCESS); } -// Only what the contract actually burned is charged against the allowance — asking for a -// near-limit budget does not mean spending it. +// Asking for a near-limit budget does not mean spending it. TEST_F(GasFees, OnlyTheGasActuallyUsedIsReported) { auto builder = transactions::EscrowFinishBuilder{carol, alice, escrowSeq};