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

Update modifiers to call public view functions #1277

Merged

Conversation

come-maiz
Copy link
Contributor

Fixes #1179.
Requires #1268, #1269, #1270.

πŸš€ Description

It should be possible to query the condition that modifiers require.

  • πŸ“˜ 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).

@come-maiz come-maiz added feature New contracts, functions, or helpers. contracts Smart contract code. labels Sep 5, 2018
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is good. Have you looked at the other modifiers in the codebase?

@@ -19,7 +19,7 @@ contract TimedCrowdsale is Crowdsale {
*/
modifier onlyWhileOpen {
// solium-disable-next-line security/no-block-members
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this Solium annotation because we moved the expression to a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@come-maiz
Copy link
Contributor Author

@frangio yes, I've checked them all.
The first thing I noticed is that we don't have as many as I though.
And the second is that this is the most common pattern:

  modifier onlyRole(string _role)                                                                                                                                            
  {                                                                                                                                                                          
    checkRole(msg.sender, _role);                                                                                                                                            
    _;                                                                                                                                                                       
  }   

I think I would like to remove of all the check functions. Call the require(hasRole) directly on the modifiers. But this is different from what we discussed, and needs more thought.

@frangio
Copy link
Contributor

frangio commented Sep 6, 2018

Is there an example of that pattern outside of roles? Those have been removed as part of @nventuro's work on roles.

@frangio frangio added this to the v2.0 milestone Sep 6, 2018
@frangio frangio force-pushed the feature/1179/modifiers-call-functions branch from 046521d to 50627db Compare September 6, 2018 13:59
@frangio
Copy link
Contributor

frangio commented Sep 6, 2018

Rebased on master.

@nventuro
Copy link
Contributor

nventuro commented Sep 6, 2018

Yup, from what I recall check was only present in Roles, and I removed it for the reasons described above.

Copy link
Contributor

@frangio frangio 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 frangio merged commit 616124e into OpenZeppelin:master Sep 6, 2018
@frangio
Copy link
Contributor

frangio commented Sep 6, 2018

Thanks @ElOpio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make modifiers call boolean functions
3 participants