diff --git a/include/xrpl/ledger/helpers/SLEBase.h b/include/xrpl/ledger/helpers/SLEBase.h index 2b18d250bd..98228976ce 100644 --- a/include/xrpl/ledger/helpers/SLEBase.h +++ b/include/xrpl/ledger/helpers/SLEBase.h @@ -48,6 +48,7 @@ public: operator=(SLEBase const&) = delete; SLEBase& operator=(SLEBase&&) = delete; + SLEBase() = delete; // --- Common interface (always available) --- @@ -176,8 +177,6 @@ public: } protected: - SLEBase() = delete; - /** Constructor for read-only context */ explicit SLEBase( SLE::const_pointer sle, diff --git a/src/libxrpl/tx/paths/DirectStep.cpp b/src/libxrpl/tx/paths/DirectStep.cpp index 6dee1478d9..3a01adffff 100644 --- a/src/libxrpl/tx/paths/DirectStep.cpp +++ b/src/libxrpl/tx/paths/DirectStep.cpp @@ -580,6 +580,8 @@ DirectStepI::setCacheLimiting( IOUAmount const& fwdOut, DebtDirection srcDebtDir) { + // NOLINTBEGIN(bugprone-unchecked-optional-access) cache_ always set before setCacheLimiting is + // called if (cache_->in < fwdIn) { IOUAmount const smallDiff(1, -9); @@ -609,6 +611,7 @@ DirectStepI::setCacheLimiting( if (fwdOut < cache_->out) cache_->out = fwdOut; cache_->srcDebtDir = srcDebtDir; + // NOLINTEND(bugprone-unchecked-optional-access) }; template @@ -620,6 +623,7 @@ DirectStepI::fwdImp( IOUAmount const& in) { XRPL_ASSERT(cache_, "xrpl::DirectStepI::fwdImp : cache is set"); + // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above auto const [maxSrcToDst, srcDebtDir] = static_cast(this)->maxFlow(sb, cache_->srcToDst); @@ -676,6 +680,7 @@ DirectStepI::fwdImp( << " srcToDst: " << to_string(srcToDst) << " out: " << to_string(out); } return {cache_->in, cache_->out}; + // NOLINTEND(bugprone-unchecked-optional-access) } template @@ -706,6 +711,7 @@ DirectStepI::validFwd(PaymentSandbox& sb, ApplyView& afView, EitherAmo return {false, EitherAmount(IOUAmount(beast::kZERO))}; } + // NOLINTBEGIN(bugprone-unchecked-optional-access) fwdImp sets cache_ on success if (maxSrcToDst < cache_->srcToDst) { JLOG(j_.warn()) << "DirectStepI: Strand re-execute check failed." @@ -725,6 +731,7 @@ DirectStepI::validFwd(PaymentSandbox& sb, ApplyView& afView, EitherAmo return {false, EitherAmount(cache_->out)}; } return {true, EitherAmount(cache_->out)}; + // NOLINTEND(bugprone-unchecked-optional-access) } // Returns srcQOut, dstQIn diff --git a/src/libxrpl/tx/paths/MPTEndpointStep.cpp b/src/libxrpl/tx/paths/MPTEndpointStep.cpp index 18581418c3..6a8db7b560 100644 --- a/src/libxrpl/tx/paths/MPTEndpointStep.cpp +++ b/src/libxrpl/tx/paths/MPTEndpointStep.cpp @@ -556,6 +556,8 @@ MPTEndpointStep::setCacheLimiting( MPTAmount const& fwdOut, DebtDirection srcDebtDir) { + // NOLINTBEGIN(bugprone-unchecked-optional-access) cache_ always set before setCacheLimiting is + // called if (cache_->in < fwdIn) { MPTAmount const smallDiff(1); @@ -585,6 +587,7 @@ MPTEndpointStep::setCacheLimiting( if (fwdOut < cache_->out) cache_->out = fwdOut; cache_->srcDebtDir = srcDebtDir; + // NOLINTEND(bugprone-unchecked-optional-access) }; template @@ -596,6 +599,7 @@ MPTEndpointStep::fwdImp( MPTAmount const& in) { XRPL_ASSERT(cache_, "MPTEndpointStep::fwdImp : valid cache"); + // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above auto const [maxSrcToDst, srcDebtDir] = static_cast(this)->maxPaymentFlow(sb); @@ -669,6 +673,7 @@ MPTEndpointStep::fwdImp( << " srcToDst: " << to_string(srcToDst) << " out: " << to_string(out); } return {cache_->in, cache_->out}; + // NOLINTEND(bugprone-unchecked-optional-access) } template @@ -698,6 +703,7 @@ MPTEndpointStep::validFwd(PaymentSandbox& sb, ApplyView& afView, Eithe return {false, EitherAmount(MPTAmount(beast::kZERO))}; } + // NOLINTBEGIN(bugprone-unchecked-optional-access) fwdImp sets cache_ on success if (maxSrcToDst < cache_->srcToDst) { JLOG(j_.warn()) << "MPTEndpointStep: Strand re-execute check failed." @@ -717,6 +723,7 @@ MPTEndpointStep::validFwd(PaymentSandbox& sb, ApplyView& afView, Eithe return {false, EitherAmount(cache_->out)}; } return {true, EitherAmount(cache_->out)}; + // NOLINTEND(bugprone-unchecked-optional-access) } // Returns srcQOut, dstQIn diff --git a/src/libxrpl/tx/transactors/dex/AMMVote.cpp b/src/libxrpl/tx/transactors/dex/AMMVote.cpp index 222d1d9ba2..83cc46205b 100644 --- a/src/libxrpl/tx/transactors/dex/AMMVote.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMVote.cpp @@ -133,10 +133,17 @@ applyVote(ApplyContext& ctx, Sandbox& sb, AccountID const& accountId, beast::Jou // Find an entry with the least tokens/fee. Make the order deterministic // if the tokens/fees are equal. - if (!minTokens || - (lpTokens < *minTokens || - (lpTokens == *minTokens && - (feeVal < minFee || (feeVal == minFee && account < minAccount))))) + if (!minTokens) + { + minTokens = lpTokens; + minPos = updatedVoteSlots.size(); + minAccount = account; + minFee = feeVal; + } + else if ( + auto const& minTokensValue = *minTokens; lpTokens < minTokensValue || + (lpTokens == minTokensValue && + (feeVal < minFee || (feeVal == minFee && account < minAccount)))) { minTokens = lpTokens; minPos = updatedVoteSlots.size(); @@ -177,13 +184,22 @@ applyVote(ApplyContext& ctx, Sandbox& sb, AccountID const& accountId, beast::Jou // Add the entry if the account has more tokens than // the least token holder or same tokens and higher fee. } - else if (lpTokensNew > *minTokens || (lpTokensNew == *minTokens && feeNew > minFee)) + else if (minTokens) { - auto const entry = updatedVoteSlots.begin() + minPos; - // Remove the least token vote entry. - num -= Number((*entry)[~sfTradingFee].valueOr(0)) * *minTokens; - den -= *minTokens; - update(minPos); + auto const& minTokensValue = *minTokens; + if (lpTokensNew > minTokensValue || (lpTokensNew == minTokensValue && feeNew > minFee)) + { + auto const entry = updatedVoteSlots.begin() + minPos; + // Remove the least token vote entry. + num -= Number((*entry)[~sfTradingFee].valueOr(0)) * minTokensValue; + den -= minTokensValue; + update(minPos); + } + else + { + JLOG(j.debug()) << "AMMVote::applyVote, insufficient tokens to " + "override other votes"; + } } // All slots are full and the account does not hold more LPTokens. // Update anyway to refresh the slots. diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 735a2aeba7..730e6a77bc 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -43,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -180,6 +182,8 @@ PathRequest::isValid(std::shared_ptr const& crCache) { if (!raSrcAccount_ || !raDstAccount_) return false; + auto const& srcAccount = *raSrcAccount_; + auto const& dstAccount = *raDstAccount_; if (!convert_all_ && (saSendMax_ || saDstAmount_ <= beast::kZERO)) { @@ -190,14 +194,14 @@ PathRequest::isValid(std::shared_ptr const& crCache) auto const& lrLedger = crCache->getLedger(); - if (!lrLedger->exists(keylet::account(*raSrcAccount_))) + if (!lrLedger->exists(keylet::account(srcAccount))) { // Source account does not exist. jvStatus_ = rpcError(RpcSrcActNotFound); return false; } - AccountRoot const acctDest(*raDstAccount_, *lrLedger); + AccountRoot const acctDest(dstAccount, *lrLedger); json::Value& jvDestCur = (jvStatus_[jss::destination_currencies] = json::ValueType::Array); @@ -222,7 +226,7 @@ PathRequest::isValid(std::shared_ptr const& crCache) { bool const disallowXRP((acctDest->getFlags() & lsfDisallowXRP) != 0u); - auto const destAssets = accountDestAssets(*raDstAccount_, crCache, !disallowXRP); + auto const destAssets = accountDestAssets(dstAccount, crCache, !disallowXRP); for (auto const& asset : destAssets) jvDestCur.append(to_string(asset)); @@ -260,6 +264,7 @@ PathRequest::doCreate(std::shared_ptr const& cache, json::Value cons { if (valid) { + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) valid is from isValid() stream << iIdentifier_ << " valid: " << toBase58(*raSrcAccount_); stream << iIdentifier_ << " deliver: " << saDstAmount_.getFullText(); } @@ -510,16 +515,12 @@ PathRequest::getPathFinder( auto i = pathassetMap.find(asset); if (i != pathassetMap.end()) return i->second; + if (!raSrcAccount_ || !raDstAccount_) + Throw("PathRequest::getPathFinder: missing accounts"); + auto const& srcAccount = *raSrcAccount_; + auto const& dstAccount = *raDstAccount_; auto pathfinder = std::make_unique( - cache, - *raSrcAccount_, - *raDstAccount_, - asset, - std::nullopt, - dstAmount, - saSendMax_, - domain_, - app_); + cache, srcAccount, dstAccount, asset, std::nullopt, dstAmount, saSendMax_, domain_, app_); if (pathfinder->findPaths(level, continueCallback)) { pathfinder->computePathRanks(kMAX_PATHS, continueCallback); @@ -538,6 +539,10 @@ PathRequest::findPaths( json::Value& jvArray, std::function const& continueCallback) { + if (!raSrcAccount_ || !raDstAccount_) + Throw("PathRequest::findPaths: missing accounts"); + auto const& srcAccount = *raSrcAccount_; + auto const& dstAccount = *raDstAccount_; auto sourceAssets = sciSourceAssets_; if (sourceAssets.empty() && saSendMax_) { @@ -545,8 +550,8 @@ PathRequest::findPaths( } if (sourceAssets.empty()) { - auto assets = accountSourceAssets(*raSrcAccount_, cache, true); - bool const sameAccount = *raSrcAccount_ == *raDstAccount_; + auto assets = accountSourceAssets(srcAccount, cache, true); + bool const sameAccount = srcAccount == dstAccount; for (auto const& asset : assets) { if (!std::visit( @@ -558,7 +563,7 @@ PathRequest::findPaths( if constexpr (std::is_same_v) { sourceAssets.insert( - Issue{a, a.isZero() ? xrpAccount() : *raSrcAccount_}); + Issue{a, a.isZero() ? xrpAccount() : srcAccount}); } else { @@ -603,7 +608,7 @@ PathRequest::findPaths( if (isXRP(asset)) return xrpAccount(); - return *raSrcAccount_; + return srcAccount; }(); STAmount const saMaxAmount = [&]() { @@ -624,13 +629,13 @@ PathRequest::findPaths( auto sandbox = std::make_unique(&*cache->getLedger(), TapNone); auto rc = path::RippleCalc::rippleCalculate( *sandbox, - saMaxAmount, // --> Amount to send is unlimited - // to get an estimate. - dstAmount, // --> Amount to deliver. - *raDstAccount_, // --> Account to deliver to. - *raSrcAccount_, // --> Account sending from. - ps, // --> Path set. - domain_, // --> Domain. + saMaxAmount, // --> Amount to send is unlimited + // to get an estimate. + dstAmount, // --> Amount to deliver. + dstAccount, // --> Account to deliver to. + srcAccount, // --> Account sending from. + ps, // --> Path set. + domain_, // --> Domain. app_, &rcInput); @@ -643,13 +648,13 @@ PathRequest::findPaths( sandbox = std::make_unique(&*cache->getLedger(), TapNone); rc = path::RippleCalc::rippleCalculate( *sandbox, - saMaxAmount, // --> Amount to send is unlimited - // to get an estimate. - dstAmount, // --> Amount to deliver. - *raDstAccount_, // --> Account to deliver to. - *raSrcAccount_, // --> Account sending from. - ps, // --> Path set. - domain_, // --> Domain. + saMaxAmount, // --> Amount to send is unlimited + // to get an estimate. + dstAmount, // --> Amount to deliver. + dstAccount, // --> Account to deliver to. + srcAccount, // --> Account sending from. + ps, // --> Path set. + domain_, // --> Domain. app_); if (!isTesSuccess(rc.result()))