From 132aa975b0262b3a93a6638cdd4bfbf2af3c05ba Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 20 Mar 2024 16:00:22 -0700 Subject: [PATCH] [FOLD] Address comments from @seelabs --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 8 +---- src/ripple/app/tx/impl/NFTokenCreateOffer.cpp | 4 +-- src/ripple/protocol/Feature.h | 2 +- src/ripple/protocol/impl/Feature.cpp | 4 +-- src/test/app/NFToken_test.cpp | 32 +++++++++---------- 5 files changed, 22 insertions(+), 28 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index c27fb188028..f61d7fe7b96 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -272,19 +272,13 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) // Fix a bug where the transfer of an NFToken with a transfer fee could // give the NFToken issuer an undesired trust line. - if (ctx.view.rules().enabled(fixNFTokenTrustlineSurprise)) + if (ctx.view.rules().enabled(fixEnforceNFTokenTrustline)) { std::shared_ptr const& offer = bo ? bo : so; if (!offer) // Should be caught in preflight. return tecINTERNAL; - // - If the NFToken has a transfer fee, and - // - If the NFToken doesn't have the flagCreateTrustLines flag set, and - // - If the Amount is not XRP, and - // - If the NFToken issuer is not the Amount issuer, and - // - If the NFToken issuer does not have a trust line for the Amount - // - Then reject the token accept. uint256 const& tokenID = offer->at(sfNFTokenID); STAmount const& amount = offer->at(sfAmount); if (nft::getTransferFee(tokenID) != 0 && diff --git a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index aab306c6b34..1155ce94a90 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -123,10 +123,10 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx) if (!ctx.view.exists(keylet::account(issuer))) return tecNO_ISSUER; - // There was a bug in a corner case that fixNFTokenTrustlineSurprise + // There was a bug in a corner case that fixEnforceNFTokenTrustline // addresses. If the IOU issuer and the NFToken issuer are the same, // then that issuer does not need a trust line to accept their fee. - if (ctx.view.rules().enabled(fixNFTokenTrustlineSurprise)) + if (ctx.view.rules().enabled(fixEnforceNFTokenTrustline)) { if (issuer != amount.getIssuer() && !ctx.view.read(keylet::line(issuer, amount.issue()))) diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 29022498068..0be01d581dc 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -355,7 +355,7 @@ extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; extern uint256 const fixInnerObjTemplate; extern uint256 const featurePriceOracle; -extern uint256 const fixNFTokenTrustlineSurprise; +extern uint256 const fixEnforceNFTokenTrustline; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index daeeb2baf90..7f872e20d5e 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -458,11 +458,11 @@ REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::De REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX (fixNFTokenTrustlineSurprise, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixEnforceNFTokenTrustline, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 10df546bd9e..19a5ff770e9 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -7144,10 +7144,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // 7. The transfer fee from Carol's purchase re-establishes issuer's // USD trustline. // - // The fixNFTokenTrustlineSurprise amendment addresses this oversight. + // The fixEnforceNFTokenTrustline amendment addresses this oversight. // // We run this test case both with and without - // fixNFTokenTrustlineSurprise enabled so we can see the change + // fixEnforceNFTokenTrustline enabled so we can see the change // in behavior. // // In both cases we remove the fixRemoveNFTokenAutoTrustLine amendment. @@ -7155,8 +7155,8 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite FeatureBitset const localFeatures = features - fixRemoveNFTokenAutoTrustLine; for (FeatureBitset feats : - {localFeatures - fixNFTokenTrustlineSurprise, - localFeatures | fixNFTokenTrustlineSurprise}) + {localFeatures - fixEnforceNFTokenTrustline, + localFeatures | fixEnforceNFTokenTrustline}) { Env env{*this, feats}; env.fund(XRP(1000), issuer, becky, cheri, gw); @@ -7251,10 +7251,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, issuer) == 0); // cheri attempts to accept the NoAutoTrustLine NFT. Behavior - // depends on whether fixNFTokenTrustlineSurprise is enabled. - if (feats[fixNFTokenTrustlineSurprise]) + // depends on whether fixEnforceNFTokenTrustline is enabled. + if (feats[fixEnforceNFTokenTrustline]) { - // With fixNFTokenTrustlineSurprise cheri can't accept the + // With fixEnforceNFTokenTrustline cheri can't accept the // offer because issuer could not get their transfer fee // without the appropriate trustline. env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex), @@ -7272,7 +7272,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } else { - // Without fixNFTokenTrustlineSurprise the offer just works + // Without fixEnforceNFTokenTrustline the offer just works // and issuer gets a trustline that they did not request. env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex)); env.close(); @@ -7319,10 +7319,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // 5. Becky attempts to create an offer to sell the NFToken for // isISU(100). The attempt fails with `tecNO_LINE`. // - // The fixNFTokenTrustlineSurprise amendment addresses this oversight. + // The fixEnforceNFTokenTrustline amendment addresses this oversight. // // We run this test case both with and without - // fixNFTokenTrustlineSurprise enabled so we can see the change + // fixEnforceNFTokenTrustline enabled so we can see the change // in behavior. // // In both cases we remove the fixRemoveNFTokenAutoTrustLine amendment. @@ -7330,8 +7330,8 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite FeatureBitset const localFeatures = features - fixRemoveNFTokenAutoTrustLine; for (FeatureBitset feats : - {localFeatures - fixNFTokenTrustlineSurprise, - localFeatures | fixNFTokenTrustlineSurprise}) + {localFeatures - fixEnforceNFTokenTrustline, + localFeatures | fixEnforceNFTokenTrustline}) { Env env{*this, feats}; env.fund(XRP(1000), issuer, becky, cheri); @@ -7379,10 +7379,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } // Behavior from here down diverges significantly based on - // fixNFTokenTrustlineSurprise. - if (!feats[fixNFTokenTrustlineSurprise]) + // fixEnforceNFTokenTrustline. + if (!feats[fixEnforceNFTokenTrustline]) { - // Without fixNFTokenTrustlineSurprise becky simply can't + // Without fixEnforceNFTokenTrustline becky simply can't // create an offer for a non-tfTrustLine NFToken that would // pay the transfer fee in issuer's own IOU. env(token::createOffer(becky, nftNoAutoTrustID, isISU(100)), @@ -7413,7 +7413,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } else { - // With fixNFTokenTrustlineSurprise things go better. + // With fixEnforceNFTokenTrustline things go better. // becky creates offers to sell the nfts for ISU. uint256 const beckyNoAutoTrustOfferIndex = keylet::nftoffer(becky, env.seq(becky)).key;