Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CheckCashMakesTrustLine amendment #3823

Closed
wants to merge 5 commits into from
Closed

CheckCashMakesTrustLine amendment #3823

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
40ebcca
Introduce CheckCashMakesTrustLine amendment:
scottschurr Apr 8, 2021
db69a8e
[FOLD] Address review comments
scottschurr May 17, 2021
f25ee5e
[FOLD] Replace macro with struct
scottschurr May 28, 2021
dea55ab
[fold] Same behavior with fewer edits
thejohnfreeman Jun 11, 2021
69a4b43
[FOLD] Address review comments regarding unit tests
scottschurr Jun 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 131 additions & 22 deletions src/ripple/app/tx/impl/CashCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <ripple/app/paths/Flow.h>
#include <ripple/app/tx/impl/CashCheck.h>
#include <ripple/basics/Log.h>
#include <ripple/basics/scope.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/Indexes.h>
#include <ripple/protocol/STAccount.h>
Expand Down Expand Up @@ -89,13 +90,13 @@ CashCheck::preclaim(PreclaimContext const& ctx)
}

// Only cash a check with this account as the destination.
AccountID const dstId{(*sleCheck)[sfDestination]};
AccountID const dstId = sleCheck->at(sfDestination);
if (ctx.tx[sfAccount] != dstId)
{
JLOG(ctx.j.warn()) << "Cashing a check with wrong Destination.";
return tecNO_PERMISSION;
}
AccountID const srcId{(*sleCheck)[sfAccount]};
AccountID const srcId = sleCheck->at(sfAccount);
if (srcId == dstId)
{
// They wrote a check to themselves. This should be caught when
Expand Down Expand Up @@ -127,7 +128,7 @@ CashCheck::preclaim(PreclaimContext const& ctx)
{
using duration = NetClock::duration;
using timepoint = NetClock::time_point;
auto const optExpiry = (*sleCheck)[~sfExpiration];
auto const optExpiry = sleCheck->at(~sfExpiration);

// Expiration is defined in terms of the close time of the parent
// ledger, because we definitively know the time that it closed but
Expand All @@ -148,7 +149,7 @@ CashCheck::preclaim(PreclaimContext const& ctx)
return optAmount ? *optAmount : tx[sfDeliverMin];
}(ctx.tx)};

STAmount const sendMax{(*sleCheck)[sfSendMax]};
STAmount const sendMax = sleCheck->at(sfSendMax);
Currency const currency{value.getCurrency()};
if (currency != sendMax.getCurrency())
{
Expand All @@ -172,7 +173,7 @@ CashCheck::preclaim(PreclaimContext const& ctx)
{
STAmount availableFunds{accountFunds(
ctx.view,
(*sleCheck)[sfAccount],
sleCheck->at(sfAccount),
value,
fhZERO_IF_FROZEN,
ctx.j)};
Expand All @@ -195,16 +196,31 @@ CashCheck::preclaim(PreclaimContext const& ctx)
// An issuer can always accept their own currency.
if (!value.native() && (value.getIssuer() != dstId))
{
auto const sleIssuer = ctx.view.read(keylet::account(issuerId));
auto const sleTrustLine =
ctx.view.read(keylet::line(dstId, issuerId, currency));

if (!sleTrustLine)
{
JLOG(ctx.j.warn())
<< "Cannot cash check for IOU without trustline.";
return tecNO_LINE;
if (!ctx.view.rules().enabled(featureCheckCashMakesTrustLine))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to do the changes from line 200 (in the original code) to line 225 that requires fewer actual code changes is to just add && !ctx.view.rules().enabled(featureCheckCashMakesTrustLine) to the condition on line 200, and insert this condition on line 218:

if (!sleTrustLine) {
    return tecNO_AUTH;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you, but I'm not really clear on the edit you'd like to see. Can you spell out the proposed change further? Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is as a commit: thejohnfreeman@d57c0e8

{
JLOG(ctx.j.warn())
<< "Cannot cash check for IOU without trustline.";
return tecNO_LINE;
}
else if (sleIssuer)
{
// We can only create a trust line if the issuer does not
// have requireAuth set.
std::uint32_t const issuerFlags = {sleIssuer->at(sfFlags)};
if (issuerFlags & lsfRequireAuth)
return tecNO_AUTH;
}
}

auto const sleIssuer = ctx.view.read(keylet::account(issuerId));
// It would be nice to check this earlier, but moving it earlier
// in the code would be transaction changing (returning different
// tec codes). So we leave it where it is.
if (!sleIssuer)
{
JLOG(ctx.j.warn())
Expand All @@ -213,16 +229,17 @@ CashCheck::preclaim(PreclaimContext const& ctx)
return tecNO_ISSUER;
}

if ((*sleIssuer)[sfFlags] & lsfRequireAuth)
if (sleIssuer->at(sfFlags) & lsfRequireAuth)
{
// Entries have a canonical representation, determined by a
// lexicographical "greater than" comparison employing strict
// weak ordering. Determine which entry we need to access.
bool const canonical_gt(dstId > issuerId);

bool const is_authorized(
(*sleTrustLine)[sfFlags] &
(canonical_gt ? lsfLowAuth : lsfHighAuth));
sleTrustLine &&
(sleTrustLine->at(sfFlags) &
(canonical_gt ? lsfLowAuth : lsfHighAuth)));

if (!is_authorized)
{
Expand Down Expand Up @@ -286,9 +303,10 @@ CashCheck::doApply()
auto viewJ = ctx_.app.journal("View");
auto const optDeliverMin = ctx_.tx[~sfDeliverMin];
bool const doFix1623{ctx_.view().rules().enabled(fix1623)};

if (srcId != account_)
{
STAmount const sendMax{sleCheck->getFieldAmount(sfSendMax)};
STAmount const sendMax = sleCheck->at(sfSendMax);

// Flow() doesn't do XRP to XRP transfers.
if (sendMax.native())
Expand Down Expand Up @@ -334,19 +352,106 @@ CashCheck::doApply()
}
else
{
// Let flow() do the heavy lifting on a check for an IOU.
//
// Note that for DeliverMin we don't know exactly how much
// currency we want flow to deliver. We can't ask for the
// maximum possible currency because there might be a gateway
// transfer rate to account for. Since the transfer rate cannot
// exceed 200%, we use 1/2 maxValue as our limit.
STAmount const flowDeliver{
optDeliverMin
? STAmount{optDeliverMin->issue(), STAmount::cMaxValue / 2, STAmount::cMaxOffset}
: static_cast<STAmount>(ctx_.tx[sfAmount])};
optDeliverMin ? STAmount(
optDeliverMin->issue(),
STAmount::cMaxValue / 2,
STAmount::cMaxOffset)
: ctx_.tx.getFieldAmount(sfAmount)};

// If a trust line does not exist yet create one.
Issue const& trustLineIssue = flowDeliver.issue();
AccountID const issuer = flowDeliver.getIssuer();
AccountID const truster = issuer == account_ ? srcId : account_;
Keylet const trustLineKey = keylet::line(truster, trustLineIssue);
bool const destLow = issuer > account_;

bool const checkCashMakesTrustLine =
psb.rules().enabled(featureCheckCashMakesTrustLine);

if (checkCashMakesTrustLine && !psb.exists(trustLineKey))
{
// 1. Can the check casher meet the reserve for the trust line?
// 2. Create trust line between destination (this) account
// and the issuer.
// 3. Apply correct noRipple settings on trust line. Use...
// a. this (destination) account and
// b. issuing account (not sending account).

// Can the account cover the trust line's reserve?
if (std::uint32_t const ownerCount = {sleDst->at(sfOwnerCount)};
mPriorBalance < psb.fees().accountReserve(ownerCount + 1))
{
JLOG(j_.trace()) << "Trust line does not exist. "
"Insufficent reserve to create line.";

// Call the payment engine's flow() to do the actual work.
return tecNO_LINE_INSUF_RESERVE;
}

Currency const currency = flowDeliver.getCurrency();
STAmount initialBalance(flowDeliver.issue());
initialBalance.setIssuer(noAccount());

// clang-format off
if (TER const ter = trustCreate(
psb, // payment sandbox
destLow, // is dest low?
issuer, // source
account_, // destination
trustLineKey.key, // ledger index
sleDst, // Account to add to
false, // authorize account
(sleDst->getFlags() & lsfDefaultRipple) == 0,
false, // freeze trust line
initialBalance, // zero initial balance
Issue(currency, account_), // limit of zero
0, // quality in
0, // quality out
viewJ); // journal
!isTesSuccess(ter))
{
return ter;
}
// clang-format on

// Note that we _don't_ need to be careful about destroying
// the trust line if the check cashing fails. The transaction
// machinery will automatically clean it up.
}

// Since the destination is signing the check, they clearly want
// the funds even if their new total funds would exceed the limit
// on their trust line. So we tweak the trust line limits before
// calling flow and then restore the trust line limits afterwards.
auto const sleTrustLine = psb.peek(trustLineKey);
if (!sleTrustLine)
return tecNO_LINE;

SF_AMOUNT const& tweakedLimit = destLow ? sfLowLimit : sfHighLimit;
STAmount const savedLimit = sleTrustLine->at(tweakedLimit);

// Make sure the tweaked limits are restored when we leave scope.
scope_exit fixup(
[&psb, &trustLineKey, &tweakedLimit, &savedLimit]() {
if (auto const sleTrustLine = psb.peek(trustLineKey))
sleTrustLine->at(tweakedLimit) = savedLimit;
});

if (checkCashMakesTrustLine)
{
// Set the trust line limit to the highest possible value
// while flow runs.
STAmount const bigAmount(
trustLineIssue, STAmount::cMaxValue, STAmount::cMaxOffset);
sleTrustLine->at(tweakedLimit) = bigAmount;
}

// Let flow() do the heavy lifting on a check for an IOU.
auto const result = flow(
psb,
flowDeliver,
Expand Down Expand Up @@ -376,18 +481,22 @@ CashCheck::doApply()
<< "flow did not produce DeliverMin.";
return tecPATH_PARTIAL;
}
if (doFix1623)
if (doFix1623 && !checkCashMakesTrustLine)
// Set the delivered_amount metadata.
ctx_.deliver(result.actualAmountOut);
}
// Set the delivered amount metadata in all cases, not just
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to checkCashMakesTrustLine, why was the delivered amount metadata set only if DeliverMin was used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @intelliot, I believe that was a bug. As part of this pull request I added some unit tests. I expected that in all cases the result of a successful CheckCash transactions would include a delivered_amount field. I found that delivered_amount was not always set. I surmised delivered_amount was not set because DeliveredAmount was not always set due to the conditional above.

When I changed the code so setting DeliveredAmount was only conditional on the amendment then the unit tests returned a delivery_amount field in all of the tested cases.

// for DeliverMin.
if (checkCashMakesTrustLine)
ctx_.deliver(result.actualAmountOut);
}
}

// Check was cashed. If not a self send (and it shouldn't be), remove
// check link from destination directory.
if (srcId != account_)
{
std::uint64_t const page{(*sleCheck)[sfDestinationNode]};
std::uint64_t const page = {sleCheck->at(sfDestinationNode)};
if (!ctx_.view().dirRemove(
keylet::ownerDir(account_), page, sleCheck->key(), true))
{
Expand All @@ -397,7 +506,7 @@ CashCheck::doApply()
}
// Remove check from check owner's directory.
{
std::uint64_t const page{(*sleCheck)[sfOwnerNode]};
std::uint64_t const page = {sleCheck->at(sfOwnerNode)};
if (!ctx_.view().dirRemove(
keylet::ownerDir(srcId), page, sleCheck->key(), true))
{
Expand Down
2 changes: 2 additions & 0 deletions src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class FeatureCollections
"FlowSortStrands",
"fixSTAmountCanonicalize",
"fixRmSmallIncreasedQOffers",
"CheckCashMakesTrustLine",
};

std::vector<uint256> features;
Expand Down Expand Up @@ -378,6 +379,7 @@ extern uint256 const featureTicketBatch;
extern uint256 const featureFlowSortStrands;
extern uint256 const fixSTAmountCanonicalize;
extern uint256 const fixRmSmallIncreasedQOffers;
extern uint256 const featureCheckCashMakesTrustLine;

} // namespace ripple

Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ detail::supportedAmendments()
"FlowSortStrands",
"fixSTAmountCanonicalize",
"fixRmSmallIncreasedQOffers",
"CheckCashMakesTrustLine",
};
return supported;
}
Expand Down Expand Up @@ -192,7 +193,8 @@ uint256 const
featureTicketBatch = *getRegisteredFeature("TicketBatch"),
featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"),
fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"),
fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers");
fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers"),
featureCheckCashMakesTrustLine = *getRegisteredFeature("CheckCashMakesTrustLine");

// The following amendments have been active for at least two years. Their
// pre-amendment code has been removed and the identifiers are deprecated.
Expand Down
Loading