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.

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

Feature/#13 erc20 timed mint #18

Merged

Conversation

Marrmee
Copy link
Collaborator

@Marrmee Marrmee commented Dec 20, 2021

No description provided.

@javier123454321
Copy link
Collaborator

javier123454321 commented Dec 20, 2021

Hey Sorry for the long list of yet to do items. The code is looking good and it makes me so happy to see you work on this. If you want to split the work and only do part we can turn any comment into their own ticket. Two main areas that I would love to see before I merge this:

  1. Tests! We just spoke about this, and sorry for not clarifying originally.
  2. It would be awesome to replicate the functionality of having a total cap such as the code here; https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Capped.sol I am thinking of the functionality something like this:
  • There should be a total max supply limit
  • if the max supply is hit, calling the _mint() function reverts
  • if the max supply is set to 0 this means there is no max supply at all and there is no upper limit to the amount of tokens.
  • there should probably be some checks that the initial requirement is < the cap, except if the cap is 0

@javier123454321 javier123454321 linked an issue Dec 20, 2021 that may be closed by this pull request
4 tasks
@javier123454321
Copy link
Collaborator

javier123454321 commented Dec 20, 2021

  1. It would be awesome to replicate the functionality of having a total cap such as the code here; https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Capped.sol I am thinking of the functionality something like this:
  • There should be a total max supply limit
  • if the max supply is hit, calling the _mint() function reverts
  • if the max supply is set to 0 this means there is no max supply at all and there is no upper limit to the amount of tokens.
  • there should probably be some checks that the initial requirement is < the cap, except if the cap is 0

By the way, this might be good to make into another ticket, let's just focus on the other comments first.


constructor(
uint256 public timeUntilNextMint;
uint256 private mintCap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should also have a 'setter' for this function (only needs to be called internally) that way we can set to 0 if there will be no new tokens issued.

@@ -48,6 +46,14 @@ contract ERC20TimedMint is ERC20 {
require(amount <= mintCap, "ERC20: Mint exceeds maximum amount");
super._mint(to, amount);
}

function _setTimeUntilNextMint(uint256 _timeDelay) external {
Copy link
Collaborator

@javier123454321 javier123454321 Dec 31, 2021

Choose a reason for hiding this comment

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

Just set this and _setMintCap() to internal and it will be called on the child contract.

)
ERC20(name, symbol)
{
timeUntilNextMint = block.timestamp + _timeDelay;
mintCap = _mintCap;
timeUntilNextMint = block.timestamp + timeDelay;
}

function claimTokens (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove claimTokens from this contract.

@javier123454321 javier123454321 mentioned this pull request Dec 31, 2021
4 tasks
@javier123454321
Copy link
Collaborator

looks good, merging now!

@javier123454321 javier123454321 merged commit c63dec0 into ValorizeDAO:staging Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERC20 Timed Mint contract
2 participants