From 510314d34465324575719ce528bdda185581464b Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 17 Sep 2025 17:34:47 -0400 Subject: [PATCH] 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. --- include/xrpl/protocol/Indexes.h | 2 + include/xrpl/protocol/detail/features.macro | 1 + .../xrpl/protocol/detail/ledger_entries.macro | 6 +- src/test/app/Escrow_test.cpp | 9 ++ src/test/app/MultiSign_test.cpp | 115 +++++++++++------- src/test/app/Oracle_test.cpp | 23 +++- src/test/app/PayChan_test.cpp | 9 ++ src/test/jtx/impl/Oracle.cpp | 4 +- src/xrpld/app/tx/detail/Escrow.cpp | 5 + src/xrpld/app/tx/detail/PayChan.cpp | 4 + src/xrpld/app/tx/detail/SetOracle.cpp | 9 ++ src/xrpld/app/tx/detail/SetSignerList.cpp | 8 +- 12 files changed, 144 insertions(+), 51 deletions(-) diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index 3e3f2843c1..79be15d906 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/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); diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index a9f5d95624..9dc40dc8e5 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/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) diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 1066986223..f76188095e 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/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}, diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 3eaf0f13ea..19b8612ef4 100644 --- a/src/test/app/Escrow_test.cpp +++ b/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 @@ public: FeatureBitset const all{testable_amendments()}; testWithFeats(all); testWithFeats(all - featureTokenEscrow); + testTags(all - fixIncludeKeyletFields); } }; diff --git a/src/test/app/MultiSign_test.cpp b/src/test/app/MultiSign_test.cpp index 571ec33417..776e163cd4 100644 --- a/src/test/app/MultiSign_test.cpp +++ b/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 @@ public: } void - test_signerListSet(FeatureBitset features) + testSignerListSet(FeatureBitset features) { testcase("SignerListSet"); @@ -215,7 +215,7 @@ public: } void - test_phantomSigners(FeatureBitset features) + testPhantomSigners(FeatureBitset features) { testcase("Phantom Signers"); @@ -282,7 +282,7 @@ public: } void - test_fee(FeatureBitset features) + testFee(FeatureBitset features) { testcase("Fee"); @@ -346,7 +346,7 @@ public: } void - test_misorderedSigners(FeatureBitset features) + testMisorderedSigners(FeatureBitset features) { testcase("Misordered Signers"); @@ -374,7 +374,7 @@ public: } void - test_masterSigners(FeatureBitset features) + testMasterSigners(FeatureBitset features) { testcase("Master Signers"); @@ -429,7 +429,7 @@ public: } void - test_regularSigners(FeatureBitset features) + testRegularSigners(FeatureBitset features) { testcase("Regular Signers"); @@ -494,7 +494,7 @@ public: } void - test_regularSignersUsingSubmitMulti(FeatureBitset features) + testRegularSignersUsingSubmitMulti(FeatureBitset features) { testcase("Regular Signers Using submit_multisigned"); @@ -734,7 +734,7 @@ public: } void - test_heterogeneousSigners(FeatureBitset features) + testHeterogeneousSigners(FeatureBitset features) { testcase("Heterogenious Signers"); @@ -881,7 +881,7 @@ public: // 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 @@ public: // 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 @@ public: // 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 @@ public: } void - test_badSignatureText(FeatureBitset features) + testBadSignatureText(FeatureBitset features) { testcase("Bad Signature Text"); @@ -1285,7 +1285,7 @@ public: } void - test_noMultiSigners(FeatureBitset features) + testNoMultiSigners(FeatureBitset features) { testcase("No Multisigners"); @@ -1304,7 +1304,7 @@ public: } void - test_multisigningMultisigner(FeatureBitset features) + testMultisigningMultisigner(FeatureBitset features) { testcase("Multisigning multisigner"); @@ -1381,7 +1381,7 @@ public: } void - test_signForHash(FeatureBitset features) + testSignForHash(FeatureBitset features) { testcase("sign_for Hash"); @@ -1464,7 +1464,7 @@ public: } void - test_amendmentTransition() + testAmendmentTransition() { testcase("Amendment Transition"); @@ -1559,7 +1559,7 @@ public: } void - test_signersWithTickets(FeatureBitset features) + testSignersWithTickets(FeatureBitset features) { testcase("Signers With Tickets"); @@ -1600,7 +1600,7 @@ public: } void - test_signersWithTags(FeatureBitset features) + testSignersWithTags(FeatureBitset features) { if (!features[featureExpandedSignerList]) return; @@ -1680,7 +1680,7 @@ public: } void - test_signerListSetFlags(FeatureBitset features) + testSignerListSetFlags(FeatureBitset features) { using namespace test::jtx; @@ -1702,27 +1702,57 @@ public: 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 @@ public: testAll(all - featureExpandedSignerList); testAll(all); - test_signerListSetFlags(all - fixInvalidTxFlags); - test_signerListSetFlags(all); + testSignerListSetFlags(all - fixInvalidTxFlags); + testSignerListSetFlags(all); - test_amendmentTransition(); + testSignerListObject(all - fixIncludeKeyletFields); + testSignerListObject(all); + + testAmendmentTransition(); } }; diff --git a/src/test/app/Oracle_test.cpp b/src/test/app/Oracle_test.cpp index fdd7ad941e..f0cde41394 100644 --- a/src/test/app/Oracle_test.cpp +++ b/src/test/app/Oracle_test.cpp @@ -398,7 +398,7 @@ private: } void - testCreate() + testCreate(FeatureBitset features) { testcase("Create"); using namespace jtx; @@ -413,18 +413,30 @@ private: env, {.owner = owner, .series = series, .fee = baseFee}); BEAST_EXPECT(oracle.exists()); BEAST_EXPECT(ownerCount(env, owner) == (count + adj)); + auto const entry = oracle.ledgerEntry(); + BEAST_EXPECT(entry[jss::node][jss::Owner] == owner.human()); + if (features[fixIncludeKeyletFields]) + { + BEAST_EXPECT( + entry[jss::node][jss::OracleDocumentID] == + oracle.documentID()); + } + else + { + BEAST_EXPECT(!entry[jss::node].isMember(jss::OracleDocumentID)); + } BEAST_EXPECT(oracle.expectLastUpdateTime(946694810)); }; { // owner count is adjusted by 1 - Env env(*this); + Env env(*this, features); test(env, {{"XRP", "USD", 740, 1}}, 1); } { // owner count is adjusted by 2 - Env env(*this); + Env env(*this, features); test( env, {{"XRP", "USD", 740, 1}, @@ -438,7 +450,7 @@ private: { // Different owner creates a new object - Env env(*this); + Env env(*this, features); auto const baseFee = static_cast(env.current()->fees().base.drops()); Account const some("some"); @@ -864,7 +876,8 @@ public: auto const all = testable_amendments(); testInvalidSet(); testInvalidDelete(); - testCreate(); + testCreate(all); + testCreate(all - fixIncludeKeyletFields); testDelete(); testUpdate(); testAmendment(); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 3a5d3d6ff5..3d0557fd5c 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1852,6 +1852,14 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + if (features[fixIncludeKeyletFields]) + { + BEAST_EXPECT((*chanSle)[sfSequence] == env.seq(alice) - 1); + } + else + { + BEAST_EXPECT(!chanSle->isFieldPresent(sfSequence)); + } // close the channel env(claim(bob, chan), txflags(tfClose)); BEAST_EXPECT(!channelExists(*env.current(), chan)); @@ -2348,6 +2356,7 @@ public: testWithFeats(all - disallowIncoming); testWithFeats(all); testDepositAuthCreds(); + testMetaAndOwnership(all - fixIncludeKeyletFields); } }; diff --git a/src/test/jtx/impl/Oracle.cpp b/src/test/jtx/impl/Oracle.cpp index 721a1c299d..97a31cbb0c 100644 --- a/src/test/jtx/impl/Oracle.cpp +++ b/src/test/jtx/impl/Oracle.cpp @@ -317,10 +317,10 @@ Oracle::ledgerEntry( if (jr.isObject()) { + if (jr.isMember(jss::error)) + return jr; if (jr.isMember(jss::result) && jr[jss::result].isMember(jss::status)) return jr[jss::result]; - else if (jr.isMember(jss::error)) - return jr; } return Json::nullValue; } diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index 3b05aa0007..3c15278efc 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -538,6 +538,11 @@ EscrowCreate::doApply() (*slep)[~sfFinishAfter] = ctx_.tx[~sfFinishAfter]; (*slep)[~sfDestinationTag] = ctx_.tx[~sfDestinationTag]; + if (ctx_.view().rules().enabled(fixIncludeKeyletFields)) + { + (*slep)[sfSequence] = ctx_.tx.getSeqValue(); + } + if (ctx_.view().rules().enabled(featureTokenEscrow) && !isXRP(amount)) { auto const xferRate = transferRate(ctx_.view(), amount); diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index d9e53ac75c..12a9d0cb75 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -286,6 +286,10 @@ PayChanCreate::doApply() (*slep)[~sfCancelAfter] = ctx_.tx[~sfCancelAfter]; (*slep)[~sfSourceTag] = ctx_.tx[~sfSourceTag]; (*slep)[~sfDestinationTag] = ctx_.tx[~sfDestinationTag]; + if (ctx_.view().rules().enabled(fixIncludeKeyletFields)) + { + (*slep)[sfSequence] = ctx_.tx.getSeqValue(); + } ctx_.view().insert(slep); diff --git a/src/xrpld/app/tx/detail/SetOracle.cpp b/src/xrpld/app/tx/detail/SetOracle.cpp index d598507cb7..ba1d4a2e47 100644 --- a/src/xrpld/app/tx/detail/SetOracle.cpp +++ b/src/xrpld/app/tx/detail/SetOracle.cpp @@ -271,6 +271,11 @@ SetOracle::doApply() if (ctx_.tx.isFieldPresent(sfURI)) sle->setFieldVL(sfURI, ctx_.tx[sfURI]); sle->setFieldU32(sfLastUpdateTime, ctx_.tx[sfLastUpdateTime]); + if (!sle->isFieldPresent(sfOracleDocumentID) && + ctx_.view().rules().enabled(fixIncludeKeyletFields)) + { + (*sle)[sfOracleDocumentID] = ctx_.tx[sfOracleDocumentID]; + } auto const newCount = pairs.size() > 5 ? 2 : 1; auto const adjust = newCount - oldCount; @@ -285,6 +290,10 @@ SetOracle::doApply() sle = std::make_shared(oracleID); sle->setAccountID(sfOwner, ctx_.tx.getAccountID(sfAccount)); + if (ctx_.view().rules().enabled(fixIncludeKeyletFields)) + { + (*sle)[sfOracleDocumentID] = ctx_.tx[sfOracleDocumentID]; + } sle->setFieldVL(sfProvider, ctx_.tx[sfProvider]); if (ctx_.tx.isFieldPresent(sfURI)) sle->setFieldVL(sfURI, ctx_.tx[sfURI]); diff --git a/src/xrpld/app/tx/detail/SetSignerList.cpp b/src/xrpld/app/tx/detail/SetSignerList.cpp index 4a1ee703a0..b52130e2fa 100644 --- a/src/xrpld/app/tx/detail/SetSignerList.cpp +++ b/src/xrpld/app/tx/detail/SetSignerList.cpp @@ -37,7 +37,7 @@ namespace ripple { // We're prepared for there to be multiple signer lists in the future, // but we don't need them yet. So for the time being we're manually // setting the sfSignerListID to zero in all cases. -static std::uint32_t const defaultSignerListID_ = 0; +static std::uint32_t const DEFAULT_SIGNER_LIST_ID = 0; std::tuple< NotTEC, @@ -424,8 +424,12 @@ SetSignerList::writeSignersToSLE( std::uint32_t flags) const { // Assign the quorum, default SignerListID, and flags. + if (ctx_.view().rules().enabled(fixIncludeKeyletFields)) + { + ledgerEntry->setAccountID(sfOwner, account_); + } ledgerEntry->setFieldU32(sfSignerQuorum, quorum_); - ledgerEntry->setFieldU32(sfSignerListID, defaultSignerListID_); + ledgerEntry->setFieldU32(sfSignerListID, DEFAULT_SIGNER_LIST_ID); if (flags) // Only set flags if they are non-default (default is zero). ledgerEntry->setFieldU32(sfFlags, flags);