update preflight checks #6094

mvadari · 2025-12-01T18:18:40Z

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

mvadari · 2025-12-01T18:36:10Z

src/xrpld/app/tx/detail/Escrow.cpp

+        }
+    }
+
+    if (auto const err = credentials::checkFields(ctx.tx, ctx.j);


Note: changing the order of preflight checks doesn't have any impact on consensus

codecov · 2025-12-01T19:05:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.3%. Comparing base (c5d178f) to head (95d2f1e).

Additional details and impacted files

@@                 Coverage Diff                 @@
##           ripple/smart-escrow   #6094   +/-   ##
===================================================
  Coverage                 79.2%   79.3%           
===================================================
  Files                      827     827           
  Lines                    71270   71281   +11     
  Branches                  8254    8251    -3     
===================================================
+ Hits                     56474   56492   +18     
+ Misses                   14796   14789    -7

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/Escrow.cpp	`99.4% <100.0%> (+<0.1%)`	⬆️
src/xrpld/app/tx/detail/Escrow.h	`100.0% <ø> (ø)`

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR refactors the preflight validation logic for Escrow transactions by introducing a new preflightSigValidated method for EscrowCreate and reorganizing validation checks between preflight and preflightSigValidated for both EscrowCreate and EscrowFinish.

Adds preflightSigValidated method to EscrowCreate for post-signature validation
Moves ComputationAllowance and credential field validation from preflightSigValidated to preflight in EscrowFinish
Adds validation check to reject escrow creation when WASM runtime is disabled (extension_compute_limit == 0)
Improves test code quality with const-correctness improvements

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/xrpld/app/tx/detail/Escrow.h	Adds declaration of `preflightSigValidated` method to `EscrowCreate` class
src/xrpld/app/tx/detail/Escrow.cpp	Implements `preflightSigValidated` for `EscrowCreate`, adds WASM runtime disabled check, and reorganizes validation checks in `EscrowFinish`
src/test/app/EscrowSmart_test.cpp	Adds test for WASM disabled scenario and improves const-correctness of test variables

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T19:44:33Z

src/test/app/EscrowSmart_test.cpp

+        {
+            // compute limit == 0
+            Env env(
+                *this,
+                envconfig([](std::unique_ptr<Config> cfg) {
+                    cfg->FEES.extension_compute_limit = 0;  // WASM disabled
+                    return cfg;
+                }),
+                features);
+            XRPAmount const txnFees = env.current()->fees().base + 1000;
+            // create escrow
+            env.fund(XRP(5000), alice, carol);
+
+            auto const escrowCreate = escrow::create(alice, carol, XRP(500));
+
+            // 11-byte string
+            env(escrowCreate,
+                escrow::finish_function(wasmHex),
+                escrow::cancel_time(env.now() + 100s),
+                fee(txnFees),
+                ter(temINVALID));
+            env.close();
+        }


Missing test coverage for the new logic check when extension_size_limit == 0 (line 213 in Escrow.cpp). The test file includes a test for extension_compute_limit == 0 (lines 83-105) but doesn't test the scenario where extension_size_limit == 0. Consider adding a test case similar to the compute limit test to verify the behavior when size limit is set to 0.

Copilot · 2025-12-01T19:44:34Z

src/xrpld/app/tx/detail/Escrow.cpp

+        // validity of WASM code happens in `preflightSigValidated`
+        // (after the signature is checked)


The comment states "validity of WASM code happens in preflightSigValidated (after the signature is checked)", but the WASM validation via preflightEscrowWasm is actually performed in both preflight (lines 227-234) and preflightSigValidated (lines 250-257), making this comment misleading. Either update the comment to accurately reflect the implementation, or remove the redundant validation from preflight as the comment suggests.

Suggested change

// validity of WASM code happens in `preflightSigValidated`

// (after the signature is checked)

// Basic validity of WASM code is checked here in `preflight`,

// and checked again in `preflightSigValidated` (after the signature is checked)

Copilot · 2025-12-01T19:44:34Z

src/xrpld/app/tx/detail/Escrow.cpp

+        HostFunctions mock;
+        auto const re =
+            preflightEscrowWasm(code, ESCROW_FUNCTION_NAME, {}, &mock, ctx.j);
+        if (!isTesSuccess(re))
+        {
+            JLOG(ctx.j.debug()) << "EscrowCreate.FinishFunction bad WASM";
+            return re;
+        }


The WASM validation logic is duplicated between preflight (lines 227-234) and preflightSigValidated (lines 250-257). The comment on line 235 states "validity of WASM code happens in preflightSigValidated", but the same preflightEscrowWasm call with identical parameters is executed in both functions. This duplication means the WASM validation is performed twice, which is inefficient.

Consider removing the validation from preflight and keeping only the one in preflightSigValidated as intended by the comment, or clarify if there's a distinction between the two validations that isn't apparent from the code.

Suggested change

HostFunctions mock;

auto const re =

preflightEscrowWasm(code, ESCROW_FUNCTION_NAME, {}, &mock, ctx.j);

if (!isTesSuccess(re))

{

JLOG(ctx.j.debug()) << "EscrowCreate.FinishFunction bad WASM";

return re;

}

Copilot · 2025-12-01T19:44:34Z

src/xrpld/app/tx/detail/Escrow.cpp

        if (code.size() == 0 ||
+            ctx.app.config().FEES.extension_size_limit == 0 ||


[nitpick] Inconsistent handling of WASM disabled states. When extension_compute_limit == 0, the code returns temINVALID with a clear message "WASM runtime disabled by fee voting" (lines 221-225). However, when extension_size_limit == 0, it's implicitly caught by the size check on line 214 and returns temMALFORMED with message "bad size", which doesn't clearly indicate that WASM is disabled.

Consider adding an explicit check for extension_size_limit == 0 similar to the compute limit check, returning temINVALID with a descriptive message for consistency and clarity.

Suggested change

if (code.size() == 0 ||

ctx.app.config().FEES.extension_size_limit == 0 ||

if (ctx.app.config().FEES.extension_size_limit == 0)

{

JLOG(ctx.j.debug()) << "WASM runtime disabled by fee voting (size limit)";

return temINVALID;

}

if (code.size() == 0 ||

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T19:53:00Z

src/xrpld/app/tx/detail/Escrow.cpp

+
        auto const code = ctx.tx.getFieldVL(sfFinishFunction);
        if (code.size() == 0 ||
+            ctx.app.config().FEES.extension_size_limit == 0 ||


Redundant check: extension_size_limit == 0 is already checked at lines 211-216. This duplicate check on line 220 is unnecessary since if extension_size_limit is 0, the function would have already returned temINVALID above.

Suggested change

ctx.app.config().FEES.extension_size_limit == 0 ||

Copilot · 2025-12-01T19:53:01Z

src/test/app/EscrowSmart_test.cpp

+
+            auto const escrowCreate = escrow::create(alice, carol, XRP(500));
+
+            // 11-byte string


Misleading comment: The comment says "11-byte string" but the actual test data is "AA" which is only 2 bytes (or 1 byte if treated as hex). This comment should be updated or removed as it doesn't accurately describe the test input.

Suggested change

// 11-byte string

// 2-byte string

mvadari added 2 commits December 1, 2025 13:01

clean up preflight checks

f656de7

fix typo

b4e54f6

mvadari commented Dec 1, 2025

View reviewed changes

oops

823601c

add test

8851d57

mvadari requested review from Copilot, oleks-rip and pwang200 December 1, 2025 19:40

Copilot started reviewing on behalf of mvadari December 1, 2025 19:41 View session

Copilot finished reviewing on behalf of mvadari December 1, 2025 19:43

Copilot AI reviewed Dec 1, 2025

View reviewed changes

respond to copilot comments

843a641

mvadari requested a review from Copilot December 1, 2025 19:49

Copilot started reviewing on behalf of mvadari December 1, 2025 19:50 View session

Copilot finished reviewing on behalf of mvadari December 1, 2025 19:52

Copilot AI reviewed Dec 1, 2025

View reviewed changes

mvadari and others added 7 commits December 1, 2025 14:58

fix more stuff

9f17e72

fix test

d78affd

Merge branch 'ripple/smart-escrow' into update-preflight-checks

0bc5e2f

disable compute explicitly

f2cb0b2

Merge branch 'ripple/smart-escrow' into update-preflight-checks

2951e85

fix typo

8203db1

oops

95d2f1e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

update preflight checks #6094

update preflight checks #6094

Uh oh!

mvadari commented Dec 1, 2025

Uh oh!

mvadari Dec 1, 2025

Uh oh!

codecov bot commented Dec 1, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Copilot AI Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		// validity of WASM code happens in `preflightSigValidated`
		// (after the signature is checked)

		if (code.size() == 0 \|\|
		ctx.app.config().FEES.extension_size_limit == 0 \|\|


		auto const escrowCreate = escrow::create(alice, carol, XRP(500));

		// 11-byte string

update preflight checks #6094

Are you sure you want to change the base?

update preflight checks #6094

Uh oh!

Conversation

mvadari commented Dec 1, 2025

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

mvadari Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov bot commented Dec 1, 2025 •

edited

Loading