Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Merged
merged 5 commits into from
Feb 13, 2023
Merged

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7e9f3e
fixNFTokenBrokerAccept
dangell7 Jan 2, 2023
723f15c
refactor
dangell7 Feb 13, 2023
5cfdcd7
remove feature test set
dangell7 Feb 13, 2023
f519f90
clean comments
dangell7 Feb 13, 2023
7cadd0c
clang-format
dangell7 Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,40 @@ 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])
return tecNFTOKEN_BUY_SELL_MISMATCH;
// fixUnburnableNFToken
if (ctx.view.rules().enabled(fixUnburnableNFToken))
{
// the destination may only be the account brokering the offer
if (*dest != ctx.tx[sfAccount])
return tecNO_PERMISSION;
}
else
{
// the destination must be the seller or the broker.
if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount])
return tecNFTOKEN_BUY_SELL_MISMATCH;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a blocker, but you are checking the rule twice when you could check it just once with some conditional re-ordering

Copy link
Collaborator Author

@dangell7 dangell7 Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored this if you want to check it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better looking to me! this is fine for merge, but IMO I would do this to remove a level of indentation:

if (ctx.view.rules().enabled(fixUnburnableNFToken))
{
  if (*dest != ctx.tx[sfAccount])
    return tecNO_PERMISSION;
}
else if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount])
  return tecNFTOKEN_BUY_SELL_MISMATCH;

}

// 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])
return tecNFTOKEN_BUY_SELL_MISMATCH;
// fixUnburnableNFToken
if (ctx.view.rules().enabled(fixUnburnableNFToken))
{
// the destination may only be the account brokering the offer
if (*dest != ctx.tx[sfAccount])
return tecNO_PERMISSION;
}
else
{
// the destination must be the buyer or the broker.
if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
return tecNFTOKEN_BUY_SELL_MISMATCH;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

// The broker can specify an amount that represents their cut; if they
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
intelliot marked this conversation as resolved.
Show resolved Hide resolved

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
113 changes: 73 additions & 40 deletions src/test/app/NFToken_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2872,15 +2872,20 @@ 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);
{
// 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.
Expand Down Expand Up @@ -2917,29 +2922,52 @@ 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);
// 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);
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,
Expand All @@ -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(
Expand Down Expand Up @@ -5088,4 +5121,4 @@ class NFToken_test : public beast::unit_test::suite

BEAST_DEFINE_TESTSUITE_PRIO(NFToken, tx, ripple, 2);

} // namespace ripple
} // namespace ripple
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a newline but not a blocker