-
Notifications
You must be signed in to change notification settings - Fork 10
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
Allow editors to upload security score #9
Conversation
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.
Please let me know what is the purpose of adding this admin capability.
We can look into openzepplin to see whether there're something we can borrowed from instead of rolling out our own solution
contracts/openzeppelin/Ownable.sol
Outdated
@@ -13,6 +13,7 @@ import "./Context.sol"; | |||
*/ | |||
contract Ownable is Context { | |||
address private _owner; | |||
mapping (address => bool) private _adminMap; |
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.
contracts under openzeppelin folder were copied from openzeppeline and untouched. better not to change it because it will create a surprise for readers.
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.
@aiolos404 let's create a new contract lib on top of Ownable to avoid touching forked codes.
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.
Will do
contracts/openzeppelin/Ownable.sol
Outdated
/** | ||
* @dev Remove address from admin list | ||
*/ | ||
function removeAdmin(address newAdmin) public onlyOwner { |
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.
newAdmin
=> someAdmin
The purpose of this PR is to allow external users push the security score to the security oracle contract |
This comment has been minimized.
This comment has been minimized.
Use the pragma solidity ^0.6.0;
import "@openzeppelin/contracts/access/AccessControl.sol";
contract CertiKSecurityOracle is AccessControl {
bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
constructor() public {
_setupRole(ADMIN_ROLE, msg.sender);
}
modifier onlyAdmin() {
require(hasRole(ADMIN_ROLE, msg.sender), "Caller is not an admin");
}
} The original |
Thanks for the ref. The reason why using 2.5 version of access control is because currently security contracts are locked to 0.5.17 version solidity compiler. Importing any libs that based on 0.6 or higher version of compiler may bring extra syntax changes in contracts. |
Why locked at 0.5.17? |
Should we consider using the OpenZeppelin 0.6.x version of |
Lock compiler version to 0.6.12 Remove obsoleted openzeppelin access control mechanism
Adopt latest 3.x openzeppelin access control mechanism. REFs: doc and code Tests:
|
|
Look good to me |
addAdmin
andremoveAdmin
to manage administratorsTests: