Make ripple_path_find more robust and clean up.

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;				// Only charged fee.
-		terResult	= tesSUCCESS;
 	}
 	else if (saTakerPays.isNative() && saTakerGets.isNative())
 	{
@@ -438,3 +436,4 @@ TER OfferCreateTransactor::doApply()
 
 	return terResult;
 }
+// vim:ts=4

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()<second->mPath.getElementCount()) return(true);
+	if (first->mPath.size()<second->mPath.size()) return(true);
-	if (first->mPath.getElementCount()>second->mPath.getElementCount()) return(false);
+	if (first->mPath.size()>second->mPath.size()) return(false);
 
 	if (first->mMinWidth<second->mMinWidth) 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.
+// If possible, returns a single path.
-// --> maxSearchSteps: unused
+// --> maxSearchSteps: unused XXX
-// --> maxPay: unused
+// --> 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()) {

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);

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(),

src/cpp/ripple/RPCHandler.cpp

@@ -695,6 +695,12 @@ Json::Value RPCHandler::doRippleLinesGet(const Json::Value &params)
 	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		uSrcCurrencyID;
-			uint160		srcIssuerID;
+			uint160		uSrcIssuerID	= raSrc.getAccountID();
 
-			if (!jvSource.isMember("currency")
+			if (
-				|| !STAmount::currencyFromString(srcCurrencyID, jvSource["currency"].asString())
+				// 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, uSrcCurrencyID, uSrcIssuerID, saDstAmount);
-			Pathfinder pf(raSrc, raDst, srcCurrencyID, 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,
+				STAmount	saMaxAmount(
-					!!srcIssuerID
+					uSrcCurrencyID,
-						? srcIssuerID
+					!!uSrcIssuerID
-						: !!srcCurrencyID
+						? 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.
+						false,					// --> Don't limit quality. Average quality is wanted for normal payments.
-												// Average quality is wanted for normal payments.
+						false,					// --> Allow direct ripple.
-						// XXX TRUE till direct path representation resolved.
+						true);					// --> Stand alone mode, no point in deleting unfundeds.
-						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.
 
 				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);
+	AccountState::pointer asSrc	= mNetOps->getAccountState(uint256(0), raSrcAddressID);
-	if(!asSrc) return rpcError(rpcSRC_ACT_MALFORMED);
+	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;
+				txJSON["Fee"] = (int) theConfig.FEE_DEFAULT;
-			else txJSON["Fee"]=(int)theConfig.FEE_ACCOUNT_CREATE;
+			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;

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)

src/cpp/ripple/SerializedTypes.h

@@ -605,7 +605,7 @@ public:
 	STPath(const std::vector<STPathElement>& 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(); }