From b7e9f3e0ccc9e481e951ba631139a5546047857c Mon Sep 17 00:00:00 2001 From: dangell7 Date: Mon, 2 Jan 2023 17:23:25 -0500 Subject: [PATCH 1/5] fixNFTokenBrokerAccept update: error response fixup test error [FOLD] fix failing tests nit: comments clang-format clang-format refactor tests update comment rename xrpl fix fixup --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 36 +++++- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/NFToken_test.cpp | 115 ++++++++++++------ 4 files changed, 108 insertions(+), 47 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 07fe9957a76..7df64147870 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -112,20 +112,44 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if ((*so)[sfAmount] > (*bo)[sfAmount]) return tecINSUFFICIENT_PAYMENT; - // If the buyer specified a destination, that destination must be - // the seller or the broker. + // If the buyer specified a destination if (auto const dest = bo->at(~sfDestination)) { - if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + // fixUnburnableNFToken: Disabled + // the destination must be the seller or the broker. + if (!ctx.view.rules().enabled(fixUnburnableNFToken) && + *dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + { return tecNFTOKEN_BUY_SELL_MISMATCH; + } + + // fixUnburnableNFToken: Enabled + // the destination may only be the account brokering the offer + if (ctx.view.rules().enabled(fixUnburnableNFToken) && + *dest != ctx.tx[sfAccount]) + { + return tecNO_PERMISSION; + } } - // If the seller specified a destination, that destination must be - // the buyer or the broker. + // If the seller specified a destination if (auto const dest = so->at(~sfDestination)) { - if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount]) + // fixUnburnableNFToken: Disabled + // the destination must be the buyer or the broker. + if (!ctx.view.rules().enabled(fixUnburnableNFToken) && + *dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount]) + { return tecNFTOKEN_BUY_SELL_MISMATCH; + } + + // fixUnburnableNFToken: Enabled + // the destination may only be the account brokering the offer + if (ctx.view.rules().enabled(fixUnburnableNFToken) && + *dest != ctx.tx[sfAccount]) + { + return tecNO_PERMISSION; + } } // The broker can specify an amount that represents their cut; if they diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d4e65a31af8..291c7c65a15 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,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 = 54; +static constexpr std::size_t numFeatures = 55; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -341,6 +341,7 @@ extern uint256 const fixTrustLinesToSelf; extern uint256 const fixRemoveNFTokenAutoTrustLine; extern uint256 const featureImmediateOfferKilled; extern uint256 const featureDisallowIncoming; +extern uint256 const fixUnburnableNFToken; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 5903603f975..31d9999511d 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -451,6 +451,7 @@ REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, DefaultVote::no) REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, DefaultVote::no); REGISTER_FEATURE(DisallowIncoming, Supported::yes, DefaultVote::no); +REGISTER_FIX (fixUnburnableNFToken, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 42a6eb4d3ce..2ab4f009a6c 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -2872,15 +2872,21 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 2); BEAST_EXPECT(ownerCount(env, buyer) == 1); - // issuer cannot broker the offers, because they are not the - // Destination. - env(token::brokerOffers( - issuer, offerBuyerToMinter, offerMinterToBroker), - ter(tecNFTOKEN_BUY_SELL_MISMATCH)); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + // fixUnburnableNFToken: Disabled + { + // issuer cannot broker the offers, because they are not the + // Destination. + TER const expectTer = features[fixUnburnableNFToken] + ? tecNO_PERMISSION + : tecNFTOKEN_BUY_SELL_MISMATCH; + env(token::brokerOffers( + issuer, offerBuyerToMinter, offerMinterToBroker), + ter(expectTer)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + } // Since broker is the sell offer's destination, they can broker // the two offers. @@ -2917,29 +2923,51 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 2); - // Cannot broker offers when the sell destination is not the buyer. - env(token::brokerOffers( - broker, offerIssuerToBuyer, offerBuyerToMinter), - ter(tecNFTOKEN_BUY_SELL_MISMATCH)); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 1); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + { + // Cannot broker offers when the sell destination is not the + // buyer. + TER const expectTer = features[fixUnburnableNFToken] + ? tecNO_PERMISSION + : tecNFTOKEN_BUY_SELL_MISMATCH; + env(token::brokerOffers( + broker, offerIssuerToBuyer, offerBuyerToMinter), + ter(expectTer)); + env.close(); - // Broker is successful when destination is buyer. - env(token::brokerOffers( - broker, offerMinterToBuyer, offerBuyerToMinter)); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 1); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == 1); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 2); - // Clean out the unconsumed offer. - env(token::cancelOffer(issuer, {offerIssuerToBuyer})); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + // Broker is successful when destination is buyer. + TER const eexpectTer = features[fixUnburnableNFToken] + ? tecNO_PERMISSION + : TER(tesSUCCESS); + env(token::brokerOffers( + broker, offerMinterToBuyer, offerBuyerToMinter), + ter(eexpectTer)); + env.close(); + + if (features[fixUnburnableNFToken]) + // Buyer is successful with acceptOffer. + env(token::acceptBuyOffer(buyer, offerMinterToBuyer)); + env.close(); + + // Clean out the unconsumed offer. + env(token::cancelOffer(buyer, {offerBuyerToMinter})); + env.close(); + + BEAST_EXPECT(ownerCount(env, issuer) == 1); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + + // Clean out the unconsumed offer. + env(token::cancelOffer(issuer, {offerIssuerToBuyer})); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + return; + } } // Show that if a buy and a sell offer both have the same destination, @@ -2957,15 +2985,20 @@ class NFToken_test : public beast::unit_test::suite token::owner(minter), token::destination(broker)); - // Cannot broker offers when the sell destination is not the buyer - // or the broker. - env(token::brokerOffers( - issuer, offerBuyerToBroker, offerMinterToBroker), - ter(tecNFTOKEN_BUY_SELL_MISMATCH)); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + { + // Cannot broker offers when the sell destination is not the + // buyer or the broker. + TER const expectTer = features[fixUnburnableNFToken] + ? tecNO_PERMISSION + : tecNFTOKEN_BUY_SELL_MISMATCH; + env(token::brokerOffers( + issuer, offerBuyerToBroker, offerMinterToBroker), + ter(expectTer)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + } // Broker is successful if they are the destination of both offers. env(token::brokerOffers( @@ -5079,13 +5112,15 @@ class NFToken_test : public beast::unit_test::suite using namespace test::jtx; FeatureBitset const all{supported_amendments()}; FeatureBitset const fixNFTDir{fixNFTokenDirV1}; + FeatureBitset const fixNFTBroker{fixUnburnableNFToken}; testWithFeats(all - fixNFTDir); testWithFeats(all - disallowIncoming); + testWithFeats(all - fixNFTBroker); testWithFeats(all); } }; BEAST_DEFINE_TESTSUITE_PRIO(NFToken, tx, ripple, 2); -} // namespace ripple +} // namespace ripple \ No newline at end of file From 723f15c2c8e85b65c6ae9799f06a4941de15e20e Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 13 Feb 2023 01:37:04 -0500 Subject: [PATCH 2/5] refactor --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 7df64147870..bd1e718a052 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -115,40 +115,38 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) // If the buyer specified a destination if (auto const dest = bo->at(~sfDestination)) { - // fixUnburnableNFToken: Disabled - // the destination must be the seller or the broker. - if (!ctx.view.rules().enabled(fixUnburnableNFToken) && - *dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + + // fixUnburnableNFToken + if (ctx.view.rules().enabled(fixUnburnableNFToken)) { - return tecNFTOKEN_BUY_SELL_MISMATCH; + // the destination may only be the account brokering the offer + if (*dest != ctx.tx[sfAccount]) + return tecNO_PERMISSION; } - - // fixUnburnableNFToken: Enabled - // the destination may only be the account brokering the offer - if (ctx.view.rules().enabled(fixUnburnableNFToken) && - *dest != ctx.tx[sfAccount]) + else { - return tecNO_PERMISSION; + // the destination must be the seller or the broker. + if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + return tecNFTOKEN_BUY_SELL_MISMATCH; } } // If the seller specified a destination if (auto const dest = so->at(~sfDestination)) { - // fixUnburnableNFToken: Disabled - // the destination must be the buyer or the broker. - if (!ctx.view.rules().enabled(fixUnburnableNFToken) && - *dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount]) + + // fixUnburnableNFToken + if (ctx.view.rules().enabled(fixUnburnableNFToken)) { - return tecNFTOKEN_BUY_SELL_MISMATCH; + // the destination may only be the account brokering the offer + if (*dest != ctx.tx[sfAccount]) + return tecNO_PERMISSION; } - - // fixUnburnableNFToken: Enabled - // the destination may only be the account brokering the offer - if (ctx.view.rules().enabled(fixUnburnableNFToken) && - *dest != ctx.tx[sfAccount]) + else { - return tecNO_PERMISSION; + // the destination must be the buyer or the broker. + if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount]) + return tecNFTOKEN_BUY_SELL_MISMATCH; } } From 5cfdcd72dc9aff0e5794e6f6e335957fcb30fef5 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 13 Feb 2023 01:39:45 -0500 Subject: [PATCH 3/5] remove feature test set --- src/test/app/NFToken_test.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 2ab4f009a6c..2513d96168a 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -5112,11 +5112,9 @@ class NFToken_test : public beast::unit_test::suite using namespace test::jtx; FeatureBitset const all{supported_amendments()}; FeatureBitset const fixNFTDir{fixNFTokenDirV1}; - FeatureBitset const fixNFTBroker{fixUnburnableNFToken}; testWithFeats(all - fixNFTDir); testWithFeats(all - disallowIncoming); - testWithFeats(all - fixNFTBroker); testWithFeats(all); } }; From f519f90f52184a70c9e2de3f011f9bd94fe952e9 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 13 Feb 2023 01:45:07 -0500 Subject: [PATCH 4/5] clean comments --- src/test/app/NFToken_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 2513d96168a..ac214a26837 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -2872,7 +2872,6 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 2); BEAST_EXPECT(ownerCount(env, buyer) == 1); - // fixUnburnableNFToken: Disabled { // issuer cannot broker the offers, because they are not the // Destination. @@ -2938,7 +2937,8 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 2); - // Broker is successful when destination is buyer. + // amendment switch: When enabled the broker fails, when + // disabled the broker succeeds if the destination is the buyer. TER const eexpectTer = features[fixUnburnableNFToken] ? tecNO_PERMISSION : TER(tesSUCCESS); From 7cadd0cc6636665f976ea4347880deaf1f78d1d9 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 13 Feb 2023 01:45:15 -0500 Subject: [PATCH 5/5] clang-format --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index bd1e718a052..3e36cac1cbe 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -115,7 +115,6 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) // If the buyer specified a destination if (auto const dest = bo->at(~sfDestination)) { - // fixUnburnableNFToken if (ctx.view.rules().enabled(fixUnburnableNFToken)) { @@ -134,7 +133,6 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) // If the seller specified a destination if (auto const dest = so->at(~sfDestination)) { - // fixUnburnableNFToken if (ctx.view.rules().enabled(fixUnburnableNFToken)) {