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

Allow editors to upload security score #9

Merged
merged 16 commits into from
Jan 21, 2021
Merged

Conversation

aiolos404
Copy link
Collaborator

  • Add admin maps to record administrators
  • Add addAdmin and removeAdmin to manage administrators
  • Allow administrators to push data and push data in batch

Tests:

TestProxy
    ✓ testProxy (60ms)
    ✓ testUpgradeOracleAddress (79ms)

  TestSecurityOracle
    ✓ testDefaultScore (44ms)
    ✓ testUpdateDefaultScore (54ms)
    ✓ testGetSecurityScoreResultMissing (46ms)
    ✓ testGetSecurityScoreStringParameter (46ms)
    ✓ testGetSecurityScoreContractAddressOnly (42ms)
    ✓ testPushResultResultAvailable (52ms)
    ✓ testPushResultResultExpired (51ms)
    ✓ testBatchPushResult (71ms)
    ✓ testGetSecurityScores (60ms)

  Contract: SecurityOracle
    ✓ should proceed for contract address only call
    ✓ should revert if contract address is 0
    ✓ should proceed for contract address and non 0 function signature call
    ✓ should revert if function signature is 0


  15 passing (9s)

Done in 15.46s.

Copy link

@bencao bencao left a 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

@@ -13,6 +13,7 @@ import "./Context.sol";
*/
contract Ownable is Context {
address private _owner;
mapping (address => bool) private _adminMap;
Copy link

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

/**
* @dev Remove address from admin list
*/
function removeAdmin(address newAdmin) public onlyOwner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newAdmin => someAdmin

@aiolos404
Copy link
Collaborator Author

The purpose of this PR is to allow external users push the security score to the security oracle contract

@aiolos404

This comment has been minimized.

Jialiang Chang added 2 commits October 12, 2020 16:55
@jon-certik
Copy link

Use the @openzeppelin/contracts/access/AccessControl.sol as in the latest doc: https://docs.openzeppelin.com/contracts/3.x/access-control#using-access-control ?

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 Roles has been deprecated: OpenZeppelin/openzeppelin-contracts#2112.

@aiolos404
Copy link
Collaborator Author

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.

@jon-certik
Copy link

Why locked at 0.5.17?

@jon-certik
Copy link

Should we consider using the OpenZeppelin 0.6.x version of Proxy.sol in order to support the usage of AccessControl @bencao ?

Jialiang Chang added 2 commits October 13, 2020 03:19
Lock compiler version to 0.6.12
Remove obsoleted openzeppelin access control mechanism
@aiolos404
Copy link
Collaborator Author

Adopt latest 3.x openzeppelin access control mechanism. REFs: doc and code

Tests:

  TestProxy
    ✓ testProxy (58ms)
    ✓ testUpgradeOracleAddress (68ms)

  TestSecurityOracle
    ✓ testDefaultScore (49ms)
    ✓ testUpdateDefaultScore (49ms)
    ✓ testGetSecurityScoreResultMissing (43ms)
    ✓ testGetSecurityScoreStringParameter (46ms)
    ✓ testGetSecurityScoreContractAddressOnly (44ms)
    ✓ testPushResultResultAvailable (54ms)
    ✓ testPushResultResultExpired (55ms)
    ✓ testBatchPushResult (73ms)
    ✓ testGetSecurityScores (51ms)

  Contract: SecurityOracle
    ✓ should proceed for contract address only call
    ✓ should revert if contract address is 0
    ✓ should proceed for contract address and non 0 function signature call
    ✓ should revert if function signature is 0


  15 passing (8s)

Done in 13.89s.

contracts/CertiKSecurityOracle.sol Outdated Show resolved Hide resolved
contracts/CertiKSecurityOracle.sol Outdated Show resolved Hide resolved
@aiolos404 aiolos404 requested a review from bencao October 13, 2020 22:33
@bencao
Copy link

bencao commented Jan 21, 2021

  TestProxy
    ✓ testProxy (67ms)
    ✓ testUpgradeOracleAddress (92ms)

  TestSecurityOracle
    ✓ testDefaultScore (99ms)
    ✓ testUpdateDefaultScore (71ms)
    ✓ testGetSecurityScoreResultMissing (109ms)
    ✓ testGetSecurityScoreStringParameter (59ms)
    ✓ testGetSecurityScoreContractAddressOnly (160ms)
    ✓ testPushResultResultAvailable (70ms)
    ✓ testPushResultResultExpired (110ms)
    ✓ testBatchPushResult (164ms)
    ✓ testGetSecurityScores (73ms)

  Contract: SecurityOracle
    ✓ should proceed for contract address only call (59ms)
    ✓ should revert if contract address is 0
    ✓ should proceed for contract address and non 0 function signature call
    ✓ should revert if function signature is 0

  Contract: SecurityOracle Role Access
    ✓ grant/revoke editor (162ms)
    ✓ should only allow admin to update default score (68ms)
    ✓ should only allow editor and admin to push result (91ms)


  18 passing (13s)

@bencao bencao changed the title Allow administrators to upload security score Allow editors to upload security score Jan 21, 2021
@aiolos404
Copy link
Collaborator Author

Look good to me

@MuhanZou MuhanZou merged commit 23b831d into master Jan 21, 2021
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.

5 participants