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.

Sign up for GitHub

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

fix: rework IOracleAccessController::setAccessModule #74

Open
wants to merge 2 commits into
base: fix/internal-review
Choose a base branch
from
Open

fix: rework IOracleAccessController::setAccessModule #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1101c3d
fix: split `setAccessModule` into two more verbose functions
xorsal Dec 18, 2024
0b5dfef
test: update `setAccessModule` related tests
xorsal Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions solidity/contracts/access/OracleAccessController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,27 @@ abstract contract OracleAccessController is IOracleAccessController, CommonAcces
}

/// @inheritdoc IOracleAccessController
function setAccessModule(address _accessModule, bool _approved) external {
function approveAccessModule(address _accessModule) external {
_updateAccessModuleApprovalStatus(_accessModule, true);
}

/// @inheritdoc IOracleAccessController
function revokeAccessModule(address _accessModule) external {
_updateAccessModuleApprovalStatus(_accessModule, false);
}

/**
* @notice Sets the approval of an access module address.
*
* @param _accessModule The address of the access module
* @param _approved True to approve, false otherwise.
*/
function _updateAccessModuleApprovalStatus(address _accessModule, bool _approved) internal {
if (isAccessModuleApproved[msg.sender][_accessModule] == _approved) {
revert OracleAccessController_AccessModuleAlreadySet();
revert OracleAccessController_AccessModuleApprovalStatusAlreadySet();
}
isAccessModuleApproved[msg.sender][_accessModule] = _approved;

emit AccessModuleSet(msg.sender, _accessModule, _approved);
emit AccessModuleApprovalStatusUpdated(msg.sender, _accessModule, _approved);
}
}
20 changes: 13 additions & 7 deletions solidity/interfaces/access/IOracleAccessController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ interface IOracleAccessController is IAccessController {
* @param _accessModule The address of the access module
* @param _approved True if approved, false otherwise
*/
event AccessModuleSet(address indexed _user, address indexed _accessModule, bool _approved);
event AccessModuleApprovalStatusUpdated(address indexed _user, address indexed _accessModule, bool _approved);

/*///////////////////////////////////////////////////////////////
ERRORS
Expand All @@ -30,9 +30,9 @@ interface IOracleAccessController is IAccessController {
error OracleAccessController_AccessModuleNotApproved();

/**
* @notice Thrown when the access module is already set
* @notice Thrown when the access module approval status is already set
*/
error OracleAccessController_AccessModuleAlreadySet();
error OracleAccessController_AccessModuleApprovalStatusAlreadySet();

/*///////////////////////////////////////////////////////////////
VARIABLES
Expand All @@ -52,10 +52,16 @@ interface IOracleAccessController is IAccessController {
//////////////////////////////////////////////////////////////*/

/**
* @notice Sets the approval of an access module address.
* @notice Approves an access module address
*
* @param _accessModule The address of the access module
* @param _approved True to approve, false otherwise.
* @param _accessModule The address of the access module to approve
*/
function approveAccessModule(address _accessModule) external;

/**
* @notice Revokes approval of an access module address
*
* @param _accessModule The address of the access module to revoke
*/
function setAccessModule(address _accessModule, bool _approved) external;
function revokeAccessModule(address _accessModule) external;
}
2 changes: 1 addition & 1 deletion solidity/test/integration/IntegrationBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ contract IntegrationBase is TestConstants, Helpers {
address[] memory _allowedCallers = new address[](1);
_allowedCallers[0] = _caller;
_accessModule.setHasAccess(_allowedCallers);
oracle.setAccessModule(address(_accessModule), true);
oracle.approveAccessModule(address(_accessModule));
vm.stopPrank();
}
}
8 changes: 5 additions & 3 deletions solidity/test/integration/ResponseProposal.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@ contract Integration_ResponseProposal is IntegrationBase {

address _eoaAccessModule = makeAddr('eoaAccessModule');
vm.prank(requester);
oracle.setAccessModule(_eoaAccessModule, true);
oracle.approveAccessModule(_eoaAccessModule);

// Revert access module already set
vm.expectRevert(
abi.encodeWithSelector(IOracleAccessController.OracleAccessController_AccessModuleAlreadySet.selector)
abi.encodeWithSelector(
IOracleAccessController.OracleAccessController_AccessModuleApprovalStatusAlreadySet.selector
)
);

vm.prank(requester);
oracle.setAccessModule(_eoaAccessModule, true);
oracle.approveAccessModule(_eoaAccessModule);

vm.startPrank(caller);

Expand Down
52 changes: 26 additions & 26 deletions solidity/test/unit/Oracle.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ contract MockOracle is Oracle {
_responseIds[_requestId] = abi.encodePacked(_responseIds[_requestId], _responseId);
}

function mock_setAccessModuleApproved(address _user, address _accessModule, bool _approved) external {
function mock_setAccessModuleApprovalStatus(address _user, address _accessModule, bool _approved) external {
isAccessModuleApproved[_user][_accessModule] = _approved;
}
}
Expand Down Expand Up @@ -105,7 +105,7 @@ contract BaseTest is Test, Helpers {
event DisputeEscalated(address indexed _caller, bytes32 indexed _disputeId, IOracle.Dispute _dispute);
event DisputeStatusUpdated(bytes32 indexed _disputeId, IOracle.Dispute _dispute, IOracle.DisputeStatus _status);
event DisputeResolved(bytes32 indexed _disputeId, IOracle.Dispute _dispute);
event AccessModuleSet(address indexed _user, address indexed _accessModule, bool _approved);
event AccessModuleApprovalStatusUpdated(address indexed _user, address indexed _accessModule, bool _approved);

address public anyone = makeAddr('anyone');

Expand Down Expand Up @@ -140,7 +140,7 @@ contract Oracle_Unit_CreateRequest is BaseTest {
modifier happyPath() {
mockAccessControl.user = requester;
vm.startPrank(requester);
oracle.mock_setAccessModuleApproved(requester, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(requester, address(accessModule), true);
_;
}
/**
Expand Down Expand Up @@ -282,7 +282,7 @@ contract Oracle_Unit_CreateRequests is BaseTest {
IAccessController.AccessControl[] memory _accessControls = new IAccessController.AccessControl[](_requestsAmount);

vm.startPrank(requester);
oracle.mock_setAccessModuleApproved(requester, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(requester, address(accessModule), true);

// Generate requests batch
for (uint256 _i = 0; _i < _requestsAmount; _i++) {
Expand Down Expand Up @@ -366,7 +366,7 @@ contract Oracle_Unit_CreateRequests is BaseTest {
}

vm.startPrank(requester);
oracle.mock_setAccessModuleApproved(requester, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(requester, address(accessModule), true);
oracle.createRequests(_requests, _ipfsHashes, _accessControls);

uint256 _newNonce = oracle.totalRequestCount();
Expand Down Expand Up @@ -455,7 +455,7 @@ contract Oracle_Unit_ProposeResponse is BaseTest {
modifier happyPath() {
mockAccessControl.user = proposer;
vm.startPrank(proposer);
oracle.mock_setAccessModuleApproved(proposer, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(proposer, address(accessModule), true);
_;
}
/**
Expand Down Expand Up @@ -553,7 +553,7 @@ contract Oracle_Unit_ProposeResponse is BaseTest {

oracle.mock_setRequestCreatedAt(_getId(mockRequest), block.timestamp);

oracle.mock_setAccessModuleApproved(_caller, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(_caller, address(accessModule), true);
mockAccessControl.user = _caller;

// Check: revert?
Expand Down Expand Up @@ -614,7 +614,7 @@ contract Oracle_Unit_DisputeResponse is BaseTest {
modifier happyPath() {
mockAccessControl.user = disputer;
vm.startPrank(disputer);
oracle.mock_setAccessModuleApproved(disputer, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(disputer, address(accessModule), true);
_;
}

Expand Down Expand Up @@ -709,7 +709,7 @@ contract Oracle_Unit_DisputeResponse is BaseTest {
function test_disputeResponse_revertIfWrongDisputer(address _caller) public {
vm.assume(_caller != disputer);

oracle.mock_setAccessModuleApproved(_caller, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(_caller, address(accessModule), true);
mockAccessControl.user = _caller;

// Check: revert?
Expand Down Expand Up @@ -811,7 +811,7 @@ contract Oracle_Unit_UpdateDisputeStatus is BaseTest {
// Mock the dispute
oracle.mock_setDisputeOf(_responseId, _disputeId);
oracle.mock_setDisputeCreatedAt(_disputeId, block.timestamp);
oracle.mock_setAccessModuleApproved(proposer, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(proposer, address(accessModule), true);

vm.mockCall(address(accessModule), abi.encodeWithSelector(IAccessModule.hasAccess.selector), abi.encode(true));

Expand All @@ -830,7 +830,7 @@ contract Oracle_Unit_UpdateDisputeStatus is BaseTest {
bytes32 _disputeId = _getId(mockDispute);

oracle.mock_setDisputeCreatedAt(_disputeId, 0);
oracle.mock_setAccessModuleApproved(address(resolutionModule), address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(address(resolutionModule), address(accessModule), true);

// Check: revert?
vm.expectRevert(abi.encodeWithSelector(IOracle.Oracle_InvalidDispute.selector));
Expand All @@ -844,7 +844,7 @@ contract Oracle_Unit_UpdateDisputeStatus is BaseTest {
contract Oracle_Unit_ResolveDispute is BaseTest {
modifier happyPath() {
mockAccessControl.user = address(resolutionModule);
oracle.mock_setAccessModuleApproved(address(resolutionModule), address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(address(resolutionModule), address(accessModule), true);
vm.startPrank(address(resolutionModule));
_;
}
Expand Down Expand Up @@ -1022,7 +1022,7 @@ contract Oracle_Unit_Finalize is BaseTest {

modifier happyPath() {
mockAccessControl.user = address(requester);
oracle.mock_setAccessModuleApproved(mockAccessControl.user, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(mockAccessControl.user, address(accessModule), true);
vm.startPrank(address(requester));
_;
}
Expand All @@ -1040,7 +1040,7 @@ contract Oracle_Unit_Finalize is BaseTest {
bytes32 _responseId = _getId(mockResponse);

mockAccessControl.user = _caller;
oracle.mock_setAccessModuleApproved(mockAccessControl.user, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(mockAccessControl.user, address(accessModule), true);

oracle.mock_addResponseId(_requestId, _responseId);
oracle.mock_setRequestCreatedAt(_requestId, block.timestamp);
Expand Down Expand Up @@ -1183,7 +1183,7 @@ contract Oracle_Unit_Finalize is BaseTest {
oracle.mock_setRequestCreatedAt(_requestId, block.timestamp);
mockResponse.requestId = bytes32(0);
mockAccessControl.user = _caller;
oracle.mock_setAccessModuleApproved(mockAccessControl.user, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(mockAccessControl.user, address(accessModule), true);

// Create mock request and store it
bytes memory _calldata = abi.encodeCall(IModule.finalizeRequest, (mockRequest, mockResponse, _caller));
Expand Down Expand Up @@ -1227,7 +1227,7 @@ contract Oracle_Unit_Finalize is BaseTest {
oracle.mock_setRequestCreatedAt(_requestId, block.timestamp);

mockAccessControl.user = _caller;
oracle.mock_setAccessModuleApproved(mockAccessControl.user, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(mockAccessControl.user, address(accessModule), true);

IOracle.DisputeStatus _disputeStatus = IOracle.DisputeStatus(_status);

Expand Down Expand Up @@ -1268,7 +1268,7 @@ contract Oracle_Unit_Finalize is BaseTest {
oracle.mock_setRequestCreatedAt(_getId(mockRequest), block.timestamp);

mockAccessControl.user = _caller;
oracle.mock_setAccessModuleApproved(mockAccessControl.user, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(mockAccessControl.user, address(accessModule), true);

// Test: finalize a finalized request
vm.expectRevert(abi.encodeWithSelector(IOracle.Oracle_AlreadyFinalized.selector, _requestId));
Expand Down Expand Up @@ -1304,7 +1304,7 @@ contract Oracle_Unit_EscalateDispute is BaseTest {
modifier happyPath(address _caller) {
mockAccessControl.user = _caller;
vm.startPrank(_caller);
oracle.mock_setAccessModuleApproved(_caller, address(accessModule), true);
oracle.mock_setAccessModuleApprovalStatus(_caller, address(accessModule), true);
_;
}
/**
Expand Down Expand Up @@ -1427,18 +1427,18 @@ contract Oracle_Unit_EscalateDispute is BaseTest {
}

contract Oracle_Unit_SetAccessModule is BaseTest {
function test_revertIfAccessModuleAlreadySet(address _accessModule, bool _approved) public {
vm.assume(_approved == true);

function test_revertIfAccessModuleApprovalIsAlreadySet_approved(address _accessModule) public {
// Set the access module
oracle.mock_setAccessModuleApproved(address(this), _accessModule, _approved);
oracle.mock_setAccessModuleApprovalStatus(address(this), _accessModule, true);

vm.expectRevert(
abi.encodeWithSelector(IOracleAccessController.OracleAccessController_AccessModuleAlreadySet.selector)
abi.encodeWithSelector(
IOracleAccessController.OracleAccessController_AccessModuleApprovalStatusAlreadySet.selector
)
);

// Test: set the access module
oracle.setAccessModule(address(_accessModule), _approved);
oracle.approveAccessModule(address(_accessModule));
}

function test_setAccessModuleTrue(address _accessModule, bool _approved) public {
Expand All @@ -1448,10 +1448,10 @@ contract Oracle_Unit_SetAccessModule is BaseTest {

// Check: emits AccessModuleSet event?
_expectEmit(address(oracle));
emit AccessModuleSet(anyone, address(_accessModule), _approved);
emit AccessModuleApprovalStatusUpdated(anyone, address(_accessModule), _approved);

// Test: set the access module
oracle.setAccessModule(address(_accessModule), _approved);
oracle.approveAccessModule(address(_accessModule));

// Check: correct access module set?
assertEq(oracle.isAccessModuleApproved(anyone, address(_accessModule)), _approved);
Expand Down
Loading
Loading