fixNFTokenRemint: prevent NFT re-mint:
#4406
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,15 +160,66 @@ NFTokenMint::doApply() | |
| // Should not happen. Checked in preclaim. | ||
| return Unexpected(tecNO_ISSUER); | ||
|
|
||
| if (!ctx_.view().rules().enabled(fixNFTokenRemint)) | ||
| { | ||
| // Get the unique sequence number for this token: | ||
| std::uint32_t const tokenSeq = | ||
| (*root)[~sfMintedNFTokens].value_or(0); | ||
| { | ||
| std::uint32_t const nextTokenSeq = tokenSeq + 1; | ||
| if (nextTokenSeq < tokenSeq) | ||
| return Unexpected(tecMAX_SEQUENCE_REACHED); | ||
|
There was a problem hiding this comment. so you're just checking for int overflow based on ledger seq? that seems unnecessary There was a problem hiding this comment. I'd leave the check as is, as it's basically free (from a computational point of view). |
||
|
|
||
| (*root)[sfMintedNFTokens] = nextTokenSeq; | ||
| } | ||
shawnxie999 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ctx_.view().update(root); | ||
| return tokenSeq; | ||
| } | ||
|
|
||
| // With fixNFTokenRemint amendment enabled: | ||
| // | ||
| // If the issuer hasn't minted an NFToken before we must add a | ||
| // FirstNFTokenSequence field to the issuer's AccountRoot. The | ||
| // value of the FirstNFTokenSequence must equal the issuer's | ||
| // current account sequence. | ||
| // | ||
| // There are three situations: | ||
| // o If the first token is being minted by the issuer and | ||
| // * If the transaction consumes a Sequence number, then the | ||
| // Sequence has been pre-incremented by the time we get here in | ||
| // doApply. We must decrement the value in the Sequence field. | ||
| // * Otherwise the transaction uses a Ticket so the Sequence has | ||
| // not been pre-incremented. We use the Sequence value as is. | ||
| // o The first token is being minted by an authorized minter. In | ||
| // this case the issuer's Sequence field has been left untouched. | ||
| // We use the issuer's Sequence value as is. | ||
shawnxie999 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (!root->isFieldPresent(sfFirstNFTokenSequence)) | ||
| { | ||
| std::uint32_t const acctSeq = root->at(sfSequence); | ||
|
|
||
| root->at(sfFirstNFTokenSequence) = | ||
| ctx_.tx.isFieldPresent(sfIssuer) || | ||
| ctx_.tx.getSeqProxy().isTicket() | ||
| ? acctSeq | ||
| : acctSeq - 1; | ||
| } | ||
|
|
||
| std::uint32_t const mintedNftCnt = | ||
| (*root)[~sfMintedNFTokens].value_or(0u); | ||
|
|
||
| (*root)[sfMintedNFTokens] = mintedNftCnt + 1u; | ||
| if ((*root)[sfMintedNFTokens] == 0u) | ||
|
There was a problem hiding this comment. doesn't need to change - but i don't think you need the There was a problem hiding this comment. it is a conservative approach to make sure things are correctly typed proposed by @scottschurr There was a problem hiding this comment. @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 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. |
||
| return Unexpected(tecMAX_SEQUENCE_REACHED); | ||
|
|
||
| // Get the unique sequence number of this token by | ||
| // sfFirstNFTokenSequence + sfMintedNFTokens | ||
| std::uint32_t const offset = (*root)[sfFirstNFTokenSequence]; | ||
| std::uint32_t const tokenSeq = offset + mintedNftCnt; | ||
|
|
||
| // Check for more overflow cases | ||
| if (tokenSeq + 1u == 0u || tokenSeq < offset) | ||
| return Unexpected(tecMAX_SEQUENCE_REACHED); | ||
|
|
||
| ctx_.view().update(root); | ||
| return tokenSeq; | ||
| }(); | ||
|
|
||
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.
Imagine someone issuing a million "Silly Billy" NFTs, with the first minted in ledger 77,777,777.
This check will prevent the issuer from deleting the account until, at the earliest, ledger 78,778,032; that's not gonna happen for around 4 million seconds, or around a month and a half.
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.
Yes, unfortunately this check needs to be in. Without it, the scenario below produces duplicate NFTID:
FirstNFTokenSequence+MintedNFTokens)FirstNFTokenSequenceinitialized to her account sequence (258), andMintedNFTokensto0. This newly minted NFT would have a sequence number of258, which is a duplicate of what she issued through authorized minting before she deleted her account.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.
I understand. My point was that this kind of test means it's hard for someone to know when their account can be deleted and, depending on the number of NFTs they issue. For some accounts (those which don't have
FirstNFTokenSequenceset) the answer is "immediately" and for others it's "well, sometime in the future."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.
But I don't think this will be a common issue, since this restriction is only applicable for authorized minting situations.
If the issuer has never issued an NFT through authorized minting, then,
FirstNFTokenSequence + MintedNFTokens <= AccountRoot Sequencewill always be true, which means the AccountDelete amendment (ledger > Account Sequence + 256) already contains the new restriction.