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

feat: Operator Access Module #52

Merged
merged 13 commits into from
Nov 18, 2024
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"scripts": {
"build": "forge build",
"build:optimized": "FOUNDRY_PROFILE=optimized forge build",
"coverage": "forge coverage --report summary --report lcov --match-path 'test/unit/*'",
"coverage": "forge coverage --ir-minimum --report summary --report lcov --match-path 'test/unit/*'",
"deploy:arbitrum": "bash -c 'source .env && forge script Deploy --rpc-url arbitrum --account $ARBITRUM_DEPLOYER_NAME --broadcast --verify --chain arbitrum -vvvvv'",
"lint:check": "yarn lint:sol-tests && yarn lint:sol-logic && forge fmt --check",
"lint:fix": "sort-package-json && forge fmt && yarn lint:sol-tests --fix && yarn lint:sol-logic --fix",
Expand All @@ -34,8 +34,8 @@
"package.json": "sort-package-json"
},
"dependencies": {
"@defi-wonderland/prophet-core": "0.0.0-8bb062e0",
"@defi-wonderland/prophet-modules": "0.0.0-86078350"
"@defi-wonderland/prophet-core": "0.0.0-819e5fe9",
"@defi-wonderland/prophet-modules": "0.0.0-022dfec8"
},
"devDependencies": {
"@commitlint/cli": "19.3.0",
Expand Down
5 changes: 2 additions & 3 deletions src/contracts/CouncilArbitrator.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/IAccessController.sol';
import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/access/IAccessController.sol';
import {ValidatorLib} from '@defi-wonderland/prophet-core/solidity/libraries/ValidatorLib.sol';
import {IArbitrator} from '@defi-wonderland/prophet-modules/solidity/interfaces/IArbitrator.sol';

Expand Down Expand Up @@ -49,8 +49,7 @@ contract CouncilArbitrator is ICouncilArbitrator {
function resolve(
IOracle.Request calldata _request,
IOracle.Response calldata _response,
IOracle.Dispute calldata _dispute,
IAccessController.AccessControl calldata _accessControl
IOracle.Dispute calldata _dispute
) external onlyArbitratorModule returns (bytes memory /* _data */ ) {
bytes32 _disputeId = _dispute._getId();

Expand Down
79 changes: 79 additions & 0 deletions src/contracts/EBOAccessModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {
IAccessModule,
IArbitrable,
IEBOAccessModule,
IHorizonAccountingExtension,
IHorizonStaking,
IOracle
} from 'interfaces/IEBOAccessModule.sol';

import {IModule, Module} from '@defi-wonderland/prophet-core/solidity/contracts/Module.sol';

contract EBOAccessModule is IEBOAccessModule, Module {
/// @inheritdoc IEBOAccessModule
IArbitrable public immutable ARBITRABLE;
/// @inheritdoc IEBOAccessModule
IHorizonStaking public horizonStaking;
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

HorizonAccountingExtension features an immutable state variable for HorizonStaking, which is inconsistent with how it is treated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn’t aware of IHorizonStakingExtension::HORIZON_STAKING(). I wouldn't say it's inconsistent, it could definitely lead to unexpected behavior if the wrong addresses are set during deployment.

But more importantly, if horizonStaking can be obtained via a call (staticcall?) to horizonAccountingExtension, then I propose removing the horizonStaking state variable and instead calling HorizonStakingExtension::HORIZON_STAKING every time it’s needed, which is only inside of the hasAccess function.

Would this make sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

Performing an external call ad infinitum to get an immutable variable doesn't make sense to me in terms of gas efficiency, compared to storing it once. The HorizonStaking address could be sent in the constructor or got through a single external call in it—however, this last option requires HorizonAccountingExtension to have been deployed, which might not be desirable.

Beware that HorizonStaking, HorizonStakingExtension and HorizonAccountingExtension refer to different contracts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beware that HorizonStaking, HorizonStakingExtension and HorizonAccountingExtension refer to different contracts.

My bad, I mixed up the contract names.

HorizonAccountingExtension is the one that has the external view HORIZON_STAKING

however, this last option requires HorizonAccountingExtension to have been deployed, which might not be desirable.

The HorizonAccountingExtension should be already deployed when EBOAccessModule is live because it's used in hasAccess to do the isAuthorized call.

And if that isn't the case, the module also have setHorizonAccountingExtension to update its address whenever necessary.

Copy link
Contributor Author

@xorsal xorsal Nov 18, 2024

Choose a reason for hiding this comment

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

now whenever the horizonAccountingExtension is updated, it will also call HORIZON_STAKING and update the horizonStaking address.

Relevant changes in: 630ce61

/// @inheritdoc IEBOAccessModule
IHorizonAccountingExtension public horizonAccountingExtension;

/**
* @notice Constructor
* @param _oracle The address of the Oracle
* @param _arbitrable The address of the Arbitrable contract
* @param _horizonAccountingExtension The address of the Horizon Accounting Extension contract
*/
constructor(
IOracle _oracle,
IArbitrable _arbitrable,
IHorizonAccountingExtension _horizonAccountingExtension
) Module(_oracle) {
ARBITRABLE = _arbitrable;
_setHorizonAccountingExtension(_horizonAccountingExtension);
}

/// @inheritdoc IAccessModule
function decodeAccessControlParameters(bytes calldata _data)
public
pure
returns (IAccessModule.AccessControlParameters memory _params)
{
_params = abi.decode(_data, (IAccessModule.AccessControlParameters));
}

/// @inheritdoc IAccessModule
function hasAccess(bytes calldata _data) external view returns (bool _hasAccess) {
IAccessModule.AccessControlParameters memory _params = decodeAccessControlParameters(_data);
_hasAccess =
horizonStaking.isAuthorized(_params.accessControl.user, address(horizonAccountingExtension), _params.sender);
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
}

/// @inheritdoc IEBOAccessModule
function setHorizonAccountingExtension(IHorizonAccountingExtension _horizonAccountingExtension) external {
ARBITRABLE.validateArbitrator(msg.sender);

_setHorizonAccountingExtension(_horizonAccountingExtension);
}

/// @inheritdoc IModule
function moduleName() external pure returns (string memory _moduleName) {
_moduleName = 'EBOAccessModule';
}

/**
* @notice Internal function to set the horizon accounting extension contract.
* @dev It also updates the `horizonStaking` address.
* @param _horizonAccountingExtension The new horizon accounting extension contract.
*/
function _setHorizonAccountingExtension(IHorizonAccountingExtension _horizonAccountingExtension) internal {
horizonAccountingExtension = _horizonAccountingExtension;

// also update horizon staking address using the accounting extension view function
horizonStaking = _horizonAccountingExtension.HORIZON_STAKING();

emit HorizonAccountingExtensionSet(_horizonAccountingExtension);
}
}
3 changes: 1 addition & 2 deletions src/contracts/EBORequestCreator.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/IAccessController.sol';
import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/access/IAccessController.sol';
import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol';

import {
Expand Down Expand Up @@ -84,7 +84,6 @@ contract EBORequestCreator is IEBORequestCreator {
_requestModuleData.chainId = _chainId;
_requestData.requestModuleData = abi.encode(_requestModuleData);

// default access control
IAccessController.AccessControl memory _accessControl =
IAccessController.AccessControl({user: address(this), data: bytes('')});
_requestId = ORACLE.createRequest(_requestData, bytes32(0), _accessControl);
Expand Down
55 changes: 55 additions & 0 deletions src/interfaces/IEBOAccessModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {IOracle} from '@defi-wonderland/prophet-core/solidity/interfaces/IOracle.sol';
import {IAccessModule} from '@defi-wonderland/prophet-core/solidity/interfaces/modules/access/IAccessModule.sol';

import {IHorizonAccountingExtension} from 'interfaces/IHorizonAccountingExtension.sol';

import {IHorizonStaking} from 'interfaces/external/IHorizonStaking.sol';

import {IArbitrable} from 'interfaces/IArbitrable.sol';

interface IEBOAccessModule is IAccessModule {
/*///////////////////////////////////////////////////////////////
EVENTS
//////////////////////////////////////////////////////////////*/

/**
* @notice Emitted when the Horizon Accounting Extension contract is set
* @param _horizonAccountingExtension The new Horizon Accounting Extension contract
*/
event HorizonAccountingExtensionSet(IHorizonAccountingExtension indexed _horizonAccountingExtension);

/*///////////////////////////////////////////////////////////////
VARIABLES
//////////////////////////////////////////////////////////////*/

/**
* @notice The Arbitrable contract
* @return _arbitrable The Arbitrable contract
*/
function ARBITRABLE() external view returns (IArbitrable _arbitrable);

/**
* @notice The Horizon Accounting Extension contract
* @return _horizonAccountingExtension The Horizon Accounting Extension contract
*/
function horizonAccountingExtension() external view returns (IHorizonAccountingExtension _horizonAccountingExtension);

/**
* @notice The Horizon Staking contract
* @return _horizonStaking The Horizon Staking contract
*/
function horizonStaking() external view returns (IHorizonStaking _horizonStaking);

/*///////////////////////////////////////////////////////////////
LOGIC
//////////////////////////////////////////////////////////////*/

/**
* @notice Sets the Horizon Accounting Extension contract
* @param _horizonAccountingExtension The new Horizon Accounting Extension contract
*/
function setHorizonAccountingExtension(IHorizonAccountingExtension _horizonAccountingExtension) external;
}
9 changes: 9 additions & 0 deletions src/interfaces/external/IHorizonStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,13 @@ interface IHorizonStaking {
* @param verifierDestination The address to transfer the verifier cut to
*/
function slash(address serviceProvider, uint256 tokens, uint256 tokensVerifier, address verifierDestination) external;

/**
* @notice Check if an operator is authorized for the caller on a specific verifier / data service.
* @param serviceProvider The service provider on behalf of whom they're claiming to act
* @param verifier The verifier / data service on which they're claiming to act
* @param operator The address to check for auth
* @return Whether the operator is authorized or not
*/
function isAuthorized(address serviceProvider, address verifier, address operator) external view returns (bool);
}
8 changes: 4 additions & 4 deletions test/integration/arbitrum/BondEscalation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ contract IntegrationBondEscalation is IntegrationBase {
_pledgeForDispute(_requestId, _disputeId);

// Do not pass the dispute deadline nor the tying buffer
vm.warp(_disputeCreatedAt + disputeDeadline);
vm.warp(_disputeCreatedAt + disputeDeadline - 1);

// Thaw some tokens
uint256 _tokensToThaw = disputeBondSize * (maxNumberOfEscalations - 1) + 1;
Expand Down Expand Up @@ -101,7 +101,7 @@ contract IntegrationBondEscalation is IntegrationBase {
_pledgeAgainstDispute(_requestId, _disputeId);

// Do not pass the dispute deadline nor the tying buffer
vm.warp(_disputeCreatedAt + disputeDeadline);
vm.warp(_disputeCreatedAt + disputeDeadline - 1);

// Thaw some tokens
uint256 _tokensToThaw = disputeBondSize * (maxNumberOfEscalations - 1) + 1;
Expand Down Expand Up @@ -226,7 +226,7 @@ contract IntegrationBondEscalation is IntegrationBase {
_settleBondEscalation(_requestId, _responseId, _disputeId);

// Do not pass the dispute deadline nor the tying buffer
vm.warp(_disputeCreatedAt + disputeDeadline);
vm.warp(_disputeCreatedAt + disputeDeadline - 1);

// Pledge against the dispute
_pledgeAgainstDispute(_requestId, _disputeId);
Expand Down Expand Up @@ -291,7 +291,7 @@ contract IntegrationBondEscalation is IntegrationBase {
_settleBondEscalation(_requestId, _responseId, _disputeId);

// Do not pass the dispute deadline nor the tying buffer
vm.warp(_disputeCreatedAt + disputeDeadline);
vm.warp(_disputeCreatedAt + disputeDeadline - 1);

// Pledge for the dispute
_pledgeForDispute(_requestId, _disputeId);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/arbitrum/DisputeResponse.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ contract IntegrationDisputeResponse is IntegrationBase {
_disputeResponse(_requestId, _responseId);

// Do not pass the dispute window
vm.warp(_responseCreation + disputeDisputeWindow);
vm.warp(_responseCreation + disputeDisputeWindow - 1);

// Thaw some tokens
_thaw(_disputer, 1);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/arbitrum/IntegrationBase.t.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/IAccessController.sol';
import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/access/IAccessController.sol';

Check warning on line 4 in test/integration/arbitrum/IntegrationBase.t.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Variable "IAccessController" is unused
import {ValidatorLib} from '@defi-wonderland/prophet-core/solidity/libraries/ValidatorLib.sol';

import {_ARBITRUM_SEPOLIA_GOVERNOR} from 'script/Constants.sol';
Expand Down
8 changes: 4 additions & 4 deletions test/unit/CouncilArbitrator.t.sol
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.26;

import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/IAccessController.sol';
import {IValidator} from '@defi-wonderland/prophet-core/solidity/interfaces/IValidator.sol';
import {IAccessController} from '@defi-wonderland/prophet-core/solidity/interfaces/access/IAccessController.sol';

Check warning on line 5 in test/unit/CouncilArbitrator.t.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Variable "IAccessController" is unused

import {Helpers} from 'test/utils/Helpers.sol';

Expand Down Expand Up @@ -116,11 +116,11 @@
changePrank(_caller);

vm.expectRevert(ICouncilArbitrator.CouncilArbitrator_OnlyArbitratorModule.selector);
councilArbitrator.resolve(_params.request, _params.response, _params.dispute, _createAccessControl());
councilArbitrator.resolve(_params.request, _params.response, _params.dispute);
}

function test_setResolutions(ICouncilArbitrator.ResolutionParameters calldata _params) public happyPath {
councilArbitrator.resolve(_params.request, _params.response, _params.dispute, _createAccessControl());
councilArbitrator.resolve(_params.request, _params.response, _params.dispute);

bytes32 _disputeId = _params.dispute._getId();
(IOracle.Request memory _request, IOracle.Response memory _response, IOracle.Dispute memory _dispute) =
Expand All @@ -136,7 +136,7 @@

vm.expectEmit();
emit ResolutionStarted(_disputeId, _params.request, _params.response, _params.dispute);
councilArbitrator.resolve(_params.request, _params.response, _params.dispute, _createAccessControl());
councilArbitrator.resolve(_params.request, _params.response, _params.dispute);
}
}

Expand Down
Loading
Loading