Improve directory creation (RIPD-928):

* Simplify quality describer
* Use keylet instead of naked uint256

src/ripple/app/ledger/Ledger.cpp

@@ -1049,34 +1049,6 @@ bool pendSaveValidated (
     return true;
 }
 
-void
-ownerDirDescriber (SLE::ref sle, bool, AccountID const& owner)
-{
-    sle->setAccountID (sfOwner, owner);
-}
-
-void
-qualityDirDescriber (
-    SLE::ref sle, bool isNew,
-    Currency const& uTakerPaysCurrency, AccountID const& uTakerPaysIssuer,
-    Currency const& uTakerGetsCurrency, AccountID const& uTakerGetsIssuer,
-    const std::uint64_t& uRate,
-    Application& app)
-{
-    sle->setFieldH160 (sfTakerPaysCurrency, uTakerPaysCurrency);
-    sle->setFieldH160 (sfTakerPaysIssuer, uTakerPaysIssuer);
-    sle->setFieldH160 (sfTakerGetsCurrency, uTakerGetsCurrency);
-    sle->setFieldH160 (sfTakerGetsIssuer, uTakerGetsIssuer);
-    sle->setFieldU64 (sfExchangeRate, uRate);
-    if (isNew)
-    {
-        // VFALCO NO! This shouldn't be done here!
-        app.getOrderBookDB().addOrderBook(
-            {{uTakerPaysCurrency, uTakerPaysIssuer},
-                {uTakerGetsCurrency, uTakerGetsIssuer}});
-    }
-}
-
 void
 Ledger::make_v2()
 {

src/ripple/app/ledger/Ledger.h

@@ -404,19 +404,6 @@ cachedRead (ReadView const& ledger, uint256 const& key,
     return ledger.read(keylet::unchecked(key));
 }
 
-//------------------------------------------------------------------------------
-
-void
-ownerDirDescriber (SLE::ref, bool, AccountID const& owner);
-
-// VFALCO NOTE This is referenced from only one place
-void
-qualityDirDescriber (
-    SLE::ref, bool,
-    Currency const& uTakerPaysCurrency, AccountID const& uTakerPaysIssuer,
-    Currency const& uTakerGetsCurrency, AccountID const& uTakerGetsIssuer,
-    const std::uint64_t & uRate, Application& app);
-
 } // ripple
 
 #endif

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

@@ -20,6 +20,7 @@
 #include <BeastConfig.h>
 #include <ripple/app/tx/impl/CreateOffer.h>
 #include <ripple/app/ledger/Ledger.h>
+#include <ripple/app/ledger/OrderBookDB.h>
 #include <ripple/basics/contract.h>
 #include <ripple/protocol/st.h>
 #include <ripple/basics/Log.h>
@@ -666,10 +667,6 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel)
     auto saTakerPays = ctx_.tx[sfTakerPays];
     auto saTakerGets = ctx_.tx[sfTakerGets];
 
-    auto const& uPaysIssuerID = saTakerPays.getIssuer ();
-
-    auto const& uGetsIssuerID = saTakerGets.getIssuer ();
-
     auto const cancelSequence = ctx_.tx[~sfOfferSequence];
 
     // FIXME understand why we use SequenceNext instead of current transaction
@@ -720,8 +717,7 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel)
         return{ tesSUCCESS, true };
     }
 
-    bool const bOpenLedger =
+    bool const bOpenLedger = ctx_.view().open();
-        ctx_.view().open();
     bool crossed = false;
 
     if (result == tesSUCCESS)
@@ -854,15 +850,11 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel)
     auto const offer_index = getOfferIndex (account_, uSequence);
 
     std::uint64_t uOwnerNode;
-    std::uint64_t uBookNode;
-    uint256 uDirectory;
 
     // Add offer to owner's directory.
-    result = dirAdd(view, uOwnerNode,
+    std::tie(result, std::ignore) = dirAdd(view, uOwnerNode,
-        getOwnerDirIndex (account_), offer_index,
+        keylet::ownerDir (account_), offer_index,
-        std::bind (
+        describeOwnerDir (account_), viewJ);
-            &ownerDirDescriber, std::placeholders::_1,
-            std::placeholders::_2, account_), viewJ);
 
     if (result == tesSUCCESS)
     {
@@ -873,28 +865,34 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel)
             "adding to book: " << to_string (saTakerPays.issue ()) <<
             " : " << to_string (saTakerGets.issue ());
 
-        uint256 const book_base (getBookBase (
+        Book const book { saTakerPays.issue(), saTakerGets.issue() };
-            { saTakerPays.issue (), saTakerGets.issue () }));
+        std::uint64_t uBookNode;
+        bool isNewBook;
 
-        // We use the original rate to place the offer.
+        // Add offer to order book, using the original rate
-        uDirectory = getQualityIndex (book_base, uRate);
+        // before any crossing occured.
+        auto dir = keylet::quality (keylet::book (book), uRate);
 
-        // Add offer to order book.
+        std::tie(result, isNewBook) = dirAdd (view, uBookNode,
-        result = dirAdd (view, uBookNode, uDirectory, offer_index,
+            dir, offer_index, [&](SLE::ref sle)
-            std::bind (
+            {
-                &qualityDirDescriber, std::placeholders::_1,
+                sle->setFieldH160 (sfTakerPaysCurrency,
-                std::placeholders::_2, saTakerPays.getCurrency (),
+                    saTakerPays.issue().currency);
-                uPaysIssuerID, saTakerGets.getCurrency (),
+                sle->setFieldH160 (sfTakerPaysIssuer,
-                uGetsIssuerID, uRate, std::ref(ctx_.app)),
+                    saTakerPays.issue().account);
-            viewJ);
+                sle->setFieldH160 (sfTakerGetsCurrency,
-    }
+                    saTakerGets.issue().currency);
+                sle->setFieldH160 (sfTakerGetsIssuer,
+                    saTakerGets.issue().account);
+                sle->setFieldU64 (sfExchangeRate, uRate);
+            }, viewJ);
 
         if (result == tesSUCCESS)
         {
             auto sleOffer = std::make_shared<SLE>(ltOFFER, offer_index);
             sleOffer->setAccountID (sfAccount, account_);
             sleOffer->setFieldU32 (sfSequence, uSequence);
-        sleOffer->setFieldH256 (sfBookDirectory, uDirectory);
+            sleOffer->setFieldH256 (sfBookDirectory, dir.key);
             sleOffer->setFieldAmount (sfTakerPays, saTakerPays);
             sleOffer->setFieldAmount (sfTakerGets, saTakerGets);
             sleOffer->setFieldU64 (sfOwnerNode, uOwnerNode);
@@ -906,6 +904,10 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel)
             if (bSell)
                 sleOffer->setFlag (lsfSell);
             view.insert(sleOffer);
+
+            if (isNewBook)
+                ctx_.app.getOrderBookDB().addOrderBook(book);
+        }
     }
 
     if (result != tesSUCCESS)

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

@@ -102,32 +102,25 @@ CreateTicket::doApply ()
 
     std::uint64_t hint;
 
-    auto describer = [&](SLE::pointer p, bool b)
-    {
-        ownerDirDescriber(p, b, account_);
-    };
-
     auto viewJ = ctx_.app.journal ("View");
-    TER result = dirAdd(view(),
+
-        hint,
+    auto result = dirAdd(view(), hint, keylet::ownerDir (account_),
-        getOwnerDirIndex (account_),
+        sleTicket->getIndex (), describeOwnerDir (account_), viewJ);
-        sleTicket->getIndex (),
-        describer,
-        viewJ);
 
     JLOG(j_.trace()) <<
         "Creating ticket " << to_string (sleTicket->getIndex ()) <<
-        ": " << transHuman (result);
+        ": " << transHuman (result.first);
-
-    if (result != tesSUCCESS)
-        return result;
 
+    if (result.first == tesSUCCESS)
+    {
         sleTicket->setFieldU64(sfOwnerNode, hint);
 
-    // If we succeeded, the new entry counts agains the creator's reserve.
+        // If we succeeded, the new entry counts agains the
+        // creator's reserve.
         adjustOwnerCount(view(), sle, 1, viewJ);
+    }
 
-    return result;
+    return result.first;
 }
 
 }

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

@@ -238,21 +238,23 @@ SetSignerList::replaceSignerList ()
     auto viewJ = ctx_.app.journal ("View");
     // Add the signer list to the account's directory.
     std::uint64_t hint;
-    TER result = dirAdd(ctx_.view (), hint, ownerDirKeylet.key,
+
+    auto result = dirAdd(ctx_.view (), hint, ownerDirKeylet,
         signerListKeylet.key, describeOwnerDir (account_), viewJ);
 
     JLOG(j_.trace()) << "Create signer list for account " <<
-        toBase58(account_) << ": " << transHuman (result);
+        toBase58(account_) << ": " << transHuman (result.first);
-
-    if (result != tesSUCCESS)
-        return result;
 
+    if (result.first == tesSUCCESS)
+    {
         signerList->setFieldU64 (sfOwnerNode, hint);
 
-    // If we succeeded, the new entry counts against the creator's reserve.
+        // If we succeeded, the new entry counts against the
+        // creator's reserve.
         adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
+    }
 
-    return result;
+    return result.first;
 }
 
 TER

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

@@ -235,11 +235,11 @@ SusPayCreate::doApply()
     // Add SusPay to owner directory
     {
         uint64_t page;
-        TER ter = dirAdd(ctx_.view(), page,
+        auto result = dirAdd(ctx_.view(), page,
-            keylet::ownerDir(account).key,
+            keylet::ownerDir(account), slep->key(),
-                slep->key(), describeOwnerDir(account), ctx_.app.journal ("View"));
+            describeOwnerDir(account), ctx_.app.journal ("View"));
-        if (! isTesSuccess(ter))
+        if (! isTesSuccess(result.first))
-            return ter;
+            return result.first;
         (*slep)[sfOwnerNode] = page;
     }
 

src/ripple/ledger/View.h

@@ -215,7 +215,7 @@ dirNext (ApplyView& view,
     uint256& uEntryIndex,       // <-- The entry, if available. Otherwise, zero.
     beast::Journal j);
 
-std::function<void (SLE::ref, bool)>
+std::function<void (SLE::ref)>
 describeOwnerDir(AccountID const& account);
 
 // <--     uNodeDir: For deletion, present to make dirDelete efficient.
@@ -223,12 +223,24 @@ describeOwnerDir(AccountID const& account);
 // --> uLedgerIndex: Value to add to directory.
 // Only append. This allow for things that watch append only structure to just monitor from the last node on ward.
 // Within a node with no deletions order of elements is sequential.  Otherwise, order of elements is random.
-TER
+
+/** Add an entry to directory, creating the directory if necessary
+
+    @param uNodeDir node of entry - makes deletion efficient
+    @param uRootIndex The index of the base of the directory.
+                      Nodes are based off of this.
+    @param uLedgerIndex Value to add to directory.
+
+    @return a pair containing a code indicating success or
+            failure, and if successful, a boolean indicating
+            whether the directory was just created.
+*/
+std::pair<TER, bool>
 dirAdd (ApplyView& view,
     std::uint64_t&                      uNodeDir,      // Node of entry.
-    uint256 const&                      uRootIndex,
+    Keylet const&                       uRootIndex,
     uint256 const&                      uLedgerIndex,
-    std::function<void (SLE::ref, bool)> fDescriber,
+    std::function<void (SLE::ref)>      fDescriber,
     beast::Journal j);
 
 TER

src/ripple/ledger/impl/View.cpp

@@ -692,51 +692,59 @@ dirNext (ApplyView& view,
     return true;
 }
 
-std::function<void (SLE::ref, bool)>
+std::function<void (SLE::ref)>
 describeOwnerDir(AccountID const& account)
 {
-    return [account](std::shared_ptr<SLE> const& sle, bool)
+    return [&account](std::shared_ptr<SLE> const& sle)
     {
         (*sle)[sfOwner] = account;
     };
 }
 
-TER
+std::pair<TER, bool>
 dirAdd (ApplyView& view,
     std::uint64_t&                          uNodeDir,
-    uint256 const&                          uRootIndex, // VFALCO Should be Keylet
+    Keylet const&                           dir,
     uint256 const&                          uLedgerIndex,
-    std::function<void (SLE::ref, bool)>    fDescriber,
+    std::function<void (SLE::ref)>          fDescriber,
     beast::Journal j)
 {
     JLOG (j.trace()) << "dirAdd:" <<
-        " uRootIndex=" << to_string (uRootIndex) <<
+        " dir=" << to_string (dir.key) <<
         " uLedgerIndex=" << to_string (uLedgerIndex);
 
-    SLE::pointer        sleNode;
+    auto sleRoot = view.peek(dir);
-    STVector256         svIndexes;
-    auto const k = keylet::page(uRootIndex);
-    auto sleRoot = view.peek(k);
 
     if (! sleRoot)
     {
         // No root, make it.
-        sleRoot = std::make_shared<SLE>(k);
+        sleRoot = std::make_shared<SLE>(dir);
-        sleRoot->setFieldH256 (sfRootIndex, uRootIndex);
+        sleRoot->setFieldH256 (sfRootIndex, dir.key);
         view.insert (sleRoot);
-        fDescriber (sleRoot, true);
+        fDescriber (sleRoot);
-        sleNode     = sleRoot;
+
+        STVector256 v;
+        v.push_back (uLedgerIndex);
+        sleRoot->setFieldV256 (sfIndexes, v);
+
+        JLOG (j.trace()) <<
+            "dirAdd: created root " << to_string (dir.key) <<
+            " for entry " << to_string (uLedgerIndex);
+
         uNodeDir = 0;
+
+        return { tesSUCCESS, true };
     }
-    else
+
-    {
+    SLE::pointer sleNode;
+    STVector256 svIndexes;
+
     uNodeDir = sleRoot->getFieldU64 (sfIndexPrevious);       // Get index to last directory node.
 
     if (uNodeDir)
     {
         // Try adding to last node.
-            sleNode = view.peek (keylet::page(uRootIndex, uNodeDir));
+        sleNode = view.peek (keylet::page(dir, uNodeDir));
-
         assert (sleNode);
     }
     else
@@ -755,7 +763,7 @@ dirAdd (ApplyView& view,
     // Add to new node.
     else if (!++uNodeDir)
     {
-            return tecDIR_FULL;
+        return { tecDIR_FULL, false };
     }
     else
     {
@@ -769,30 +777,29 @@ dirAdd (ApplyView& view,
 
         // Create the new node.
         sleNode = std::make_shared<SLE>(
-                keylet::page(uRootIndex, uNodeDir));
+            keylet::page(dir, uNodeDir));
-            sleNode->setFieldH256 (sfRootIndex, uRootIndex);
+        sleNode->setFieldH256 (sfRootIndex, dir.key);
         view.insert (sleNode);
 
         if (uNodeDir != 1)
             sleNode->setFieldU64 (sfIndexPrevious, uNodeDir - 1);
 
-            fDescriber (sleNode, false);
+        fDescriber (sleNode);
 
         svIndexes   = STVector256 ();
     }
-    }
 
     svIndexes.push_back (uLedgerIndex); // Append entry.
     sleNode->setFieldV256 (sfIndexes, svIndexes);   // Save entry.
 
     JLOG (j.trace()) <<
-        "dirAdd:   creating: root: " << to_string (uRootIndex);
+        "dirAdd:   creating: root: " << to_string (dir.key);
     JLOG (j.trace()) <<
         "dirAdd:  appending: Entry: " << to_string (uLedgerIndex);
     JLOG (j.trace()) <<
         "dirAdd:  appending: Node: " << strHex (uNodeDir);
 
-    return tesSUCCESS;
+    return { tesSUCCESS, false };
 }
 
 // Ledger must be in a state for this to work.
@@ -1016,27 +1023,19 @@ trustCreate (ApplyView& view,
     std::uint64_t   uLowNode;
     std::uint64_t   uHighNode;
 
-    TER terResult = dirAdd (view,
+    TER terResult;
-        uLowNode,
+
-        getOwnerDirIndex (uLowAccountID),
+    std::tie (terResult, std::ignore) = dirAdd (view,
+        uLowNode, keylet::ownerDir (uLowAccountID),
         sleRippleState->getIndex (),
-        [uLowAccountID](std::shared_ptr<SLE> const& sle, bool)
+        describeOwnerDir (uLowAccountID), j);
-            {
-                sle->setAccountID (sfOwner, uLowAccountID);
-            },
-        j);
 
     if (tesSUCCESS == terResult)
     {
-        terResult = dirAdd (view,
+        std::tie (terResult, std::ignore) = dirAdd (view,
-            uHighNode,
+            uHighNode, keylet::ownerDir (uHighAccountID),
-            getOwnerDirIndex (uHighAccountID),
             sleRippleState->getIndex (),
-            [uHighAccountID](std::shared_ptr<SLE> const& sle, bool)
+            describeOwnerDir (uHighAccountID), j);
-                {
-                    sle->setAccountID (sfOwner, uHighAccountID);
-                },
-            j);
     }
 
     if (tesSUCCESS == terResult)