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

Introduce fixNFTokenNegOffer amendment #4215

Closed
wants to merge 1 commit into from
Closed

Introduce fixNFTokenNegOffer amendment #4215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 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 @@ -75,6 +75,12 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration]))
return {nullptr, tecEXPIRED};

// The initial implementation had a bug that allowed a negative
// amount. The fixNFTokenNegOffer amendment fixes that.
if ((*offerSLE)[sfAmount].negative() &&
ctx.view.rules().enabled(fixNFTokenNegOffer))
return {nullptr, temBAD_OFFER};

return {std::move(offerSLE), tesSUCCESS};
}
return {nullptr, tesSUCCESS};
Expand Down Expand Up @@ -103,6 +109,14 @@ 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 (auto const dest = bo->at(~sfDestination))
{
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 (auto const dest = so->at(~sfDestination))
Expand Down Expand Up @@ -142,6 +156,15 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
!nft::findToken(ctx.view, ctx.tx[sfAccount], (*bo)[sfNFTokenID]))
return tecNO_PERMISSION;

// If not in bridged mode...
if (!so)
{
// If the offer has a Destination field, the acceptor must be the
// Destination.
if (auto const dest = bo->at(~sfDestination);
dest.has_value() && *dest != ctx.tx[sfAccount])
return tecNO_PERMISSION;
}
// The account offering to buy must have funds:
auto const needed = bo->at(sfAmount);

Expand Down
17 changes: 13 additions & 4 deletions src/ripple/app/tx/impl/NFTokenCreateOffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx)
auto const nftFlags = nft::getFlags(ctx.tx[sfNFTokenID]);

{
auto const amount = ctx.tx[sfAmount];
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))
{
Expand Down Expand Up @@ -78,9 +82,14 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx)

if (auto dest = ctx.tx[~sfDestination])
{
// The destination field is only valid on a sell offer; it makes no
// sense in a buy offer.
if (!isSellOffer)
// 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.
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 = 48;
static constexpr std::size_t numFeatures = 49;

/** 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 @@ -336,6 +336,7 @@ extern uint256 const featureCheckCashMakesTrustLine;
extern uint256 const featureNonFungibleTokensV1;
extern uint256 const featureExpandedSignerList;
extern uint256 const fixNFTokenDirV1;
extern uint256 const fixNFTokenNegOffer;

} // namespace ripple

Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes
REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::yes);
Copy link
Contributor

Choose a reason for hiding this comment

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

when is it safe to change the default vote like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I figured it was particularly safe since the NegativeUNL amendment is already live on the main net. So this specific change should be very safe.

In general the guidance has been that an amendment/fix should be votable on the network for one full release cycle before we DefaultVote::yes it. Since NegativeUNL was introduced in 1.7.3, that means anyone running 1.8.X (or 1.9.X for that matter) would not be amendment blocked if the amendment went live... And it already has gone live.

REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes);
Expand All @@ -440,6 +440,7 @@ REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, DefaultVote::no)
REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(ExpandedSignerList, Supported::yes, DefaultVote::no);
REGISTER_FIX (fixNFTokenDirV1, Supported::yes, DefaultVote::no);
REGISTER_FIX (fixNFTokenNegOffer, 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
Loading