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

Make some private functions internal to allow the developpement of "withSignature" functions (like permit) #2568

Merged
merged 12 commits into from
Sep 14, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 6, 2021

Fixes #2567

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Mar 8, 2021
@frangio
Copy link
Contributor

frangio commented Mar 8, 2021

Being discussed in #2567.

contracts/access/AccessControl.sol Show resolved Hide resolved
contracts/access/Ownable.sol Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Sep 10, 2021

Sorry to come back to this but I have one remaining concern. At some point we thought it was not such a good idea to have a single underscore determine such different semantics. I think we should try to have different names... the name _setupRole was pretty good in that regard I think. We can have _setupOwner, but I don't know how we would handle revoke, unless we do _setupRole(..., false).

@Amxx
Copy link
Collaborator Author

Amxx commented Sep 11, 2021

I personally think that a prefix underscore is clearly a mark of "this is internal, with no access check".

We already use this pattern in a lot of contracts, particularly transfer vs _transfer and approve vs _approve.
I think the logic is clear, and the discoverability is way better that way than if we use a different name.

@Amxx Amxx merged commit 7237b16 into OpenZeppelin:master Sep 14, 2021
@Amxx Amxx deleted the feature/internalSetOwner branch September 14, 2021 07:03
This was referenced Sep 29, 2021
@Mehduzza420

This comment was marked as spam.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 27, 2022

How can I retrieve my account since it has been severely compromised and I cannot access my account to transfer to my fiat wallet or financial institution

Hello @Mehduzza420

This is not the place for this question. You should try your wallet's community channel.

@Mehduzza420

This comment was marked as spam.

@Mehduzza420

This comment was marked as spam.

@Amxx
Copy link
Collaborator Author

Amxx commented May 16, 2022

@Mehduzza420 Please open a new issue if needed.

@Mehduzza420

This comment was marked as spam.

@Mehduzza420

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Put on hold for some reason that must be specified in a comment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make some private functions internal to allow the development of "withSignature" functions (like permit)
3 participants