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.

Sign up for GitHub

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

Jump to bottom

#252: add null address check #247

Merged
merged 10 commits into from
Jan 16, 2019
Merged

#252: add null address check #247

merged 10 commits into from
Jan 16, 2019

Conversation

0xj0hnny
Copy link
Contributor

No description provided.

mirathewhite
mirathewhite reviewed Jan 11, 2019
View reviewed changes
contracts/minting/MintController.sol Outdated
@@ -58,6 +58,7 @@ contract MintController is Controller {
* @dev sets the minterManager
*/
function setMinterManager(address _newMinterManager) onlyOwner public returns (bool) {
require(_newMinterManager != address(0), "Minter manager must be a non-zero address");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this feature is a good idea. We may want to be able to disable the MasterMinter contract (in case of an emergency). We have several options:

  1. Change the USDC masterMinter to point to a different contract
  2. Set the MasterMinter minterManager to some random address - of which 0 is the cleanest.

We should discuss this with the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove null address check for setMintManager

mirathewhite
mirathewhite suggested changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@mirathewhite mirathewhite left a comment

Choose a reason for hiding this comment

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

Summary:

  1. We need to discuss whether to allow minterManager to equal 0
  2. Use expectError instead of expectRevert to check error messages
  3. If we decide to require(minterManager != 0) we will need to delete all unit tests that check what happens what it is 0.

@eztierney eztierney changed the title CENT-630: add null address check #252: add null address check Jan 11, 2019
@mirathewhite mirathewhite merged commit a2b26cd into circlefin:multi-issuer Jan 16, 2019
@walkerq walkerq mentioned this pull request Jan 16, 2019
Merged
@walkerq walkerq mentioned this pull request Jan 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirathewhite mirathewhite mirathewhite approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@0xj0hnny @mirathewhite