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.

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

Use payment flow code for offer crossing (RIPD-1094): #1884

Closed
wants to merge 2 commits into from

Conversation

@scottschurr
Copy link
Contributor

commented Oct 22, 2016

Replace Taker.cpp with calls to the new payment flow code.

This change required a number of tweaks in the payment flow code. These tweaks are conditionalized on whether or not offer crossing is taking place. The flag is explicitly passed as a parameter to the flow code.

For testing, a class was added that identifies differences in the contents of two PaymentSandboxes. That code may be reusable in the future.

None of the V1 offer crossing code is removed. Both versions of the code are co-resident to support an amendment cut-over.

The code that identifies differences between V1 and V2 offer crossing is also on an amendment. That makes it easy to enable or disable difference logging by changing the config file. This approach models what was done with the payment Flow code. The differencing code should never be enabled on the network.

Extensive offer crossing unit tests are added to examine and verify the behavior of corner cases. The tests are currently configured to run against both V1 and V2 offer crossing. This gives us confidence that most cases run identically and some of the (few) differences in behavior are documented.

Documentation preview: https://scottschurr.github.io

@codecov-io

This comment has been minimized.

Copy link

commented Oct 22, 2016

Codecov Report

Merging #1884 into develop will decrease coverage by 0.29%.
The diff coverage is 54.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #1884     +/-   ##
==========================================
- Coverage    69.23%   68.94%   -0.3%     
==========================================
  Files          682      684      +2     
  Lines        49698    50406    +708     
==========================================
+ Hits         34409    34750    +341     
- Misses       15289    15656    +367
Impacted Files Coverage Δ
src/ripple/ledger/View.h 100% <ø> (ø) ⬆️
src/ripple/app/paths/RippleCalc.cpp 81.65% <ø> (ø) ⬆️
src/ripple/app/tx/impl/CreateOffer.h 100% <ø> (ø) ⬆️
src/ripple/app/tx/impl/OfferStream.h 90.9% <ø> (ø) ⬆️
src/ripple/app/main/Amendments.cpp 100% <ø> (ø) ⬆️
src/ripple/ledger/detail/ApplyViewBase.h 100% <ø> (ø) ⬆️
src/ripple/ledger/detail/ApplyStateTable.h 60% <ø> (ø) ⬆️
src/ripple/app/paths/impl/Steps.h 83.33% <ø> (+0.35%) ⬆️
src/ripple/ledger/impl/PaymentSandbox.cpp 60.67% <0%> (ø) ⬆️
src/ripple/ledger/CashDiff.h 0% <0%> (ø)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84816d1...cb6920c. Read the comment docs.

std::uint32_t const txFlags =ctx_.tx.getFlags();
if (txFlags & tfPassive)
{
return { tesSUCCESS, takerAmount };

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 22, 2016

Member

I haven't delved deeply into the code, so this is just a quick comment based on a cursory reading.

I'm not sure that this is the right thing. tfPassive means to not consume offers at the same quality level, but crossing still needs to be performed, as there might be offers at a better quality level. This code seems to just not do any crossing when tfPassive is set.

Am I missing something obvious here?

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 24, 2016

Author Contributor

Nope, you're reading it correctly. And I think you just found a bug. I misunderstood what the tfPassive flag did. Interestingly, this particular implementation worked with all of the unit tests. I'll need to fix the implementation and add a unit test.

@seelabs
Copy link
Contributor

left a comment

We also need to add OfferCrossV2 to Amendments.cpp

@@ -44,9 +44,11 @@ struct FlowDebugInfo;
@param defaultPaths Include defaultPaths in the path set
@param partialPayment If the payment cannot deliver the entire
requested amount, deliver as much as possible, given the constraints
@param ownerPaysTransferFee If true then owner, not receiver, pays fee

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

If true then owner, not sender, pays fee

@@ -44,9 +44,11 @@ struct FlowDebugInfo;
@param defaultPaths Include defaultPaths in the path set
@param partialPayment If the payment cannot deliver the entire
requested amount, deliver as much as possible, given the constraints
@param ownerPaysTransferFee If true then owner, not receiver, pays fee
@param offerCrossing We are offer crossing, so use special rules

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

Don't use We, and the fact there is a flag implies there are special rules, not need to call it out. How about: If true then flow is executing offer crossing, not payments

@@ -58,6 +60,7 @@ flow (PaymentSandbox& view,
bool defaultPaths,
bool partialPayment,
bool ownerPaysTransferFee,
bool offerCrossing,
boost::optional<Quality> const& limitQuality,
boost::optional<STAmount> const& sendMax,

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

There are four boolean parameters followed by two boost optionals. Calls are going to look like flow(...false, false, false, false, boost::none, boost::none,...). Thoughts on how to improve this interface?

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 26, 2016

Author Contributor

Yeah. This wasn't pretty before, with 3 bools and an optional. I made it worse by adding another bool

I can see two options: an OR-able enum, or make flow a class with an invoke() method.

We could replace the four bools with a single OR-able enum. I'd have to try it to see if it was an improvement.

If we made flow a class then we could call setup methods on it prior to the invoke. That would hide some of the messy aspects of the setup, but it would also make it less clear what the essential parameters are.

I'd need to run an experiment to see whether either of these would be a win. Shall I try the enum experiment and see what it looks like?

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

We tried the class solution, with RippleCalc (see RippleCalc::Input) and I'm not quite sold. The OR-able enum solution seems better to me.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Nov 1, 2016

Author Contributor

Here's a look at an approach using an OR-able enum:
scottschurr@8e6e0c9.
If this looks like an improvement to you I'll cherry pick it into this pull request.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 1, 2016

Contributor

Or-able enums are an improvement over what's there. The downside is we can't require that all the flags be specified (at compile time). I think it's a good idea to force the caller to specify all these flags.

I know we frown on enum classes for boolean switches, but I think that's the best solution here. It's type safe, documents the parameters, and forces the caller to specify each parameter. I also don't want to re-litigate an issue that's already been decided. If there are objections I'd go with OR-able enums. @vinniefalco What do you think?

@@ -103,6 +103,18 @@ struct EitherAmount
else
iou = a.iou;
}

explicit
EitherAmount (STAmount const& a)

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

This constructor is not needed. We can remove it.

Step const* const prevStep_ = nullptr;
bool const ownerPaysTransferFee_;
bool const removeFullyConsumedOffers_;
bool const noXferFeeIfSourceIsDest_;
boost::optional<Quality> const limitSelfCrossingQuality_;

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

I would love to keep the state used for offer crossing separate from the state used in payments. Let's consider what separating new new state into a new subclass would look like. We'd have a two new concrete classes, one for offer crossing and one for payments. We'd have virtual functions for transfer rate out, and transfer rate in. This makes it very clear what the differences are between offer x-ing and payments, and it also removes unused state (which we have to remember is unused when understanding just payments). I understand there are costs to this - the new virtual functions can make the existing code harder to follow and new classes also add complexity.

I'm not asking you to code this up. I'm asking you consider what such a solution would look like and if you think the trade-offs involved in that are worth the change or not.


// A class to identify differences between two ApplyStateTable instances
// for debugging.
class CashDiff

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

Bikeshed: Cash?

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

I'm way open to suggestions here. I was looking for a short word that was reminiscent of money. At least at this point the class only looks at money-related changes to ApplyStateTables, although that could change in the future. Possibly ApplyStateDiff?

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Nov 1, 2016

Contributor

This belongs in the unit test source directory since its only used for testing. And because its not part of any public APIs, the requirements are relaxed when choosing identifier names. Its fine the way it is (but the code has to be moved).

AccountID, XRPAmount>> xrpChanges;

std::vector<std::pair<
std::tuple<AccountID, AccountID, Currency>, STAmount>> trustChanges;

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

A pair where the first element is a 2-element tuple seems a little strange. I'd probably flatten these (tho no change needed)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

There's actually a reason for the odd layout of these pairs. It makes

template <typename T, typename U>
static std::array<std::size_t, 3> countKeys (...

and

template <typename T, typename U, typename L>
static bool rmVecDust (...

work for all of these types. The first element of the pair acts like the key in a map, the second pair element is the value.

return false;

// v1 and v2 must be the same Issue for their difference to make sense.
if (v1.native() ^ v2.native())

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

!= is clearer

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Excellent point. I must have had my bit-twiddling hat on when I wrote that....

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 27, 2016

Member

Same for negative above. You can run that comparison using signum.

}

// Non-native. Note that even though large and small may not be equal,
// their difference may be zero if they are large values. We treat

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

I think it's when their mantissas differ by less than ten, not when they are "large values".

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Thanks for the nudge on that. I was not clear on how this could happen; now I am. Here's the new comment:

    // Non-native.  Note that even though large and small may not be equal,
    // their difference may be zero.  One way that can happen is if two
    // values are different, but their difference results in an STAmount
    // with an exponent less than -96.
@@ -312,8 +312,7 @@ PaymentSandbox::balanceChanges (ReadView const& view) const
break;
}
}

if (!before)
else if (!before)

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 25, 2016

Contributor

Wasn't this handled on line 290 already?

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

I think the if (!before) on line 290 is in one scope too far to help here. Good question though.

@scottschurr
Copy link
Contributor Author

left a comment

Thanks for the excellent comments. I've addressed the easy ones. I'll need to think a little harder on a few of them.

@@ -231,6 +231,7 @@ bool operator==(Strand const& lhs, Strand const& rhs)
@param dst Account that is receiving assets
@param deliver Asset the dst account will receive
(if issuer of deliver == dst, then accept any issuer)
@param limitQuality Do not use liquidity below this quality threshold

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

True. Do you have a suggestion for improved wording?

@@ -245,9 +246,11 @@ toStrand (
AccountID const& src,
AccountID const& dst,
Issue const& deliver,
boost::optional<Quality> const& limitQuality,

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

At this point it's exactly the same value as was passed to the top-most flow. The BookStep will only pick up the value if it's offer crossing. I think the BookStep constructor is where the meaning changes?

// we're handling a FillOrKill offer. In this case remainingIn must
// be zero (all funds must be consumed) or else we kill the offer.
assert (remainingIn);
if (remainingIn && *remainingIn != zero)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Interesting. I'm not one to second guess gcc, but a code inspection didn't show me what it's worried about. The declaration of remainingIn is this:

boost::optional<TInAmt> remainingIn (sendMax);

Presumably in the case gcc is worried about is_same<TInAmt, XRPAmount>::value == true. I can't see any way for remainingIn != boost::none and the contained XRPAmount not to be fully initialized. Thoughts? Thanks for noticing.

// currently exist.
bool reduceReserve = ctx.offerCrossing && ctx.isFirst;
if (reduceReserve)
reduceReserve = !ctx.view.read (keylet::line (acc, ctx.strandDeliver));

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

I'm ever so good with that suggested change. But, as you say, that would need to go up the chain.

ss << ctx_.tx.getTransactionID();
auto const txID = ss.str();
char const* txIDptr = txID.c_str();
(void) txIDptr;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Gone. Yup, it was just for debugging individual transactions. If I need it again I can recreate it.

AccountID, XRPAmount>> xrpChanges;

std::vector<std::pair<
std::tuple<AccountID, AccountID, Currency>, STAmount>> trustChanges;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

There's actually a reason for the odd layout of these pairs. It makes

template <typename T, typename U>
static std::array<std::size_t, 3> countKeys (...

and

template <typename T, typename U, typename L>
static bool rmVecDust (...

work for all of these types. The first element of the pair acts like the key in a map, the second pair element is the value.

return false;

// v1 and v2 must be the same Issue for their difference to make sense.
if (v1.native() ^ v2.native())

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Excellent point. I must have had my bit-twiddling hat on when I wrote that....

}

// Non-native. Note that even though large and small may not be equal,
// their difference may be zero if they are large values. We treat

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

Thanks for the nudge on that. I was not clear on how this could happen; now I am. Here's the new comment:

    // Non-native.  Note that even though large and small may not be equal,
    // their difference may be zero.  One way that can happen is if two
    // values are different, but their difference results in an STAmount
    // with an exponent less than -96.
@@ -312,8 +312,7 @@ PaymentSandbox::balanceChanges (ReadView const& view) const
break;
}
}

if (!before)
else if (!before)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

I think the if (!before) on line 290 is in one scope too far to help here. Good question though.

Step const* const prevStep_ = nullptr;
bool const ownerPaysTransferFee_;
bool const removeFullyConsumedOffers_;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 27, 2016

Author Contributor

I think you're right. This adaptation allowed me to run in parallel while you were working on your fix. If I set all of my unit tests to use the amendmentRIPD1298SoTime() then your suggestion should work. I'd like to make that entire change in a single commit.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

Documentation preview?

@nbougalis
Copy link
Member

left a comment

No show-stoppers. Most of my comments are nits.

@@ -58,6 +60,7 @@ flow (PaymentSandbox& view,
bool defaultPaths,
bool partialPayment,
bool ownerPaysTransferFee,
bool offerCrossing,
boost::optional<Quality> const& limitQuality,
boost::optional<STAmount> const& sendMax,

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

We tried the class solution, with RippleCalc (see RippleCalc::Input) and I'm not quite sold. The OR-able enum solution seems better to me.

@@ -47,7 +47,8 @@ supportedAmendments ()
{ "42426C4D4F1009EE67080A9B7965B44656D7714D104A72F9B4369F97ABF044EE FeeEscalation" },
{ "9178256A980A86CF3D70D0260A7DA6402AAFE43632FDBCB88037978404188871 OwnerPaysFee"},
{ "08DE7D96082187F6E6578530258C77FAABABE4C20474BDB82F04B021F1A68647 PayChan"},
{ "740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D628A06927F11 Flow"}
{ "740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D628A06927F11 Flow"},
{ "19C4083F561CFE3ED5ABCA8F3B4AC1EFDEEC8225A11E247247EDE827DF9F91AE OfferCrossV2"}

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Minor nit: I'd just call this CrossFlow; no V2 or anything.

auto const ofrOutRate =
noXferFeeIfSourceIsDest_ && // If offer crossing
prevStep_ && prevStep_->bookStepBook() && // && prevStep is BookStep
offer.owner() == strandDst_ // && dest is offer owner

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Agreed.

// When offerCrossing then ignore trust line quality.
bool const offerCrossing_;

// When isLast and offerCrossing then ignore trustline limits.

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Why? A bit more exposition might be helpful.

// The previous checks apply in all cases. The following checks apply
// only if we're not offer crossing because they require a preexisting
// trustline.
if (offerCrossing_)

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Let's say I want to place an offer to acquire IOU/GATEWAY, an asset for which I do not have a trust line. My offer goes on the books. At some point in the future, GATEWAY adds lsfRequireAuth and then, a couple of minutes later, my offer is crossed.

I shouldn't be able to acquire any funds, because I'm not authorized to hold GATEWAY assets; where, exactly, do we check for that?

// currently exist.
bool reduceReserve = ctx.offerCrossing && ctx.isFirst;
if (reduceReserve)
reduceReserve = !ctx.view.read (keylet::line (acc, ctx.strandDeliver));

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

I'm fine with changing this behavior. @JoelKatz?

// below the reserve) so we check this case again.
STAmount const inStartBalance = accountFunds (
psb, account_, takerAmount.in, fhZERO_IF_FROZEN, j_);
if (inStartBalance.signum() <= 0)

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

I'd prefer the more "obvious" check here: if (inStartBalance <= zero) which clearly expresses the code's intent.

@@ -262,6 +210,89 @@ accountFunds (ReadView const& view, AccountID const& id,
return saFunds;
}

static std::uint32_t
computeOwnerCount (std::uint32_t curOwnerCount, std::int32_t adj)

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Minor nit: we replicate this logic in adjustOwnerCount. I'd like to see only a single function to handle this.

@@ -234,6 +234,20 @@ class Quality

friend
bool
operator<= (Quality const& lhs, Quality const& rhs) noexcept

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 30, 2016

Member

Should we consider using boost::totally_ordered? See Rate, XRPAmount and IOUAmount.

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Nov 1, 2016

Contributor

I would ask @HowardHinnant

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Nov 1, 2016

Contributor

I don't think it is a huge deal either way. But I don't recommend it. The benefit is small and the cost is small. The benefit is that you don't have to write 4 boiler-plate operators. The cost is that Quality now has boost as an associated namespace which will be used for ADL. Also readers have to look into boost docs to make sure that this does exactly what one would guess it does. And trying to figure that out from reading the boost implementation is virtually impossible.


// Used by CashDiff to specify filters applied while processing differences.
// Entries are bit flags that can be ANDed and ORed.
enum class CashFilter : std::uint8_t

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Nov 1, 2016

Contributor

If this is test code it should not be in the mainline sources, please move it to src/test somewhere

This comment has been minimized.

Copy link
@scottschurr

scottschurr Nov 1, 2016

Author Contributor

CashDiff performs a similar function for offer crossing that FlowDebugInfo performs for flow: https://github.com/ripple/rippled/blob/develop/src/ripple/app/paths/impl/FlowDebugInfo.h#L41. The primary difference being that CashDiff looks at changes to offers in addition to changes to trust lines and changes to the XRP balance in account roots. Scott and I talked about whether I should modify his differencing tool. We agreed that it would probably be better if I made my own.

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

I finished an approach to separating the payment-specific code from the offer-crossing-specific code and state in the three StepImp classes. The approach uses a low-budget version of policy-oriented design. I did the work on a different branch so you folks can look it over and see what you think. If what I've done looks good enough then I can cherry pick the commit onto this branch. Here's the code: scottschurr@6dca5f2

Let me know what you think. Thanks.

@seelabs

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

(I left a similar comment on the policy commit)

I am in favor of the changes introduced here: scottschurr@6dca5f2. It clearly marks the differences between payments and offer crossing, makes them easier to reason about separately, and makes it easier to try to unify the rules between offer x-ing and payments. Two nits:

Move the comments pertaining to offer x-ing to the policy class. The fact that a function call goes through a policy class already tells me there's a difference, no need to call it out in a comment in the mainline code. Then put the reason for the difference in the policy class. I'll mark the places I have in mind in the main PR if this is cherry picked.

In DirectStepI I'd make the verify functions return a bool and then put them into an assert in the main-line code.

@bachase

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

Since no good engineering deed goes unpunished ;), it would be cool to see the documentation for the policy type. It would certainly help me get a better handle on how the different use cases intersect.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

This needs a doc preview

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

@nbougalis, do you have an opinion one way or the other on the policy-based design: scottschurr@6dca5f2? If you are okay with that approach I'll cherry-pick that commit (with a couple of tweaks) into this branch.

@bachase, regarding the documentation, not disagreeing, I'm just wondering what information you are looking for. Are you wondering why there are policies? Why each policy does what it does? Should the documentation be doxygen-style, or are simple comments adequate?

@vinniefalco, thanks for the reminder. Yes I know I need to produce a doc preview. That's coming after some of the technical issues are settled.

@seelabs, I'm working on a couple of different approaches as alternatives to the OR-able enum. Still under construction.

@scottschurr scottschurr added the Rebase label Nov 4, 2016

@bachase

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

A mix of those, both what a policy is abstractly and what it must implement. Then, the general purpose of
each individual policy. I was able to identify comments on the latter, but I'm not as familiar with exactly how they interact with the flow code to provide different behavior.

Ideally as doxygen style but I'm not sure how much work that is on top of everything else.

@nbougalis

This comment has been minimized.

Copy link
Member

commented Nov 4, 2016

@scottschurr, I am fine with the Policy

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2016

In addition to the OR-able enum, I now have two other approaches to keeping the arguments to flow() distinct: 2-valued enums, and a tagged bool-like template. So at this point the options are:

o Leave the parameters as is.
o Reduce the total number of arguments with an OR-able enum: scottschurr@8e6e0c9
o Turn the four bools into 2-valued enums: scottschurr@3c31ba3
o Turn the four bools into TaggedBool<>: scottschurr@818c633

Let me know your favorite or if there's yet another approach I should try. Thanks.

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from 4e00f29 to 474d12c Nov 6, 2016

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2016

Rebased to 0.40.0-b9.

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from 474d12c to fb7d46b Nov 6, 2016

@scottschurr scottschurr removed the Rebase label Nov 6, 2016

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch 2 times, most recently from f486384 to 0a19fa2 Nov 7, 2016

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

Policies to separate payment behavior from offer crossing behavior in StepImp classes introduced.

@JoelKatz

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

Phew. LGTM. 👍

}

// For offer crossing don't pay the transfer fee if alice is paying alice.
// The justification here is that the funds are not really changing hands;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

The justification here (that "funds are not really changing hands") is misleading. It implies that transfer fees apply when funds change hands. In the chain of accounts A->B->C, a transfer fee applies when A redeems to B and B issues to C. Funds can flow in other ways (A issues to B and B issues to C) with no fee.

// For offer crossing don't pay the transfer fee if alice is paying alice.
// The justification here is that the funds are not really changing hands;
// they go from alice to alice. So it is inappropriate to charge a
// transfer fee. Payments are too complicated to apply this rule uniformly.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

I would just say "Payments do not apply this rule".

The reason this is harder to apply to payments isn't that "Payments are too complicated to apply this rule". "flow" decomposes a payment into pairs of accounts (direct steps) or offers (book steps). The transfer fee complicates this - we need to look at triples of accounts to know if a transfer fee applies (note: offer x-ing can get away with looking at the strand endpoints, payments cannot).

This rule adds additional complexity to payments because it requires a step to look into the previous and next step and depending on accounts in those steps charge different fees. For direct steps/offer pairs this is probably doable (and we may want to consider doing that since it removes a difference between payments and offer x-ing). For offer/offer pairs I think it's too much complexity for too little benefit.


//------------------------------------------------------------------------------

template<class TIn, class TOut, template <class In, class Out> class Policy>

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

The policy class doesn't need to be a template template. Make a concrete policy class when instantiating BookStep (i.e. BookStep<IOUAmount, IOUAmount, BookPyamentPolicy<IOUAmount, IOUAmount>>

This comment has been minimized.

Copy link
@scottschurr

scottschurr Nov 22, 2016

Author Contributor

My personal preference is to leave the Policy as a template template. The Policy actually is a template. Also, the template template implementation guarantees that the TIn and TOut parameters of the Policy and the BookStep are the same, which is exactly what we want. Yes, since we're doing explicit instantiation we could just do the right thing at the construction site. But I think this approach is slightly more maintainable and more representative of what's actually up.

That said, I'm willing to go with your approach if you feel strongly. Since flow is primarily your baby, you need to be comfortable with what's here.

// Return the quality for a given offer.
template <class Policy>
std::uint32_t
quality (DirectStepI<Policy> const& step,

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

The step parameter's policy should match this class. Can we change the parameter from generic to DirectStepI<DirectIPaymentPolicy>? (Same comment for other functions that take a DirectStepI parameter in the policy class).

// The definition uses DirectStepI, so it comes later in the file.
template <class Policy>
std::pair<IOUAmount, bool>
inline maxFlow (DirectStepI<Policy> const& step,

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

Nit: no need for inline

// return: first element is max amount that can flow,
// second is true if dst holds an iou from src.
//
// The definition uses DirectStepI, so it comes later in the file.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

Consider moving the declaration of policy classes after the step classes (I'd also remove the comment even if you keep the decl here).

bool const offerCrossing_;
// Policy that distinguishes behavior between payments and offer crossing.
Policy policy_;
friend Policy;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

Policy is feeling more like it should be a subclass.

The goal of this commit is to isolate the differences between payments and offer crossing into their own entities. the Policy implementation does that, and it's a win. We can use inheritance to implement the same thing (and CRTP to avoid virtual function callls). Now that you've implemented a policy-based design, do you still favor this over inheritance?

This comment has been minimized.

Copy link
@scottschurr

scottschurr Nov 22, 2016

Author Contributor

Yeah, I feel good about aggregating the Policy rather than inheriting from it. That said, I have a long history of preferring aggregation over inheritance. I also have prior experience with using aggregation for Policy-based designs, so this feels pretty comfortable to me. And I've never personally used the CRTP in one of my own designs. I can't definitively say that the aggregation approach is unquestionably superior, but I like it.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 28, 2016

Contributor

Consider the direct step policy classes. There are five functions defined there. Three of them pass in the step as a parameter and these policy classes are friends. It seems cleaner to implement that kind of relationship through inheritance.

This kind of relationship is also showing up in new code. In the optimization commit. qualityUpperBound passes in three parameters that are member vars on the step class. It seems cleaner if qualityUpperBound was a member of the step class rather than passing that state in.

}

return tesSUCCESS;
// Perform any checks that are specific to the Policy.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 15, 2016

Contributor

Nit: This is commenting obvious code. I'm making a note because I noticed a couple comments like this in the PR. No change needed for this PR, but I would raise your threshold for what to comment one notch.

@scottschurr scottschurr added the Rebase label Nov 23, 2016

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from bbfd8bf to 0f72ab0 Nov 24, 2016

@scottschurr scottschurr removed the Rebase label Nov 24, 2016

@scottschurr scottschurr removed the Rebase label Jan 4, 2017

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2017

Squashed and rebased.

@HowardHinnant

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

With boost 1.63:

src/ripple/ledger/impl/CashDiff.cpp:555:20: error: no viable overloaded '='
        dropsGone_ = {lhs.dropsDestroyed(), rhs.dropsDestroyed()};
        ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Do we want to fix this? The fix is simple:

        dropsGone_ = DropsGone{lhs.dropsDestroyed(), rhs.dropsDestroyed()};

With this change, this P/R compiles against boost 1.63.

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2017

@HowardHinnant: Great catch! I'll put in a fix for this today.

@HowardHinnant

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

👍 Sorry for the late review. The Assignee system we previously used is so much better at telling reviewers who they are.

@scottschurr scottschurr added the Rebase label Jan 17, 2017

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from 214a09a to eaae9b3 Jan 18, 2017

@scottschurr scottschurr removed the Rebase label Jan 18, 2017

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from eaae9b3 to 022e8c8 Jan 30, 2017

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2017

Rebased to 0.50.1

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from 022e8c8 to e463979 Mar 17, 2017

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2017

Rebased to 0.60.0. Waiting for the Ripd1375 pull request (#2015) to be merged. Then I'll rebase on top of that again.

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from e463979 to 30f1782 Mar 28, 2017

@scottschurr scottschurr removed the Rebase label Mar 28, 2017

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

Rebased to 0.70.0. I spent some additional time re-testing the rebased code against network traffic, which is still looking good.

@scottschurr scottschurr added the Rebase label Apr 1, 2017

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from 30f1782 to c115e97 Apr 4, 2017

@scottschurr scottschurr removed the Rebase label Apr 4, 2017

// testAll({ });
// testAll({ featureFlowCross});
// testAll({featureFlow });
// testAll({featureFlow, featureFlowCross});

This comment has been minimized.

Copy link
@seelabs

seelabs Apr 4, 2017

Contributor

The test above should not be commented out. It's an important test case.

Edit: To be clear, I only mean the test on line 4213

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from c115e97 to 3839001 Apr 5, 2017

@scottschurr scottschurr added the Passed label Apr 6, 2017

scottschurr added some commits Mar 16, 2016

Use payment flow code for offer crossing (RIPD-1094):
Replace Taker.cpp with calls to the payment flow() code.

This change required a number of tweaks in the payment flow code.
These tweaks are conditionalized on whether or not offer crossing
is taking place.  The flag is explicitly passed as a parameter to
the flow code.

For testing, a class was added that identifies differences in the
contents of two PaymentSandboxes.  That code may be reusable in
the future.

None of the Taker offer crossing code is removed.  Both versions
of the code are co-resident to support an amendment cut-over.

The code that identifies differences between Taker and Flow offer
crossing is enabled by a feature.  That makes it easy to enable
or disable difference logging by changing the config file.  This
approach models what was done with the payment flow code.  The
differencing code should never be enabled on a production server.

Extensive offer crossing unit tests are added to examine and
verify the behavior of corner cases.  The tests are currently
configured to run against both Taker and Flow offer crossing.
This gives us confidence that most cases run identically and
some of the (few) differences in behavior are documented.

@scottschurr scottschurr force-pushed the scottschurr:flow-offer-cross-8 branch from e9dbb68 to cb6920c Apr 20, 2017

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

Rebased to 0.70.0-b4. I also ran the rebased code against the live network for 3 hours. The agreement between Taker and Flow remained pretty consistent with previous test results. So, as far as I can tell, we're good to go.

@scottschurr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

Merged to rippled develop as 369909d and d5dc715.

@scottschurr scottschurr deleted the scottschurr:flow-offer-cross-8 branch Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.