finish fn result refactor - #7947
Conversation
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // Gas consumed, reported in the tx metadata whenever the engine has a | ||
| // trustworthy number: a completed run, out of gas, or a wasm fault. | ||
| std::optional<std::int64_t> const cost = | ||
| re.has_value() ? std::optional<std::int64_t>{re->cost} : re.error().cost; |
There was a problem hiding this comment.
nit
| re.has_value() ? std::optional<std::int64_t>{re->cost} : re.error().cost; | |
| re.has_value() ? re->cost : re.error().cost; |
| // should already be checked in the updateData host function | ||
| // The engine cannot spend more than it was given, and pins the cost | ||
| // to the allowance when it runs out. | ||
| if (*cost < 0 || *cost > allowance) |
There was a problem hiding this comment.
nit: Can we change the cost to be unsigned to avoid this need for checking for a negative gas cost?
| // recorded before we hit it is not trustworthy, so it is left unreported. | ||
| // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor | ||
| view_->setGasUsed(gasUsed_); | ||
| view_->setGasUsed(ter == tecINTERNAL ? std::nullopt : gasUsed_); |
There was a problem hiding this comment.
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?
2 participants
High Level Overview of Change
Makes EscrowFinish's handling of the finish function's result explicit. Three rules:
sfGasUsedwhenever 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.set_datais stored only when the function ran to completion, and it survives only on a reject, where the escrow is not deleted.sfVMReturnCode.