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

Refactoring Superuser contract to allow Owners to transfer ownership … #978

Conversation

pmosse
Copy link
Contributor

@pmosse pmosse commented Jun 5, 2018

Fixes #50

Refactor to the Superuser contract to allow owners that are not superusers to transfer the ownership of the contract.

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • ✅ I've added tests where applicable to test my new functionality.
  • 📖 I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@pmosse
Copy link
Contributor Author

pmosse commented Jun 5, 2018

In Ownable.sol I extracted the functionality of transferOwnership to an internal function and moved the renounceOwnership function above the transferOwnership function.

Copy link
Contributor

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

LGTM. @frangio ?


it('should change owner after the owner transfers the ownership', async function () {
await expectEvent.inTransaction(
this.superuser.transferOwnership(finalOwner, { from: newOwner }),
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused that newOwner was sending this tx, but I saw that you're using the same Superuser instance for all tests! You should create the Superuser instance in a beforeEach block instead of before, so as to have the tests be independent of one another.

Then it will be firstOwner sending this tx.

@frangio frangio merged commit 7fb84b4 into OpenZeppelin:master Jun 5, 2018
@frangio
Copy link
Contributor

frangio commented Jun 5, 2018

Thanks @pmosse and @shrugs!

@pmosse pmosse deleted the feature/refactor-superuser-contract-transfer-ownership-#50 branch June 5, 2018 22:27
@pmosse
Copy link
Contributor Author

pmosse commented Jun 5, 2018

Great! @frangio

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.

Superuser: Upgradeable security for Smart Contracts
3 participants