Introduce fixNFTokenNegOffer amendment #4215
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The primary point is to forbid negative amounts in NFTokenOffers. - The secondary point is to allow a Destination field in an NFToken buy offer so a specific broker can be specified.
ledhed2222
reviewed
Jun 23, 2022
| @@ -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); | |||
There was a problem hiding this comment.
when is it safe to change the default vote like this?
There was a problem hiding this comment.
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.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Destinationfield on an NFToken buy offer.Context of Change
Testing revealed a bug in the NFTokenCreateOffer implementation. The implementation allows a negative
Amountto be specified in an offer for an NFToken. Since the implementation already explicitly disallows offers with an amount of 0 for IOUs, allowing negative amounts makes no sense. This amendment fixes the bug.Additionally, there was a request by folks who want to use the broker feature. Currently the implementation does not allow a buy offer to specify a
Destination. This prevents a buy offer from specifying a desired broker for that offer. So this amendment adds the capability for a buy offer to specify aDestination.If an NFToken buy offer includes a
Destinationfield, the account ID in that field may only be one of two accounts:This second capability was not considered important enough to justify its own amendment. So it is piggybacking on the amendment that fixes the negative amount problem.
Type of Change
Preventing an NFToken offer from containing a negative amount is a bug fix.
Allowing an NFToken buy offer to provide a
Destinationis a feature add.The champion for
Destinationfor a buy offer has been @ledhed2222. I'm hoping to get a review from them as well. Also heads up to @DennisDawson that this change is in the winds.