From 8f8508468f4b9743fbf20d9726b032a07e008168 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Mon, 29 Apr 2024 14:47:02 -0700 Subject: [PATCH] [FOLD] Refactor NFTokenMint and NFTokenCreateOffer to share code --- src/ripple/app/tx/impl/NFTokenCreateOffer.cpp | 244 ++--------------- src/ripple/app/tx/impl/NFTokenMint.cpp | 175 ++++-------- .../app/tx/impl/details/NFTokenUtils.cpp | 255 ++++++++++++++++++ src/ripple/app/tx/impl/details/NFTokenUtils.h | 27 ++ src/test/app/NFToken_test.cpp | 194 +++++++++++-- 5 files changed, 527 insertions(+), 368 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index 22eca2dffdd..a9a5237d400 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -37,65 +37,16 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) return ret; auto const txFlags = ctx.tx.getFlags(); - bool const isSellOffer = txFlags & tfSellNFToken; if (txFlags & tfNFTokenCreateOfferMask) return temINVALID_FLAG; - auto const account = ctx.tx[sfAccount]; auto const nftFlags = nft::getFlags(ctx.tx[sfNFTokenID]); - { - STAmount const amount = ctx.tx[sfAmount]; - - if (amount.negative() && ctx.rules.enabled(fixNFTokenNegOffer)) - // An offer for a negative amount makes no sense. - return temBAD_AMOUNT; - - if (!isXRP(amount)) - { - if (nftFlags & nft::flagOnlyXRP) - return temBAD_AMOUNT; - - if (!amount) - return temBAD_AMOUNT; - } - - // If this is an offer to buy, you must offer something; if it's an - // offer to sell, you can ask for nothing. - if (!isSellOffer && !amount) - return temBAD_AMOUNT; - } - - if (auto exp = ctx.tx[~sfExpiration]; exp == 0) - return temBAD_EXPIRATION; - - auto const owner = ctx.tx[~sfOwner]; - - // The 'Owner' field must be present when offering to buy, but can't - // be present when selling (it's implicit): - if (owner.has_value() == isSellOffer) - return temMALFORMED; - - if (owner && owner == account) - return temMALFORMED; - - if (auto dest = ctx.tx[~sfDestination]) - { - // Some folks think it makes sense for a buy offer to specify a - // specific broker using the Destination field. This change doesn't - // deserve it's own amendment, so we're piggy-backing on - // fixNFTokenNegOffer. - // - // Prior to fixNFTokenNegOffer any use of the Destination field on - // a buy offer was malformed. - if (!isSellOffer && !ctx.rules.enabled(fixNFTokenNegOffer)) - return temMALFORMED; - - // The destination can't be the account executing the transaction. - if (dest == account) - return temMALFORMED; - } + // Use implementation shared with NFTokenMint + if (NotTEC notTec = nft::tokenOfferCreatePreflight(ctx, nftFlags, txFlags); + !isTesSuccess(notTec)) + return notTec; return preflight2(ctx); } @@ -106,182 +57,35 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx) if (hasExpired(ctx.view, ctx.tx[~sfExpiration])) return tecEXPIRED; - auto const nftokenID = ctx.tx[sfNFTokenID]; - bool const isSellOffer = ctx.tx.isFlag(tfSellNFToken); + uint256 const nftokenID = ctx.tx[sfNFTokenID]; + std::uint32_t const txFlags = {ctx.tx.getFlags()}; if (!nft::findToken( - ctx.view, ctx.tx[isSellOffer ? sfAccount : sfOwner], nftokenID)) - return tecNO_ENTRY; - - auto const nftFlags = nft::getFlags(nftokenID); - auto const issuer = nft::getIssuer(nftokenID); - auto const amount = ctx.tx[sfAmount]; - - if (!(nftFlags & nft::flagCreateTrustLines) && !amount.native() && - nft::getTransferFee(nftokenID)) - { - if (!ctx.view.exists(keylet::account(issuer))) - return tecNO_ISSUER; - - if (!ctx.view.exists(keylet::line(issuer, amount.issue()))) - return tecNO_LINE; - - if (isFrozen( - ctx.view, issuer, amount.getCurrency(), amount.getIssuer())) - return tecFROZEN; - } - - if (issuer != ctx.tx[sfAccount] && !(nftFlags & nft::flagTransferable)) - { - auto const root = ctx.view.read(keylet::account(issuer)); - assert(root); - - if (auto minter = (*root)[~sfNFTokenMinter]; - minter != ctx.tx[sfAccount]) - return tefNFTOKEN_IS_NOT_TRANSFERABLE; - } - - if (isFrozen( ctx.view, - ctx.tx[sfAccount], - amount.getCurrency(), - amount.getIssuer())) - return tecFROZEN; - - // If this is an offer to buy the token, the account must have the - // needed funds at hand; but note that funds aren't reserved and the - // offer may later become unfunded. - if (!isSellOffer) - { - // After this amendment, we allow an IOU issuer to make a buy offer - // using their own currency. - if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2)) - { - if (accountFunds( - ctx.view, - ctx.tx[sfAccount], - amount, - FreezeHandling::fhZERO_IF_FROZEN, - ctx.j) - .signum() <= 0) - return tecUNFUNDED_OFFER; - } - else if ( - accountHolds( - ctx.view, - ctx.tx[sfAccount], - amount.getCurrency(), - amount.getIssuer(), - FreezeHandling::fhZERO_IF_FROZEN, - ctx.j) - .signum() <= 0) - return tecUNFUNDED_OFFER; - } - - if (auto const destination = ctx.tx[~sfDestination]) - { - // If a destination is specified, the destination must already be in - // the ledger. - auto const sleDst = ctx.view.read(keylet::account(*destination)); - - if (!sleDst) - return tecNO_DST; - - // check if the destination has disallowed incoming offers - if (ctx.view.rules().enabled(featureDisallowIncoming)) - { - // flag cannot be set unless amendment is enabled but - // out of an abundance of caution check anyway - - if (sleDst->getFlags() & lsfDisallowIncomingNFTokenOffer) - return tecNO_PERMISSION; - } - } - - if (auto const owner = ctx.tx[~sfOwner]) - { - // Check if the owner (buy offer) has disallowed incoming offers - if (ctx.view.rules().enabled(featureDisallowIncoming)) - { - auto const sleOwner = ctx.view.read(keylet::account(*owner)); - - // defensively check - // it should not be possible to specify owner that doesn't exist - if (!sleOwner) - return tecNO_TARGET; - - if (sleOwner->getFlags() & lsfDisallowIncomingNFTokenOffer) - return tecNO_PERMISSION; - } - } + ctx.tx[(txFlags & tfSellNFToken) ? sfAccount : sfOwner], + nftokenID)) + return tecNO_ENTRY; - return tesSUCCESS; + // Use implementation shared with NFTokenMint + return nft::tokenOfferCreatePreclaim( + ctx, + nft::getIssuer(nftokenID), + nft::getFlags(nftokenID), + nft::getTransferFee(nftokenID), + txFlags); } TER NFTokenCreateOffer::doApply() { - if (auto const acct = view().read(keylet::account(ctx_.tx[sfAccount])); - mPriorBalance < view().fees().accountReserve((*acct)[sfOwnerCount] + 1)) - return tecINSUFFICIENT_RESERVE; - - auto const nftokenID = ctx_.tx[sfNFTokenID]; - - auto const offerID = - keylet::nftoffer(account_, ctx_.tx.getSeqProxy().value()); - - // Create the offer: - { - // Token offers are always added to the owner's owner directory: - auto const ownerNode = view().dirInsert( - keylet::ownerDir(account_), offerID, describeOwnerDir(account_)); - - if (!ownerNode) - return tecDIR_FULL; - - bool const isSellOffer = ctx_.tx.isFlag(tfSellNFToken); - - // Token offers are also added to the token's buy or sell offer - // directory - auto const offerNode = view().dirInsert( - isSellOffer ? keylet::nft_sells(nftokenID) - : keylet::nft_buys(nftokenID), - offerID, - [&nftokenID, isSellOffer](std::shared_ptr const& sle) { - (*sle)[sfFlags] = - isSellOffer ? lsfNFTokenSellOffers : lsfNFTokenBuyOffers; - (*sle)[sfNFTokenID] = nftokenID; - }); - - if (!offerNode) - return tecDIR_FULL; - - std::uint32_t sleFlags = 0; - - if (isSellOffer) - sleFlags |= lsfSellNFToken; - - auto offer = std::make_shared(offerID); - (*offer)[sfOwner] = account_; - (*offer)[sfNFTokenID] = nftokenID; - (*offer)[sfAmount] = ctx_.tx[sfAmount]; - (*offer)[sfFlags] = sleFlags; - (*offer)[sfOwnerNode] = *ownerNode; - (*offer)[sfNFTokenOfferNode] = *offerNode; - - if (auto const expiration = ctx_.tx[~sfExpiration]) - (*offer)[sfExpiration] = *expiration; - - if (auto const destination = ctx_.tx[~sfDestination]) - (*offer)[sfDestination] = *destination; - - view().insert(offer); - } - - // Update owner count. - adjustOwnerCount(view(), view().peek(keylet::account(account_)), 1, j_); - - return tesSUCCESS; + // Use implementation shared with NFTokenMint + return nft::tokenOfferCreateApply( + ctx_, + view(), + ctx_.tx[sfNFTokenID], + mPriorBalance, + ctx_.tx.getFlags(), + j_); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/NFTokenMint.cpp b/src/ripple/app/tx/impl/NFTokenMint.cpp index aa1588baa95..eb7f3bc4e50 100644 --- a/src/ripple/app/tx/impl/NFTokenMint.cpp +++ b/src/ripple/app/tx/impl/NFTokenMint.cpp @@ -31,16 +31,23 @@ namespace ripple { +static std::uint16_t +extractNFTokenFlagsFromTxFlags(std::uint32_t txFlags) +{ + return static_cast(txFlags & 0x0000FFFF); +} + NotTEC NFTokenMint::preflight(PreflightContext const& ctx) { if (!ctx.rules.enabled(featureNonFungibleTokensV1)) return temDISABLED; - if (!ctx.rules.enabled(featureNFTokenMintOffer) && - (ctx.tx.isFieldPresent(sfAmount) || - ctx.tx.isFieldPresent(sfDestination) || - ctx.tx.isFieldPresent(sfExpiration))) + bool const hasOfferFields = ctx.tx.isFieldPresent(sfAmount) || + ctx.tx.isFieldPresent(sfDestination) || + ctx.tx.isFieldPresent(sfExpiration); + + if (!ctx.rules.enabled(featureNFTokenMintOffer) && hasOfferFields) return temDISABLED; if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) @@ -86,40 +93,26 @@ NFTokenMint::preflight(PreflightContext const& ctx) return temMALFORMED; } - // Amount field should be set if Destination and/or Expiration is set - if (ctx.tx.isFieldPresent(sfDestination) || - ctx.tx.isFieldPresent(sfExpiration)) + if (hasOfferFields) { + // The Amount field must be present if either the Destination or + // Expiration fields are present. if (!ctx.tx.isFieldPresent(sfAmount)) return temMALFORMED; - } - - if (auto const amount = ctx.tx[~sfAmount]) - { - if (amount->negative() && ctx.rules.enabled(fixNFTokenNegOffer)) - // An offer for a negative amount makes no sense. - return temBAD_AMOUNT; - if (!isXRP(*amount)) + // Rely on the common code shared with NFTokenCreateOffer to + // do the validation. We pass tfSellNFToken as the transaction flags + // because a Mint is only allowed to create a sell offer. + if (NotTEC notTec = nft::tokenOfferCreatePreflight( + ctx, + extractNFTokenFlagsFromTxFlags(ctx.tx.getFlags()), + tfSellNFToken); + !isTesSuccess(notTec)) { - if (ctx.tx.isFlag(tfOnlyXRP)) - return temBAD_AMOUNT; - - if (!*amount) - return temBAD_AMOUNT; + return notTec; } } - if (auto const exp = ctx.tx[~sfExpiration]; exp == 0) - return temBAD_EXPIRATION; - - if (auto const dest = ctx.tx[~sfDestination]) - { - // The destination can't be the account executing the transaction. - if (dest == ctx.tx[sfAccount]) - return temMALFORMED; - } - return preflight2(ctx); } @@ -186,56 +179,23 @@ NFTokenMint::preclaim(PreclaimContext const& ctx) return tecNO_PERMISSION; } - if (auto const amount = ctx.tx[~sfAmount]) + if (ctx.tx.isFieldPresent(sfAmount)) { + // The Amount field says create an offer for the minted token. if (hasExpired(ctx.view, ctx.tx[~sfExpiration])) return tecEXPIRED; - if (!(ctx.tx.getFlags() & nft::flagCreateTrustLines) && - !amount->native() && ctx.tx[~sfTransferFee]) - { - auto const nftIssuer = - ctx.tx[~sfIssuer].value_or(ctx.tx[sfAccount]); - - // 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 (nftIssuer != amount->getIssuer() && - !ctx.view.read(keylet::line(nftIssuer, amount->issue()))) - return tecNO_LINE; - - if (isFrozen( - ctx.view, - nftIssuer, - amount->getCurrency(), - amount->getIssuer())) - return tecFROZEN; - } - - if (isFrozen( - ctx.view, - ctx.tx[sfAccount], - amount->getCurrency(), - amount->getIssuer())) - return tecFROZEN; - - if (auto const destination = ctx.tx[~sfDestination]) - { - // If a destination is specified, the destination must already be in - // the ledger. - auto const sleDst = ctx.view.read(keylet::account(*destination)); - - if (!sleDst) - return tecNO_DST; - - // check if the destination has disallowed incoming offers - if (ctx.view.rules().enabled(featureDisallowIncoming)) - { - // flag cannot be set unless amendment is enabled but - // out of an abundance of caution check anyway - if (sleDst->getFlags() & lsfDisallowIncomingNFTokenOffer) - return tecNO_PERMISSION; - } - } + // Rely on the common code shared with NFTokenCreateOffer to + // do the validation. We pass tfSellNFToken as the transaction flags + // because a Mint is only allowed to create a sell offer. + if (TER const ter = nft::tokenOfferCreatePreclaim( + ctx, + ctx.tx[~sfIssuer].value_or(ctx.tx[sfAccount]), + extractNFTokenFlagsFromTxFlags(ctx.tx.getFlags()), + ctx.tx[~sfTransferFee].value_or(0), + tfSellNFToken); + !isTesSuccess(ter)) + return ter; } return tesSUCCESS; } @@ -330,7 +290,7 @@ NFTokenMint::doApply() return tecINTERNAL; auto const nftokenID = createNFTokenID( - static_cast(ctx_.tx.getFlags() & 0x0000FFFF), + extractNFTokenFlagsFromTxFlags(ctx_.tx.getFlags()), ctx_.tx[~sfTransferFee].value_or(0), issuer, nft::toTaxon(ctx_.tx[sfNFTokenTaxon]), @@ -351,64 +311,19 @@ NFTokenMint::doApply() if (ctx_.tx.isFieldPresent(sfAmount)) { - auto const offerID = - keylet::nftoffer(account_, ctx_.tx.getSeqProxy().value()); - - // Create the offer: - { - // Token offers are always added to the owner's owner directory: - auto const ownerNode = ctx_.view().dirInsert( - keylet::ownerDir(account_), - offerID, - describeOwnerDir(account_)); - - if (!ownerNode) - return tecDIR_FULL; - - // Token offers are also added to the token's sell offer - // directory - auto const offerNode = ctx_.view().dirInsert( - keylet::nft_sells(nftokenID), - offerID, - [&nftokenID](std::shared_ptr const& sle) { - (*sle)[sfFlags] = lsfNFTokenSellOffers; - (*sle)[sfNFTokenID] = nftokenID; - }); - - if (!offerNode) - return tecDIR_FULL; - - // Only sell offer are supported at NFTokenMint. - std::uint32_t const sleFlags = lsfSellNFToken; - - auto offer = std::make_shared(offerID); - (*offer)[sfOwner] = account_; - (*offer)[sfNFTokenID] = nftokenID; - (*offer)[sfAmount] = ctx_.tx[sfAmount]; - (*offer)[sfFlags] = sleFlags; - (*offer)[sfOwnerNode] = *ownerNode; - (*offer)[sfNFTokenOfferNode] = *offerNode; - - if (auto const expiration = ctx_.tx[~sfExpiration]) - (*offer)[sfExpiration] = *expiration; - - if (auto const destination = ctx_.tx[~sfDestination]) - (*offer)[sfDestination] = *destination; - - ctx_.view().insert(offer); - - adjustOwnerCount( - ctx_.view(), - ctx_.view().peek(keylet::account(account_)), - 1, - j_); - } + // Rely on the common code shared with NFTokenCreateOffer to create + // the offer. We pass tfSellNFToken as the transaction flags + // because a Mint is only allowed to create a sell offer. + if (TER const ter = nft::tokenOfferCreateApply( + ctx_, view(), nftokenID, mPriorBalance, tfSellNFToken, j_); + !isTesSuccess(ter)) + return ter; } // Only check the reserve if the owner count actually changed. This // allows NFTs to be added to the page (and burn fees) without // requiring the reserve to be met each time. The reserve is - // only managed when a new NFT page is added. + // only managed when a new NFT page or sell offer is added. if (auto const ownerCountAfter = view().read(keylet::account(account_))->getFieldU32(sfOwnerCount); ownerCountAfter > ownerCountBefore) diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp index 09ff8f13caa..781fd321169 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp +++ b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp @@ -636,5 +636,260 @@ deleteTokenOffer(ApplyView& view, std::shared_ptr const& offer) return true; } +NotTEC +tokenOfferCreatePreflight( + PreflightContext const& ctx, std::uint16_t nftFlags, std::uint32_t txFlags) +{ + bool const isSellOffer = txFlags & tfSellNFToken; + { + // Scope amount. We don't need it for the entire function. + STAmount const amount = ctx.tx[sfAmount]; + + if (amount.negative() && ctx.rules.enabled(fixNFTokenNegOffer)) + // An offer for a negative amount makes no sense. + return temBAD_AMOUNT; + + if (!isXRP(amount)) + { + if (nftFlags & nft::flagOnlyXRP) + return temBAD_AMOUNT; + + if (!amount) + return temBAD_AMOUNT; + } + + // If this is an offer to buy, you must offer something; if it's an + // offer to sell, you can ask for nothing. + if (!isSellOffer && !amount) + return temBAD_AMOUNT; + } + + if (auto exp = ctx.tx[~sfExpiration]; exp == 0) + return temBAD_EXPIRATION; + + auto const owner = ctx.tx[~sfOwner]; + + // The 'Owner' field must be present when offering to buy, but can't + // be present when selling (it's implicit): + if (owner.has_value() == isSellOffer) + return temMALFORMED; + + auto const account = ctx.tx[sfAccount]; + if (owner && owner == account) + return temMALFORMED; + + if (auto dest = ctx.tx[~sfDestination]) + { + // Some folks think it makes sense for a buy offer to specify a + // specific broker using the Destination field. This change doesn't + // deserve it's own amendment, so we're piggy-backing on + // fixNFTokenNegOffer. + // + // Prior to fixNFTokenNegOffer any use of the Destination field on + // a buy offer was malformed. + if (!isSellOffer && !ctx.rules.enabled(fixNFTokenNegOffer)) + return temMALFORMED; + + // The destination can't be the account executing the transaction. + if (dest == account) + return temMALFORMED; + } + return tesSUCCESS; +} + +TER +tokenOfferCreatePreclaim( + PreclaimContext const& ctx, + AccountID const& nftIssuer, // Issuer if present, otherwise Account + std::uint16_t nftFlags, + std::uint16_t xferFee, + std::uint32_t txFlags) +{ + bool const isSellOffer = txFlags & tfSellNFToken; + auto const amount = ctx.tx[sfAmount]; + + if (!(nftFlags & nft::flagCreateTrustLines) && !amount.native() && xferFee) + { + if (!ctx.view.exists(keylet::account(nftIssuer))) + return tecNO_ISSUER; + + // 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(featureNFTokenMintOffer)) + { + if (nftIssuer != amount.getIssuer() && + !ctx.view.read(keylet::line(nftIssuer, amount.issue()))) + return tecNO_LINE; + } + else if (!ctx.view.exists(keylet::line(nftIssuer, amount.issue()))) + { + return tecNO_LINE; + } + + if (isFrozen( + ctx.view, nftIssuer, amount.getCurrency(), amount.getIssuer())) + return tecFROZEN; + } + + AccountID const acctID = ctx.tx[sfAccount]; + if (nftIssuer != acctID && !(nftFlags & nft::flagTransferable)) + { + auto const root = ctx.view.read(keylet::account(nftIssuer)); + assert(root); + + if (auto minter = (*root)[~sfNFTokenMinter]; minter != acctID) + return tefNFTOKEN_IS_NOT_TRANSFERABLE; + } + + if (isFrozen( + ctx.view, + acctID, + amount.getCurrency(), + amount.getIssuer())) + return tecFROZEN; + + // If this is an offer to buy the token, the account must have the + // needed funds at hand; but note that funds aren't reserved and the + // offer may later become unfunded. + if (!isSellOffer) + { + // After this amendment, we allow an IOU issuer to make a buy offer + // using their own currency. + if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2)) + { + if (accountFunds( + ctx.view, + acctID, + amount, + FreezeHandling::fhZERO_IF_FROZEN, + ctx.j) + .signum() <= 0) + return tecUNFUNDED_OFFER; + } + else if ( + accountHolds( + ctx.view, + acctID, + amount.getCurrency(), + amount.getIssuer(), + FreezeHandling::fhZERO_IF_FROZEN, + ctx.j) + .signum() <= 0) + return tecUNFUNDED_OFFER; + } + + if (auto const destination = ctx.tx[~sfDestination]) + { + // If a destination is specified, the destination must already be in + // the ledger. + auto const sleDst = ctx.view.read(keylet::account(*destination)); + + if (!sleDst) + return tecNO_DST; + + // check if the destination has disallowed incoming offers + if (ctx.view.rules().enabled(featureDisallowIncoming)) + { + // flag cannot be set unless amendment is enabled but + // out of an abundance of caution check anyway + + if (sleDst->getFlags() & lsfDisallowIncomingNFTokenOffer) + return tecNO_PERMISSION; + } + } + + if (auto const owner = ctx.tx[~sfOwner]) + { + // Check if the owner (buy offer) has disallowed incoming offers + if (ctx.view.rules().enabled(featureDisallowIncoming)) + { + auto const sleOwner = ctx.view.read(keylet::account(*owner)); + + // defensively check + // it should not be possible to specify owner that doesn't exist + if (!sleOwner) + return tecNO_TARGET; + + if (sleOwner->getFlags() & lsfDisallowIncomingNFTokenOffer) + return tecNO_PERMISSION; + } + } + + return tesSUCCESS; +} + +TER +tokenOfferCreateApply( + ApplyContext const& ctx, + ApplyView& view, + uint256 const& nftokenID, + XRPAmount const& priorBalance, + std::uint32_t txFlags, + beast::Journal j) +{ + AccountID const acctID = ctx.tx[sfAccount]; + + Keylet const acctKeylet = keylet::account(acctID); + if (auto const acct = view.read(acctKeylet); + priorBalance < view.fees().accountReserve((*acct)[sfOwnerCount] + 1)) + return tecINSUFFICIENT_RESERVE; + + auto const offerID = + keylet::nftoffer(acctID, ctx.tx.getSeqProxy().value()); + + // Create the offer: + { + // Token offers are always added to the owner's owner directory: + auto const ownerNode = view.dirInsert( + keylet::ownerDir(acctID), offerID, describeOwnerDir(acctID)); + + if (!ownerNode) + return tecDIR_FULL; + + bool const isSellOffer = txFlags & tfSellNFToken; + + // Token offers are also added to the token's buy or sell offer + // directory + auto const offerNode = view.dirInsert( + isSellOffer ? keylet::nft_sells(nftokenID) + : keylet::nft_buys(nftokenID), + offerID, + [&nftokenID, isSellOffer](std::shared_ptr const& sle) { + (*sle)[sfFlags] = + isSellOffer ? lsfNFTokenSellOffers : lsfNFTokenBuyOffers; + (*sle)[sfNFTokenID] = nftokenID; + }); + + if (!offerNode) + return tecDIR_FULL; + + std::uint32_t sleFlags = 0; + + if (isSellOffer) + sleFlags |= lsfSellNFToken; + + auto offer = std::make_shared(offerID); + (*offer)[sfOwner] = acctID; + (*offer)[sfNFTokenID] = nftokenID; + (*offer)[sfAmount] = ctx.tx[sfAmount]; + (*offer)[sfFlags] = sleFlags; + (*offer)[sfOwnerNode] = *ownerNode; + (*offer)[sfNFTokenOfferNode] = *offerNode; + + if (auto const expiration = ctx.tx[~sfExpiration]) + (*offer)[sfExpiration] = *expiration; + + if (auto const destination = ctx.tx[~sfDestination]) + (*offer)[sfDestination] = *destination; + + view.insert(offer); + } + + // Update owner count. + adjustOwnerCount(view, view.peek(acctKeylet), 1, j); + + return tesSUCCESS; +} + } // namespace nft } // namespace ripple diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.h b/src/ripple/app/tx/impl/details/NFTokenUtils.h index 5242bf38ff3..b5db87607a1 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.h +++ b/src/ripple/app/tx/impl/details/NFTokenUtils.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_TX_IMPL_DETAILS_NFTOKENUTILS_H_INCLUDED #define RIPPLE_TX_IMPL_DETAILS_NFTOKENUTILS_H_INCLUDED +#include #include #include #include @@ -97,6 +98,32 @@ deleteTokenOffer(ApplyView& view, std::shared_ptr const& offer); bool compareTokens(uint256 const& a, uint256 const& b); +/** Preflight checks shared by NFTokenCreateOffer and NFTokenMint */ +NotTEC +tokenOfferCreatePreflight( + PreflightContext const& ctx, + std::uint16_t nftFlags, + std::uint32_t txFlags); + +/** Preclaim checks shared by NFTokenCreateOffer and NFTokenMint */ +TER +tokenOfferCreatePreclaim( + PreclaimContext const& ctx, + AccountID const& nftIssuer, // Issuer if present, otherwise Account + std::uint16_t nftFlags, + std::uint16_t xferFee, + std::uint32_t txFlags); + +/** doApply implementation shared by NFTokenCreateOffer and NFTokenMint */ +TER +tokenOfferCreateApply( + ApplyContext const& ctx, + ApplyView& view, + uint256 const& nftokenID, + XRPAmount const& priorBalance, + std::uint32_t txFlags, + beast::Journal j); + } // namespace nft } // namespace ripple diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index f6277a61014..0d98a564b9f 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6616,9 +6616,11 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite token::expiration(lastClose(env) + 25), ter(temDISABLED)); env.close(); + + return; } - if (features[featureNFTokenMintOffer]) + // The remaining tests assume featureNFTokenMintOffer is enabled. { Env env{*this, features}; Account const alice("alice"); @@ -6838,27 +6840,24 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // Test sell offers with a destination with and without // fixNFTokenNegOffer. - if (features[featureNFTokenMintOffer]) + for (auto const& tweakedFeatures : + {features - fixNFTokenNegOffer - featureNonFungibleTokensV1_1, + features | fixNFTokenNegOffer}) { - for (auto const& tweakedFeatures : - {features - fixNFTokenNegOffer - featureNonFungibleTokensV1_1, - features | fixNFTokenNegOffer}) - { - Env env{*this, tweakedFeatures}; - Account const alice("alice"); + Env env{*this, tweakedFeatures}; + Account const alice("alice"); - env.fund(XRP(1000000), alice); + env.fund(XRP(1000000), alice); - TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] - ? static_cast(temBAD_AMOUNT) - : static_cast(tesSUCCESS); + TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(temBAD_AMOUNT) + : static_cast(tesSUCCESS); - // Make offers with negative amounts for the NFTs - env(token::mint(alice), - token::amount(XRP(-2)), - ter(offerCreateTER)); - env.close(); - } + // Make offers with negative amounts for the NFTs + env(token::mint(alice), + token::amount(XRP(-2)), + ter(offerCreateTER)); + env.close(); } } @@ -7417,6 +7416,164 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } } + void + testNFTIssuerIsIOUIssuer(FeatureBitset features) + { + testcase("Test fix NFT issuer is IOU issuer"); + + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const becky{"becky"}; + Account const cheri{"cheri"}; + IOU const isISU(issuer["ISU"]); + + // This test case covers issue... + // https://github.com/XRPLF/rippled/issues/4941 + // + // If an NFToken has a transfer fee then, when an offer is accepted, + // a portion of the sale price goes to the issuer. + // + // It is possible for an issuer to issue both an IOU (for remittances) + // and NFTokens. If the issuer's IOU is used to pay for the transfer + // of one of the issuer's NFTokens, then paying the fee for that + // transfer will fail with a tecNO_LINE. + // + // The problem occurs because the NFT code looks for a trust line to + // pay the transfer fee. However the issuer of an IOU does not need + // a trust line to accept their own issuance and, in fact, is not + // allowed to have a trust line to themselves. + // + // This test looks at a situation where transfer of an NFToken is + // prevented by this bug: + // 1. Issuer issues an IOU (e.g, isISU). + // 2. Becky and Cheri get trust lines for, and acquire, some isISU. + // 3. Issuer mints NFToken with transfer fee. + // 4. Becky acquires the NFToken, paying with XRP. + // 5. Becky attempts to create an offer to sell the NFToken for + // isISU(100). The attempt fails with `tecNO_LINE`. + // + // The featureNFTokenMintOffer amendment addresses this oversight. + // + // We remove the fixRemoveNFTokenAutoTrustLine amendment. Otherwise + // we can't create NFTokens with tfTrustLine enabled. + FeatureBitset const localFeatures = + features - fixRemoveNFTokenAutoTrustLine; + + Env env{*this, localFeatures}; + env.fund(XRP(1000), issuer, becky, cheri); + env.close(); + + // Set trust lines so becky and cheri can use isISU. + env(trust(becky, isISU(1000))); + env(trust(cheri, isISU(1000))); + env.close(); + env(pay(issuer, cheri, isISU(500))); + env.close(); + + // issuer creates two NFTs: one with and one without AutoTrustLine. + std::uint16_t xferFee = 5000; // 5% + uint256 const nftAutoTrustID{token::getNextID( + env, issuer, 0u, tfTransferable | tfTrustLine, xferFee)}; + env(token::mint(issuer, 0u), + token::xferFee(xferFee), + txflags(tfTransferable | tfTrustLine)); + env.close(); + + uint256 const nftNoAutoTrustID{ + token::getNextID(env, issuer, 0u, tfTransferable, xferFee)}; + env(token::mint(issuer, 0u), + token::xferFee(xferFee), + txflags(tfTransferable)); + env.close(); + + // becky buys the nfts for 1 drop each. + { + uint256 const beckyBuyOfferIndex1 = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, drops(1)), + token::owner(issuer)); + + uint256 const beckyBuyOfferIndex2 = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftNoAutoTrustID, drops(1)), + token::owner(issuer)); + + env.close(); + env(token::acceptBuyOffer(issuer, beckyBuyOfferIndex1)); + env(token::acceptBuyOffer(issuer, beckyBuyOfferIndex2)); + env.close(); + } + + // Behavior from here down diverges significantly based on + // featureNFTokenMintOffer. + if (!localFeatures[featureNFTokenMintOffer]) + { + // Without featureNFTokenMintOffer 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)), + txflags(tfSellNFToken), + ter(tecNO_LINE)); + env.close(); + + // And issuer can't create a trust line to themselves. + env(trust(issuer, isISU(1000)), ter(temDST_IS_SRC)); + env.close(); + + // However if the NFToken has the tfTrustLine flag set, + // then becky can create the offer. + uint256 const beckyAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + + // And cheri can accept the offer. + env(token::acceptSellOffer(cheri, beckyAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // ISU(5) has disappeared out of cheri's and becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(95)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(400)); + } + else + { + // With featureNFTokenMintOffer things go better. + // becky creates offers to sell the nfts for ISU. + uint256 const beckyNoAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftNoAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + uint256 const beckyAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + + // cheri accepts becky's offers. Behavior is uniform: + // issuer gets paid. + env(token::acceptSellOffer(cheri, beckyAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // ISU(5) has disappeared out of cheri's and becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(95)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(400)); + + env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // an additional ISU(5) has disappeared out of cheri's and + // becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(190)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(300)); + } + } + void testWithFeats(FeatureBitset features) { @@ -7452,6 +7609,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite testFeatMintWithOffer(features); testTxJsonMetaFields(features); testFixNFTokenBuyerReserve(features); + testNFTIssuerIsIOUIssuer(features); } public: