Add a bool return to _grantRole and _revokeRole

[Amxx]

Similarly to EnumerableSet's add and remove, I believe AccessControl's _grantRole and _revokeRole should return a boolean value. This would help overrides version handle no-ops.

I believe the public versions should not return the boolean, but that is open to discussion.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 7dc7e62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[socket-security]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

[frangio]

This would need to be rebased. However, I would also like us to consider the alternative of making the functions revert in the cases where they would return false, as an alternative to adding a return value.

The argument so far has been that we want to allow no-ops, but we've seen how in this case it can actually lead to bugs when you override the function, and you very easily assume that there is an actual revoke ongoing. I think this is stronger for revoke than it is for grant.

[Amxx]

@frangio @ernestognw

This got lost, I think we should do it in 5.0

[ernestognw]

I'd like to consider this change for 5.0, but we didn't address @frangio's comments.

I would also like us to consider the alternative of making the functions revert in the cases where they would return false, as an alternative to adding a return value.

Sounds like a good idea but it'll require us to check how it behaves with AccessControl extensions. For example, AccessControlDefaultAdminRules does a _revokeRole and _grantRole together in _acceptDefaultAdminTransfer because it assumes no-reverts. If a user extends the contract to add an alternative mechanism for revoking the DEFAULT_ADMIN_ROLE after a change was scheduled via beginDefaultAdminTransfer, there won't be way of accepting the role anymore.

imo we don't have time to check for those cases if we decide to do reverts. I'm more in favor of the current PR proposal.

[ernestognw]

btw, should we add tests for the internal function return values?

[Amxx]

btw, should we add tests for the internal function return values?

Added

[Amxx]

@frangio @ernestognw

[frangio]

However, I would also like us to consider the alternative of making the functions revert in the cases where they would return false, as an alternative to adding a return value.

Following up on this question. Our current thinking is that if these functions reverted, it would be possible to frontrun a batch of revokeRole operations with a single renounceRole and cause the entire batch to fail, which doesn't seem good either.

If the concern is that revokeRole may appear to succeed when no roles have been revoked, the recommendation should be to check the transaction events to confirm the desired end result.

We should write this in the contract.

[ernestognw]

LGTM but we also discussed making TimelockController return the proposalId, I'd like to merge this and add that in another PR if it's not already out there.

[Amxx]

LGTM but we also discussed making TimelockController return the proposalId, I'd like to merge this and add that in another PR if it's not already out there.

In TimelockController I was talking about public function. Changing the return type of public functions is an issue because of composability. If the new implementations of TimelockController return the proposalId when doing propose, would we use that return value in the Governor modules ? If yes, then the module would not be compatible with pre-existing instances.