Skip to content

Commit

Permalink
fix: review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@rkolpakov
rkolpakov committed Feb 6, 2023
1 parent 60c8870 commit 96889a6
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 25 deletions.
29 changes: 18 additions & 11 deletions contracts/0.8.9/OracleDaemonConfig.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ contract OracleDaemonConfig is AccessControlEnumerable {
EnumerableSet.Bytes32Set private _keyHashes;

constructor(address _admin, address[] memory _configManagers) {
if (_admin == address(0)) revert ErrorZeroAddress();

_grantRole(DEFAULT_ADMIN_ROLE, _admin);

for (uint256 i = 0; i < _configManagers.length; ) {
if (_configManagers[i] == address(0)) revert ErrorZeroAddress();
_grantRole(CONFIG_MANAGER_ROLE, _configManagers[i]);

unchecked {
Expand All @@ -32,37 +35,37 @@ contract OracleDaemonConfig is AccessControlEnumerable {
}
}

function set(string memory _key, bytes memory _value) external onlyRole(CONFIG_MANAGER_ROLE) {
function set(string calldata _key, bytes calldata _value) external onlyRole(CONFIG_MANAGER_ROLE) {
bytes32 keyHash = bytes32(keccak256(abi.encodePacked(_key)));
require(!_keyHashes.contains(keyHash), "VALUE_EXISTS");
if (_keyHashes.contains(keyHash)) revert ErrorValueExists(_key);

_keyHashes.add(keyHash);
_values[keyHash] = _value;

emit ConfigValueSet(keyHash, _key, _value);
}

function update(string memory _key, bytes memory _value) external onlyRole(CONFIG_MANAGER_ROLE) {
function update(string calldata _key, bytes calldata _value) external onlyRole(CONFIG_MANAGER_ROLE) {
bytes32 keyHash = bytes32(keccak256(abi.encodePacked(_key)));
require(_keyHashes.contains(keyHash), "VALUE_DOESNT_EXIST");
if (!_keyHashes.contains(keyHash)) revert ErrorValueDoesntExist(_key);
_values[keyHash] = _value;

emit ConfigValueUpdated(keyHash, _key, _value);
}

function unset(string memory _key) external onlyRole(CONFIG_MANAGER_ROLE) {
function unset(string calldata _key) external onlyRole(CONFIG_MANAGER_ROLE) {
bytes32 keyHash = bytes32(keccak256(abi.encodePacked(_key)));
require(_keyHashes.contains(keyHash), "VALUE_DOESNT_EXIST");
if (!_keyHashes.contains(keyHash)) revert ErrorValueDoesntExist(_key);

_keyHashes.remove(keyHash);
delete _values[keyHash];

emit ConfigValueUnset(keyHash, _key);
}

function get(string memory _key) external view returns (Item memory value) {
function get(string calldata _key) external view returns (Item memory value) {
bytes32 keyHash = bytes32(keccak256(abi.encodePacked(_key)));
require(_keyHashes.contains(keyHash), "VALUE_DOESNT_EXIST");
if (!_keyHashes.contains(keyHash)) revert ErrorValueDoesntExist(_key);

return Item({keyHash: keyHash, value: _values[keyHash]});
}
Expand All @@ -83,7 +86,11 @@ contract OracleDaemonConfig is AccessControlEnumerable {
return values;
}

event ConfigValueSet(bytes32 indexed keyHash_, string key_, bytes value_);
event ConfigValueUpdated(bytes32 indexed keyHash_, string key_, bytes value_);
event ConfigValueUnset(bytes32 indexed keyHash_, string key_);
error ErrorValueExists(string key);
error ErrorValueDoesntExist(string key);
error ErrorZeroAddress();

event ConfigValueSet(bytes32 indexed keyHash, string key, bytes value);
event ConfigValueUpdated(bytes32 indexed keyHash, string key, bytes value);
event ConfigValueUnset(bytes32 indexed keyHash, string key);
}
2 changes: 1 addition & 1 deletion lib/abi/OracleDaemonConfig.json
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
[{"inputs":[{"internalType":"address","name":"_admin","type":"address"},{"internalType":"address[]","name":"_configManagers","type":"address[]"}],"stateMutability":"nonpayable","type":"constructor"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash_","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key_","type":"string"},{"indexed":false,"internalType":"bytes","name":"value_","type":"bytes"}],"name":"ConfigValueSet","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash_","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key_","type":"string"}],"name":"ConfigValueUnset","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash_","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key_","type":"string"},{"indexed":false,"internalType":"bytes","name":"value_","type":"bytes"}],"name":"ConfigValueUpdated","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"bytes32","name":"previousAdminRole","type":"bytes32"},{"indexed":true,"internalType":"bytes32","name":"newAdminRole","type":"bytes32"}],"name":"RoleAdminChanged","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"address","name":"account","type":"address"},{"indexed":true,"internalType":"address","name":"sender","type":"address"}],"name":"RoleGranted","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"address","name":"account","type":"address"},{"indexed":true,"internalType":"address","name":"sender","type":"address"}],"name":"RoleRevoked","type":"event"},{"inputs":[],"name":"CONFIG_MANAGER_ROLE","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"DEFAULT_ADMIN_ROLE","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"}],"name":"get","outputs":[{"components":[{"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"internalType":"bytes","name":"value","type":"bytes"}],"internalType":"struct Item","name":"value","type":"tuple"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"}],"name":"getRoleAdmin","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"uint256","name":"index","type":"uint256"}],"name":"getRoleMember","outputs":[{"internalType":"address","name":"","type":"address"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"}],"name":"getRoleMemberCount","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"grantRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"hasRole","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"renounceRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"revokeRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"},{"internalType":"bytes","name":"_value","type":"bytes"}],"name":"set","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes4","name":"interfaceId","type":"bytes4"}],"name":"supportsInterface","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"}],"name":"unset","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"},{"internalType":"bytes","name":"_value","type":"bytes"}],"name":"update","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[],"name":"values","outputs":[{"components":[{"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"internalType":"bytes","name":"value","type":"bytes"}],"internalType":"struct Item[]","name":"","type":"tuple[]"}],"stateMutability":"view","type":"function"}]
[{"inputs":[{"internalType":"address","name":"_admin","type":"address"},{"internalType":"address[]","name":"_configManagers","type":"address[]"}],"stateMutability":"nonpayable","type":"constructor"},{"inputs":[{"internalType":"string","name":"key","type":"string"}],"name":"ErrorValueDoesntExist","type":"error"},{"inputs":[{"internalType":"string","name":"key","type":"string"}],"name":"ErrorValueExists","type":"error"},{"inputs":[],"name":"ErrorZeroAddress","type":"error"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key","type":"string"},{"indexed":false,"internalType":"bytes","name":"value","type":"bytes"}],"name":"ConfigValueSet","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key","type":"string"}],"name":"ConfigValueUnset","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"indexed":false,"internalType":"string","name":"key","type":"string"},{"indexed":false,"internalType":"bytes","name":"value","type":"bytes"}],"name":"ConfigValueUpdated","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"bytes32","name":"previousAdminRole","type":"bytes32"},{"indexed":true,"internalType":"bytes32","name":"newAdminRole","type":"bytes32"}],"name":"RoleAdminChanged","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"address","name":"account","type":"address"},{"indexed":true,"internalType":"address","name":"sender","type":"address"}],"name":"RoleGranted","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"bytes32","name":"role","type":"bytes32"},{"indexed":true,"internalType":"address","name":"account","type":"address"},{"indexed":true,"internalType":"address","name":"sender","type":"address"}],"name":"RoleRevoked","type":"event"},{"inputs":[],"name":"CONFIG_MANAGER_ROLE","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"DEFAULT_ADMIN_ROLE","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"}],"name":"get","outputs":[{"components":[{"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"internalType":"bytes","name":"value","type":"bytes"}],"internalType":"struct Item","name":"value","type":"tuple"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"}],"name":"getRoleAdmin","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"uint256","name":"index","type":"uint256"}],"name":"getRoleMember","outputs":[{"internalType":"address","name":"","type":"address"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"}],"name":"getRoleMemberCount","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"grantRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"hasRole","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"renounceRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"role","type":"bytes32"},{"internalType":"address","name":"account","type":"address"}],"name":"revokeRole","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"},{"internalType":"bytes","name":"_value","type":"bytes"}],"name":"set","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes4","name":"interfaceId","type":"bytes4"}],"name":"supportsInterface","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"}],"name":"unset","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"string","name":"_key","type":"string"},{"internalType":"bytes","name":"_value","type":"bytes"}],"name":"update","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[],"name":"values","outputs":[{"components":[{"internalType":"bytes32","name":"keyHash","type":"bytes32"},{"internalType":"bytes","name":"value","type":"bytes"}],"internalType":"struct Item[]","name":"","type":"tuple[]"}],"stateMutability":"view","type":"function"}]
34 changes: 21 additions & 13 deletions test/0.8.9/oracle-daemon-config.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const { assert } = require('chai')
const hre = require('hardhat')
const { keccak256 } = require('js-sha3')
const { ZERO_ADDRESS } = require('@aragon/contract-helpers-test')

const { assertRevert } = require('../helpers/assertThrow')
const { assert } = require('../helpers/assert')
const { EvmSnapshot } = require('../helpers/blockchain')

const OracleDaemonConfig = hre.artifacts.require('OracleDaemonConfig.sol')
Expand Down Expand Up @@ -64,7 +64,7 @@ contract('OracleDaemonConfig', async ([deployer, manager, stranger]) => {
it('removes a value', async () => {
await config.unset(defaultKey, { from: manager })

assertRevert(config.get(defaultKey))
assert.reverts(config.get(defaultKey))
})

it('gets all values (empty)', async () => {
Expand All @@ -88,16 +88,24 @@ contract('OracleDaemonConfig', async ([deployer, manager, stranger]) => {
})

it("reverts when defaultValue for update doesn't exist", async () => {
assertRevert(config.update(defaultKeyHash, defaultValue, { from: manager }), 'VALUE_DOESNT_EXIST')
assert.reverts(config.update(defaultKey, defaultValue, { from: manager }), `ErrorValueDoesntExist(${defaultKey})`)
})

it("reverts when defaultValue for unset doen't exist", async () => {
assertRevert(config.unset(defaultKeyHash, { from: manager }), 'VALUE_DOESNT_EXIST')
assert.reverts(config.unset(defaultKey, { from: manager }), `ErrorValueDoesntExist(${defaultKey})`)
})

it('reverts when defaultValue for set already exists', async () => {
await config.set(defaultKeyHash, defaultValue, { from: manager })
assertRevert(config.set(defaultKeyHash, updatedDefaultValue, { from: manager }), 'VALUE_EXISTS')
await config.set(defaultKey, defaultValue, { from: manager })
assert.reverts(config.set(defaultKey, updatedDefaultValue, { from: manager }), `ErrorValueExists(${defaultKey})`)
})

it('reverts when admin is zero address', async () => {
assert.reverts(OracleDaemonConfig.new(ZERO_ADDRESS, [manager], { from: deployer }), 'ErrorZeroAddress()')
})

it('reverts when one of managers is zero address', async () => {
assert.reverts(OracleDaemonConfig.new(deployer, [manager, ZERO_ADDRESS], { from: deployer }), 'ErrorZeroAddress()')
})
})

Expand All @@ -107,31 +115,31 @@ contract('OracleDaemonConfig', async ([deployer, manager, stranger]) => {
})

it('stranger cannot set a defaultValue', async () => {
assertRevert(config.set(defaultKeyHash, defaultValue, { from: stranger }))
assert.reverts(config.set(defaultKeyHash, defaultValue, { from: stranger }))
})

it('admin cannot set a defaultValue', async () => {
assertRevert(config.set(defaultKeyHash, defaultValue, { from: deployer }))
assert.reverts(config.set(defaultKeyHash, defaultValue, { from: deployer }))
})

it('stranger cannot update a defaultValue', async () => {
await config.set(defaultKeyHash, defaultValue, { from: manager })
assertRevert(config.update(defaultKeyHash, updatedDefaultValue, { from: stranger }))
assert.reverts(config.update(defaultKeyHash, updatedDefaultValue, { from: stranger }))
})

it('admin cannot update a defaultValue', async () => {
await config.set(defaultKeyHash, defaultValue, { from: manager })
assertRevert(config.update(defaultKeyHash, updatedDefaultValue, { from: deployer }))
assert.reverts(config.update(defaultKeyHash, updatedDefaultValue, { from: deployer }))
})

it('stranger cannot unset a defaultValue', async () => {
await config.set(defaultKeyHash, defaultValue, { from: manager })
assertRevert(config.unset(defaultKeyHash, { from: stranger }))
assert.reverts(config.unset(defaultKeyHash, { from: stranger }))
})

it('stranger cannot unset a defaultValue', async () => {
await config.set(defaultKeyHash, defaultValue, { from: manager })
assertRevert(config.unset(defaultKeyHash, { from: deployer }))
assert.reverts(config.unset(defaultKeyHash, { from: deployer }))
})
})
})

0 comments on commit 96889a6

Please sign in to comment.