From 118c25c0f0640649fa690224db0857584929f6f2 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Mon, 16 Apr 2018 17:51:36 -0700 Subject: [PATCH] Compile time check preflight returns no tec (RIPD-1624): The six different ranges of TER codes are broken up into six different enumerations. A template class allows subsets of these enumerations to be aggregated. This technique allows verification at compile time that no TEC codes are returned before the signature is checked. Conversion between TER instance and integer is provided by named functions. This makes accidental conversion almost impossible and makes type abuse easier to spot in the code base. --- src/ripple/app/tx/applySteps.h | 4 +- src/ripple/app/tx/impl/ApplyContext.cpp | 8 +- src/ripple/app/tx/impl/CancelCheck.cpp | 4 +- src/ripple/app/tx/impl/CancelCheck.h | 2 +- src/ripple/app/tx/impl/CancelOffer.cpp | 2 +- src/ripple/app/tx/impl/CancelOffer.h | 2 +- src/ripple/app/tx/impl/CancelTicket.cpp | 2 +- src/ripple/app/tx/impl/CancelTicket.h | 2 +- src/ripple/app/tx/impl/CashCheck.cpp | 4 +- src/ripple/app/tx/impl/CashCheck.h | 2 +- src/ripple/app/tx/impl/Change.cpp | 2 +- src/ripple/app/tx/impl/Change.h | 2 +- src/ripple/app/tx/impl/CreateCheck.cpp | 4 +- src/ripple/app/tx/impl/CreateCheck.h | 2 +- src/ripple/app/tx/impl/CreateOffer.cpp | 22 +- src/ripple/app/tx/impl/CreateOffer.h | 2 +- src/ripple/app/tx/impl/CreateTicket.cpp | 2 +- src/ripple/app/tx/impl/CreateTicket.h | 2 +- src/ripple/app/tx/impl/Escrow.cpp | 6 +- src/ripple/app/tx/impl/Escrow.h | 6 +- src/ripple/app/tx/impl/PayChan.cpp | 26 +-- src/ripple/app/tx/impl/PayChan.h | 6 +- src/ripple/app/tx/impl/Payment.cpp | 2 +- src/ripple/app/tx/impl/Payment.h | 2 +- src/ripple/app/tx/impl/SetAccount.cpp | 4 +- src/ripple/app/tx/impl/SetAccount.h | 2 +- src/ripple/app/tx/impl/SetRegularKey.cpp | 2 +- src/ripple/app/tx/impl/SetRegularKey.h | 2 +- src/ripple/app/tx/impl/SetSignerList.cpp | 8 +- src/ripple/app/tx/impl/SetSignerList.h | 6 +- src/ripple/app/tx/impl/SetTrust.cpp | 2 +- src/ripple/app/tx/impl/SetTrust.h | 2 +- src/ripple/app/tx/impl/SignerEntries.cpp | 4 +- src/ripple/app/tx/impl/SignerEntries.h | 9 +- src/ripple/app/tx/impl/Transactor.cpp | 15 +- src/ripple/app/tx/impl/Transactor.h | 15 +- src/ripple/app/tx/impl/applySteps.cpp | 2 +- src/ripple/ledger/TxMeta.h | 2 +- src/ripple/ledger/impl/TxMeta.cpp | 2 +- src/ripple/ledger/impl/View.cpp | 8 +- src/ripple/protocol/TER.h | 258 +++++++++++++++++++++- src/ripple/protocol/impl/STInteger.cpp | 4 +- src/ripple/protocol/impl/STParsedJSON.cpp | 6 +- src/ripple/protocol/impl/TER.cpp | 11 +- src/ripple/rpc/Status.h | 15 +- src/test/app/DepositAuth_test.cpp | 10 +- src/test/app/Escrow_test.cpp | 21 +- src/test/app/Flow_test.cpp | 11 +- src/test/app/Offer_test.cpp | 12 +- src/test/app/PayStrand_test.cpp | 2 +- src/test/jtx/JTx.h | 2 +- src/test/jtx/impl/Env.cpp | 3 +- src/test/ledger/Invariants_test.cpp | 7 +- src/test/protocol/TER_test.cpp | 208 ++++++++++++++++- 54 files changed, 612 insertions(+), 161 deletions(-) diff --git a/src/ripple/app/tx/applySteps.h b/src/ripple/app/tx/applySteps.h index 90b83d348..b57e89a71 100644 --- a/src/ripple/app/tx/applySteps.h +++ b/src/ripple/app/tx/applySteps.h @@ -47,12 +47,12 @@ public: beast::Journal const j; /// Intermediate transaction result - TER const ter; + NotTEC const ter; /// Constructor template PreflightResult(Context const& ctx_, - TER ter_) + NotTEC ter_) : tx(ctx_.tx) , rules(ctx_.rules) , flags(ctx_.flags) diff --git a/src/ripple/app/tx/impl/ApplyContext.cpp b/src/ripple/app/tx/impl/ApplyContext.cpp index c13a9c799..59f0a07e3 100644 --- a/src/ripple/app/tx/impl/ApplyContext.cpp +++ b/src/ripple/app/tx/impl/ApplyContext.cpp @@ -104,8 +104,8 @@ ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence) [](auto const& b) { return b; })) { terResult = (terResult == tecINVARIANT_FAILED) ? - tefINVARIANT_FAILED : - tecINVARIANT_FAILED ; + TER {tefINVARIANT_FAILED} : + TER {tecINVARIANT_FAILED} ; JLOG(journal.fatal()) << "Transaction has failed one or more invariants: " << to_string(tx.getJson (0)); @@ -114,8 +114,8 @@ ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence) catch(std::exception const& ex) { terResult = (terResult == tecINVARIANT_FAILED) ? - tefINVARIANT_FAILED : - tecINVARIANT_FAILED ; + TER {tefINVARIANT_FAILED} : + TER {tecINVARIANT_FAILED} ; JLOG(journal.fatal()) << "Transaction caused an exception in an invariant" << ", ex: " << ex.what() << diff --git a/src/ripple/app/tx/impl/CancelCheck.cpp b/src/ripple/app/tx/impl/CancelCheck.cpp index bf7068043..09143c10f 100644 --- a/src/ripple/app/tx/impl/CancelCheck.cpp +++ b/src/ripple/app/tx/impl/CancelCheck.cpp @@ -28,13 +28,13 @@ namespace ripple { -TER +NotTEC CancelCheck::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled (featureChecks)) return temDISABLED; - TER const ret {preflight1 (ctx)}; + NotTEC const ret {preflight1 (ctx)}; if (! isTesSuccess (ret)) return ret; diff --git a/src/ripple/app/tx/impl/CancelCheck.h b/src/ripple/app/tx/impl/CancelCheck.h index 1c1ab3627..7e544a299 100644 --- a/src/ripple/app/tx/impl/CancelCheck.h +++ b/src/ripple/app/tx/impl/CancelCheck.h @@ -34,7 +34,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/CancelOffer.cpp b/src/ripple/app/tx/impl/CancelOffer.cpp index c7c48c118..eaa76572d 100644 --- a/src/ripple/app/tx/impl/CancelOffer.cpp +++ b/src/ripple/app/tx/impl/CancelOffer.cpp @@ -24,7 +24,7 @@ namespace ripple { -TER +NotTEC CancelOffer::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/CancelOffer.h b/src/ripple/app/tx/impl/CancelOffer.h index c471d72fb..42e9f891d 100644 --- a/src/ripple/app/tx/impl/CancelOffer.h +++ b/src/ripple/app/tx/impl/CancelOffer.h @@ -37,7 +37,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/CancelTicket.cpp b/src/ripple/app/tx/impl/CancelTicket.cpp index 7e93903a9..b3d5544e8 100644 --- a/src/ripple/app/tx/impl/CancelTicket.cpp +++ b/src/ripple/app/tx/impl/CancelTicket.cpp @@ -26,7 +26,7 @@ namespace ripple { -TER +NotTEC CancelTicket::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureTickets)) diff --git a/src/ripple/app/tx/impl/CancelTicket.h b/src/ripple/app/tx/impl/CancelTicket.h index 2e205024a..cdc1c4f4c 100644 --- a/src/ripple/app/tx/impl/CancelTicket.h +++ b/src/ripple/app/tx/impl/CancelTicket.h @@ -36,7 +36,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER doApply () override; diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index c5e27f9e0..526805a3e 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -31,13 +31,13 @@ namespace ripple { -TER +NotTEC CashCheck::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled (featureChecks)) return temDISABLED; - TER const ret {preflight1 (ctx)}; + NotTEC const ret {preflight1 (ctx)}; if (! isTesSuccess (ret)) return ret; diff --git a/src/ripple/app/tx/impl/CashCheck.h b/src/ripple/app/tx/impl/CashCheck.h index 4114be5d0..e3b469e36 100644 --- a/src/ripple/app/tx/impl/CashCheck.h +++ b/src/ripple/app/tx/impl/CashCheck.h @@ -34,7 +34,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/Change.cpp b/src/ripple/app/tx/impl/Change.cpp index 707cfa8b8..cb216e9af 100644 --- a/src/ripple/app/tx/impl/Change.cpp +++ b/src/ripple/app/tx/impl/Change.cpp @@ -27,7 +27,7 @@ namespace ripple { -TER +NotTEC Change::preflight (PreflightContext const& ctx) { auto const ret = preflight0(ctx); diff --git a/src/ripple/app/tx/impl/Change.h b/src/ripple/app/tx/impl/Change.h index 2a9a53f5b..47a493c71 100644 --- a/src/ripple/app/tx/impl/Change.h +++ b/src/ripple/app/tx/impl/Change.h @@ -39,7 +39,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER doApply () override; diff --git a/src/ripple/app/tx/impl/CreateCheck.cpp b/src/ripple/app/tx/impl/CreateCheck.cpp index 03d0b9179..5bddbbd46 100644 --- a/src/ripple/app/tx/impl/CreateCheck.cpp +++ b/src/ripple/app/tx/impl/CreateCheck.cpp @@ -28,13 +28,13 @@ namespace ripple { -TER +NotTEC CreateCheck::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled (featureChecks)) return temDISABLED; - TER const ret {preflight1 (ctx)}; + NotTEC const ret {preflight1 (ctx)}; if (! isTesSuccess (ret)) return ret; diff --git a/src/ripple/app/tx/impl/CreateCheck.h b/src/ripple/app/tx/impl/CreateCheck.h index 49cdfce16..805aefae0 100644 --- a/src/ripple/app/tx/impl/CreateCheck.h +++ b/src/ripple/app/tx/impl/CreateCheck.h @@ -34,7 +34,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 46dbe8c41..1bc607e21 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -38,7 +38,7 @@ CreateOffer::calculateMaxSpend(STTx const& tx) saTakerGets.xrp() : beast::zero; } -TER +NotTEC CreateOffer::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); @@ -198,7 +198,9 @@ CreateOffer::preclaim(PreclaimContext const& ctx) // // The return code change is attached to featureChecks as a convenience. // The change is not big enough to deserve its own amendment. - return ctx.view.rules().enabled(featureChecks) ? tecEXPIRED : tesSUCCESS; + return ctx.view.rules().enabled(featureChecks) + ? TER {tecEXPIRED} + : TER {tesSUCCESS}; } // Make sure that we are authorized to hold what the taker will pay us. @@ -231,8 +233,8 @@ CreateOffer::checkAcceptAsset(ReadView const& view, to_string (issue.account); return (flags & tapRETRY) - ? terNO_ACCOUNT - : tecNO_ISSUER; + ? TER {terNO_ACCOUNT} + : TER {tecNO_ISSUER}; } // This code is attached to the FlowCross amendment as a matter of @@ -250,8 +252,8 @@ CreateOffer::checkAcceptAsset(ReadView const& view, if (!trustLine) { return (flags & tapRETRY) - ? terNO_LINE - : tecNO_LINE; + ? TER {terNO_LINE} + : TER {tecNO_LINE}; } // Entries have a canonical representation, determined by a @@ -268,8 +270,8 @@ CreateOffer::checkAcceptAsset(ReadView const& view, "delay: can't receive IOUs from issuer without auth."; return (flags & tapRETRY) - ? terNO_AUTH - : tecNO_AUTH; + ? TER {terNO_AUTH} + : TER {tecNO_AUTH}; } } @@ -1073,7 +1075,7 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel) auto viewJ = ctx_.app.journal("View"); - auto result = tesSUCCESS; + TER result = tesSUCCESS; // Process a cancellation request that's passed along with an offer. if (cancelSequence) @@ -1107,7 +1109,7 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel) // The return code change is attached to featureChecks as a convenience. // The change is not big enough to deserve its own amendment. TER const ter {ctx_.view().rules().enabled( - featureChecks) ? tecEXPIRED : tesSUCCESS}; + featureChecks) ? TER {tecEXPIRED} : TER {tesSUCCESS}}; return{ ter, true }; } diff --git a/src/ripple/app/tx/impl/CreateOffer.h b/src/ripple/app/tx/impl/CreateOffer.h index 92064b0a5..93026ecc5 100644 --- a/src/ripple/app/tx/impl/CreateOffer.h +++ b/src/ripple/app/tx/impl/CreateOffer.h @@ -49,7 +49,7 @@ public: /** Enforce constraints beyond those of the Transactor base class. */ static - TER + NotTEC preflight (PreflightContext const& ctx); /** Enforce constraints beyond those of the Transactor base class. */ diff --git a/src/ripple/app/tx/impl/CreateTicket.cpp b/src/ripple/app/tx/impl/CreateTicket.cpp index a5dfd1f09..2f9fae474 100644 --- a/src/ripple/app/tx/impl/CreateTicket.cpp +++ b/src/ripple/app/tx/impl/CreateTicket.cpp @@ -26,7 +26,7 @@ namespace ripple { -TER +NotTEC CreateTicket::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureTickets)) diff --git a/src/ripple/app/tx/impl/CreateTicket.h b/src/ripple/app/tx/impl/CreateTicket.h index 57db3cad2..411307aaa 100644 --- a/src/ripple/app/tx/impl/CreateTicket.h +++ b/src/ripple/app/tx/impl/CreateTicket.h @@ -37,7 +37,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER doApply () override; diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 3a3d1b772..f14333dd9 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -92,7 +92,7 @@ EscrowCreate::calculateMaxSpend(STTx const& tx) return tx[sfAmount].xrp(); } -TER +NotTEC EscrowCreate::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureEscrow)) @@ -293,7 +293,7 @@ checkCondition (Slice f, Slice c) return validate (*fulfillment, *condition); } -TER +NotTEC EscrowFinish::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureEscrow)) @@ -503,7 +503,7 @@ EscrowFinish::doApply() //------------------------------------------------------------------------------ -TER +NotTEC EscrowCancel::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureEscrow)) diff --git a/src/ripple/app/tx/impl/Escrow.h b/src/ripple/app/tx/impl/Escrow.h index bc42c2c60..45e617daf 100644 --- a/src/ripple/app/tx/impl/Escrow.h +++ b/src/ripple/app/tx/impl/Escrow.h @@ -39,7 +39,7 @@ public: calculateMaxSpend(STTx const& tx); static - TER + NotTEC preflight (PreflightContext const& ctx); TER @@ -59,7 +59,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static @@ -83,7 +83,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index bc002cbf1..923ae0cf3 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -154,7 +154,7 @@ closeChannel ( //------------------------------------------------------------------------------ -TER +NotTEC PayChanCreate::preflight (PreflightContext const& ctx) { if (!ctx.rules.enabled (featurePayChan)) @@ -262,7 +262,7 @@ PayChanCreate::doApply() //------------------------------------------------------------------------------ -TER +NotTEC PayChanFund::preflight (PreflightContext const& ctx) { if (!ctx.rules.enabled (featurePayChan)) @@ -347,13 +347,17 @@ PayChanFund::doApply() //------------------------------------------------------------------------------ -TER +NotTEC PayChanClaim::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featurePayChan)) return temDISABLED; - bool const noTecs = ctx.rules.enabled(fix1512); + // A search through historic MainNet ledgers by the data team found no + // occurrences of a transaction with the error that fix1512 fixed. That + // means there are no old transactions with that error that we might + // need to replay. So the check for fix1512 is removed. Apr 2018. +// bool const noTecs = ctx.rules.enabled(fix1512); auto const ret = preflight1 (ctx); if (!isTesSuccess (ret)) @@ -368,12 +372,7 @@ PayChanClaim::preflight (PreflightContext const& ctx) return temBAD_AMOUNT; if (bal && amt && *bal > *amt) - { - if (noTecs) - return temBAD_AMOUNT; - else - return tecNO_PERMISSION; - } + return temBAD_AMOUNT; { auto const flags = ctx.tx.getFlags(); @@ -398,12 +397,7 @@ PayChanClaim::preflight (PreflightContext const& ctx) auto const authAmt = amt ? amt->xrp() : reqBalance; if (reqBalance > authAmt) - { - if (noTecs) - return temBAD_AMOUNT; - else - return tecNO_PERMISSION; - } + return temBAD_AMOUNT; Keylet const k (ltPAYCHAN, ctx.tx[sfPayChannel]); if (!publicKeyType(ctx.tx[sfPublicKey])) diff --git a/src/ripple/app/tx/impl/PayChan.h b/src/ripple/app/tx/impl/PayChan.h index d04687ac3..f223dcebf 100644 --- a/src/ripple/app/tx/impl/PayChan.h +++ b/src/ripple/app/tx/impl/PayChan.h @@ -35,7 +35,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static @@ -59,7 +59,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER @@ -79,7 +79,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER diff --git a/src/ripple/app/tx/impl/Payment.cpp b/src/ripple/app/tx/impl/Payment.cpp index 01308b946..eb7e7a1a5 100644 --- a/src/ripple/app/tx/impl/Payment.cpp +++ b/src/ripple/app/tx/impl/Payment.cpp @@ -44,7 +44,7 @@ Payment::calculateMaxSpend(STTx const& tx) return saDstAmount.native() ? saDstAmount.xrp() : beast::zero; } -TER +NotTEC Payment::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/Payment.h b/src/ripple/app/tx/impl/Payment.h index de925acee..66a9d688a 100644 --- a/src/ripple/app/tx/impl/Payment.h +++ b/src/ripple/app/tx/impl/Payment.h @@ -49,7 +49,7 @@ public: calculateMaxSpend(STTx const& tx); static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index e5a30177c..dfff835a5 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -48,7 +48,7 @@ SetAccount::affectsSubsequentTransactionAuth(STTx const& tx) *uClearFlag == asfAccountTxnID); } -TER +NotTEC SetAccount::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); @@ -190,7 +190,7 @@ SetAccount::preclaim(PreclaimContext const& ctx) keylet::ownerDir(id))) { JLOG(ctx.j.trace()) << "Retry: Owner directory not empty."; - return (ctx.flags & tapRETRY) ? terOWNERS : tecOWNERS; + return (ctx.flags & tapRETRY) ? TER {terOWNERS} : TER {tecOWNERS}; } } diff --git a/src/ripple/app/tx/impl/SetAccount.h b/src/ripple/app/tx/impl/SetAccount.h index af3df2e27..83eaacbf2 100644 --- a/src/ripple/app/tx/impl/SetAccount.h +++ b/src/ripple/app/tx/impl/SetAccount.h @@ -45,7 +45,7 @@ public: affectsSubsequentTransactionAuth(STTx const& tx); static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index 83d7cc6da..d3e269086 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -48,7 +48,7 @@ SetRegularKey::calculateBaseFee ( return Transactor::calculateBaseFee (ctx); } -TER +NotTEC SetRegularKey::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/SetRegularKey.h b/src/ripple/app/tx/impl/SetRegularKey.h index a7b536a77..668be7488 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.h +++ b/src/ripple/app/tx/impl/SetRegularKey.h @@ -44,7 +44,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index 758515ced..1773be349 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -35,7 +35,7 @@ namespace ripple { // setting the sfSignerListID to zero in all cases. static std::uint32_t const defaultSignerListID_ = 0; -std::tuple, SetSignerList::Operation> SetSignerList::determineOperation(STTx const& tx, @@ -69,7 +69,7 @@ SetSignerList::determineOperation(STTx const& tx, return std::make_tuple(tesSUCCESS, quorum, sign, op); } -TER +NotTEC SetSignerList::preflight (PreflightContext const& ctx) { if (! ctx.rules.enabled(featureMultiSign)) @@ -95,7 +95,7 @@ SetSignerList::preflight (PreflightContext const& ctx) { // Validate our settings. auto const account = ctx.tx.getAccountID(sfAccount); - TER const ter = + NotTEC const ter = validateQuorumAndSignerEntries(std::get<1>(result), std::get<2>(result), account, ctx.j); if (ter != tesSUCCESS) @@ -141,7 +141,7 @@ SetSignerList::preCompute() return Transactor::preCompute(); } -TER +NotTEC SetSignerList::validateQuorumAndSignerEntries ( std::uint32_t quorum, std::vector const& signers, diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h index c6f9cb563..4c2d35f26 100644 --- a/src/ripple/app/tx/impl/SetSignerList.h +++ b/src/ripple/app/tx/impl/SetSignerList.h @@ -61,7 +61,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); TER doApply () override; @@ -69,14 +69,14 @@ public: private: static - std::tuple, Operation> determineOperation(STTx const& tx, ApplyFlags flags, beast::Journal j); static - TER validateQuorumAndSignerEntries ( + NotTEC validateQuorumAndSignerEntries ( std::uint32_t quorum, std::vector const& signers, AccountID const& account, diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index 5e801a311..a34922a5d 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -27,7 +27,7 @@ namespace ripple { -TER +NotTEC SetTrust::preflight (PreflightContext const& ctx) { auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/SetTrust.h b/src/ripple/app/tx/impl/SetTrust.h index 9ec7936e6..c804bfdca 100644 --- a/src/ripple/app/tx/impl/SetTrust.h +++ b/src/ripple/app/tx/impl/SetTrust.h @@ -38,7 +38,7 @@ public: } static - TER + NotTEC preflight (PreflightContext const& ctx); static diff --git a/src/ripple/app/tx/impl/SignerEntries.cpp b/src/ripple/app/tx/impl/SignerEntries.cpp index f1e0a3ed5..e15634a9f 100644 --- a/src/ripple/app/tx/impl/SignerEntries.cpp +++ b/src/ripple/app/tx/impl/SignerEntries.cpp @@ -25,11 +25,11 @@ namespace ripple { -std::pair, TER> +std::pair, NotTEC> SignerEntries::deserialize ( STObject const& obj, beast::Journal journal, std::string const& annotation) { - std::pair, TER> s; + std::pair, NotTEC> s; if (!obj.isFieldPresent (sfSignerEntries)) { diff --git a/src/ripple/app/tx/impl/SignerEntries.h b/src/ripple/app/tx/impl/SignerEntries.h index 6a41f8f44..fab916039 100644 --- a/src/ripple/app/tx/impl/SignerEntries.h +++ b/src/ripple/app/tx/impl/SignerEntries.h @@ -20,9 +20,10 @@ #ifndef RIPPLE_TX_IMPL_SIGNER_ENTRIES_H_INCLUDED #define RIPPLE_TX_IMPL_SIGNER_ENTRIES_H_INCLUDED -#include // STTx::maxMultiSigners -#include // AccountID -#include // temMALFORMED +#include // NotTEC +#include // STTx::maxMultiSigners +#include // AccountID +#include // temMALFORMED #include // beast::Journal namespace ripple { @@ -60,7 +61,7 @@ public: // Deserialize a SignerEntries array from the network or from the ledger. static - std::pair, TER> + std::pair, NotTEC> deserialize ( STObject const& obj, beast::Journal journal, diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index c56c80019..e6a2e1faa 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -35,7 +35,7 @@ namespace ripple { /** Performs early sanity checks on the txid */ -TER +NotTEC preflight0(PreflightContext const& ctx) { auto const txID = ctx.tx.getTransactionID(); @@ -51,7 +51,7 @@ preflight0(PreflightContext const& ctx) } /** Performs early sanity checks on the account and fee fields */ -TER +NotTEC preflight1 (PreflightContext const& ctx) { auto const ret = preflight0(ctx); @@ -85,7 +85,7 @@ preflight1 (PreflightContext const& ctx) } /** Checks whether the signature appears valid */ -TER +NotTEC preflight2 (PreflightContext const& ctx) { if(!( ctx.flags & tapNO_CHECK_SIGN)) @@ -226,7 +226,7 @@ TER Transactor::payFee () return tesSUCCESS; } -TER +NotTEC Transactor::checkSeq (PreclaimContext const& ctx) { auto const id = ctx.tx.getAccountID(sfAccount); @@ -327,7 +327,7 @@ TER Transactor::apply () return doApply (); } -TER +NotTEC Transactor::checkSign (PreclaimContext const& ctx) { // Make sure multisigning is enabled before we check for multisignatures. @@ -341,7 +341,7 @@ Transactor::checkSign (PreclaimContext const& ctx) return checkSingleSign (ctx); } -TER +NotTEC Transactor::checkSingleSign (PreclaimContext const& ctx) { auto const id = ctx.tx.getAccountID(sfAccount); @@ -390,7 +390,8 @@ Transactor::checkSingleSign (PreclaimContext const& ctx) return tesSUCCESS; } -TER Transactor::checkMultiSign (PreclaimContext const& ctx) +NotTEC +Transactor::checkMultiSign (PreclaimContext const& ctx) { auto const id = ctx.tx.getAccountID(sfAccount); // Get mTxnAccountID's SignerList and Quorum. diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index 105688945..9ea5c7b0b 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/src/ripple/app/tx/impl/Transactor.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_APP_TX_TRANSACTOR_H_INCLUDED #define RIPPLE_APP_TX_TRANSACTOR_H_INCLUDED +#include #include #include #include @@ -116,7 +117,7 @@ public: */ static - TER + NotTEC checkSeq (PreclaimContext const& ctx); static @@ -124,7 +125,7 @@ public: checkFee (PreclaimContext const& ctx, std::uint64_t baseFee); static - TER + NotTEC checkSign (PreclaimContext const& ctx); // Returns the fee in fee units, not scaled for load. @@ -173,20 +174,20 @@ private: void setSeq (); TER payFee (); void claimFee (XRPAmount& fee, TER terResult, std::vector const& removedOffers); - static TER checkSingleSign (PreclaimContext const& ctx); - static TER checkMultiSign (PreclaimContext const& ctx); + static NotTEC checkSingleSign (PreclaimContext const& ctx); + static NotTEC checkMultiSign (PreclaimContext const& ctx); }; /** Performs early sanity checks on the txid */ -TER +NotTEC preflight0(PreflightContext const& ctx); /** Performs early sanity checks on the account and fee fields */ -TER +NotTEC preflight1 (PreflightContext const& ctx); /** Checks whether the signature appears valid */ -TER +NotTEC preflight2 (PreflightContext const& ctx); } diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index b64b5959d..5a28a3011 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/src/ripple/app/tx/impl/applySteps.cpp @@ -38,7 +38,7 @@ namespace ripple { static -TER +NotTEC invoke_preflight (PreflightContext const& ctx) { switch(ctx.tx.getTxnType()) diff --git a/src/ripple/ledger/TxMeta.h b/src/ripple/ledger/TxMeta.h index 2e7fec586..6566e5ec9 100644 --- a/src/ripple/ledger/TxMeta.h +++ b/src/ripple/ledger/TxMeta.h @@ -87,7 +87,7 @@ public: } TER getResultTER () const { - return static_cast (mResult); + return TER::fromInt (mResult); } std::uint32_t getIndex () const { diff --git a/src/ripple/ledger/impl/TxMeta.cpp b/src/ripple/ledger/impl/TxMeta.cpp index cac23a062..76f10cbee 100644 --- a/src/ripple/ledger/impl/TxMeta.cpp +++ b/src/ripple/ledger/impl/TxMeta.cpp @@ -257,7 +257,7 @@ STObject TxMeta::getAsObject () const void TxMeta::addRaw (Serializer& s, TER result, std::uint32_t index) { - mResult = static_cast (result); + mResult = TERtoInt (result); mIndex = index; assert ((mResult == 0) || ((mResult > 100) && (mResult <= 255))); diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 88978f189..79ab3a534 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1581,8 +1581,8 @@ accountSend (ApplyView& view, // VFALCO Its laborious to have to mutate the // TER based on params everywhere terResult = view.open() - ? telFAILED_PROCESSING - : tecFAILED_PROCESSING; + ? TER {telFAILED_PROCESSING} + : TER {tecFAILED_PROCESSING}; } else { @@ -1847,8 +1847,8 @@ transferXRP (ApplyView& view, // mutating these TER everywhere // FIXME: this logic should be moved to callers maybe? return view.open() - ? telFAILED_PROCESSING - : tecFAILED_PROCESSING; + ? TER {telFAILED_PROCESSING} + : TER {tecFAILED_PROCESSING}; } // Decrement XRP balance. diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 791221bd8..5af438c42 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. + Copyright (c) 2012 - 2018 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above @@ -20,7 +20,10 @@ #ifndef RIPPLE_PROTOCOL_TER_H_INCLUDED #define RIPPLE_PROTOCOL_TER_H_INCLUDED +#include + #include +#include #include namespace ripple { @@ -30,9 +33,13 @@ namespace ripple { // "Transaction Engine Result" // or Transaction ERror. // -enum TER +using TERUnderlyingType = int; + +//------------------------------------------------------------------------------ + +enum TELcodes : TERUnderlyingType { - // Note: Range is stable. Exact numbers are currently unstable. Use tokens. + // Note: Range is stable. Exact numbers are unstable. Use tokens. // -399 .. -300: L Local error (transaction fee inadequate, exceeds local limit) // Only valid during non-consensus processing. @@ -51,7 +58,14 @@ enum TER telCAN_NOT_QUEUE_BLOCKS, telCAN_NOT_QUEUE_BLOCKED, telCAN_NOT_QUEUE_FEE, - telCAN_NOT_QUEUE_FULL, + telCAN_NOT_QUEUE_FULL +}; + +//------------------------------------------------------------------------------ + +enum TEMcodes : TERUnderlyingType +{ + // Note: Range is stable. Exact numbers are unstable. Use tokens. // -299 .. -200: M Malformed (bad signature) // Causes: @@ -95,7 +109,14 @@ enum TER // An intermediate result used internally, should never be returned. temUNCERTAIN, - temUNKNOWN, + temUNKNOWN +}; + +//------------------------------------------------------------------------------ + +enum TEFcodes : TERUnderlyingType +{ + // Note: Range is stable. Exact numbers are unstable. Use tokens. // -199 .. -100: F // Failure (sequence number previously used) @@ -127,6 +148,13 @@ enum TER tefNOT_MULTI_SIGNING, tefBAD_AUTH_MASTER, tefINVARIANT_FAILED, +}; + +//------------------------------------------------------------------------------ + +enum TERcodes : TERUnderlyingType +{ + // Note: Range is stable. Exact numbers are unstable. Use tokens. // -99 .. -1: R Retry // sequence too high, no funds for txn fee, originating -account @@ -155,7 +183,15 @@ enum TER // burden network. terLAST, // Process after all other transactions terNO_RIPPLE, // Rippling not allowed - terQUEUED, // Transaction is being held in TxQ until fee drops + terQUEUED // Transaction is being held in TxQ until fee drops +}; + +//------------------------------------------------------------------------------ + +enum TEScodes : TERUnderlyingType +{ + // Note: Exact number must stay stable. This code is stored by value + // in metadata for historic transactions. // 0: S Success (success) // Causes: @@ -163,7 +199,15 @@ enum TER // Implications: // - Applied // - Forwarded - tesSUCCESS = 0, + tesSUCCESS = 0 +}; + +//------------------------------------------------------------------------------ + +enum TECcodes : TERUnderlyingType +{ + // Note: Exact numbers must stay stable. These codes are stored by + // value in metadata for historic transactions. // 100 .. 159 C // Claim fee only (ripple transaction with no good paths, pay to @@ -218,6 +262,201 @@ enum TER tecEXPIRED = 148 }; +//------------------------------------------------------------------------------ + +// For generic purposes, a free function that returns the value of a TE*codes. +constexpr TERUnderlyingType TERtoInt (TELcodes v) +{ return static_cast(v); } + +constexpr TERUnderlyingType TERtoInt (TEMcodes v) +{ return static_cast(v); } + +constexpr TERUnderlyingType TERtoInt (TEFcodes v) +{ return static_cast(v); } + +constexpr TERUnderlyingType TERtoInt (TERcodes v) +{ return static_cast(v); } + +constexpr TERUnderlyingType TERtoInt (TEScodes v) +{ return static_cast(v); } + +constexpr TERUnderlyingType TERtoInt (TECcodes v) +{ return static_cast(v); } + +//------------------------------------------------------------------------------ +// Template class that is specific to selected ranges of error codes. The +// Trait tells std::enable_if which ranges are allowed. +template class Trait> +class TERSubset +{ + TERUnderlyingType code_; + +public: + // Constructors + constexpr TERSubset() : code_ (tesSUCCESS) { } + constexpr TERSubset (TERSubset const& rhs) = default; + constexpr TERSubset (TERSubset&& rhs) = default; +private: + constexpr explicit TERSubset (int rhs) : code_ (rhs) { } +public: + static constexpr TERSubset fromInt (int from) + { + return TERSubset (from); + } + + // Trait tells enable_if which types are allowed for construction. + template ::value>> + constexpr TERSubset (T rhs) + : code_ (TERtoInt (rhs)) + { } + + // Assignment + constexpr TERSubset& operator=(TERSubset const& rhs) = default; + constexpr TERSubset& operator=(TERSubset&& rhs) = default; + + // Trait tells enable_if which types are allowed for assignment. + template + constexpr auto + operator= (T rhs) -> std::enable_if_t::value, TERSubset&> + { + code_ = TERtoInt (rhs); + return *this; + } + + // Conversion to bool. + explicit operator bool() const + { + return code_ != tesSUCCESS; + } + + // Conversion to Json::Value allows assignment to Json::Objects + // without casting. + operator Json::Value() const + { + return Json::Value {code_}; + } + + // Streaming operator. + friend std::ostream& operator<< (std::ostream& os, TERSubset const& rhs) + { + return os << rhs.code_; + } + + // Return the underlying value. Not a member so similarly named free + // functions can do the same work for the enums. + // + // It's worth noting that an explicit conversion operator was considered + // and rejected. Consider this case, taken from Status.h + // + // class Status { + // int code_; + // public: + // Status (TER ter) + // : code_ (ter) {} + // } + // + // This code compiles with no errors or warnings if TER has an explicit + // (unnamed) conversion to int. To avoid silent conversions like these + // we provide (only) a named conversion. + friend constexpr TERUnderlyingType TERtoInt (TERSubset v) + { + return v.code_; + } +}; + +// Comparison operators. +// Only enabled if both arguments return int if TERtiInt is called with them. +template +constexpr auto +operator== (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) == TERtoInt(rhs); +} + +template +constexpr auto +operator!= (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) != TERtoInt(rhs); +} + +template +constexpr auto +operator< (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) < TERtoInt(rhs); +} + +template +constexpr auto +operator<= (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) <= TERtoInt(rhs); +} + +template +constexpr auto +operator> (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) > TERtoInt(rhs); +} + +template +constexpr auto +operator>= (L const& lhs, R const& rhs) -> std::enable_if_t< + std::is_same::value && + std::is_same::value, bool> +{ + return TERtoInt(lhs) >= TERtoInt(rhs); +} + +//------------------------------------------------------------------------------ + +// Use traits to build a TERSubset that can convert from any of the TE*codes +// enums *except* TECcodes: NotTEC + +// NOTE: NotTEC is useful for codes returned by preflight in transactors. +// Preflight checks occur prior to signature checking. If preflight returned +// a tec code, then a malicious user could submit a transaction with a very +// large fee and have that fee charged against an account without using that +// account's valid signature. +template class CanCvtToNotTEC : public std::false_type {}; +template <> class CanCvtToNotTEC : public std::true_type {}; +template <> class CanCvtToNotTEC : public std::true_type {}; +template <> class CanCvtToNotTEC : public std::true_type {}; +template <> class CanCvtToNotTEC : public std::true_type {}; +template <> class CanCvtToNotTEC : public std::true_type {}; + +using NotTEC = TERSubset; + +//------------------------------------------------------------------------------ + +// Use traits to build a TERSubset that can convert from any of the TE*codes +// enums as well as from NotTEC. +template class CanCvtToTER : public std::false_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; +template <> class CanCvtToTER : public std::true_type {}; + +// TER allows all of the subsets. +using TER = TERSubset; + +//------------------------------------------------------------------------------ + inline bool isTelLocal(TER x) { return ((x) >= telLOCAL_ERROR && (x) < temMALFORMED); @@ -248,20 +487,15 @@ inline bool isTecClaim(TER x) return ((x) >= tecCLAIM); } -// VFALCO TODO group these into a shell class along with the defines above. -extern bool transResultInfo (TER code, std::string& token, std::string& text); -extern std::string transToken (TER code); -extern std::string transHuman (TER code); -extern boost::optional transCode(std::string const& token); diff --git a/src/ripple/protocol/impl/STInteger.cpp b/src/ripple/protocol/impl/STInteger.cpp index dd8448a18..ab44f24ac 100644 --- a/src/ripple/protocol/impl/STInteger.cpp +++ b/src/ripple/protocol/impl/STInteger.cpp @@ -48,7 +48,7 @@ STUInt8::getText () const { std::string token, human; - if (transResultInfo (static_cast (value_), token, human)) + if (transResultInfo (TER::fromInt (value_), token, human)) return human; JLOG (debugLog().error()) @@ -66,7 +66,7 @@ STUInt8::getJson (int) const { std::string token, human; - if (transResultInfo (static_cast (value_), token, human)) + if (transResultInfo (TER::fromInt (value_), token, human)) return token; JLOG (debugLog().error()) diff --git a/src/ripple/protocol/impl/STParsedJSON.cpp b/src/ripple/protocol/impl/STParsedJSON.cpp index d8ef3adeb..ef7c95104 100644 --- a/src/ripple/protocol/impl/STParsedJSON.cpp +++ b/src/ripple/protocol/impl/STParsedJSON.cpp @@ -202,14 +202,16 @@ static boost::optional parseLeaf ( { auto ter = transCode(strValue); - if (!ter || *ter < minValue || *ter > maxValue) + if (!ter || + TERtoInt (*ter) < minValue || + TERtoInt (*ter) > maxValue) { error = out_of_range(json_name, fieldName); return ret; } ret = detail::make_stvar(field, - static_cast(*ter)); + static_cast(TERtoInt (*ter))); } else { diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index 23a7238a8..1cc3166c8 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -28,13 +28,13 @@ namespace detail { static std::unordered_map< - std::underlying_type_t, + TERUnderlyingType, std::pair> const& transResults() { static std::unordered_map< - std::underlying_type_t, + TERUnderlyingType, std::pair> const results { @@ -162,8 +162,7 @@ bool transResultInfo (TER code, std::string& token, std::string& text) { auto& results = detail::transResults(); - auto const r = results.find ( - static_cast> (code)); + auto const r = results.find (TERtoInt (code)); if (r == results.end()) return false; @@ -207,7 +206,7 @@ transCode(std::string const& token) ); std::unordered_map< std::string, - std::underlying_type_t> const + TERUnderlyingType> const byToken(tRange.begin(), tRange.end()); return byToken; }(); @@ -217,7 +216,7 @@ transCode(std::string const& token) if (r == results.end()) return boost::none; - return static_cast(r->second); + return TER::fromInt (r->second); } } // ripple diff --git a/src/ripple/rpc/Status.h b/src/ripple/rpc/Status.h index 586bc888a..1f497a923 100644 --- a/src/ripple/rpc/Status.h +++ b/src/ripple/rpc/Status.h @@ -47,23 +47,26 @@ public: Status () = default; - Status (Code code, Strings d = {}) - : type_ (Type::none), code_ (code), messages_ (std::move (d)) + // The enable_if allows only integers (not enums). Prevents enum narrowing. + template ::value>> + Status (T code, Strings d = {}) + : type_ (Type::none), code_ (code), messages_ (std::move (d)) { } Status (TER ter, Strings d = {}) - : type_ (Type::TER), code_ (ter), messages_ (std::move (d)) + : type_ (Type::TER), code_ (TERtoInt (ter)), messages_ (std::move (d)) { } Status (error_code_i e, Strings d = {}) - : type_ (Type::error_code_i), code_ (e), messages_ (std::move (d)) + : type_ (Type::error_code_i), code_ (e), messages_ (std::move (d)) { } Status (error_code_i e, std::string const& s) - : type_ (Type::error_code_i), code_ (e), messages_ ({s}) + : type_ (Type::error_code_i), code_ (e), messages_ ({s}) { } @@ -89,7 +92,7 @@ public: TER toTER () const { assert (type_ == Type::TER); - return TER (code_); + return TER::fromInt (code_); } /** Returns the Status as an error_code_i. diff --git a/src/test/app/DepositAuth_test.cpp b/src/test/app/DepositAuth_test.cpp index fab30dd86..68336a69d 100644 --- a/src/test/app/DepositAuth_test.cpp +++ b/src/test/app/DepositAuth_test.cpp @@ -316,8 +316,9 @@ struct DepositAuth_test : public beast::unit_test::suite if (withDepositAuth) env(fset(gw1, asfDepositAuth)); - auto const result = - (noRippleNext && noRipplePrev) ? tecPATH_DRY : tesSUCCESS; + TER const result = (noRippleNext && noRipplePrev) + ? TER {tecPATH_DRY} + : TER {tesSUCCESS}; env (pay (alice, bob, USD1(10)), path (gw1), ter (result)); }; @@ -338,8 +339,9 @@ struct DepositAuth_test : public beast::unit_test::suite if (withDepositAuth) env(fset(alice, asfDepositAuth)); - auto const result = - (noRippleNext && noRipplePrev) ? tecPATH_DRY : tesSUCCESS; + TER const result = (noRippleNext && noRipplePrev) + ? TER {tecPATH_DRY} + : TER {tesSUCCESS}; env (pay (gw1, gw2, USD2 (10)), path (alice), sendmax (USD1 (10)), ter (result)); }; diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index d077e600b..9b973188d 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -1028,7 +1028,8 @@ struct Escrow_test : public beast::unit_test::suite auto const seq = env.seq(alice); env(escrow(alice, carol, XRP(1000)), finish_time(env.now() + 1s)); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); auto const escrow = env.le(keylet::escrow(alice.id(), seq)); BEAST_EXPECT(escrow); @@ -1050,7 +1051,8 @@ struct Escrow_test : public beast::unit_test::suite env(escrow(alice, alice, XRP(1000)), finish_time(env.now() + 1s), cancel_time(env.now() + 500s)); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); env.close(5s); auto const aa = env.le(keylet::escrow(alice.id(), aseq)); BEAST_EXPECT(aa); @@ -1063,7 +1065,8 @@ struct Escrow_test : public beast::unit_test::suite env(escrow(bruce, bruce, XRP(1000)), finish_time(env.now() + 1s), cancel_time(env.now() + 2s)); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); env.close(5s); auto const bb = env.le(keylet::escrow(bruce.id(), bseq)); BEAST_EXPECT(bb); @@ -1078,7 +1081,8 @@ struct Escrow_test : public beast::unit_test::suite env(finish(alice, alice, aseq)); { BEAST_EXPECT(!env.le(keylet::escrow(alice.id(), aseq))); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 0); @@ -1093,7 +1097,8 @@ struct Escrow_test : public beast::unit_test::suite env(cancel(bruce, bruce, bseq)); { BEAST_EXPECT(!env.le(keylet::escrow(bruce.id(), bseq))); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 0); @@ -1109,11 +1114,13 @@ struct Escrow_test : public beast::unit_test::suite auto const bseq = env.seq(bruce); env(escrow(alice, bruce, XRP(1000)), finish_time(env.now() + 1s)); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); env.close(5s); env(escrow(bruce, carol, XRP(1000)), finish_time(env.now() + 1s), cancel_time(env.now() + 2s)); - BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == + static_cast(tesSUCCESS)); env.close(5s); auto const ab = env.le(keylet::escrow(alice.id(), aseq)); diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index b655da7dc..081064e77 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -753,8 +753,9 @@ struct Flow_test : public beast::unit_test::suite env (offer (bob, XRP (50), USD (50))); env (offer (bob, XRP (100), USD (50))); - auto expectedResult = - closeTime < fix1141Time () ? tecPATH_DRY : tesSUCCESS; + TER const expectedResult = closeTime < fix1141Time () + ? TER {tecPATH_DRY} + : TER {tesSUCCESS}; env (pay (alice, carol, USD (100)), path (~USD), sendmax (XRP (100)), txflags (tfNoRippleDirect | tfPartialPayment | tfLimitQuality), ter (expectedResult)); @@ -1131,7 +1132,7 @@ struct Flow_test : public beast::unit_test::suite env(pay(alice, alice, XRP(1)), path(gw, bob, ~XRP), sendmax(gw["USD"](1000)), txflags(tfNoRippleDirect), - ter(withFix ? tecPATH_DRY : tesSUCCESS)); + ter(withFix ? TER {tecPATH_DRY} : TER {tesSUCCESS})); env.close(); if (withFix) @@ -1145,7 +1146,7 @@ struct Flow_test : public beast::unit_test::suite env(pay (carol, carol, gw["USD"](1000)), path(~bob["USD"], gw), sendmax(XRP(100000)), txflags(tfNoRippleDirect), - ter(withFix ? tecPATH_DRY : tesSUCCESS)); + ter(withFix ? TER {tecPATH_DRY} : TER {tesSUCCESS})); env.close(); pass(); @@ -1189,7 +1190,7 @@ struct Flow_test : public beast::unit_test::suite env(pay(alice, alice, USD(1000)), path(~bob["USD"], bob, gw), sendmax(XRP(1)), txflags(tfNoRippleDirect), - ter(withFix ? tecPATH_DRY : tesSUCCESS)); + ter(withFix ? TER {tecPATH_DRY} : TER {tesSUCCESS})); env.close(); } diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index a61bf7923..0de95f29f 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -239,7 +239,7 @@ public: env (offer (alice, XRP (5), USD (2)), json (sfExpiration.fieldName, lastClose(env)), json (jss::OfferSequence, offer2Seq), - ter (featChecks ? tecEXPIRED : tesSUCCESS)); + ter (featChecks ? TER {tecEXPIRED} : TER {tesSUCCESS})); env.close(); env.require (offers (alice, 2)); @@ -289,8 +289,8 @@ public: (hasFeature(env, featureFeeEscalation) && !hasFeature(env, fix1513)); // Will fail without the underflow fix - auto expectedResult = *stAmountCalcSwitchover ? - tesSUCCESS : tecPATH_PARTIAL; + TER const expectedResult = *stAmountCalcSwitchover ? + TER {tesSUCCESS} : TER {tecPATH_PARTIAL}; env (pay (alice, bob, EUR (epsilon)), path (~EUR), sendmax (USD (100)), ter (expectedResult)); } @@ -959,7 +959,7 @@ public: env (offer (alice, xrpOffer, usdOffer), json (sfExpiration.fieldName, lastClose(env)), - ter (featChecks ? tecEXPIRED : tesSUCCESS)); + ter (featChecks ? TER {tecEXPIRED} : TER {tesSUCCESS})); env.require ( balance (alice, startBalance - f - f), @@ -3422,7 +3422,7 @@ public: // Determine which TEC code we expect. TER const tecExpect = - features[featureFlow] ? temBAD_PATH : tecPATH_DRY; + features[featureFlow] ? TER {temBAD_PATH} : TER {tecPATH_DRY}; // This payment caused the assert. env (pay (ann, ann, D_BUX(30)), @@ -4397,7 +4397,7 @@ public: // create an offer to buy their own currency. After FlowCross // they can. env (offer (gw, gwUSD(40), XRP(4000)), - ter (flowCross ? tesSUCCESS : tecNO_LINE)); + ter (flowCross ? TER {tesSUCCESS} : TER {tecNO_LINE})); env.close(); env.require (offers (gw, flowCross ? 1 : 0)); diff --git a/src/test/app/PayStrand_test.cpp b/src/test/app/PayStrand_test.cpp index 682d0549d..7a39cf586 100644 --- a/src/test/app/PayStrand_test.cpp +++ b/src/test/app/PayStrand_test.cpp @@ -1375,7 +1375,7 @@ struct PayStrand_test : public beast::unit_test::suite env(offer(bob, XRP(100), USD(100)), txflags(tfPassive)); env(offer(bob, USD(100), XRP(100)), txflags(tfPassive)); - auto const expectedResult = [&] { + auto const expectedResult = [&] () -> TER { if (features[featureFlow] && !features[fix1373]) return tesSUCCESS; return temBAD_PATH_LOOP; diff --git a/src/test/jtx/JTx.h b/src/test/jtx/JTx.h index da82be28a..dfc310208 100644 --- a/src/test/jtx/JTx.h +++ b/src/test/jtx/JTx.h @@ -42,7 +42,7 @@ struct JTx { Json::Value jv; requires_t requires; - boost::optional ter = tesSUCCESS; + boost::optional ter = TER {tesSUCCESS}; bool fill_fee = true; bool fill_seq = true; bool fill_sig = true; diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index f10e1d59a..3995b1e24 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -278,8 +278,7 @@ Env::parseResult(Json::Value const& jr) TER ter; if (jr.isObject() && jr.isMember(jss::result) && jr[jss::result].isMember(jss::engine_result_code)) - ter = static_cast( - jr[jss::result][jss::engine_result_code].asInt()); + ter = TER::fromInt (jr[jss::result][jss::engine_result_code].asInt()); else ter = temINVALID; return std::make_pair(ter, diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index c2ecc5451..8919b9030 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -94,15 +94,16 @@ class Invariants_test : public beast::unit_test::suite BEAST_EXPECT(precheck(A1, A2, ac)); - auto tr = tesSUCCESS; + TER tr = tesSUCCESS; // invoke check twice to cover tec and tef cases for (auto i : {0,1}) { tr = ac.checkInvariants(tr); if (enabled) { - BEAST_EXPECT( - tr == (i == 0 ? tecINVARIANT_FAILED : tefINVARIANT_FAILED)); + BEAST_EXPECT(tr == (i == 0 + ? TER {tecINVARIANT_FAILED} + : TER {tefINVARIANT_FAILED})); BEAST_EXPECT( boost::starts_with(sink.strm_.str(), "Invariant failed:") || boost::starts_with(sink.strm_.str(), diff --git a/src/test/protocol/TER_test.cpp b/src/test/protocol/TER_test.cpp index 4cbaa0f68..7bb40baac 100644 --- a/src/test/protocol/TER_test.cpp +++ b/src/test/protocol/TER_test.cpp @@ -20,6 +20,9 @@ #include #include +#include +#include + namespace ripple { struct TER_test : public beast::unit_test::suite @@ -29,7 +32,7 @@ struct TER_test : public beast::unit_test::suite { for (auto i = -400; i < 400; ++i) { - TER t = static_cast(i); + TER t = TER::fromInt (i); auto inRange = isTelLocal(t) || isTemMalformed(t) || isTefFailure(t) || @@ -49,13 +52,214 @@ struct TER_test : public beast::unit_test::suite } } + // Helper template that makes sure two types are not convertible or + // assignable if not the same. + // o I1 one tuple index. + // o I2 other tuple index. + // o Tup is expected to be a tuple. + // It's a functor, rather than a function template, since a class template + // can be a template argument without being full specified. + template + class NotConvertible + { + public: + template + void operator()(Tup const& tup, beast::unit_test::suite&) const + { + // Entries in the tuple should not be convertible or assignable + // unless they are the same types. + using To_t = std::decay_t(tup))>; + using From_t = std::decay_t(tup))>; + static_assert (std::is_same::value == + std::is_convertible::value, "Convert err"); + static_assert (std::is_same::value == + std::is_constructible::value, "Construct err"); + static_assert (std::is_same ::value == + std::is_assignable::value, "Assign err"); + + // Assignment or conversion from integer to type should never work. + static_assert ( + ! std::is_convertible::value, "Convert err"); + static_assert ( + ! std::is_constructible::value, "Construct err"); + static_assert ( + ! std::is_assignable::value, "Assign err"); + } + }; + + // Fast iteration over the tuple. + template class Func, typename Tup> + std::enable_if_t + testIterate (Tup const& tup, beast::unit_test::suite& suite) + { + Func func; + func (tup, suite); + testIterate(tup, suite); + } + + // Slow iteration over the tuple. + template class Func, typename Tup> + std::enable_if_t + testIterate (Tup const& tup, beast::unit_test::suite& suite) + { + Func func; + func (tup, suite); + testIterate::value - 1, I2 - 1, Func>(tup, suite); + } + + // Finish iteration over the tuple. + template class Func, typename Tup> + std::enable_if_t + testIterate (Tup const& tup, beast::unit_test::suite& suite) + { + Func func; + func (tup, suite); + } + + void testConversion() + { + // Verify that valid conversions are valid and invalid conversions + // are not valid. + + // Examples of each kind of enum. + static auto const terEnums = std::make_tuple (telLOCAL_ERROR, + temMALFORMED, tefFAILURE, terRETRY, tesSUCCESS, tecCLAIM); + static const int hiIndex { + std::tuple_size::value - 1}; + + // Verify that enums cannot be converted to other enum types. + testIterate (terEnums, *this); + + // Lambda that verifies assignability and convertibility. + auto isConvertable = [] (auto from, auto to) + { + using From_t = std::decay_t; + using To_t = std::decay_t; + static_assert ( + std::is_convertible::value, "Convert err"); + static_assert ( + std::is_constructible::value, "Construct err"); + static_assert ( + std::is_assignable::value, "Assign err"); + }; + + // Verify the right types convert to NotTEC. + NotTEC const notTec; + isConvertable (telLOCAL_ERROR, notTec); + isConvertable (temMALFORMED, notTec); + isConvertable (tefFAILURE, notTec); + isConvertable (terRETRY, notTec); + isConvertable (tesSUCCESS, notTec); + isConvertable (notTec, notTec); + + // Lambda that verifies types and not assignable or convertible. + auto notConvertible = [] (auto from, auto to) + { + using To_t = std::decay_t; + using From_t = std::decay_t; + static_assert ( + !std::is_convertible::value, "Convert err"); + static_assert ( + !std::is_constructible::value, "Construct err"); + static_assert ( + !std::is_assignable::value, "Assign err"); + }; + + // Verify types that shouldn't convert to NotTEC. + TER const ter; + notConvertible (tecCLAIM, notTec); + notConvertible (ter, notTec); + notConvertible (4, notTec); + + // Verify the right types convert to TER. + isConvertable (telLOCAL_ERROR, ter); + isConvertable (temMALFORMED, ter); + isConvertable (tefFAILURE, ter); + isConvertable (terRETRY, ter); + isConvertable (tesSUCCESS, ter); + isConvertable (tecCLAIM, ter); + isConvertable (notTec, ter); + isConvertable (ter, ter); + + // Verify that you can't convert from int to ter. + notConvertible (4, ter); + } + + // Helper template that makes sure two types are comparable. Also + // verifies that one of the types does not compare to int. + // o I1 one tuple index. + // o I2 other tuple index. + // o Tup is expected to be a tuple. + // It's a functor, rather than a function template, since a class template + // can be a template argument without being full specified. + template + class CheckComparable + { + public: + template + void operator()(Tup const& tup, beast::unit_test::suite& suite) const + { + // All entries in the tuple should be comparable one to the other. + auto const lhs = std::get(tup); + auto const rhs = std::get(tup); + + static_assert (std::is_same< + decltype (operator== (lhs, rhs)), bool>::value, "== err"); + + static_assert (std::is_same< + decltype (operator!= (lhs, rhs)), bool>::value, "!= err"); + + static_assert (std::is_same< + decltype (operator< (lhs, rhs)), bool>::value, "< err"); + + static_assert (std::is_same< + decltype (operator<= (lhs, rhs)), bool>::value, "<= err"); + + static_assert (std::is_same< + decltype (operator> (lhs, rhs)), bool>::value, "> err"); + + static_assert (std::is_same< + decltype (operator>= (lhs, rhs)), bool>::value, ">= err"); + + // Make sure a sampling of TER types exhibit the expected behavior + // for all comparison operators. + suite.expect ((lhs == rhs) == (TERtoInt (lhs) == TERtoInt (rhs))); + suite.expect ((lhs != rhs) == (TERtoInt (lhs) != TERtoInt (rhs))); + suite.expect ((lhs < rhs) == (TERtoInt (lhs) < TERtoInt (rhs))); + suite.expect ((lhs <= rhs) == (TERtoInt (lhs) <= TERtoInt (rhs))); + suite.expect ((lhs > rhs) == (TERtoInt (lhs) > TERtoInt (rhs))); + suite.expect ((lhs >= rhs) == (TERtoInt (lhs) >= TERtoInt (rhs))); + } + }; + + void testComparison() + { + // All of the TER-related types should be comparable. + + // Examples of all the types we expect to successfully compare. + static auto const ters = std::make_tuple (telLOCAL_ERROR, temMALFORMED, + tefFAILURE, terRETRY, tesSUCCESS, tecCLAIM, + NotTEC {telLOCAL_ERROR}, TER {tecCLAIM}); + static const int hiIndex { + std::tuple_size::value - 1}; + + // Verify that all types in the ters tuple can be compared with all + // the other types in ters. + testIterate (ters, *this); + } + void run() override { testTransResultInfo(); + testConversion(); + testComparison(); } }; BEAST_DEFINE_TESTSUITE(TER,protocol,ripple); -} +} //namespace ripple