finish fn result refactor

[pwang200]

High Level Overview of Change

Makes EscrowFinish's handling of the finish function's result explicit. Three rules:

• Gas is reported in the transaction metadata as sfGasUsed whenever the engine reports a cost: the function ran to completion, ran out of gas, or trapped. It is not reported when the result is tecINTERNAL.
• Data written through set_data is stored only when the function ran to completion, and it survives only on a reject, where the escrow is not deleted.
• Return value of 1 or greater releases the escrow, with tesSUCCESS. 0 or negative is a reject: the escrow stays, the result is tecBYTECODE_REJECTED, and the value is reported as sfVMReturnCode.

[xrplf-ai-reviewer]

This is a clean, well-reasoned refactor of EscrowFinish's result handling that matches the three stated rules (gas reporting excludes tecINTERNAL, data persistence only on completed runs, return-code semantics for accept/reject). I traced the control flow carefully: gas is set via ctx_.setGasUsed for both the success and error paths (using the unified cost optional) before any early return, data writes now only occur after confirming re.has_value() (fixing the prior bug where partial data could be written even on trap/out-of-gas), and the tecINTERNAL override in ApplyContext::apply correctly nulls out any gas value set earlier in doApply when a tecINTERNAL is ultimately returned. The bound check *cost < 0 || *cost > allowance is a reasonable invariant check (defensive, LCOV_EXCL_LINE) consistent with the engine's contract described in the comments. Cast to std::uint32_t after the bounds check is safe. I did not find any correctness, security, or resource issues in the changed lines; the added test assertions in EscrowSmart_test.cpp look consistent with the new behavior.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[TimothyBanks]

nit

Suggested change

	re.has_value() ? std::optional<std::int64_t>{re->cost} : re.error().cost;
	re.has_value() ? re->cost : re.error().cost;

[pwang200]

thanks, fixed

[TimothyBanks]

nit: Can we change the cost to be unsigned to avoid this need for checking for a negative gas cost?

[pwang200]

I like unsigned too. But cost is int64 in the engine, that is the wasmi-host-functions branch. The change should come from there.

[TimothyBanks]

Do you think there might be some confusing behavior between this and this line above view_->setVMReturnCode(*vmReturnCode_);?

For instance there is a wasm return code but there is no gas used?

[pwang200]

Good catch. It can happen when the wasm runs fine but later logic returns tecINTERNAL.
fixed, now tecINTERNAL sets neither.

[mvadari]

Any reason you switched the order of checks here?

[pwang200]

Data is only stored when the fn ran to completion. before, on out of gas or a trap, we set the data and then discarded it. There is no behavior change.

[xrplf-ai-reviewer]

This diff tightens EscrowFinish's WASM result handling: gas is now charged for both success and failure paths (whenever the engine reports a cost), data is only persisted when the function ran to completion (fixing a prior bug where data could be written even on a failed run), and ApplyContext now suppresses both sfGasUsed and sfVMReturnCode reporting on tecINTERNAL. I traced through all branches (success/reject, out-of-gas, trap, tecINTERNAL) against the stated rules and the accompanying tests, and the logic is internally consistent — no clear correctness or security issues found in the added lines. The one thing worth a quick sanity check with the author is that the ApplyContext.cpp change is not scoped to escrow transactors; it suppresses sfGasUsed/sfVMReturnCode metadata for any transactor that sets those fields whenever ter == tecINTERNAL, which seems intentional per the comment but is worth confirming no other caller relies on the old unconditional behavior.