Track escrow in recipient's owner directory (RIPD-1523): #2212

nbougalis · 2017-08-25T09:30:14Z

Introduce "fix1523" which corrects a minor technical flaw with
the original implementation of the escrow feature.

When creating an escrow, the entry would only be tracked in the
owner directory of the sender; as a result, an escrow recipient
would not be able to detect incoming escrows without monitoring
the ledger in real-time for transactions of interest or without
the sender communicating this information out of band.

With the fix in place, escrows where the recipient differs from
the sender will be listed in the recipient's owner directory as
well.

bachase

Looks good with some minor caveats.

bachase · 2017-08-25T14:23:51Z

src/ripple/app/main/Amendments.cpp

@@ -57,7 +57,8 @@ supportedAmendments ()
        { "DC9CA96AEA1DCF83E527D1AFC916EFAF5D27388ECA4060A88817C1238CAEE0BF EnforceInvariants" },
        { "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" },
        { "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" },
-        { "B4D44CC3111ADD964E846FC57760C8B50FFCD5A82C86A72756F6B058DDDF96AD fix1201" }
+        { "B4D44CC3111ADD964E846FC57760C8B50FFCD5A82C86A72756F6B058DDDF96AD fix1201" },
+        { "6E3FF89E20D93A6C51FAE78409EB68B74DCEA0FA8AA50A2A364630241DC18976 fix1523" }


B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D for fix1523?

Oops. Last minute rename!

bachase · 2017-08-25T14:30:23Z

src/test/app/Escrow_test.cpp

+        {
+            testcase ("Metadata (with fix1523, to self)");
+
+            Env env(*this, with_features(featureEscrow));


Should this be with_features(featureEscrow, fix1523) to match the message on L710?

bachase · 2017-08-25T14:31:39Z

src/test/app/Escrow_test.cpp

+            // Verify that Carol's owner directory tracks the escrow entry too.
+            ripple::Dir cod (*env.current(), keylet::ownerDir(carol.id()));
+            BEAST_EXPECT(std::distance(cod.begin(), cod.end()) == 1);
+            BEAST_EXPECT(std::find(cod.begin(), cod.end(), escrow) != cod.end());


Is it worth adding a test to confirm EscrowFinish and EscrowCancel properly delete the entries in both directories?

bachase · 2017-08-25T14:38:51Z

src/ripple/app/tx/impl/Escrow.cpp

+    {
+        TER const ter = dirDelete(ctx_.view(), true,
+            (*slep)[sfAltOwnerNode], keylet::ownerDir((*slep)[sfDestination]),
+            k.key, false, false, ctx_.app.journal ("View"));


The owner deletion checks page==0 for the bSoft argument (second to last) of dirDelete. Is that important to mirror here?

No: that's legacy code (that should have never been there for things like escrow).

scottschurr

I only have one suggestion beyond the comments that Brad already made...

scottschurr · 2017-08-25T16:21:05Z

src/ripple/protocol/SField.h

@@ -387,6 +387,7 @@ extern SF_U64 const sfBaseFee;
 extern SF_U64 const sfExchangeRate;
 extern SF_U64 const sfLowNode;
 extern SF_U64 const sfHighNode;
+extern SF_U64 const sfAltOwnerNode;


Consider sfDestinationNode, as suggested in the Checks proposal: https://ripplelabs.atlassian.net/wiki/spaces/RIPD/pages/111566093/Checks. I think sfDestinationNode is descriptive of the intent for both Checks and Escrow. It would probably work well for Payment Channels as well.

mellery451 · 2017-08-25T17:30:12Z

src/test/app/Escrow_test.cpp

+
+        {
+            testcase ("Metadata & Ownership (without fix1523)");
+            Env env(*this, with_features(featureEscrow));


I'd lean toward all_features_except() in this case and then just the default Env (with all features) for the remaining tests, but I realize there is some refactoring by @scottschurr that might impact some of this too.

I squinted at the features handling in these unit tests. I figured I'd make a tidying pass on this test after my other changes are merged. Which is not to say I disagree with @mellery451's suggestions. They are good ones...

Just to be clear, are you suggesting that this be: Env env(*this, all_features_except(fix1523))?

@nbougalis yes, that's what I was thinking

scottschurr · 2017-08-26T00:27:58Z

Looks like one of the unit tests is failing. But the unit test output is a bit too quiet, so I can't tell which of the tests is failing...

mellery451 · 2017-08-26T00:52:30Z

pretty sure that the failure is #65 failed: AmendmentTable_test.cpp(744)....which I think is failing because the sha512_half hash is wrong in the supportedAmendments (as @bachase pointed out)

codecov-io · 2017-08-28T22:42:21Z

Codecov Report

Merging #2212 into develop will increase coverage by 0.01%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2212      +/-   ##
===========================================
+ Coverage    70.05%   70.07%   +0.01%     
===========================================
  Files          689      689              
  Lines        50724    50729       +5     
===========================================
+ Hits         35533    35546      +13     
+ Misses       15191    15183       -8

Impacted Files	Coverage Δ
src/ripple/ledger/Directory.h	`100% <ø> (ø)`	⬆️
src/ripple/app/main/Amendments.cpp	`100% <ø> (ø)`	⬆️
src/ripple/protocol/Feature.h	`100% <ø> (ø)`	⬆️
src/ripple/protocol/SField.h	`100% <ø> (ø)`	⬆️
src/ripple/ledger/impl/Directory.cpp	`85.71% <ø> (+23.02%)`	⬆️
src/ripple/protocol/impl/SField.cpp	`91.78% <100%> (+0.03%)`	⬆️
src/ripple/protocol/impl/LedgerFormats.cpp	`100% <100%> (ø)`	⬆️
src/ripple/protocol/impl/Feature.cpp	`94.54% <100%> (+0.1%)`	⬆️
src/ripple/app/tx/impl/Escrow.cpp	`90.65% <85%> (-0.59%)`	⬇️
src/ripple/basics/DecayingSample.h	`77.77% <0%> (-8.34%)`	⬇️
... and 3 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 589570d...b05f16c. Read the comment docs.

HowardHinnant

Looks good to me with all the suggested changes. Tests out fine on macOS.

bachase

Added tests look great.

nbougalis · 2017-09-19T21:19:10Z

Rebased and squashed. For inclusion into 0.80.0.

Introduce "fix1523" which corrects a minor technical flaw with the original implementation of the escrow feature. When creating an escrow, the entry would only be tracked in the owner directory of the sender; as a result, an escrow recipient would not be able to detect incoming escrows without monitoring the ledger in real-time for transactions of interest or without the sender communicating this information out of band. With the fix in place, escrows where the recipient differs from the sender will be listed in the recipient's owner directory as well.

nbougalis · 2017-09-25T19:15:30Z

Merged as c7c1b3c

nbougalis requested review from bachase, mellery451 and HowardHinnant August 25, 2017 09:30

bachase requested changes Aug 25, 2017

View reviewed changes

scottschurr reviewed Aug 25, 2017

View reviewed changes

mellery451 reviewed Aug 25, 2017

View reviewed changes

HowardHinnant approved these changes Aug 29, 2017

View reviewed changes

bachase approved these changes Aug 29, 2017

View reviewed changes

scottschurr assigned bachase, mellery451 and HowardHinnant Sep 7, 2017

scottschurr added the Rebase label Sep 7, 2017

mellery451 approved these changes Sep 19, 2017

View reviewed changes

nbougalis mentioned this pull request Sep 23, 2017

Proposed 0.80.0-rc3 #2232

Merged

nbougalis closed this Sep 25, 2017

nbougalis deleted the fix1523 branch March 22, 2018 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track escrow in recipient's owner directory (RIPD-1523): #2212

Track escrow in recipient's owner directory (RIPD-1523): #2212

nbougalis commented Aug 25, 2017

bachase left a comment

bachase Aug 25, 2017

nbougalis Aug 25, 2017

bachase Aug 25, 2017

nbougalis Aug 25, 2017

bachase Aug 25, 2017

nbougalis Aug 25, 2017

bachase Aug 25, 2017

nbougalis Aug 25, 2017

scottschurr left a comment

scottschurr Aug 25, 2017

nbougalis Aug 25, 2017

mellery451 Aug 25, 2017 •

edited

Loading

scottschurr Aug 25, 2017

nbougalis Aug 26, 2017

mellery451 Aug 27, 2017

scottschurr commented Aug 26, 2017

mellery451 commented Aug 26, 2017

codecov-io commented Aug 28, 2017

HowardHinnant left a comment

bachase left a comment

nbougalis commented Sep 19, 2017

nbougalis commented Sep 25, 2017

Track escrow in recipient's owner directory (RIPD-1523): #2212

Track escrow in recipient's owner directory (RIPD-1523): #2212

Conversation

nbougalis commented Aug 25, 2017

bachase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellery451 Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented Aug 26, 2017

mellery451 commented Aug 26, 2017

codecov-io commented Aug 28, 2017

Codecov Report

HowardHinnant left a comment

Choose a reason for hiding this comment

bachase left a comment

Choose a reason for hiding this comment

nbougalis commented Sep 19, 2017

nbougalis commented Sep 25, 2017

mellery451 Aug 25, 2017 •

edited

Loading