-
Notifications
You must be signed in to change notification settings - Fork 23
Add ERC-7969 compliant DKIMRegistry #184
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
base: master
Are you sure you want to change the base?
Conversation
* } | ||
* ``` | ||
*/ | ||
abstract contract DKIMRegistry is IDKIMRegistry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abstract contract DKIMRegistry is IDKIMRegistry { | |
contract DKIMRegistry is IDKIMRegistry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xknon, we generally mark contracts as abstract
to make it clear that it's not our intention for people deploy them directly. Note it's missing the public
setKeyHash
, setKeyHashes
and revokeKeyHash
functions that require access control.
Alternatively, we can define this functions within the contract but add a modifier with an internal virtual _checkKeyHashUpdate
:
abstract contract MyDKIMRegistry is DKIMRegistry, Ownable {
modifier onlyKeyHashUpdater() {
_checkKeyHashUpdater();
_;
}
function setKeyHash(bytes32 domainHash, bytes32 keyHash) public onlyKeyHashUpdater {
_setKeyHash(domainHash, keyHash);
}
function setKeyHashes(bytes32 domainHash, bytes32[] memory keyHashes) public onlyKeyHashUpdater {
_setKeyHashes(domainHash, keyHashes);
}
function revokeKeyHash(bytes32 domainHash, bytes32 keyHash) public onlyKeyHashUpdater {
_revokeKeyHash(domainHash, keyHash);
}
...
function _checkKeyHashUpdater() internal virtual view;
}
It would be still abstract, but maybe closer to a straightforward deployment. wdyt?
See https://eip.tools/eip/7969