Remove token.finishMinting() from default finalization #364
Conversation
This file contains hidden or 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
frangio
force-pushed
the
remove-crowdsale-finish-minting
branch
from
bfba316 to
4fc6bb7
Compare
maraoz
approved these changes
Aug 14, 2017
Contributor
maraoz
left a comment
maraoz
left a comment
There was a problem hiding this comment.
Approved, with one small question for you to consider before merging.
| */ | ||
| function finalization() internal { | ||
| token.finishMinting(); | ||
| } |
Contributor
There was a problem hiding this comment.
shouldn't finalization be abstract now that it's empty?
Contributor
Author
There was a problem hiding this comment.
I thought about it. If it's left abstract you can't call super.finalization() in the deriving contract, which we want everyone to do to make sure the whole finalization chain runs.
SylTi
pushed a commit
to SylTi/zeppelin-solidity
that referenced
this pull request
Aug 24, 2017
AFter this PR OpenZeppelin#364 the comment was left.
pash7ka
pushed a commit
to pash7ka/zeppelin-solidity
that referenced
this pull request
Aug 28, 2017
AFter this PR OpenZeppelin#364 the comment was left.
ProphetDaniel
pushed a commit
to classicdelta/Smart-Contracts
that referenced
this pull request
Mar 9, 2018
…ish-minting Remove token.finishMinting() from default finalization
ProphetDaniel
pushed a commit
to classicdelta/Smart-Contracts
that referenced
this pull request
Mar 9, 2018
AFter this PR OpenZeppelin#364 the comment was left.
jinliman
added a commit
to jinliman/solidity
that referenced
this pull request
Mar 27, 2018
AFter this PR OpenZeppelin/openzeppelin-contracts#364 the comment was left.
ninjacrypto
added a commit
to ninjacrypto/openzeppelin-solidity
that referenced
this pull request
Aug 24, 2023
AFter this PR OpenZeppelin/openzeppelin-contracts#364 the comment was left.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The default
finalizationimplementation inFinalizableCrowdsalecallstoken.finishMinting(). This is orthogonal to having finalization logic. If one wants to be able to mint further tokens in the future, the defaultfinalizationfunction cannot be called. See decentraland/mana and how they worked around this:Since we want to make sure that all contracts deriving from
FinalizableCrowdsalecallsuper.finalization(), to ensure all of the finalization logic is executed, we have to remove the call tofinishMinting.The caveat here is that if a custom finalization transfers ownership of the token to an address, that address will be able to mint tokens unless the custom logic also calls
finishMinting. This is something that will come up in an audit, however.