Skip to content

Allow editors to upload security score#9

Merged
MuhanZou merged 16 commits intomasterfrom
improve/adminlist
Jan 21, 2021
Merged

Allow editors to upload security score#9
MuhanZou merged 16 commits intomasterfrom
improve/adminlist

Conversation

@aiolos404
Copy link
Contributor

  • 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

*/
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
Contributor 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
Contributor 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
Contributor 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
Contributor 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.

@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
Contributor 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