-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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 development of "withSignature" functions (like permit) #2567
Comments
I'm not fond of this change, I still believe we need to protect the internal admin logic of For generic "withSignature" functions it would be interesting to evaluate whether a For |
I agree that having the contract itself have the admin role can work ... even though it would be a mess if there are many different admin roles. I don't think its a good solution though. Also tweaking that in the Context doesn't sound right. It would be possible but I fear it would require sload in context which would be bad in the long run. |
Using storage in a Context variant may not be so bad, since this would be one of the cheaper uses of storage, where the slot is reset to its original value in the end. Regardless though, I think the major point of disagrement here is whether |
Two things:
|
I'm just as undecided with respect to I get the point about I do want to explore the What if we had a |
I'm using an ERC1167 clones pattern where I'm trying to inherit Ownable on the deployed clone. Since a constructor is not available for ERC1167 clones, I have to set the initial owner by having an initialize function on the clone contract called through factory contract exactly after deploying the clone. Such clones have zero address as owner when deployed and hence without an internal |
Hello @zemse When building "underlying implementaton" for proxy / clones, I recommend you use It does however have the same restriction than the constructor:
I continue to believe that having an internal |
Note: #2832 somehow fits in the scope of this issue. |
…ithSignature" functions (like permit) (#2568) * add internal _setOwner in Ownable * address issues raised in #2567 * updte changelog entry * improve changelog and documentation * rephrasing doc * add cahngelog improvement lost in merge * notify deprecation of _setupRole in changelog * Demote caution to note * Update CHANGELOG.md Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Hi! question are these changes going to be included in a release soon? Im particularly interested in the |
These changes will be included in a 4.4 release candidate that will be published this week. However, note that it will remain a release candidate for several weeks before 4.4 is released. We are likely announcing the timeline and details next week. |
Thanks @frangio works for us! |
v4.4.0-rc.0 was just released. |
🧐 Motivation
It seems to be a global trend to allow some "priviledged" operation to be performed by a third party provided they can show a valid signature by the legitimate account. For example, the
ERC20Permit
contract allows anyone to forward a signed message (ERC712) that will set token approval. The comp governance token offers a similar feature to delegate vote using a signature.This mechanism is very powerfull, and we can expect more of these "with signature" function to be implemented.
As
ERC20Permit
shows, this is easy to do when an internal function is available (_approve
), and can be called when the signature is verified. This is something that is widely available in the token smart contract.However, some of these functions are either non-existing, or private, which prevent the implementation of such "with signature" functions. In particular I can think of:
AccessControl.grantRole
: ThemsgSender()
must be admin, and the underlying function_grantRole
is privateAccessControl.revokeRole
: ThemsgSender()
must be admin, and the underlying function_revokeRole
is privateAccessControl.renounceRole
: ThemsgSender()
must be the account, and the underlying function_revokeRole
is privateOwnable.transferOwnership
: ThemsgSender()
must be owner, and there is no equivalent_transferOwnership
internal function📝 Details
In order to improve the possibility to write such wrappers, I believe:
_grantRole
,_revokeRole
and_revokeRole
should be internalOwnable._transferOwnership
that changes the owner and emit an event, without doing any msgSender() check.The text was updated successfully, but these errors were encountered: