-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
fixNFTokenRemint
: prevent NFT re-mint:
#4406
Conversation
ede18a2
to
78dd062
Compare
40db6af
to
6211841
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't see a problem with this kind or re-minting (burn NFT, delete issuing account, re-create issuing account, re-mint NFT) unless the issue you're trying to solve is leftover offers from previous operations. If that's the concern, then the fix is easy: check for the presence of those during minting, and if present, remove them and if you can't fail the mint.
Even if that's not the concern and there is some other issue, I don't think what this PR does is the right approach anyways.
I am not vetoing this. Just offering my 2¢.
This doesn't directly have to do with the the previous change of leftover offers. It was a concern that duplicate NFT's could cause issues in the long run Something brought up by @ledhed2222:
|
Also could you please elaborate why this wouldn't be the right approach? This change has minimal effect on the ledger and does not change how the people use NFTs. |
…han 500 (XRPLF#4346) * Allow offers to be removable * Delete sell offers first Signed-off-by: Shawn Xie <shawnxie920@gmail.com>
fixUnburnableNFToken
: New NFTokenID sequence construct + new account deletion constraintfixUnburnableNFToken
: Prevent NFT re-mint with NFTokenID sequence construct + account deletion constraint
this is ready for re-review. cc @scottschurr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks for accommodating my requests. I noticed one minor comment problem in a test. Sorry I missed that last time. But it doesn't need to hold up this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks great!
(*root)[~sfMintedNFTokens].value_or(0u); | ||
|
||
(*root)[sfMintedNFTokens] = mintedNftCnt + 1u; | ||
if ((*root)[sfMintedNFTokens] == 0u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to change - but i don't think you need the u
specifier since you've already specified that the type is unsigned and arithmetic with a mix of signed/unsigned values should leave the value unsigned in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a conservative approach to make sure things are correctly typed proposed by @scottschurr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ledhed2222, what you describe is not always the case. There are situations where operations on a combination of unsigned and signed integers produce a signed value. See "Usual Arithmetic Conversions": https://en.cppreference.com/w/cpp/language/usual_arithmetic_conversions#Integer_conversion_rank
You'll notice that the rules changed with C++11 and again with C++23.
What the rules seem to be pretty good at holding onto is that if both arguments are unsigned then the result will be unsigned. Since the literals 0
and 1
are implicitly signed, they can influence the type yielded by the operation. That's why, since overflow is possible in this calculation, using 0u
and 1u
is safer; they are explicitly unsigned.
I did not go to the effort of determining exactly what conversions the compiler would make -- I'm not that smart. I just applied a heuristic so I'm increasing the likelihood the compiler will do what I want.
This is ready for quick re-review. Forgot to add fixNFTokenMint flag in tests to disable the amendment cc. @scottschurr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM. 👍
At merging, this PR will be squashed into 1 commit. The commit needs a commit message. Here is what I suggest. @shawnxie999 or anyone else, please feel free to make suggestions or offer an alternative.
This PR will be merged to |
fixNFTokenRemint
: prevent NFT re-mint:
@intelliot looks good |
High Level Overview of Change
Currently, a NFT ID can be reminted by the following steps:
We want to come up with a new sequence number construct to avoid this scenario
New NFT sequence construct
We create a new AccountRoot field
FirstNFTSequence
, which stays constant over time.This field is set to the current account sequence when the account issues their first NFT. Otherwise, it is not set.The sequence of a newly minted NFT is computed by
FirstNFTSequence
+MintedNFTokens
(thenMintedNFTokens
increments by 1).New Account Deletion Restriction
An account can only be deleted if
FirstNFTSequence
+MintedNFTokens
+256
is less than the current ledger sequence (256 was chosen as a heuristic restriction for account deletion and already exists in the account deletion constraint). Without this restriction, NFT is still remintable with the following scenario:Type of Change