Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the parallel-apply return type to avoid “sentinel” failure values by returning std::optional and only constructing a success payload on successful parallel apply, addressing #5084.
Changes:
- Replaces
ParallelTxReturnValwithParallelTxSuccessValand returns it viastd::optionalfrom parallel-apply APIs. - Updates transaction/operation parallel-apply call sites to use
if (res)/*resinstead ofgetSuccess(). - Simplifies
TxParallelApplyLedgerStateresult handling by returningstd::nullopton failure.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/transactions/test/TransactionTestFrame.h | Updates test wrapper parallel-apply signature to return optional success value. |
| src/transactions/test/TransactionTestFrame.cpp | Updates implementation to forward optional success value. |
| src/transactions/TransactionMeta.h | Updates meta builder API to accept success-only parallel apply value. |
| src/transactions/TransactionMeta.cpp | Updates implementation to match new success-only type. |
| src/transactions/TransactionFrameBase.h | Renames return payload type and switches parallelApply to std::optional<ParallelTxSuccessVal>. |
| src/transactions/TransactionFrame.h | Updates TransactionFrame parallelApply override to optional success value. |
| src/transactions/TransactionFrame.cpp | Returns std::nullopt on failures and updates success checks/dereferences. |
| src/transactions/RestoreFootprintOpFrame.h | Updates parallel apply signature to return optional success value. |
| src/transactions/RestoreFootprintOpFrame.cpp | Simplifies helper result handling to return optional directly. |
| src/transactions/ParallelApplyUtils.h | Updates thread-state APIs and tx-state result API to use success-only value + optional. |
| src/transactions/ParallelApplyUtils.cpp | Removes getSuccess() assertions and returns std::nullopt on failure in takeResult. |
| src/transactions/OperationFrame.h | Updates operation parallel-apply APIs to return optional success value. |
| src/transactions/OperationFrame.cpp | Updates definitions to match optional success value return type. |
| src/transactions/InvokeHostFunctionOpFrame.h | Updates parallel apply signature to return optional success value. |
| src/transactions/InvokeHostFunctionOpFrame.cpp | Simplifies helper result handling to return optional directly. |
| src/transactions/FeeBumpTransactionFrame.h | Updates fee-bump parallelApply override to return optional success value. |
| src/transactions/FeeBumpTransactionFrame.cpp | Updates delegation to return optional directly. |
| src/transactions/ExtendFootprintTTLOpFrame.h | Updates parallel apply signature to return optional success value. |
| src/transactions/ExtendFootprintTTLOpFrame.cpp | Simplifies helper result handling to return optional directly. |
| src/ledger/LedgerManagerImpl.cpp | Updates parallel apply thread logic to handle optional success result. |
| void setLedgerChangesFromSuccessfulOp( | ||
| ThreadParallelApplyLedgerState const& threadState, | ||
| ParallelTxReturnVal const& res, uint32_t ledgerSeq); | ||
| ParallelTxSuccessVal const& res, uint32_t ledgerSeq); |
There was a problem hiding this comment.
TransactionMeta.h is not self-contained: it declares setLedgerChangesFromSuccessfulOp(...) using ThreadParallelApplyLedgerState and ParallelTxSuccessVal, but neither type is declared/included in this header. Additionally, this header uses std::optional (e.g. mSorobanReturnValue) without including <optional>. Add the needed forward declarations and include <optional> (or include the headers that define these types) so the header compiles regardless of include order.
Description
Resolves #5084
Simplifies
ParallelTxReturnValon failed TXs by returning a std::nullopt instead of a weird, half baked version of the return val.Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)