fix(amendment): Add missing fields for keylets to ledger objects (#5646)

This change adds a fix amendment (`fixIncludeKeyletFields`) that adds:
* `sfSequence` to `Escrow` and `PayChannel`
* `sfOwner` to `SignerList`
* `sfOracleDocumentID` to `Oracle`

This ensures that all ledger entries hold all the information needed to determine their keylet.

Author: mvadari

include/xrpl/protocol/Indexes.h

@@ -287,9 +287,11 @@ delegate(AccountID const& account, AccountID const& authorizedAccount) noexcept;
 Keylet
 bridge(STXChainBridge const& bridge, STXChainBridge::ChainType chainType);
 
+// `seq` is stored as `sfXChainClaimID` in the object
 Keylet
 xChainClaimID(STXChainBridge const& bridge, std::uint64_t seq);
 
+// `seq` is stored as `sfXChainAccountCreateCount` in the object
 Keylet
 xChainCreateAccountClaimID(STXChainBridge const& bridge, std::uint64_t seq);
 

include/xrpl/protocol/detail/features.macro

@@ -32,6 +32,7 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.
 
+XRPL_FIX    (IncludeKeyletFields,        Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(DynamicMPT,                 Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (TokenEscrowV1,              Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (DelegateV1_1,               Supported::no,  VoteBehavior::DefaultNo)

include/xrpl/protocol/detail/ledger_entries.macro

@@ -120,6 +120,7 @@ LEDGER_ENTRY(ltNFTOKEN_PAGE, 0x0050, NFTokenPage, nft_page, ({
 // All fields are soeREQUIRED because there is always a SignerEntries.
 // If there are no SignerEntries the node is deleted.
 LEDGER_ENTRY(ltSIGNER_LIST, 0x0053, SignerList, signer_list, ({
+    {sfOwner,                soeOPTIONAL},
     {sfOwnerNode,            soeREQUIRED},
     {sfSignerQuorum,         soeREQUIRED},
     {sfSignerEntries,        soeREQUIRED},
@@ -188,7 +189,7 @@ LEDGER_ENTRY(ltDIR_NODE, 0x0064, DirectoryNode, directory, ({
     {sfNFTokenID,            soeOPTIONAL},
     {sfPreviousTxnID,        soeOPTIONAL},
     {sfPreviousTxnLgrSeq,    soeOPTIONAL},
-    {sfDomainID,             soeOPTIONAL}
+    {sfDomainID,             soeOPTIONAL}   // order book directories
 }))
 
 /** The ledger object which lists details about amendments on the network.
@@ -343,6 +344,7 @@ LEDGER_ENTRY(ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID, 0x0074, XChainOwnedCreateAc
  */
 LEDGER_ENTRY(ltESCROW, 0x0075, Escrow, escrow, ({
     {sfAccount,              soeREQUIRED},
+    {sfSequence,             soeOPTIONAL},
     {sfDestination,          soeREQUIRED},
     {sfAmount,               soeREQUIRED},
     {sfCondition,            soeOPTIONAL},
@@ -365,6 +367,7 @@ LEDGER_ENTRY(ltESCROW, 0x0075, Escrow, escrow, ({
 LEDGER_ENTRY(ltPAYCHAN, 0x0078, PayChannel, payment_channel, ({
     {sfAccount,              soeREQUIRED},
     {sfDestination,          soeREQUIRED},
+    {sfSequence,             soeOPTIONAL},
     {sfAmount,               soeREQUIRED},
     {sfBalance,              soeREQUIRED},
     {sfPublicKey,            soeREQUIRED},
@@ -433,6 +436,7 @@ LEDGER_ENTRY(ltMPTOKEN, 0x007f, MPToken, mptoken, ({
  */
 LEDGER_ENTRY(ltORACLE, 0x0080, Oracle, oracle, ({
     {sfOwner,                soeREQUIRED},
+    {sfOracleDocumentID,     soeOPTIONAL},
     {sfProvider,             soeREQUIRED},
     {sfPriceDataSeries,      soeREQUIRED},
     {sfAssetClass,           soeREQUIRED},

src/test/app/Escrow_test.cpp

@@ -253,6 +253,14 @@ struct Escrow_test : public beast::unit_test::suite
         BEAST_EXPECT(sle);
         BEAST_EXPECT((*sle)[sfSourceTag] == 1);
         BEAST_EXPECT((*sle)[sfDestinationTag] == 2);
+        if (features[fixIncludeKeyletFields])
+        {
+            BEAST_EXPECT((*sle)[sfSequence] == seq);
+        }
+        else
+        {
+            BEAST_EXPECT(!sle->isFieldPresent(sfSequence));
+        }
     }
 
     void
@@ -1718,6 +1726,7 @@ struct Escrow_test : public beast::unit_test::suite
         FeatureBitset const all{testable_amendments()};
         testWithFeats(all);
         testWithFeats(all - featureTokenEscrow);
+        testTags(all - fixIncludeKeyletFields);
     }
 };
 

src/test/app/MultiSign_test.cpp

@@ -63,7 +63,7 @@ class MultiSign_test : public beast::unit_test::suite
 
 public:
     void
-    test_noReserve(FeatureBitset features)
+    testNoReserve(FeatureBitset features)
     {
         testcase("No Reserve");
 
@@ -133,7 +133,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_signerListSet(FeatureBitset features)
+    testSignerListSet(FeatureBitset features)
     {
         testcase("SignerListSet");
 
@@ -215,7 +215,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_phantomSigners(FeatureBitset features)
+    testPhantomSigners(FeatureBitset features)
     {
         testcase("Phantom Signers");
 
@@ -282,7 +282,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_fee(FeatureBitset features)
+    testFee(FeatureBitset features)
     {
         testcase("Fee");
 
@@ -346,7 +346,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_misorderedSigners(FeatureBitset features)
+    testMisorderedSigners(FeatureBitset features)
     {
         testcase("Misordered Signers");
 
@@ -374,7 +374,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_masterSigners(FeatureBitset features)
+    testMasterSigners(FeatureBitset features)
     {
         testcase("Master Signers");
 
@@ -429,7 +429,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_regularSigners(FeatureBitset features)
+    testRegularSigners(FeatureBitset features)
     {
         testcase("Regular Signers");
 
@@ -494,7 +494,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_regularSignersUsingSubmitMulti(FeatureBitset features)
+    testRegularSignersUsingSubmitMulti(FeatureBitset features)
     {
         testcase("Regular Signers Using submit_multisigned");
 
@@ -734,7 +734,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_heterogeneousSigners(FeatureBitset features)
+    testHeterogeneousSigners(FeatureBitset features)
     {
         testcase("Heterogenious Signers");
 
@@ -881,7 +881,7 @@ class MultiSign_test : public beast::unit_test::suite
     // We want to always leave an account signable.  Make sure the that we
     // disallow removing the last way a transaction may be signed.
     void
-    test_keyDisable(FeatureBitset features)
+    testKeyDisable(FeatureBitset features)
     {
         testcase("Key Disable");
 
@@ -963,7 +963,7 @@ class MultiSign_test : public beast::unit_test::suite
     // Verify that the first regular key can be made for free using the
     // master key, but not when multisigning.
     void
-    test_regKey(FeatureBitset features)
+    testRegKey(FeatureBitset features)
     {
         testcase("Regular Key");
 
@@ -1000,7 +1000,7 @@ class MultiSign_test : public beast::unit_test::suite
 
     // See if every kind of transaction can be successfully multi-signed.
     void
-    test_txTypes(FeatureBitset features)
+    testTxTypes(FeatureBitset features)
     {
         testcase("Transaction Types");
 
@@ -1089,7 +1089,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_badSignatureText(FeatureBitset features)
+    testBadSignatureText(FeatureBitset features)
     {
         testcase("Bad Signature Text");
 
@@ -1285,7 +1285,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_noMultiSigners(FeatureBitset features)
+    testNoMultiSigners(FeatureBitset features)
     {
         testcase("No Multisigners");
 
@@ -1304,7 +1304,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_multisigningMultisigner(FeatureBitset features)
+    testMultisigningMultisigner(FeatureBitset features)
     {
         testcase("Multisigning multisigner");
 
@@ -1381,7 +1381,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_signForHash(FeatureBitset features)
+    testSignForHash(FeatureBitset features)
     {
         testcase("sign_for Hash");
 
@@ -1464,7 +1464,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_amendmentTransition()
+    testAmendmentTransition()
     {
         testcase("Amendment Transition");
 
@@ -1559,7 +1559,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_signersWithTickets(FeatureBitset features)
+    testSignersWithTickets(FeatureBitset features)
     {
         testcase("Signers With Tickets");
 
@@ -1600,7 +1600,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_signersWithTags(FeatureBitset features)
+    testSignersWithTags(FeatureBitset features)
     {
         if (!features[featureExpandedSignerList])
             return;
@@ -1680,7 +1680,7 @@ class MultiSign_test : public beast::unit_test::suite
     }
 
     void
-    test_signerListSetFlags(FeatureBitset features)
+    testSignerListSetFlags(FeatureBitset features)
     {
         using namespace test::jtx;
 
@@ -1702,27 +1702,57 @@ class MultiSign_test : public beast::unit_test::suite
         env.close();
     }
 
+    void
+    testSignerListObject(FeatureBitset features)
+    {
+        testcase("SignerList Object");
+
+        // Verify that the SignerList object is created correctly.
+        using namespace jtx;
+        Env env{*this, features};
+        Account const alice{"alice", KeyType::ed25519};
+        env.fund(XRP(1000), alice);
+        env.close();
+
+        // Attach phantom signers to alice.
+        env(signers(alice, 1, {{bogie, 1}, {demon, 1}}));
+        env.close();
+
+        // Verify that the SignerList object was created correctly.
+        auto const& sle = env.le(keylet::signers(alice.id()));
+        BEAST_EXPECT(sle);
+        BEAST_EXPECT(sle->getFieldArray(sfSignerEntries).size() == 2);
+        if (features[fixIncludeKeyletFields])
+        {
+            BEAST_EXPECT((*sle)[sfOwner] == alice.id());
+        }
+        else
+        {
+            BEAST_EXPECT(!sle->isFieldPresent(sfOwner));
+        }
+    }
+
     void
     testAll(FeatureBitset features)
     {
-        test_noReserve(features);
-        test_signerListSet(features);
-        test_phantomSigners(features);
-        test_fee(features);
-        test_misorderedSigners(features);
-        test_masterSigners(features);
-        test_regularSigners(features);
-        test_regularSignersUsingSubmitMulti(features);
-        test_heterogeneousSigners(features);
-        test_keyDisable(features);
-        test_regKey(features);
-        test_txTypes(features);
-        test_badSignatureText(features);
-        test_noMultiSigners(features);
-        test_multisigningMultisigner(features);
-        test_signForHash(features);
-        test_signersWithTickets(features);
-        test_signersWithTags(features);
+        testNoReserve(features);
+        testSignerListSet(features);
+        testPhantomSigners(features);
+        testFee(features);
+        testMisorderedSigners(features);
+        testMasterSigners(features);
+        testRegularSigners(features);
+        testRegularSignersUsingSubmitMulti(features);
+        testHeterogeneousSigners(features);
+        testKeyDisable(features);
+        testRegKey(features);
+        testTxTypes(features);
+        testBadSignatureText(features);
+        testNoMultiSigners(features);
+        testMultisigningMultisigner(features);
+        testSignForHash(features);
+        testSignersWithTickets(features);
+        testSignersWithTags(features);
     }
 
     void
@@ -1739,10 +1769,13 @@ class MultiSign_test : public beast::unit_test::suite
         testAll(all - featureExpandedSignerList);
         testAll(all);
 
-        test_signerListSetFlags(all - fixInvalidTxFlags);
-        test_signerListSetFlags(all);
+        testSignerListSetFlags(all - fixInvalidTxFlags);
+        testSignerListSetFlags(all);
+
+        testSignerListObject(all - fixIncludeKeyletFields);
+        testSignerListObject(all);
 
-        test_amendmentTransition();
+        testAmendmentTransition();
     }
 };