Cleanup LedgerEntryType & TxType:

This commit removes the `ltINVALID` pseudo-type identifier from
`LedgerEntryType` and the `ttINVALID` pseudo-type identifier from
`TxType` and includes several small additional improvements that
help to simplify the code base.

It also improves the documentation `LedgerEntryType` and `TxType`,
which was all over the place, and highlights some important caveats
associated with making changes to the ledger and transaction type
identifiers.

The commit also adds a safety check to the `KnownFormats<>` class,
that will catch the the accidental reuse of format identifiers.
Ideally, this should be done at compile time but C++ does not (yet?)
allow for the sort of introspection that would enable this.

src/ripple/app/ledger/LedgerToJson.h

@@ -37,7 +37,7 @@ struct LedgerFill
         RPC::Context* ctx,
         int o = 0,
         std::vector<TxQ::TxDetails> q = {},
-        LedgerEntryType t = ltINVALID)
+        LedgerEntryType t = ltANY)
         : ledger(l), options(o), txQueue(std::move(q)), type(t), context(ctx)
     {
     }

src/ripple/app/ledger/impl/LedgerToJson.cpp

@@ -209,7 +209,7 @@ fillJsonState(Object& json, LedgerFill const& fill)
 
     for (auto const& sle : ledger.sles)
     {
-        if (fill.type == ltINVALID || sle->getType() == fill.type)
+        if (fill.type == ltANY || sle->getType() == fill.type)
         {
             if (binary)
             {

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

@@ -208,8 +208,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx)
     {
         // Make sure any directory node types that we find are the kind
         // we can delete.
-        Keylet const itemKeylet{ltCHILD, dirEntry};
+        auto sleItem = ctx.view.read(keylet::child(dirEntry));
-        auto sleItem = ctx.view.read(itemKeylet);
         if (!sleItem)
         {
             // Directory node has an invalid index.  Bail out.
@@ -261,8 +260,7 @@ DeleteAccount::doApply()
         do
         {
             // Choose the right way to delete each directory node.
-            Keylet const itemKeylet{ltCHILD, dirEntry};
+            auto sleItem = view().peek(keylet::child(dirEntry));
-            auto sleItem = view().peek(itemKeylet);
             if (!sleItem)
             {
                 // Directory node has an invalid index.  Bail out.

src/ripple/protocol/InnerObjectFormats.h

@@ -26,7 +26,7 @@ namespace ripple {
 
 /** Manages the list of known inner object formats.
  */
-class InnerObjectFormats : public KnownFormats<int>
+class InnerObjectFormats : public KnownFormats<int, InnerObjectFormats>
 {
 private:
     /** Create the object.

src/ripple/protocol/Keylet.h

@@ -37,10 +37,10 @@ class STLedgerEntry;
 */
 struct Keylet
 {
-    LedgerEntryType type;
     uint256 key;
+    LedgerEntryType type;
 
-    Keylet(LedgerEntryType type_, uint256 const& key_) : type(type_), key(key_)
+    Keylet(LedgerEntryType type_, uint256 const& key_) : key(key_), type(type_)
     {
     }
 

src/ripple/protocol/KnownFormats.h

@@ -24,6 +24,7 @@
 #include <ripple/protocol/SOTemplate.h>
 #include <boost/container/flat_map.hpp>
 #include <algorithm>
+#include <beast/type_name.h>
 #include <forward_list>
 
 namespace ripple {
@@ -35,7 +36,7 @@ namespace ripple {
 
     @tparam KeyType The type of key identifying the format.
 */
-template <class KeyType>
+template <class KeyType, class Derived>
 class KnownFormats
 {
 public:
@@ -90,7 +91,9 @@ public:
 
         Derived classes will load the object with all the known formats.
     */
-    KnownFormats() = default;
+    KnownFormats() : name_(beast::type_name<Derived>())
+    {
+    }
 
     /** Destroy the known formats object.
 
@@ -111,12 +114,11 @@ public:
     KeyType
     findTypeByName(std::string const& name) const
     {
-        Item const* const result = findByName(name);
+        if (auto const result = findByName(name))
-
-        if (result != nullptr)
             return result->getType();
-        Throw<std::runtime_error>("Unknown format name");
+        Throw<std::runtime_error>(
-        return {};  // Silence compiler warning.
+            name_ + ": Unknown format name '" +
+            name.substr(0, std::min(name.size(), std::size_t(32))) + "'");
     }
 
     /** Retrieve a format based on its type.
@@ -170,6 +172,13 @@ protected:
         std::initializer_list<SOElement> uniqueFields,
         std::initializer_list<SOElement> commonFields = {})
     {
+        if (auto const item = findByType(type))
+        {
+            LogicError(
+                std::string("Duplicate key for item '") + name +
+                "': already maps to " + item->getName());
+        }
+
         formats_.emplace_front(name, type, uniqueFields, commonFields);
         Item const& item{formats_.front()};
 
@@ -180,6 +189,8 @@ protected:
     }
 
 private:
+    std::string name_;
+
     // One of the situations where a std::forward_list is useful.  We want to
     // store each Item in a place where its address won't change.  So a node-
     // based container is appropriate.  But we don't need searchability.

src/ripple/protocol/LedgerFormats.h

@@ -24,76 +24,184 @@
 
 namespace ripple {
 
-/** Ledger entry types.
+/** Identifiers for on-ledger objects.
 
-    These are stored in serialized data.
+    Each ledger object requires a unique type identifier, which is stored
+    within the object itself; this makes it possible to iterate the entire
+    ledger and determine each object's type and verify that the object you
+    retrieved from a given hash matches the expected type.
 
-    @note Changing these values results in a hard fork.
+    @warning Since these values are stored inside objects stored on the ledger
+             they are part of the protocol. **Changing them should be avoided
+             because without special handling, this will result in a hard
+   fork.**
+
+    @note Values outside this range may be used internally by the code for
+          various purposes, but attempting to use such values to identify
+          on-ledger objects will results in an invariant failure.
+
+    @note When retiring types, the specific values should not be removed but
+          should be marked as [[deprecated]]. This is to avoid accidental
+          reuse of identifiers.
+
+    @todo The C++ language does not enable checking for duplicate values
+          here. If it becomes possible then we should do this.
 
     @ingroup protocol
 */
-// Used as the type of a transaction or the type of a ledger entry.
+// clang-format off
-enum LedgerEntryType {
+enum LedgerEntryType : std::uint16_t
-    /** Special type, anything
+{
-        This is used when the type in the Keylet is unknown,
+    /** A ledger object which describes an account.
-        such as when building metadata.
-    */
-    ltANY = -3,
 
-    /** Special type, anything not a directory
+        \sa keylet::account
-        This is used when the type in the Keylet is unknown,
+     */
-        such as when iterating
+    ltACCOUNT_ROOT = 0x0061,
-    */
-    ltCHILD = -2,
 
-    ltINVALID = -1,
+    /** A ledger object which contains a list of object identifiers.
+
+        \sa keylet::page, keylet::quality, keylet::book, keylet::next and
+            keylet::ownerDir
+     */
+    ltDIR_NODE = 0x0064,
+
+    /** A ledger object which describes a bidirectional trust line.
+
+        @note Per Vinnie Falco this should be renamed to ltTRUST_LINE
+
+        \sa keylet::line
+     */
+    ltRIPPLE_STATE = 0x0072,
+
+    /** A ledger object which describes a ticket.
+
+        \sa keylet::ticket
+     */
+    ltTICKET = 0x0054,
+
+    /** A ledger object which contains a signer list for an account.
+
+        \sa keylet::signers
+     */
+    ltSIGNER_LIST = 0x0053,
+
+    /** A ledger object which describes an offer on the DEX.
+
+        \sa keylet::offer
+     */
+    ltOFFER = 0x006f,
+
+    /** A ledger object that contains a list of ledger hashes.
+
+        This type is used to store the ledger hashes which the protocol uses
+        to implement skip lists that allow for efficient backwards (and, in
+        theory, forward) forward iteration across large ledger ranges.
+
+        \sa keylet::skip
+     */
+    ltLEDGER_HASHES = 0x0068,
+
+    /** The ledger object which lists details about amendments on the network.
+
+        \note This is a singleton: only one such object exists in the ledger.
+
+        \sa keylet::amendments
+     */
+    ltAMENDMENTS = 0x0066,
+
+    /** The ledger object which lists the network's fee settings.
+
+        \note This is a singleton: only one such object exists in the ledger.
+
+        \sa keylet::fees
+     */
+    ltFEE_SETTINGS = 0x0073,
+
+    /** A ledger object describing a single escrow.
+
+        \sa keylet::escrow
+     */
+    ltESCROW = 0x0075,
+
+    /** A ledger object describing a single unidirectional XRP payment channel.
+
+        \sa keylet::payChan
+     */
+    ltPAYCHAN = 0x0078,
+
+    /** A ledger object which describes a check.
+
+        \sa keylet::check
+     */
+    ltCHECK = 0x0043,
+
+    /** A ledger object which describes a deposit preauthorization.
+
+        \sa keylet::depositPreauth
+     */
+    ltDEPOSIT_PREAUTH = 0x0070,
+
+    /** The ledger object which tracks the current negative UNL state.
+
+        \note This is a singleton: only one such object exists in the ledger.
+
+        \sa keylet::negativeUNL
+     */
+    ltNEGATIVE_UNL = 0x004e,
 
     //---------------------------------------------------------------------------
+    /** A special type, matching any ledger entry type.
 
-    ltACCOUNT_ROOT = 'a',
+        The value does not represent a concrete type, but rather is used in
+        contexts where the specific type of a ledger object is unimportant,
+        unknown or unavailable.
 
-    /** Directory node.
+        Objects with this special type cannot be created or stored on the
+        ledger.
 
-        A directory is a vector 256-bit values. Usually they represent
+        \sa keylet::unchecked
-        hashes of other objects in the ledger.
-
-        Used in an append-only fashion.
-
-        (There's a little more information than this, see the template)
     */
-    ltDIR_NODE = 'd',
+    ltANY = 0,
 
-    ltRIPPLE_STATE = 'r',
+    /** A special type, matching any ledger type except directory nodes.
 
-    ltTICKET = 'T',
+        The value does not represent a concrete type, but rather is used in
+        contexts where the ledger object must not be a directory node but
+        its specific type is otherwise unimportant, unknown or unavailable.
 
-    ltSIGNER_LIST = 'S',
+        Objects with this special type cannot be created or stored on the
+        ledger.
 
-    ltOFFER = 'o',
+        \sa keylet::child
+     */
+    ltCHILD = 0x1CD2,
 
-    ltLEDGER_HASHES = 'h',
+    //---------------------------------------------------------------------------
+    /** A legacy, deprecated type.
 
-    ltAMENDMENTS = 'f',
+        \deprecated **This object type is not supported and should not be used.**
+                    Support for this type of object was never implemented.
+                    No objects of this type were ever created.
+     */
+    ltNICKNAME [[deprecated("This object type is not supported and should not be used.")]] = 0x006e,
 
-    ltFEE_SETTINGS = 's',
+    /** A legacy, deprecated type.
 
-    ltESCROW = 'u',
+        \deprecated **This object type is not supported and should not be used.**
+                    Support for this type of object was never implemented.
+                    No objects of this type were ever created.
+     */
+    ltCONTRACT [[deprecated("This object type is not supported and should not be used.")]] = 0x0063,
 
-    // Simple unidirection xrp channel
+    /** A legacy, deprecated type.
-    ltPAYCHAN = 'x',
 
-    ltCHECK = 'C',
+        \deprecated **This object type is not supported and should not be used.**
-
+                    Support for this type of object was never implemented.
-    ltDEPOSIT_PREAUTH = 'p',
+                    No objects of this type were ever created.
-
+     */
-    ltNEGATIVE_UNL = 'N',
+    ltGENERATOR_MAP [[deprecated("This object type is not supported and should not be used.")]]  = 0x0067,
-
-    // No longer used or supported. Left here to prevent accidental
-    // reassignment of the ledger type.
-    ltNICKNAME [[deprecated]] = 'n',
-
-    ltNotUsed01 [[deprecated]] = 'c',
 };
+// clang-format off
 
 /**
     @ingroup protocol
@@ -135,7 +243,7 @@ enum LedgerSpecificFlags {
 
 /** Holds the list of known ledger entry formats.
  */
-class LedgerFormats : public KnownFormats<LedgerEntryType>
+class LedgerFormats : public KnownFormats<LedgerEntryType, LedgerFormats>
 {
 private:
     /** Create the object.

src/ripple/protocol/TxFormats.h

@@ -30,42 +30,123 @@ namespace ripple {
 
     @ingroup protocol
 */
-enum TxType {
+/** Transaction type identifieers
-    ttINVALID = -1,
 
+    Each ledger object requires a unique type identifier, which is stored
+    within the object itself; this makes it possible to iterate the entire
+    ledger and determine each object's type and verify that the object you
+    retrieved from a given hash matches the expected type.
+
+    @warning Since these values are included in transactions, which are signed
+             objects, and used by the code to determine the type of transaction
+             being invoked, they are part of the protocol. **Changing them
+             should be avoided because without special handling, this will
+             result in a hard fork.**
+
+    @note When retiring types, the specific values should not be removed but
+          should be marked as [[deprecated]]. This is to avoid accidental
+          reuse of identifiers.
+
+    @todo The C++ language does not enable checking for duplicate values
+          here. If it becomes possible then we should do this.
+
+    @ingroup protocol
+*/
+// clang-format off
+enum TxType : std::uint16_t
+{
+    /** This transaction type executes a payment. */
     ttPAYMENT = 0,
+
+    /** This transaction type creates an escrow object. */
     ttESCROW_CREATE = 1,
+
+    /** This transaction type completes an existing escrow. */
     ttESCROW_FINISH = 2,
+
+    /** This transaction type adjusts various account settings. */
     ttACCOUNT_SET = 3,
+
+    /** This transaction type cancels an existing escrow. */
     ttESCROW_CANCEL = 4,
+
+    /** This transaction type sets or clears an account's "regular key". */
     ttREGULAR_KEY_SET = 5,
-    ttNICKNAME_SET = 6,  // open
+
+    /** This transaction type is deprecated; it is retained for historical purposes. */
+    ttNICKNAME_SET [[deprecated("This transaction type is not supported and should not be used.")]] = 6,
+
+    /** This transaction type creates an offer to trade one asset for another. */
     ttOFFER_CREATE = 7,
+
+    /** This transaction type cancels existing offers to trade one asset for another. */
     ttOFFER_CANCEL = 8,
-    no_longer_used = 9,
+
+    /** This transaction type is deprecated; it is retained for historical purposes. */
+    ttCONTRACT [[deprecated("This transaction type is not supported and should not be used.")]] = 9,
+
+    /** This transaction type creates a new set of tickets. */
     ttTICKET_CREATE = 10,
-    //                      = 11, // open
+
+    /** This identifier was never used, but the slot is reserved for historical purposes. */
+    ttSPINAL_TAP [[deprecated("This transaction type is not supported and should not be used.")]] = 11,
+
+    /** This transaction type modifies the signer list associated with an account. */
     ttSIGNER_LIST_SET = 12,
+
+    /** This transaction type creates a new unidirectional XRP payment channel. */
     ttPAYCHAN_CREATE = 13,
+
+    /** This transaction type funds an existing unidirectional XRP payment channel. */
     ttPAYCHAN_FUND = 14,
+
+    /** This transaction type submits a claim against an existing unidirectional payment channel. */
     ttPAYCHAN_CLAIM = 15,
+
+    /** This transaction type creates a new check. */
     ttCHECK_CREATE = 16,
+
+    /** This transaction type cashes an existing check. */
     ttCHECK_CASH = 17,
+
+    /** This transaction type cancels an existing check. */
     ttCHECK_CANCEL = 18,
+
+    /** This transaction type grants or revokes authorization to transfer funds. */
     ttDEPOSIT_PREAUTH = 19,
+
+    /** This transaction type modifies a trustline between two accounts. */
     ttTRUST_SET = 20,
+
+    /** This transaction type deletes an existing account. */
     ttACCOUNT_DELETE = 21,
 
+    /** This transaction type installs a hook. */
     ttHOOK_SET [[maybe_unused]] = 22,
 
+    /** This system-generated transaction type is used to update the status of the various amendments.
+
+        For details, see: https://xrpl.org/amendments.html
+     */
     ttAMENDMENT = 100,
+
+    /** This system-generated transaction type is used to update the network's fee settings.
+
+        For details, see: https://xrpl.org/fee-voting.html
+     */
     ttFEE = 101,
+
+    /** This system-generated transaction type is used to update the network's negative UNL
+
+        For details, see: https://xrpl.org/negative-unl.html
+     */
     ttUNL_MODIFY = 102,
 };
+// clang-format on
 
 /** Manages the list of known transaction formats.
  */
-class TxFormats : public KnownFormats<TxType>
+class TxFormats : public KnownFormats<TxType, TxFormats>
 {
 private:
     /** Create the object.

src/ripple/protocol/impl/Keylet.cpp

@@ -23,17 +23,16 @@
 namespace ripple {
 
 bool
-Keylet::check(SLE const& sle) const
+Keylet::check(STLedgerEntry const& sle) const
 {
+    assert(sle.getType() != ltANY || sle.getType() != ltCHILD);
+
     if (type == ltANY)
         return true;
-    if (type == ltINVALID)
+
-        return false;
     if (type == ltCHILD)
-    {
-        assert(sle.getType() != ltDIR_NODE);
         return sle.getType() != ltDIR_NODE;
-    }
+
     return sle.getType() == type;
 }
 

src/ripple/protocol/impl/STLedgerEntry.cpp

@@ -32,22 +32,12 @@ namespace ripple {
 STLedgerEntry::STLedgerEntry(Keylet const& k)
     : STObject(sfLedgerEntry), key_(k.key), type_(k.type)
 {
-    // The on-ledger representation of a key type is a 16-bit unsigned integer
-    // but the LedgerEntryType enum has a larger range (including negative
-    // values), so catch obviously wrong values:
-    constexpr auto const minValidValue =
-        static_cast<LedgerEntryType>(std::numeric_limits<std::uint16_t>::min());
-
-    constexpr auto const maxValidValue =
-        static_cast<LedgerEntryType>(std::numeric_limits<std::uint16_t>::max());
-
-    if (type_ < minValidValue || type_ > maxValidValue)
-        Throw<std::runtime_error>("invalid ledger entry type: out of range");
-
     auto const format = LedgerFormats::getInstance().findByType(type_);
 
     if (format == nullptr)
-        Throw<std::runtime_error>("unknown ledger entry type");
+        Throw<std::runtime_error>(
+            "Attempt to create a SLE of unknown type " +
+            std::to_string(safe_cast<std::uint16_t>(k.type)));
 
     set(format->getSOTemplate());
 

src/ripple/protocol/impl/STParsedJSON.cpp

@@ -293,37 +293,22 @@ parseLeaf(
                     {
                         if (field == sfTransactionType)
                         {
-                            TxType const txType(
-                                TxFormats::getInstance().findTypeByName(
-                                    strValue));
-
-                            if (txType == ttINVALID)
-                                Throw<std::runtime_error>(
-                                    "Invalid transaction format name");
                             ret = detail::make_stvar<STUInt16>(
-                                field, static_cast<std::uint16_t>(txType));
+                                field,
+                                static_cast<std::uint16_t>(
+                                    TxFormats::getInstance().findTypeByName(
+                                        strValue)));
 
                             if (*name == sfGeneric)
                                 name = &sfTransaction;
                         }
                         else if (field == sfLedgerEntryType)
                         {
-                            LedgerEntryType const type(
-                                LedgerFormats::getInstance().findTypeByName(
-                                    strValue));
-
-                            if (!(0u <= type &&
-                                  type <=
-                                      std::min<unsigned>(
-                                          std::numeric_limits<
-                                              std::uint16_t>::max(),
-                                          std::numeric_limits<
-                                              std::underlying_type_t<
-                                                  LedgerEntryType>>::max())))
-                                Throw<std::runtime_error>(
-                                    "Invalid ledger entry type: out of range");
                             ret = detail::make_stvar<STUInt16>(
-                                field, static_cast<std::uint16_t>(type));
+                                field,
+                                static_cast<std::uint16_t>(
+                                    LedgerFormats::getInstance().findTypeByName(
+                                        strValue)));
 
                             if (*name == sfGeneric)
                                 name = &sfLedgerEntry;

src/ripple/rpc/handlers/AccountObjects.cpp

@@ -111,7 +111,7 @@ doAccountObjects(RPC::JsonContext& context)
             rpcStatus.inject(result);
             return result;
         }
-        else if (type != ltINVALID)
+        else if (type != ltANY)
         {
             typeFilter = std::vector<LedgerEntryType>({type});
         }

src/ripple/rpc/handlers/LedgerData.cpp

@@ -108,7 +108,7 @@ doLedgerData(RPC::JsonContext& context)
             break;
         }
 
-        if (type == ltINVALID || sle->getType() == type)
+        if (type == ltANY || sle->getType() == type)
         {
             if (isBinary)
             {

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -836,7 +836,7 @@ keypairForSignature(Json::Value const& params, Json::Value& error)
 std::pair<RPC::Status, LedgerEntryType>
 chooseLedgerEntryType(Json::Value const& params)
 {
-    std::pair<RPC::Status, LedgerEntryType> result{RPC::Status::OK, ltINVALID};
+    std::pair<RPC::Status, LedgerEntryType> result{RPC::Status::OK, ltANY};
     if (params.isMember(jss::type))
     {
         static constexpr std::array<std::pair<char const*, LedgerEntryType>, 13>

src/test/protocol/KnownFormatToGRPC_test.cpp

@@ -768,12 +768,13 @@ private:
     }
 
     // Compare a protobuf descriptor to a KnownFormat::Item
-    template <typename FmtType>
+    template <typename FmtType, typename FmtName>
     void
     validateFields(
         google::protobuf::Descriptor const* const pbufDescriptor,
         google::protobuf::Descriptor const* const commonFields,
-        typename KnownFormats<FmtType>::Item const* const knownFormatItem)
+        typename KnownFormats<FmtType, FmtName>::Item const* const
+            knownFormatItem)
     {
         // Create namespace aliases for shorter names.
         namespace pbuf = google::protobuf;
@@ -806,10 +807,10 @@ private:
             std::move(sFields));
     }
 
-    template <typename FmtType>
+    template <typename FmtType, typename FmtName>
     void
     testKnownFormats(
-        KnownFormats<FmtType> const& knownFormat,
+        KnownFormats<FmtType, FmtName> const& knownFormat,
         std::string const& knownFormatName,
         google::protobuf::Descriptor const* const commonFields,
         google::protobuf::OneofDescriptor const* const oneOfDesc)
@@ -822,7 +823,9 @@ private:
             return;
 
         // Get corresponding names for all KnownFormat Items.
-        std::map<std::string, typename KnownFormats<FmtType>::Item const*>
+        std::map<
+            std::string,
+            typename KnownFormats<FmtType, FmtName>::Item const*>
             formatTypes;
 
         for (auto const& item : knownFormat)
@@ -897,7 +900,7 @@ private:
             }
 
             // Validate that the gRPC and KnownFormat fields align.
-            validateFields<FmtType>(
+            validateFields<FmtType, FmtName>(
                 fieldDesc->message_type(), commonFields, fmtIter->second);
 
             // Remove the checked KnownFormat from the map.  This way we