From 64ec8a7b6ffac77840609e73e380e68ef5599216 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 22 Nov 2012 19:10:43 -0800 Subject: [PATCH] Make ripple_path_find more robust and clean up. --- src/cpp/ripple/OfferCreateTransactor.cpp | 5 +- src/cpp/ripple/Pathfinder.cpp | 23 +++-- src/cpp/ripple/Pathfinder.h | 3 +- src/cpp/ripple/PaymentTransactor.cpp | 2 +- src/cpp/ripple/RPCHandler.cpp | 126 +++++++++++++---------- src/cpp/ripple/RippleCalc.cpp | 18 +++- src/cpp/ripple/SerializedTypes.h | 4 +- 7 files changed, 107 insertions(+), 74 deletions(-) diff --git a/src/cpp/ripple/OfferCreateTransactor.cpp b/src/cpp/ripple/OfferCreateTransactor.cpp index 9f940b6e3..27136b476 100644 --- a/src/cpp/ripple/OfferCreateTransactor.cpp +++ b/src/cpp/ripple/OfferCreateTransactor.cpp @@ -8,7 +8,6 @@ // <-- saTakerPaid: What taker paid not including fees. To reduce an offer. // <-- saTakerGot: What taker got not including fees. To reduce an offer. // <-- terResult: tesSUCCESS or terNO_ACCOUNT -// XXX: Fees should be paid by the source of the currency. TER OfferCreateTransactor::takeOffers( bool bPassive, const uint256& uBookBase, @@ -284,8 +283,7 @@ TER OfferCreateTransactor::doApply() { Log(lsWARNING) << "doOfferCreate: Expired transaction: offer expired"; - // XXX CHARGE FEE ONLY. - terResult = tesSUCCESS; + terResult = tesSUCCESS; // Only charged fee. } else if (saTakerPays.isNative() && saTakerGets.isNative()) { @@ -438,3 +436,4 @@ TER OfferCreateTransactor::doApply() return terResult; } +// vim:ts=4 diff --git a/src/cpp/ripple/Pathfinder.cpp b/src/cpp/ripple/Pathfinder.cpp index d79aac74c..ad6ca9485 100644 --- a/src/cpp/ripple/Pathfinder.cpp +++ b/src/cpp/ripple/Pathfinder.cpp @@ -52,8 +52,8 @@ bool sortPathOptions(PathOption::pointer first, PathOption::pointer second) if (first->mCorrectCurrency && !second->mCorrectCurrency) return(true); if (!first->mCorrectCurrency && second->mCorrectCurrency) return(false); - if (first->mPath.getElementCount()mPath.getElementCount()) return(true); - if (first->mPath.getElementCount()>second->mPath.getElementCount()) return(false); + if (first->mPath.size()mPath.size()) return(true); + if (first->mPath.size()>second->mPath.size()) return(false); if (first->mMinWidthmMinWidth) return true; @@ -75,15 +75,22 @@ PathOption::PathOption(PathOption::pointer other) } #endif -Pathfinder::Pathfinder(RippleAddress& srcAccountID, RippleAddress& dstAccountID, uint160& srcCurrencyID, STAmount dstAmount) : - mSrcAccountID(srcAccountID.getAccountID()), mDstAccountID(dstAccountID.getAccountID()), mDstAmount(dstAmount), mSrcCurrencyID(srcCurrencyID), mOrderBook(theApp->getMasterLedger().getCurrentLedger()) +// +// XXX Optionally, specifying a source and destination issuer might be nice. Especially, to convert between issuers. However, this +// functionality is left to the future. +// +Pathfinder::Pathfinder(const RippleAddress& srcAccountID, const RippleAddress& dstAccountID, const uint160& srcCurrencyID, const uint160& srcIssuerID, const STAmount& dstAmount) + : mSrcAccountID(srcAccountID.getAccountID()), mDstAccountID(dstAccountID.getAccountID()), mDstAmount(dstAmount), mSrcCurrencyID(srcCurrencyID), mSrcIssuerID(srcIssuerID), mOrderBook(theApp->getMasterLedger().getCurrentLedger()) { mLedger=theApp->getMasterLedger().getCurrentLedger(); } -// Returns a single path, if possible. -// --> maxSearchSteps: unused -// --> maxPay: unused +// If possible, returns a single path. +// --> maxSearchSteps: unused XXX +// --> maxPay: unused XXX +// When generating a path set blindly, don't allow the empty path, it is implied by default. +// When generating a path set for estimates, allow an empty path instead of no paths to indicate a path exists. The caller will +// need to strip the empty path when submitting the transaction. bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet, bool bAllowEmpty) { if (mLedger) { @@ -104,8 +111,6 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet ele = path.mPath.back(); // Get the last node from the path. - // Determine if path is solved. - // Done, if dest wants XRP and last element produces XRP. if (!ele.mCurrencyID // Tail output is XRP && !mDstAmount.getCurrency()) { diff --git a/src/cpp/ripple/Pathfinder.h b/src/cpp/ripple/Pathfinder.h index 8e91842d2..6537f0161 100644 --- a/src/cpp/ripple/Pathfinder.h +++ b/src/cpp/ripple/Pathfinder.h @@ -34,6 +34,7 @@ class Pathfinder uint160 mDstAccountID; STAmount mDstAmount; uint160 mSrcCurrencyID; + uint160 mSrcIssuerID; OrderBookDB mOrderBook; Ledger::pointer mLedger; @@ -49,7 +50,7 @@ class Pathfinder // void addPathOption(PathOption::pointer pathOption); public: - Pathfinder(RippleAddress& srcAccountID, RippleAddress& dstAccountID, uint160& srcCurrencyID, STAmount dstAmount); + Pathfinder(const RippleAddress& srcAccountID, const RippleAddress& dstAccountID, const uint160& srcCurrencyID, const uint160& srcIssuerID, const STAmount& dstAmount); // returns false if there is no path. otherwise fills out retPath bool findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet, bool bAllowEmpty); diff --git a/src/cpp/ripple/PaymentTransactor.cpp b/src/cpp/ripple/PaymentTransactor.cpp index 5dfb73155..81404c24f 100644 --- a/src/cpp/ripple/PaymentTransactor.cpp +++ b/src/cpp/ripple/PaymentTransactor.cpp @@ -118,7 +118,7 @@ TER PaymentTransactor::doApply() STAmount saMaxAmountAct; STAmount saDstAmountAct; - terResult = isSetBit(mParams, tapOPEN_LEDGER) && spsPaths.getPathCount() > RIPPLE_PATHS_MAX + terResult = isSetBit(mParams, tapOPEN_LEDGER) && spsPaths.size() > RIPPLE_PATHS_MAX ? telBAD_PATH_COUNT // Too many paths for proposed ledger. : RippleCalc::rippleCalc( mEngine->getNodes(), diff --git a/src/cpp/ripple/RPCHandler.cpp b/src/cpp/ripple/RPCHandler.cpp index 4bc417f19..d293c875a 100644 --- a/src/cpp/ripple/RPCHandler.cpp +++ b/src/cpp/ripple/RPCHandler.cpp @@ -695,6 +695,12 @@ Json::Value RPCHandler::doRippleLinesGet(const Json::Value ¶ms) return ret; } +// TODO: +// - Add support for specifying non-endpoint issuer. +// - Return fully expanded path with proof. +// - Allows clients to verify path exists. +// - Return canonicalized path. +// - From a trusted server, allows clients to use path without manipulation. Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) { Json::Value jvResult(Json::objectValue); @@ -723,7 +729,8 @@ Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) else if ( // Parse saDstAmount. !jvRequest.isMember("destination_amount") - || !saDstAmount.bSetJson(jvRequest["destination_amount"])) + || !saDstAmount.bSetJson(jvRequest["destination_amount"]) + || (!!saDstAmount.getCurrency() && (!saDstAmount.getIssuer() || ACCOUNT_ONE == saDstAmount.getIssuer()))) { cLog(lsINFO) << "Bad destination_amount."; jvResult = rpcError(rpcINVALID_PARAMS); @@ -744,18 +751,29 @@ Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) Json::Value jvArray(Json::arrayValue); Ledger::pointer lpCurrent = mNetOps->getCurrentLedger(); + + ScopedUnlock su(theApp->getMasterLock()); // As long as we have a locked copy of the ledger, we can unlock. + LedgerEntrySet lesSnapshot(lpCurrent); for (unsigned int i=0; i != jvSrcCurrencies.size(); ++i) { Json::Value jvSource = jvSrcCurrencies[i]; - uint160 srcCurrencyID; - uint160 srcIssuerID; + uint160 uSrcCurrencyID; + uint160 uSrcIssuerID = raSrc.getAccountID(); - if (!jvSource.isMember("currency") - || !STAmount::currencyFromString(srcCurrencyID, jvSource["currency"].asString()) + if ( + // Parse currency. + !jvSource.isMember("currency") + || !STAmount::currencyFromString(uSrcCurrencyID, jvSource["currency"].asString()) + + // Parse issuer. || ((jvSource.isMember("issuer")) && (!jvSource["issuer"].isString() - || !STAmount::issuerFromString(srcIssuerID, jvSource["issuer"].asString())))) + || !STAmount::issuerFromString(uSrcIssuerID, jvSource["issuer"].asString()))) + + // Don't allow illegal issuers. + || !uSrcIssuerID + || ACCOUNT_ONE == uSrcIssuerID) { cLog(lsINFO) << "Bad currency/issuer."; return rpcError(rpcINVALID_PARAMS); @@ -763,8 +781,7 @@ Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) STPathSet spsPaths; - // XXX Need to add support for srcIssuerID. - Pathfinder pf(raSrc, raDst, srcCurrencyID, saDstAmount); + Pathfinder pf(raSrc, raDst, uSrcCurrencyID, uSrcIssuerID, saDstAmount); pf.findPaths(5, 1, spsPaths, true); @@ -774,36 +791,39 @@ Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) } else { - // XXX Also need to check liquidity. STAmount saMaxAmountAct; STAmount saDstAmountAct; - STAmount saMaxAmount(srcCurrencyID, - !!srcIssuerID - ? srcIssuerID - : !!srcCurrencyID + STAmount saMaxAmount( + uSrcCurrencyID, + !!uSrcIssuerID + ? uSrcIssuerID + : !!uSrcCurrencyID ? raSrc.getAccountID() : ACCOUNT_XRP, 1); saMaxAmount.negate(); + // Strip empty/default path. + if (1 == spsPaths.size() && !spsPaths.begin()->size()) + { + spsPaths.clear(); + } + TER terResult = RippleCalc::rippleCalc( lesSnapshot, saMaxAmountAct, saDstAmountAct, - saMaxAmount, // --> -1/xxx/yyy unlimited + saMaxAmount, // --> Amount to send is unlimited to get an estimate. saDstAmount, // --> Amount to deliver. raDst.getAccountID(), // --> Account to deliver to. raSrc.getAccountID(), // --> Account sending from. spsPaths, // --> Path set. - false, // --> bPartialPayment - XXX might allow sometimes. + false, // --> Don't allow partial payment. This is for normal fill or kill payments. // Must achive delivery goal. - false, // --> bLimitQuality - XXX might allow sometimes. - // Average quality is wanted for normal payments. - // XXX TRUE till direct path representation resolved. - true, // --> bNoRippleDirect - XXX might allow sometimes. - // XXX No reason not to take the direct, unless set is merely direct. - true); //--> Stand alone mode, don't delete unfundeds. + false, // --> Don't limit quality. Average quality is wanted for normal payments. + false, // --> Allow direct ripple. + true); // --> Stand alone mode, no point in deleting unfundeds. cLog(lsDEBUG) << boost::str(boost::format("ripple_path_find: saMaxAmount=%s saDstAmount=%s saMaxAmountAct=%s saDstAmountAct=%s") @@ -817,7 +837,7 @@ Json::Value RPCHandler::doRipplePathFind(const Json::Value& jvRequest) Json::Value jvEntry(Json::objectValue); jvEntry["source_amount"] = saMaxAmountAct.getJson(0); - jvEntry["paths"] = spsPaths.getJson(0); + jvEntry["paths_expanded"] = spsPaths.getJson(0); jvArray.append(jvEntry); } @@ -875,7 +895,7 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) { Json::Value jvResult; RippleAddress naSeed; - RippleAddress srcAddress; + RippleAddress raSrcAddressID; Json::Value txJSON = jvRequest["tx_json"]; if (!naSeed.setSeedGeneric(jvRequest["secret"].asString())) @@ -886,15 +906,23 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) { return rpcError(rpcSRC_ACT_MISSING); } - if (!srcAddress.setAccountID(txJSON["Account"].asString())) + if (!raSrcAddressID.setAccountID(txJSON["Account"].asString())) { return rpcError(rpcSRC_ACT_MALFORMED); } - AccountState::pointer asSrc = mNetOps->getAccountState(uint256(0), srcAddress); - if(!asSrc) return rpcError(rpcSRC_ACT_MALFORMED); + AccountState::pointer asSrc = mNetOps->getAccountState(uint256(0), raSrcAddressID); + if (!asSrc) return rpcError(rpcSRC_ACT_MALFORMED); - if( txJSON["TransactionType"]=="Payment") + if (!txJSON.isMember("Fee") + && ("OfferCreate" == txJSON["TransactionType"].asString() + || "OfferCancel" == txJSON["TransactionType"].asString() + || "TrustSet" == txJSON["TransactionType"].asString())) + { + txJSON["Fee"] = (int) theConfig.FEE_DEFAULT; + } + + if ("Payment" == txJSON["TransactionType"].asString()) { RippleAddress dstAccountID; @@ -911,8 +939,9 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) if (!txJSON.isMember("Fee")) { if (mNetOps->getAccountState(uint256(0), dstAccountID)) - txJSON["Fee"]=(int)theConfig.FEE_DEFAULT; - else txJSON["Fee"]=(int)theConfig.FEE_ACCOUNT_CREATE; + txJSON["Fee"] = (int) theConfig.FEE_DEFAULT; + else + txJSON["Fee"] = (int) theConfig.FEE_ACCOUNT_CREATE; } if (!txJSON.isMember("Paths") && jvRequest.isMember("build_path")) @@ -920,15 +949,21 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) if (txJSON["Amount"].isObject() || txJSON.isMember("SendMax")) { // we need a ripple path STPathSet spsPaths; - uint160 srcCurrencyID; + uint160 uSrcCurrencyID; + uint160 uSrcIssuerID; if (txJSON.isMember("SendMax") && txJSON["SendMax"].isMember("currency")) { - STAmount::currencyFromString(srcCurrencyID, txJSON["SendMax"]["currency"].asString()); + STAmount::currencyFromString(uSrcCurrencyID, txJSON["SendMax"]["currency"].asString()); } else { - srcCurrencyID = CURRENCY_XRP; + uSrcCurrencyID = CURRENCY_XRP; + } + + if (!!uSrcCurrencyID) + { + uSrcIssuerID = raSrcAddressID.getAccountID(); } STAmount dstAmount; @@ -938,7 +973,7 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) return rpcError(rpcDST_AMT_MALFORMED); } - Pathfinder pf(srcAddress, dstAccountID, srcCurrencyID, dstAmount); + Pathfinder pf(raSrcAddressID, dstAccountID, uSrcCurrencyID, uSrcIssuerID, dstAmount); pf.findPaths(5, 1, spsPaths, false); @@ -951,29 +986,12 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) } } } - else if( txJSON["type"]=="OfferCreate" ) - { - txJSON["TransactionType"]=7; - if(!txJSON.isMember("Fee")) txJSON["Fee"]=(int)theConfig.FEE_DEFAULT; - } - else if( txJSON["type"]=="TrustSet") - { - txJSON["TransactionType"]=20; - if(!txJSON.isMember("Fee")) txJSON["Fee"]=(int)theConfig.FEE_DEFAULT; - } - else if( txJSON["type"]=="OfferCancel") - { - txJSON["TransactionType"]=8; - if(!txJSON.isMember("Fee")) txJSON["Fee"]=(int)theConfig.FEE_DEFAULT; - } - - txJSON.removeMember("type"); if(!txJSON.isMember("Sequence")) txJSON["Sequence"]=asSrc->getSeq(); if(!txJSON.isMember("Flags")) txJSON["Flags"]=0; Ledger::pointer lpCurrent = mNetOps->getCurrentLedger(); - SLE::pointer sleAccountRoot = mNetOps->getSLE(lpCurrent, Ledger::getAccountRootIndex(srcAddress.getAccountID())); + SLE::pointer sleAccountRoot = mNetOps->getSLE(lpCurrent, Ledger::getAccountRootIndex(raSrcAddressID.getAccountID())); if (!sleAccountRoot) { @@ -999,9 +1017,9 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) { naMasterAccountPublic.setAccountPublic(naMasterGenerator, iIndex); - Log(lsWARNING) << "authorize: " << iIndex << " : " << naMasterAccountPublic.humanAccountID() << " : " << srcAddress.humanAccountID(); + Log(lsWARNING) << "authorize: " << iIndex << " : " << naMasterAccountPublic.humanAccountID() << " : " << raSrcAddressID.humanAccountID(); - bFound = srcAddress.getAccountID() == naMasterAccountPublic.getAccountID(); + bFound = raSrcAddressID.getAccountID() == naMasterAccountPublic.getAccountID(); if (!bFound) ++iIndex; } @@ -1020,7 +1038,7 @@ Json::Value RPCHandler::handleJSONSubmit(const Json::Value& jvRequest) // The generated pair must match authorized... && naAuthorizedPublic.getAccountID() != naAccountPublic.getAccountID() // ... or the master key must have been used. - && srcAddress.getAccountID() != naAccountPublic.getAccountID()) + && raSrcAddressID.getAccountID() != naAccountPublic.getAccountID()) { // std::cerr << "iIndex: " << iIndex << std::endl; // std::cerr << "sfAuthorizedKey: " << strHex(asSrc->getAuthorizedKey().getAccountID()) << std::endl; diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index 022b25a85..0e44d8cff 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -1813,10 +1813,10 @@ cLog(lsDEBUG) << boost::str(boost::format("PathState: pushed: account=%s currenc // May have an implied node. // Figure out next node properties for implied node. - const uint160 uNxtCurrencyID = spSourcePath.getElementCount() + const uint160 uNxtCurrencyID = spSourcePath.size() ? spSourcePath.getElement(0).getCurrency() : uOutCurrencyID; - const uint160 uNxtAccountID = spSourcePath.getElementCount() + const uint160 uNxtAccountID = spSourcePath.size() ? spSourcePath.getElement(0).getAccountID() : !!uOutCurrencyID ? uOutIssuerID == uReceiverID @@ -2127,11 +2127,21 @@ void RippleCalc::pathNext(PathState::ref pspCur, const int iPaths, const LedgerE } TER RippleCalc::rippleCalc( - LedgerEntrySet& lesActive, // <-> --> = Fee applied to src balance. + // Compute paths vs this ledger entry set. Up to caller to actually apply to ledger. + LedgerEntrySet& lesActive, // <-> --> = Fee already applied to src balance. STAmount& saMaxAmountAct, // <-- The computed input amount. STAmount& saDstAmountAct, // <-- The computed output amount. + + // Issuer: + // XRP: ACCOUNT_XRP + // non-XRP: uSrcAccountID (for any issuer) or another account with trust node. const STAmount& saMaxAmountReq, // --> -1 = no limit. + + // Issuer: + // XRP: ACCOUNT_XRP + // non-XRP: uDstAccountID (for any issuer) or another account with trust node. const STAmount& saDstAmountReq, + const uint160& uDstAccountID, const uint160& uSrcAccountID, const STPathSet& spsPaths, @@ -2194,7 +2204,7 @@ cLog(lsDEBUG) << boost::str(boost::format("rippleCalc: Build direct: add: %d sta } } - cLog(lsINFO) << "rippleCalc: Paths in set: " << spsPaths.getPathCount(); + cLog(lsINFO) << "rippleCalc: Paths in set: " << spsPaths.size(); int iIndex = 0; BOOST_FOREACH(const STPath& spPath, spsPaths) diff --git a/src/cpp/ripple/SerializedTypes.h b/src/cpp/ripple/SerializedTypes.h index ac6cb98b2..7f892bc04 100644 --- a/src/cpp/ripple/SerializedTypes.h +++ b/src/cpp/ripple/SerializedTypes.h @@ -605,7 +605,7 @@ public: STPath(const std::vector& p) : mPath(p) { ; } void printDebug(); - int getElementCount() const { return mPath.size(); } + int size() const { return mPath.size(); } bool isEmpty() const { return mPath.empty(); } const STPathElement& getElement(int offset) const { return mPath[offset]; } const STPathElement& getElement(int offset) { return mPath[offset]; } @@ -683,7 +683,7 @@ public: virtual Json::Value getJson(int) const; SerializedTypeID getSType() const { return STI_PATHSET; } - int getPathCount() const { return value.size(); } + int size() const { return value.size(); } const STPath& getPath(int off) const { return value[off]; } STPath& peekPath(int off) { return value[off]; } bool isEmpty() const { return value.empty(); }