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

Remove 'external' functions #2162

Merged
merged 11 commits into from
Apr 2, 2020
Merged

Conversation

nventuro
Copy link
Contributor

The goal behind making every function overrideable (virtual) is to, well, allow for overrides. Often these are 'extensions' of the original function, adding new checks, events, side effects, etc., and therefore require calling into the original:

function foo() override {
  super.foo();
  do_more_things();
}

However, this is not possible if foo is external, because super.foo() is considered an internal function call by the language (!).

This PR changes all external functions and makes them public. This creates a slight issue where external and internal functions had similar names: they will now both be internally-callable, which may lead to hard to detect mistakes. A prime example were AccessControl's grantRole and _grantRole. After some consideration, we decided to avoid these scenarios, by either renaming the internal functions, or removing them where we considered they weren't really necessary.

This PR is best reviewed commit-by-commit.

@nventuro nventuro requested a review from frangio March 30, 2020 22:08
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Show resolved Hide resolved
nventuro and others added 2 commits April 1, 2020 11:42
@nventuro nventuro merged commit 5b5d91c into OpenZeppelin:master Apr 2, 2020
@nventuro nventuro deleted the no-external branch April 2, 2020 18:30
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.

None yet

2 participants