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

MINT prototype #209

Merged
merged 2 commits into from
Sep 12, 2018
Merged

MINT prototype #209

merged 2 commits into from
Sep 12, 2018

Conversation

mirathewhite
Copy link
Contributor

Prototype for MINT P0.
contracts/minting: p0 contracts Controller, MintController, and MasterMinter
test/minting: Sample unit tests for p0 contracts
* AccountUtils.js is the unit test framework
* ControllerUtils.js framework for testing contracts that inherit from Controller
* MintControllerUtils.js framework for testing contracts that inherit from MintController.
* MintControllerTests.js two sample unit tests that use the new framework
contracts/experimental: prototypes for p1 and p2
test/experimental: sample unit tests for p1 and p2

contracts/minting/Controller.sol Outdated Show resolved Hide resolved
*/
function configureController(address _controller, address _minter) onlyOwner public returns (bool) {
controllers[_controller] = _minter;
emit ControllerConfigured(_controller, _minter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove references to _minter and replace with something more generic? I assume you are thinking that we will use the generic Controller as a base class for other kinds of controllers we may write (blacklister, pauser, etc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Changed _minter to _worker everywhere and updated comments inside Controller.sol. All other files are unaffected since these are just arguments to functions/events.

import 'openzeppelin-solidity/contracts/math/SafeMath.sol';

// make an interface instead of including FiatTokenV1 since
// FiatTokenV1 will include a different impl of Ownable
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is out of date now? Controller uses the same Ownable, right?

Probably still worth having the interface considering P1 or P2 features which will need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing comment.

return internal_setMinterAllowance(minter, newAllowance);
}

function incrementMinterAllowance(uint256 allowanceIncrement) onlyController public returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

MintableTokenInterface public token;

event TokenSet(address indexed oldToken, address indexed newToken);
event MinterModified(address indexed msgSender, address indexed minter);
Copy link
Contributor

Choose a reason for hiding this comment

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

This event is emitted for remove/configure/increment, but nothing records which operation was done. I know we could look at what other events were emitted by the actual token contract to figure it out, but would it be easier if MintController emitted different events for each of those operations?

MinterIncrement would have the most value as we could log the increment value, whereas the token will only log what the allowance was set to. The other two do seem duplicative of what the token logs.

Copy link
Contributor

@eztierney eztierney left a comment

Choose a reason for hiding this comment

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

Looks good - I'm going to create a multi-issuer branch and merge this PR into it.

@eztierney eztierney changed the base branch from master to multi-issuer September 12, 2018 17:27
@eztierney eztierney merged commit a2d9abd into circlefin:multi-issuer Sep 12, 2018
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.

2 participants