Skip to content

Commit

Permalink
[FOLD] Address comments from @seelabs
Browse files Browse the repository at this point in the history
  • Loading branch information
scottschurr committed Mar 20, 2024
1 parent b40d3fc commit 132aa97
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 28 deletions.
8 changes: 1 addition & 7 deletions src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<SLE const> 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 &&
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/app/tx/impl/NFTokenCreateOffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())))
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/Feature.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions src/ripple/protocol/impl/Feature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
32 changes: 16 additions & 16 deletions src/test/app/NFToken_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7144,19 +7144,19 @@ 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.
// Otherwise we can't create NFTokens with tfTrustLine enabled.
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);
Expand Down Expand Up @@ -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),
Expand All @@ -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();
Expand Down Expand Up @@ -7319,19 +7319,19 @@ 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.
// Otherwise we can't create NFTokens with tfTrustLine enabled.
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);
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 132aa97

Please sign in to comment.