From 9fec615dcab5de7f9b422dcda57e82480c20184a Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Thu, 27 Jun 2024 11:52:02 -0700 Subject: [PATCH 1/2] fixInnerObjTemplate2 amendment (#5047) * fixInnerObjTemplate2 amendment: Apply inner object templates to all remaining (non-AMM) inner objects. Adds a unit test for applying the template to sfMajorities. Other remaining inner objects showed no problems having templates applied. * Move CMake directory * Rearrange sources * Rewrite includes * Recompute loops --------- Co-authored-by: Pretty Printer --- include/xrpl/protocol/Feature.h | 3 +- include/xrpl/protocol/STObject.h | 3 +- src/libxrpl/protocol/Feature.cpp | 1 + src/libxrpl/protocol/STObject.cpp | 13 +- src/libxrpl/protocol/XChainAttestations.cpp | 10 +- src/test/rpc/LedgerData_test.cpp | 355 ++++++++++---------- src/xrpld/app/ledger/Ledger.cpp | 2 +- src/xrpld/app/misc/detail/AMMUtils.cpp | 4 +- src/xrpld/app/tx/detail/AMMVote.cpp | 6 +- src/xrpld/app/tx/detail/Change.cpp | 8 +- src/xrpld/app/tx/detail/SetSignerList.cpp | 6 +- src/xrpld/rpc/detail/TransactionSign.cpp | 2 +- 12 files changed, 219 insertions(+), 194 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index d2be4c934f6..49f94b88d86 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -80,7 +80,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 76; +static constexpr std::size_t numFeatures = 77; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -369,6 +369,7 @@ extern uint256 const fixAMMv1_1; extern uint256 const featureNFTokenMintOffer; extern uint256 const fixReducedOffersV2; extern uint256 const fixEnforceNFTokenTrustline; +extern uint256 const fixInnerObjTemplate2; } // namespace ripple diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index 5259994a976..b3cef83de5f 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -44,7 +44,6 @@ namespace ripple { class STArray; -class Rules; inline void throwFieldNotFound(SField const& field) @@ -105,7 +104,7 @@ class STObject : public STBase, public CountedObject explicit STObject(SField const& name); static STObject - makeInnerObject(SField const& name, Rules const& rules); + makeInnerObject(SField const& name); iterator begin() const; diff --git a/src/libxrpl/protocol/Feature.cpp b/src/libxrpl/protocol/Feature.cpp index 992d51ca974..8a978955086 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -496,6 +496,7 @@ REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::De REGISTER_FEATURE(NFTokenMintOffer, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixEnforceNFTokenTrustline, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixInnerObjTemplate2, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index a369974c2d6..bde83ec31a1 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -61,10 +61,19 @@ STObject::STObject(SerialIter& sit, SField const& name, int depth) noexcept( } STObject -STObject::makeInnerObject(SField const& name, Rules const& rules) +STObject::makeInnerObject(SField const& name) { STObject obj{name}; - if (rules.enabled(fixInnerObjTemplate)) + + // The if is complicated because inner object templates were added in + // two phases: + // 1. If there are no available Rules, then always apply the template. + // 2. fixInnerObjTemplate added templates to two AMM inner objects. + // 3. fixInnerObjTemplate2 added templates to all remaining inner objects. + std::optional const& rules = getCurrentTransactionRules(); + bool const isAMMObj = name == sfAuctionSlot || name == sfVoteEntry; + if (!rules || (rules->enabled(fixInnerObjTemplate) && isAMMObj) || + (rules->enabled(fixInnerObjTemplate2) && !isAMMObj)) { if (SOTemplate const* elements = InnerObjectFormats::getInstance().findSOTemplateBySField(name)) diff --git a/src/libxrpl/protocol/XChainAttestations.cpp b/src/libxrpl/protocol/XChainAttestations.cpp index f87f1c3e681..82e73445693 100644 --- a/src/libxrpl/protocol/XChainAttestations.cpp +++ b/src/libxrpl/protocol/XChainAttestations.cpp @@ -203,7 +203,8 @@ AttestationClaim::AttestationClaim(Json::Value const& v) STObject AttestationClaim::toSTObject() const { - STObject o{sfXChainClaimAttestationCollectionElement}; + STObject o = + STObject::makeInnerObject(sfXChainClaimAttestationCollectionElement); addHelper(o); o[sfXChainClaimID] = claimID; if (dst) @@ -345,7 +346,8 @@ AttestationCreateAccount::AttestationCreateAccount( STObject AttestationCreateAccount::toSTObject() const { - STObject o{sfXChainCreateAccountAttestationCollectionElement}; + STObject o = STObject::makeInnerObject( + sfXChainCreateAccountAttestationCollectionElement); addHelper(o); o[sfXChainAccountCreateCount] = createCount; @@ -497,7 +499,7 @@ XChainClaimAttestation::XChainClaimAttestation( STObject XChainClaimAttestation::toSTObject() const { - STObject o{sfXChainClaimProofSig}; + STObject o = STObject::makeInnerObject(sfXChainClaimProofSig); o[sfAttestationSignerAccount] = STAccount{sfAttestationSignerAccount, keyAccount}; o[sfPublicKey] = publicKey; @@ -609,7 +611,7 @@ XChainCreateAccountAttestation::XChainCreateAccountAttestation( STObject XChainCreateAccountAttestation::toSTObject() const { - STObject o{sfXChainCreateAccountProofSig}; + STObject o = STObject::makeInnerObject(sfXChainCreateAccountProofSig); o[sfAttestationSignerAccount] = STAccount{sfAttestationSignerAccount, keyAccount}; diff --git a/src/test/rpc/LedgerData_test.cpp b/src/test/rpc/LedgerData_test.cpp index 34dfb3e011b..1e4f97a935f 100644 --- a/src/test/rpc/LedgerData_test.cpp +++ b/src/test/rpc/LedgerData_test.cpp @@ -300,201 +300,214 @@ class LedgerData_test : public beast::unit_test::suite { // Put a bunch of different LedgerEntryTypes into a ledger using namespace test::jtx; - using namespace std::chrono; - Env env{*this, envconfig(validator, "")}; - Account const gw{"gateway"}; - auto const USD = gw["USD"]; - env.fund(XRP(100000), gw); - - auto makeRequest = [&env](Json::StaticString const& type) { - Json::Value jvParams; - jvParams[jss::ledger_index] = "current"; - jvParams[jss::type] = type; - return env.rpc( - "json", - "ledger_data", - boost::lexical_cast(jvParams))[jss::result]; - }; - - // Assert that state is an empty array. - for (auto const& type : - {jss::amendments, - jss::check, - jss::directory, - jss::offer, - jss::signer_list, - jss::state, - jss::ticket, - jss::escrow, - jss::payment_channel, - jss::deposit_preauth}) + // Make sure fixInnerObjTemplate2 doesn't break amendments. + for (FeatureBitset const& features : + {supported_amendments() - fixInnerObjTemplate2, + supported_amendments() | fixInnerObjTemplate2}) { - auto const jrr = makeRequest(type); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 0)); - } + using namespace std::chrono; + Env env{*this, envconfig(validator, ""), features}; + + Account const gw{"gateway"}; + auto const USD = gw["USD"]; + env.fund(XRP(100000), gw); + + auto makeRequest = [&env](Json::StaticString const& type) { + Json::Value jvParams; + jvParams[jss::ledger_index] = "current"; + jvParams[jss::type] = type; + return env.rpc( + "json", + "ledger_data", + boost::lexical_cast(jvParams))[jss::result]; + }; + + // Assert that state is an empty array. + for (auto const& type : + {jss::amendments, + jss::check, + jss::directory, + jss::offer, + jss::signer_list, + jss::state, + jss::ticket, + jss::escrow, + jss::payment_channel, + jss::deposit_preauth}) + { + auto const jrr = makeRequest(type); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 0)); + } - int const num_accounts = 10; + int const num_accounts = 10; - for (auto i = 0; i < num_accounts; i++) - { - Account const bob{std::string("bob") + std::to_string(i)}; - env.fund(XRP(1000), bob); - } - env(offer(Account{"bob0"}, USD(100), XRP(100))); - env.trust(Account{"bob2"}["USD"](100), Account{"bob3"}); + for (auto i = 0; i < num_accounts; i++) + { + Account const bob{std::string("bob") + std::to_string(i)}; + env.fund(XRP(1000), bob); + } + env(offer(Account{"bob0"}, USD(100), XRP(100))); + env.trust(Account{"bob2"}["USD"](100), Account{"bob3"}); - auto majorities = getMajorityAmendments(*env.closed()); - for (int i = 0; i <= 256; ++i) - { - env.close(); - majorities = getMajorityAmendments(*env.closed()); - if (!majorities.empty()) - break; - } - env(signers( - Account{"bob0"}, 1, {{Account{"bob1"}, 1}, {Account{"bob2"}, 1}})); - env(ticket::create(env.master, 1)); + auto majorities = getMajorityAmendments(*env.closed()); + for (int i = 0; i <= 256; ++i) + { + env.close(); + majorities = getMajorityAmendments(*env.closed()); + if (!majorities.empty()) + break; + } - { - Json::Value jv; - jv[jss::TransactionType] = jss::EscrowCreate; - jv[jss::Flags] = tfUniversal; - jv[jss::Account] = Account{"bob5"}.human(); - jv[jss::Destination] = Account{"bob6"}.human(); - jv[jss::Amount] = XRP(50).value().getJson(JsonOptions::none); - jv[sfFinishAfter.fieldName] = NetClock::time_point{env.now() + 10s} - .time_since_epoch() - .count(); - env(jv); - } + env(signers( + Account{"bob0"}, + 1, + {{Account{"bob1"}, 1}, {Account{"bob2"}, 1}})); + env(ticket::create(env.master, 1)); - { - Json::Value jv; - jv[jss::TransactionType] = jss::PaymentChannelCreate; - jv[jss::Flags] = tfUniversal; - jv[jss::Account] = Account{"bob6"}.human(); - jv[jss::Destination] = Account{"bob7"}.human(); - jv[jss::Amount] = XRP(100).value().getJson(JsonOptions::none); - jv[jss::SettleDelay] = NetClock::duration{10s}.count(); - jv[sfPublicKey.fieldName] = strHex(Account{"bob6"}.pk().slice()); - jv[sfCancelAfter.fieldName] = NetClock::time_point{env.now() + 300s} - .time_since_epoch() - .count(); - env(jv); - } + { + Json::Value jv; + jv[jss::TransactionType] = jss::EscrowCreate; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = Account{"bob5"}.human(); + jv[jss::Destination] = Account{"bob6"}.human(); + jv[jss::Amount] = XRP(50).value().getJson(JsonOptions::none); + jv[sfFinishAfter.fieldName] = + NetClock::time_point{env.now() + 10s} + .time_since_epoch() + .count(); + env(jv); + } - env(check::create("bob6", "bob7", XRP(100))); + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelCreate; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = Account{"bob6"}.human(); + jv[jss::Destination] = Account{"bob7"}.human(); + jv[jss::Amount] = XRP(100).value().getJson(JsonOptions::none); + jv[jss::SettleDelay] = NetClock::duration{10s}.count(); + jv[sfPublicKey.fieldName] = + strHex(Account{"bob6"}.pk().slice()); + jv[sfCancelAfter.fieldName] = + NetClock::time_point{env.now() + 300s} + .time_since_epoch() + .count(); + env(jv); + } - // bob9 DepositPreauths bob4 and bob8. - env(deposit::auth(Account{"bob9"}, Account{"bob4"})); - env(deposit::auth(Account{"bob9"}, Account{"bob8"})); - env.close(); + env(check::create("bob6", "bob7", XRP(100))); - // Now fetch each type + // bob9 DepositPreauths bob4 and bob8. + env(deposit::auth(Account{"bob9"}, Account{"bob4"})); + env(deposit::auth(Account{"bob9"}, Account{"bob8"})); + env.close(); - { // jvParams[jss::type] = "account"; - auto const jrr = makeRequest(jss::account); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 12)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::AccountRoot); - } + // Now fetch each type - { // jvParams[jss::type] = "amendments"; - auto const jrr = makeRequest(jss::amendments); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::Amendments); - } + { // jvParams[jss::type] = "account"; + auto const jrr = makeRequest(jss::account); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 12)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::AccountRoot); + } - { // jvParams[jss::type] = "check"; - auto const jrr = makeRequest(jss::check); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::Check); - } + { // jvParams[jss::type] = "amendments"; + auto const jrr = makeRequest(jss::amendments); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::Amendments); + } - { // jvParams[jss::type] = "directory"; - auto const jrr = makeRequest(jss::directory); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 9)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::DirectoryNode); - } + { // jvParams[jss::type] = "check"; + auto const jrr = makeRequest(jss::check); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::Check); + } - { // jvParams[jss::type] = "fee"; - auto const jrr = makeRequest(jss::fee); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::FeeSettings); - } + { // jvParams[jss::type] = "directory"; + auto const jrr = makeRequest(jss::directory); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 9)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::DirectoryNode); + } - { // jvParams[jss::type] = "hashes"; - auto const jrr = makeRequest(jss::hashes); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 2)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::LedgerHashes); - } + { // jvParams[jss::type] = "fee"; + auto const jrr = makeRequest(jss::fee); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::FeeSettings); + } - { // jvParams[jss::type] = "offer"; - auto const jrr = makeRequest(jss::offer); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::Offer); - } + { // jvParams[jss::type] = "hashes"; + auto const jrr = makeRequest(jss::hashes); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 2)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::LedgerHashes); + } - { // jvParams[jss::type] = "signer_list"; - auto const jrr = makeRequest(jss::signer_list); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::SignerList); - } + { // jvParams[jss::type] = "offer"; + auto const jrr = makeRequest(jss::offer); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::Offer); + } - { // jvParams[jss::type] = "state"; - auto const jrr = makeRequest(jss::state); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::RippleState); - } + { // jvParams[jss::type] = "signer_list"; + auto const jrr = makeRequest(jss::signer_list); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::SignerList); + } - { // jvParams[jss::type] = "ticket"; - auto const jrr = makeRequest(jss::ticket); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::Ticket); - } + { // jvParams[jss::type] = "state"; + auto const jrr = makeRequest(jss::state); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::RippleState); + } - { // jvParams[jss::type] = "escrow"; - auto const jrr = makeRequest(jss::escrow); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::Escrow); - } + { // jvParams[jss::type] = "ticket"; + auto const jrr = makeRequest(jss::ticket); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::Ticket); + } - { // jvParams[jss::type] = "payment_channel"; - auto const jrr = makeRequest(jss::payment_channel); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::PayChannel); - } + { // jvParams[jss::type] = "escrow"; + auto const jrr = makeRequest(jss::escrow); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::Escrow); + } - { // jvParams[jss::type] = "deposit_preauth"; - auto const jrr = makeRequest(jss::deposit_preauth); - BEAST_EXPECT(checkArraySize(jrr[jss::state], 2)); - for (auto const& j : jrr[jss::state]) - BEAST_EXPECT(j["LedgerEntryType"] == jss::DepositPreauth); - } + { // jvParams[jss::type] = "payment_channel"; + auto const jrr = makeRequest(jss::payment_channel); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 1)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::PayChannel); + } - { // jvParams[jss::type] = "misspelling"; - Json::Value jvParams; - jvParams[jss::ledger_index] = "current"; - jvParams[jss::type] = "misspelling"; - auto const jrr = env.rpc( - "json", - "ledger_data", - boost::lexical_cast(jvParams))[jss::result]; - BEAST_EXPECT(jrr.isMember("error")); - BEAST_EXPECT(jrr["error"] == "invalidParams"); - BEAST_EXPECT(jrr["error_message"] == "Invalid field 'type'."); + { // jvParams[jss::type] = "deposit_preauth"; + auto const jrr = makeRequest(jss::deposit_preauth); + BEAST_EXPECT(checkArraySize(jrr[jss::state], 2)); + for (auto const& j : jrr[jss::state]) + BEAST_EXPECT(j["LedgerEntryType"] == jss::DepositPreauth); + } + + { // jvParams[jss::type] = "misspelling"; + Json::Value jvParams; + jvParams[jss::ledger_index] = "current"; + jvParams[jss::type] = "misspelling"; + auto const jrr = env.rpc( + "json", + "ledger_data", + boost::lexical_cast(jvParams))[jss::result]; + BEAST_EXPECT(jrr.isMember("error")); + BEAST_EXPECT(jrr["error"] == "invalidParams"); + BEAST_EXPECT(jrr["error_message"] == "Invalid field 'type'."); + } } } diff --git a/src/xrpld/app/ledger/Ledger.cpp b/src/xrpld/app/ledger/Ledger.cpp index afed8f4870b..bcd3b6d4ba7 100644 --- a/src/xrpld/app/ledger/Ledger.cpp +++ b/src/xrpld/app/ledger/Ledger.cpp @@ -793,7 +793,7 @@ Ledger::updateNegativeUNL() if (hasToDisable) { - newNUnl.emplace_back(sfDisabledValidator); + newNUnl.push_back(STObject::makeInnerObject(sfDisabledValidator)); newNUnl.back().setFieldVL( sfPublicKey, sle->getFieldVL(sfValidatorToDisable)); newNUnl.back().setFieldU32(sfFirstLedgerSequence, seq()); diff --git a/src/xrpld/app/misc/detail/AMMUtils.cpp b/src/xrpld/app/misc/detail/AMMUtils.cpp index 0014ab01118..efc80cf17b6 100644 --- a/src/xrpld/app/misc/detail/AMMUtils.cpp +++ b/src/xrpld/app/misc/detail/AMMUtils.cpp @@ -312,7 +312,7 @@ initializeFeeAuctionVote( auto const& rules = view.rules(); // AMM creator gets the voting slot. STArray voteSlots; - STObject voteEntry = STObject::makeInnerObject(sfVoteEntry, rules); + STObject voteEntry = STObject::makeInnerObject(sfVoteEntry); if (tfee != 0) voteEntry.setFieldU16(sfTradingFee, tfee); voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR); @@ -325,7 +325,7 @@ initializeFeeAuctionVote( if (rules.enabled(fixInnerObjTemplate) && !ammSle->isFieldPresent(sfAuctionSlot)) { - STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); + STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot); ammSle->set(std::move(auctionSlot)); } STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); diff --git a/src/xrpld/app/tx/detail/AMMVote.cpp b/src/xrpld/app/tx/detail/AMMVote.cpp index 6da320f83e2..c4b6c612c63 100644 --- a/src/xrpld/app/tx/detail/AMMVote.cpp +++ b/src/xrpld/app/tx/detail/AMMVote.cpp @@ -104,7 +104,7 @@ applyVote( Number den{0}; // Account already has vote entry bool foundAccount = false; - auto const& rules = ctx_.view().rules(); + // Iterate over the current vote entries and update each entry // per current total tokens balance and each LP tokens balance. // Find the entry with the least tokens and whether the account @@ -120,7 +120,7 @@ applyVote( continue; } auto feeVal = entry[sfTradingFee]; - STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); + STObject newEntry = STObject::makeInnerObject(sfVoteEntry); // The account already has the vote entry. if (account == account_) { @@ -159,7 +159,7 @@ applyVote( { auto update = [&](std::optional const& minPos = std::nullopt) { - STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); + STObject newEntry = STObject::makeInnerObject(sfVoteEntry); if (feeNew != 0) newEntry.setFieldU16(sfTradingFee, feeNew); newEntry.setFieldU32( diff --git a/src/xrpld/app/tx/detail/Change.cpp b/src/xrpld/app/tx/detail/Change.cpp index 0ebdb1e93ba..909f35fc799 100644 --- a/src/xrpld/app/tx/detail/Change.cpp +++ b/src/xrpld/app/tx/detail/Change.cpp @@ -300,11 +300,11 @@ Change::applyAmendment() if (gotMajority) { // This amendment now has a majority - newMajorities.push_back(STObject(sfMajority)); + newMajorities.push_back(STObject::makeInnerObject(sfMajority)); auto& entry = newMajorities.back(); - entry.emplace_back(STUInt256(sfAmendment, amendment)); - entry.emplace_back(STUInt32( - sfCloseTime, view().parentCloseTime().time_since_epoch().count())); + entry[sfAmendment] = amendment; + entry[sfCloseTime] = + view().parentCloseTime().time_since_epoch().count(); if (!ctx_.app.getAmendmentTable().isSupported(amendment)) { diff --git a/src/xrpld/app/tx/detail/SetSignerList.cpp b/src/xrpld/app/tx/detail/SetSignerList.cpp index 37790f14f40..0949fbbe775 100644 --- a/src/xrpld/app/tx/detail/SetSignerList.cpp +++ b/src/xrpld/app/tx/detail/SetSignerList.cpp @@ -416,11 +416,11 @@ SetSignerList::writeSignersToSLE( STArray toLedger(signers_.size()); for (auto const& entry : signers_) { - toLedger.emplace_back(sfSignerEntry); + toLedger.push_back(STObject::makeInnerObject(sfSignerEntry)); STObject& obj = toLedger.back(); obj.reserve(2); - obj.setAccountID(sfAccount, entry.account); - obj.setFieldU16(sfSignerWeight, entry.weight); + obj[sfAccount] = entry.account; + obj[sfSignerWeight] = entry.weight; // This is a defensive check to make absolutely sure we will never write // a tag into the ledger while featureExpandedSignerList is not enabled diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 5ea895b60e4..1fee84c683b 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -1051,7 +1051,7 @@ transactionSignFor( auto& sttx = preprocResult.second; { // Make the signer object that we'll inject. - STObject signer(sfSigner); + STObject signer = STObject::makeInnerObject(sfSigner); signer[sfAccount] = *signerAccountID; signer.setFieldVL(sfTxnSignature, signForParams.getSignature()); signer.setFieldVL( From e1534a3200834ac8ed66f0413dee94b20f2250d6 Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Tue, 2 Jul 2024 14:58:03 -0400 Subject: [PATCH 2/2] fix "account_nfts" with unassociated marker returning issue (#5045) * fix "account_nfts" with unassociated marker returning issue * create unit test for fixing nft page invalid marker not returning error add more test change test name create unit test * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * fix "account_nfts" with unassociated marker returning issue * [FOLD] accumulated review suggestions * move BEAST check out of lambda function --------- Authored-by: Scott Schurr --- src/test/rpc/AccountObjects_test.cpp | 125 ++++++++++++++++++++++ src/xrpld/app/tx/detail/NFTokenMint.h | 1 + src/xrpld/rpc/handlers/AccountObjects.cpp | 28 +++-- 3 files changed, 148 insertions(+), 6 deletions(-) diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 18291bf2b95..efa9e10cc34 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -20,10 +20,12 @@ #include #include #include +#include #include #include #include #include +#include #include @@ -1032,6 +1034,128 @@ class AccountObjects_test : public beast::unit_test::suite BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hashes), 0)); } + void + testNFTsMarker() + { + // there's some bug found in account_nfts method that it did not + // return invalid params when providing unassociated nft marker. + // this test tests both situations when providing valid nft marker + // and unassociated nft marker. + testcase("NFTsMarker"); + + using namespace jtx; + Env env(*this); + + Account const bob{"bob"}; + env.fund(XRP(10000), bob); + + static constexpr unsigned nftsSize = 10; + for (unsigned i = 0; i < nftsSize; i++) + { + env(token::mint(bob, 0)); + } + + env.close(); + + // save the NFTokenIDs to use later + std::vector tokenIDs; + { + Json::Value params; + params[jss::account] = bob.human(); + params[jss::ledger_index] = "validated"; + Json::Value const resp = + env.rpc("json", "account_nfts", to_string(params)); + Json::Value const& nfts = resp[jss::result][jss::account_nfts]; + for (Json::Value const& nft : nfts) + tokenIDs.push_back(nft["NFTokenID"]); + } + + // this lambda function is used to check if the account_nfts method + // returns the correct token information. lastIndex is used to query the + // last marker. + auto compareNFTs = [&tokenIDs, &env, &bob]( + unsigned const limit, unsigned const lastIndex) { + Json::Value params; + params[jss::account] = bob.human(); + params[jss::limit] = limit; + params[jss::marker] = tokenIDs[lastIndex]; + params[jss::ledger_index] = "validated"; + Json::Value const resp = + env.rpc("json", "account_nfts", to_string(params)); + + if (resp[jss::result].isMember(jss::error)) + return false; + + Json::Value const& nfts = resp[jss::result][jss::account_nfts]; + unsigned const nftsCount = tokenIDs.size() - lastIndex - 1 < limit + ? tokenIDs.size() - lastIndex - 1 + : limit; + + if (nfts.size() != nftsCount) + return false; + + for (unsigned i = 0; i < nftsCount; i++) + { + if (nfts[i]["NFTokenID"] != tokenIDs[lastIndex + 1 + i]) + return false; + } + + return true; + }; + + // test a valid marker which is equal to the third tokenID + BEAST_EXPECT(compareNFTs(4, 2)); + + // test a valid marker which is equal to the 8th tokenID + BEAST_EXPECT(compareNFTs(4, 7)); + + // lambda that holds common code for invalid cases. + auto testInvalidMarker = [&env, &bob]( + auto marker, char const* errorMessage) { + Json::Value params; + params[jss::account] = bob.human(); + params[jss::limit] = 4; + params[jss::ledger_index] = jss::validated; + params[jss::marker] = marker; + Json::Value const resp = + env.rpc("json", "account_nfts", to_string(params)); + return resp[jss::result][jss::error_message] == errorMessage; + }; + + // test an invalid marker that is not a string + BEAST_EXPECT( + testInvalidMarker(17, "Invalid field \'marker\', not string.")); + + // test an invalid marker that has a non-hex character + BEAST_EXPECT(testInvalidMarker( + "00000000F51DFC2A09D62CBBA1DFBDD4691DAC96AD98B900000000000000000G", + "Invalid field \'marker\'.")); + + // this lambda function is used to create some fake marker using given + // taxon and sequence because we want to test some unassociated markers + // later + auto createFakeNFTMarker = [](AccountID const& issuer, + std::uint32_t taxon, + std::uint32_t tokenSeq, + std::uint16_t flags = 0, + std::uint16_t fee = 0) { + // the marker has the exact same format as an NFTokenID + return to_string(NFTokenMint::createNFTokenID( + flags, fee, issuer, nft::toTaxon(taxon), tokenSeq)); + }; + + // test an unassociated marker which does not exist in the NFTokenIDs + BEAST_EXPECT(testInvalidMarker( + createFakeNFTMarker(bob.id(), 0x000000000, 0x00000000), + "Invalid field \'marker\'.")); + + // test an unassociated marker which exceeds the maximum value of the + // existing NFTokenID + BEAST_EXPECT(testInvalidMarker( + createFakeNFTMarker(bob.id(), 0xFFFFFFFF, 0xFFFFFFFF), + "Invalid field \'marker\'.")); + } + void run() override { @@ -1039,6 +1163,7 @@ class AccountObjects_test : public beast::unit_test::suite testUnsteppedThenStepped(); testUnsteppedThenSteppedWithNFTs(); testObjectTypes(); + testNFTsMarker(); } }; diff --git a/src/xrpld/app/tx/detail/NFTokenMint.h b/src/xrpld/app/tx/detail/NFTokenMint.h index e0eb54bbc93..c95fd5944e4 100644 --- a/src/xrpld/app/tx/detail/NFTokenMint.h +++ b/src/xrpld/app/tx/detail/NFTokenMint.h @@ -22,6 +22,7 @@ #include #include +#include namespace ripple { diff --git a/src/xrpld/rpc/handlers/AccountObjects.cpp b/src/xrpld/rpc/handlers/AccountObjects.cpp index dfc4e9b3388..32fe15ec264 100644 --- a/src/xrpld/rpc/handlers/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/AccountObjects.cpp @@ -75,8 +75,9 @@ doAccountNFTs(RPC::JsonContext& context) return *err; uint256 marker; + bool const markerSet = params.isMember(jss::marker); - if (params.isMember(jss::marker)) + if (markerSet) { auto const& m = params[jss::marker]; if (!m.isString()) @@ -98,6 +99,7 @@ doAccountNFTs(RPC::JsonContext& context) // Continue iteration from the current page: bool pastMarker = marker.isZero(); + bool markerFound = false; uint256 const maskedMarker = marker & nft::pageMask; while (cp) { @@ -119,12 +121,23 @@ doAccountNFTs(RPC::JsonContext& context) uint256 const nftokenID = o[sfNFTokenID]; uint256 const maskedNftokenID = nftokenID & nft::pageMask; - if (!pastMarker && maskedNftokenID < maskedMarker) - continue; + if (!pastMarker) + { + if (maskedNftokenID < maskedMarker) + continue; - if (!pastMarker && maskedNftokenID == maskedMarker && - nftokenID <= marker) - continue; + if (maskedNftokenID == maskedMarker && nftokenID < marker) + continue; + + if (nftokenID == marker) + { + markerFound = true; + continue; + } + } + + if (markerSet && !markerFound) + return RPC::invalid_field_error(jss::marker); pastMarker = true; @@ -155,6 +168,9 @@ doAccountNFTs(RPC::JsonContext& context) cp = nullptr; } + if (markerSet && !markerFound) + return RPC::invalid_field_error(jss::marker); + result[jss::account] = toBase58(accountID); context.loadType = Resource::feeMediumBurdenRPC; return result;