From 8c25df2b1d4bf06877e3a92b0222bf3fbb09d66d Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Tue, 25 Jun 2024 14:46:51 -0700 Subject: [PATCH] Cleaned up checkValid interface --- src/herder/TransactionQueue.cpp | 4 ++-- src/herder/TxSetUtils.cpp | 5 ++--- src/transactions/FeeBumpTransactionFrame.cpp | 12 ++++++------ src/transactions/FeeBumpTransactionFrame.h | 2 +- src/transactions/MutableTransactionResult.cpp | 12 ++++++++++++ src/transactions/MutableTransactionResult.h | 4 ++++ src/transactions/TransactionFrame.cpp | 11 ++++++----- src/transactions/TransactionFrame.h | 4 ++-- src/transactions/TransactionFrameBase.h | 2 +- src/transactions/test/TransactionTestFrame.cpp | 16 +++++++--------- src/transactions/test/TransactionTestFrame.h | 2 +- 11 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/herder/TransactionQueue.cpp b/src/herder/TransactionQueue.cpp index ff105136e1..31e884d6bf 100644 --- a/src/herder/TransactionQueue.cpp +++ b/src/herder/TransactionQueue.cpp @@ -405,9 +405,9 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx, mApp.getLedgerManager().getLastClosedLedgerNum() + 1; } - auto [isValid, txResult] = tx->checkValid( + auto txResult = tx->checkValid( mApp, ltx, 0, 0, getUpperBoundCloseTimeOffset(mApp, closeTime)); - if (!isValid) + if (!txResult->isSuccess()) { return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR, txResult); diff --git a/src/herder/TxSetUtils.cpp b/src/herder/TxSetUtils.cpp index 77c9820a1a..5f38b2ae50 100644 --- a/src/herder/TxSetUtils.cpp +++ b/src/herder/TxSetUtils.cpp @@ -192,17 +192,16 @@ TxSetUtils::getInvalidTxList(TxSetTransactions const& txs, Application& app, (tx->getMinSeqAge() != 0 || tx->getMinSeqLedgerGap() != 0); // Only call checkValid if we passed the seqNum check - bool checkValid = false; MutableTxResultPtr txResult{}; if (!minSeqCheckIsInvalid) { - std::tie(checkValid, txResult) = + txResult = tx->checkValid(app, ltx, lastSeq, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); releaseAssertOrThrow(txResult); } - if (minSeqCheckIsInvalid || !checkValid) + if (minSeqCheckIsInvalid || !txResult->isSuccess()) { invalidTxs.emplace_back(tx); iter = accountQueue->mTxs.erase(iter); diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp index 6fe1f91308..6c7f6e1667 100644 --- a/src/transactions/FeeBumpTransactionFrame.cpp +++ b/src/transactions/FeeBumpTransactionFrame.cpp @@ -153,7 +153,7 @@ FeeBumpTransactionFrame::checkSignature(SignatureChecker& signatureChecker, return signatureChecker.checkSignature(signers, neededWeight); } -std::pair +MutableTxResultPtr FeeBumpTransactionFrame::checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, @@ -164,7 +164,7 @@ FeeBumpTransactionFrame::checkValid(Application& app, { auto txResult = createSuccessResult(); txResult->setResultCode(txMALFORMED); - return {false, txResult}; + return txResult; } LedgerTxn ltx(ltxOuter); @@ -178,22 +178,22 @@ FeeBumpTransactionFrame::checkValid(Application& app, if (commonValid(signatureChecker, ltx, false, *txResult) != ValidationType::kFullyValid) { - return {false, txResult}; + return txResult; } if (!signatureChecker.checkAllSignaturesUsed()) { txResult->setResultCode(txBAD_AUTH_EXTRA); - return {false, txResult}; + return txResult; } - auto [res, innerTxResult] = mInnerTx->checkValidWithOptionallyChargedFee( + auto innerTxResult = mInnerTx->checkValidWithOptionallyChargedFee( app, ltx, current, false, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); auto finalTxResult = createSuccessResultWithNewInnerTx( std::move(txResult), std::move(innerTxResult), mInnerTx); - return {res, finalTxResult}; + return finalTxResult; } bool diff --git a/src/transactions/FeeBumpTransactionFrame.h b/src/transactions/FeeBumpTransactionFrame.h index 008b2e08fe..03e635e072 100644 --- a/src/transactions/FeeBumpTransactionFrame.h +++ b/src/transactions/FeeBumpTransactionFrame.h @@ -76,7 +76,7 @@ class FeeBumpTransactionFrame : public TransactionFrameBase TransactionMetaFrame& meta, MutableTxResultPtr txResult) const override; - std::pair + MutableTxResultPtr checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const override; diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 6bf5e90e71..1266c50cbd 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -305,6 +305,12 @@ MutableTransactionResult::refundSorobanFee(int64_t feeRefund, mTxResult->feeCharged -= feeRefund; } +bool +MutableTransactionResult::isSuccess() const +{ + return getResult().result.code() == txSUCCESS; +} + FeeBumpMutableTransactionResult::FeeBumpMutableTransactionResult( MutableTxResultPtr innerTxResult) : MutableTransactionResultBase(), mInnerTxResult(innerTxResult) @@ -432,4 +438,10 @@ FeeBumpMutableTransactionResult::refundSorobanFee(int64_t feeRefund, mTxResult->feeCharged -= feeRefund; } } + +bool +FeeBumpMutableTransactionResult::isSuccess() const +{ + return mTxResult->result.code() == txFEE_BUMP_INNER_SUCCESS; +} } \ No newline at end of file diff --git a/src/transactions/MutableTransactionResult.h b/src/transactions/MutableTransactionResult.h index ac50643b9b..3527f20274 100644 --- a/src/transactions/MutableTransactionResult.h +++ b/src/transactions/MutableTransactionResult.h @@ -97,6 +97,8 @@ class MutableTransactionResultBase : public NonMovableOrCopyable virtual void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) = 0; + + virtual bool isSuccess() const = 0; }; class MutableTransactionResult : public MutableTransactionResultBase @@ -128,6 +130,7 @@ class MutableTransactionResult : public MutableTransactionResultBase xdr::xvector const& getDiagnosticEvents() const override; void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) override; + bool isSuccess() const override; }; class FeeBumpMutableTransactionResult : public MutableTransactionResultBase @@ -173,5 +176,6 @@ class FeeBumpMutableTransactionResult : public MutableTransactionResultBase xdr::xvector const& getDiagnosticEvents() const override; void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) override; + bool isSuccess() const override; }; } \ No newline at end of file diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index ff53525382..dfdc55f00f 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1382,7 +1382,7 @@ TransactionFrame::removeAccountSigner(AbstractLedgerTxn& ltxOuter, } } -std::pair +MutableTxResultPtr TransactionFrame::checkValidWithOptionallyChargedFee( Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, @@ -1395,7 +1395,7 @@ TransactionFrame::checkValidWithOptionallyChargedFee( { auto txResult = createSuccessResult(); txResult->setInnermostResultCode(txMALFORMED); - return {false, txResult}; + return txResult; } LedgerTxn ltx(ltxOuter); @@ -1439,7 +1439,7 @@ TransactionFrame::checkValidWithOptionallyChargedFee( // checkValid on all operations as the resulting object // is only used by applications txResult->setInnermostResultCode(txFAILED); - return {false, txResult}; + return txResult; } } @@ -1449,10 +1449,11 @@ TransactionFrame::checkValidWithOptionallyChargedFee( txResult->setInnermostResultCode(txBAD_AUTH_EXTRA); } } - return {res, txResult}; + + return txResult; } -std::pair +MutableTxResultPtr TransactionFrame::checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index fbe596242f..97b1b0613f 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -205,11 +205,11 @@ class TransactionFrame : public TransactionFrameBase AccountID const& accountID) const; bool checkExtraSigners(SignatureChecker& signatureChecker) const; - std::pair checkValidWithOptionallyChargedFee( + MutableTxResultPtr checkValidWithOptionallyChargedFee( Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const; - std::pair + MutableTxResultPtr checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const override; diff --git a/src/transactions/TransactionFrameBase.h b/src/transactions/TransactionFrameBase.h index 41655468c9..b7553b2cae 100644 --- a/src/transactions/TransactionFrameBase.h +++ b/src/transactions/TransactionFrameBase.h @@ -44,7 +44,7 @@ class TransactionFrameBase TransactionMetaFrame& meta, MutableTxResultPtr txResult, Hash const& sorobanBasePrngSeed = Hash{}) const = 0; - virtual std::pair + virtual MutableTxResultPtr checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const = 0; diff --git a/src/transactions/test/TransactionTestFrame.cpp b/src/transactions/test/TransactionTestFrame.cpp index 145343b256..db7496c48d 100644 --- a/src/transactions/test/TransactionTestFrame.cpp +++ b/src/transactions/test/TransactionTestFrame.cpp @@ -88,17 +88,16 @@ TransactionTestFrame::apply(Application& app, AbstractLedgerTxn& ltx, return ret; } -std::pair +MutableTxResultPtr TransactionTestFrame::checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const { - auto result = mTransactionFrame->checkValid(app, ltxOuter, current, - lowerBoundCloseTimeOffset, - upperBoundCloseTimeOffset); - mTransactionTxResult = result.second; - return result; + mTransactionTxResult = mTransactionFrame->checkValid( + app, ltxOuter, current, lowerBoundCloseTimeOffset, + upperBoundCloseTimeOffset); + return mTransactionTxResult; } void @@ -122,11 +121,10 @@ TransactionTestFrame::checkValidForTesting(Application& app, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) { - bool res; - std::tie(res, mTransactionTxResult) = + mTransactionTxResult = checkValid(app, ltxOuter, current, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); - return res; + return mTransactionTxResult->isSuccess(); } bool diff --git a/src/transactions/test/TransactionTestFrame.h b/src/transactions/test/TransactionTestFrame.h index 5016fbaea1..8729a8eaef 100644 --- a/src/transactions/test/TransactionTestFrame.h +++ b/src/transactions/test/TransactionTestFrame.h @@ -61,7 +61,7 @@ class TransactionTestFrame : public TransactionFrameBase TransactionMetaFrame& meta, MutableTxResultPtr txResult, Hash const& sorobanBasePrngSeed = Hash{}) const override; - std::pair + MutableTxResultPtr checkValid(Application& app, AbstractLedgerTxn& ltxOuter, SequenceNumber current, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const override;