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

refactor: approved relays, jobs and selectors #7

Merged
merged 15 commits into from
Jan 4, 2024
3 changes: 1 addition & 2 deletions .solhintignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
solidity/contracts/utils/AutomateReady.sol
solidity/contracts/utils/Types.sol
solidity/interfaces/external/**
*.ignore
172 changes: 93 additions & 79 deletions solidity/contracts/AutomationVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity 0.8.19;
import {IAutomationVault} from '@interfaces/IAutomationVault.sol';
import {IERC20, SafeERC20} from '@openzeppelin/token/ERC20/utils/SafeERC20.sol';
import {EnumerableSet} from '@openzeppelin/utils/structs/EnumerableSet.sol';
import {_ETH, _NULL} from '@utils/Constants.sol';
import {_ETH, _ALL} from '@utils/Constants.sol';

/**
* @title AutomationVault
Expand All @@ -23,12 +23,13 @@ contract AutomationVault is IAutomationVault {
/**
* @notice Callers that are approved to call a relay
*/
mapping(address _relay => EnumerableSet.AddressSet _enabledCallers) internal _relayEnabledCallers;
mapping(address _relay => EnumerableSet.AddressSet _enabledCallers) internal _relayCallers;

/**
* @notice Selectors that are approved to be called
* @notice Relays that are approved to execute jobs with an specific selector
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
*/
mapping(address _job => EnumerableSet.Bytes32Set _enabledSelectors) internal _jobEnabledSelectors;
mapping(address _relay => mapping(address _job => EnumerableSet.Bytes32Set _enabledSelectors)) internal
_relayJobSelectors;

/**
* @notice List of approved relays
Expand All @@ -43,28 +44,30 @@ contract AutomationVault is IAutomationVault {
/**
* @param _owner The address of the owner
*/
constructor(address _owner) payable {
constructor(address _owner) {
owner = _owner;
}

/// @inheritdoc IAutomationVault
function relayEnabledCallers(address _relay) external view returns (address[] memory _enabledCallers) {
_enabledCallers = _relayEnabledCallers[_relay].values();
function getRelayData(
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
address _relay,
address _job
) public view returns (address[] memory _callers, bytes32[] memory _selectors) {
// Get the list of callers
_callers = _relayCallers[_relay].values();

// Get the list of selectors
_selectors = _relayJobSelectors[_relay][_job].values();
Comment on lines +61 to +62
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a method to get a relay's approved jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be in the front end part. Ardy can collect all jobs and check if the job is for that relay

}

/// @inheritdoc IAutomationVault
function jobEnabledSelectors(address _job) external view returns (bytes32[] memory _enabledSelectors) {
_enabledSelectors = _jobEnabledSelectors[_job].values();
function relays() external view returns (address[] memory _listRelays) {
_listRelays = _relays.values();
}

/// @inheritdoc IAutomationVault
function relays() external view returns (address[] memory __relays) {
__relays = _relays.values();
}

/// @inheritdoc IAutomationVault
function jobs() external view returns (address[] memory __jobs) {
__jobs = _jobs.values();
function jobs() external view returns (address[] memory _listJobs) {
_listJobs = _jobs.values();
}

/// @inheritdoc IAutomationVault
Expand All @@ -81,7 +84,7 @@ contract AutomationVault is IAutomationVault {
}

/// @inheritdoc IAutomationVault
function withdrawFunds(address _token, uint256 _amount, address _receiver) external payable onlyOwner {
function withdrawFunds(address _token, uint256 _amount, address _receiver) external onlyOwner {
// If the token is ETH, transfer the funds to the receiver, otherwise transfer the tokens
if (_token == _ETH) {
(bool _success,) = _receiver.call{value: _amount}('');
Expand All @@ -95,9 +98,12 @@ contract AutomationVault is IAutomationVault {
}

/// @inheritdoc IAutomationVault
function approveRelayCallers(address _relay, address[] calldata _callers) external onlyOwner {
// Get the list of enabled callers for the relay
EnumerableSet.AddressSet storage _enabledCallers = _relayEnabledCallers[_relay];
function approveRelayData(
address _relay,
address[] calldata _callers,
IAutomationVault.JobData[] calldata _jobsData
) external onlyOwner {
if (_relay == address(0)) revert AutomationVault_RelayZero();

// If the relay is not approved, add it to the list of relays
if (_relays.add(_relay)) {
Expand All @@ -106,135 +112,143 @@ contract AutomationVault is IAutomationVault {

// Iterate over the callers to approve them
for (uint256 _i; _i < _callers.length;) {
if (_enabledCallers.add(_callers[_i])) {
if (_relayCallers[_relay].add(_callers[_i])) {
emit ApproveRelayCaller(_relay, _callers[_i]);
}

unchecked {
++_i;
}
}
}

/// @inheritdoc IAutomationVault
function revokeRelayCallers(address _relay, address[] calldata _callers) external onlyOwner {
// Get the list of enabled callers for the relay
EnumerableSet.AddressSet storage _enabledCallers = _relayEnabledCallers[_relay];
// Iterate over the jobs to approve them and their selectors
for (uint256 _i; _i < _jobsData.length;) {
IAutomationVault.JobData memory _jobData = _jobsData[_i];

// Iterate over the callers to revoke them
for (uint256 _i; _i < _callers.length;) {
if (_enabledCallers.remove(_callers[_i])) {
emit RevokeRelayCaller(_relay, _callers[_i]);
// If the job is not approved, add it to the list of relays
if (_jobs.add(_jobData.job)) {
emit ApproveJob(_jobData.job);
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
}

// Iterate over the selectors to approve them
for (uint256 _j; _j < _jobData.functionSelectors.length;) {
if (_relayJobSelectors[_relay][_jobData.job].add(_jobData.functionSelectors[_j])) {
emit ApproveJobSelector(_jobData.job, _jobData.functionSelectors[_j]);
}

unchecked {
++_j;
}
}

unchecked {
++_i;
}
}

// If the relay has no more callers, remove it from the list of relays
if (_enabledCallers.length() == 0) {
_relays.remove(_relay);
emit RevokeRelay(_relay);
}
}

/// @inheritdoc IAutomationVault
function approveJobSelectors(address _job, bytes4[] calldata _functionSelectors) external onlyOwner {
// Get the list of enabled selectors for the job
EnumerableSet.Bytes32Set storage _enabledSelectors = _jobEnabledSelectors[_job];

// If the job is not approved, add it to the list of jobs
if (_jobs.add(_job)) {
emit ApproveJob(_job);
}
function revokeRelayData(
address _relay,
address[] calldata _callers,
IAutomationVault.JobData[] calldata _jobsData
) external onlyOwner {
if (_relay == address(0)) revert AutomationVault_RelayZero();

// Iterate over the selectors to approve them
for (uint256 _i; _i < _functionSelectors.length;) {
if (_enabledSelectors.add(_functionSelectors[_i])) {
emit ApproveJobSelector(_job, _functionSelectors[_i]);
// Iterate over the callers to revoke them
for (uint256 _i; _i < _callers.length;) {
if (_relayCallers[_relay].remove(_callers[_i])) {
emit RevokeRelayCaller(_relay, _callers[_i]);
}

unchecked {
++_i;
}

// If the relay has no enabled callers, remove it from the list of relays
if (_relayCallers[_relay].length() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be outside of the for loop, to avoid rechecking on each iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

_relays.remove(_relay);
emit RevokeRelay(_relay);
}
}
}

/// @inheritdoc IAutomationVault
function revokeJobSelectors(address _job, bytes4[] calldata _functionSelectors) external onlyOwner {
// Get the list of enabled selectors for the job
EnumerableSet.Bytes32Set storage _enabledSelectors = _jobEnabledSelectors[_job];

// Iterate over the selectors to revoke them
for (uint256 _i; _i < _functionSelectors.length;) {
if (_enabledSelectors.remove(_functionSelectors[_i])) {
emit RevokeJobSelector(_job, _functionSelectors[_i]);
// Iterate over the jobs to revoke them and their selectors
for (uint256 _i; _i < _jobsData.length;) {
IAutomationVault.JobData memory _jobData = _jobsData[_i];

// Iterate over the selectors to revoke them
for (uint256 _j; _j < _jobData.functionSelectors.length;) {
if (_relayJobSelectors[_relay][_jobData.job].remove(_jobData.functionSelectors[_j])) {
emit RevokeJobSelector(_jobData.job, _jobData.functionSelectors[_j]);
}

unchecked {
++_j;
}
}

if (_relayJobSelectors[_relay][_jobData.job].length() == 0) {
_jobs.remove(_jobData.job);
emit RevokeJob(_jobData.job);
}

unchecked {
++_i;
}
}

// If the job has no more selectors, remove it from the list of jobs
if (_enabledSelectors.length() == 0) {
_jobs.remove(_job);
emit RevokeJob(_job);
}
}

/// @inheritdoc IAutomationVault
function exec(address _relayCaller, ExecData[] calldata _execData, FeeData[] calldata _feeData) external payable {
function exec(address _relayCaller, ExecData[] calldata _execData, FeeData[] calldata _feeData) external {
// Check that the specific caller is approved to call the relay
if (!_relayEnabledCallers[msg.sender].contains(_relayCaller) && !_relayEnabledCallers[msg.sender].contains(_NULL)) {
if (!_relayCallers[msg.sender].contains(_relayCaller) && !_relayCallers[msg.sender].contains(_ALL)) {
revert AutomationVault_NotApprovedRelayCaller();
}

// Create the exec data needed variables
ExecData memory _execDatum;
ExecData memory _dataToExecute;
Comment on lines 210 to +211
Copy link
Member

Choose a reason for hiding this comment

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

In views of normalization, I suggest naming this variable _dataInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the naming was review'd

uint256 _dataLength = _execData.length;
uint256 _i;
bool _success;

// Iterate over the exec data to execute the jobs
for (_i; _i < _dataLength;) {
_execDatum = _execData[_i];
_dataToExecute = _execData[_i];

// Check that the selector is approved to be called
if (!_jobEnabledSelectors[_execDatum.job].contains(bytes4(_execDatum.jobData))) {
if (!_relayJobSelectors[msg.sender][_dataToExecute.job].contains(bytes4(_dataToExecute.jobData))) {
revert AutomationVault_NotApprovedJobSelector();
}
(_success,) = _execDatum.job.call(_execDatum.jobData);
(_success,) = _dataToExecute.job.call(_dataToExecute.jobData);
if (!_success) revert AutomationVault_ExecFailed();

// Emit the event
emit JobExecuted(msg.sender, _relayCaller, _execDatum.job, _execDatum.jobData);
emit JobExecuted(msg.sender, _relayCaller, _dataToExecute.job, _dataToExecute.jobData);

unchecked {
++_i;
}
}

// Create the fee data needed variables
FeeData memory _feeDatum;
FeeData memory _dataToFee;
_dataLength = _feeData.length;
_i = 0;

// Iterate over the fee data to issue the payments
for (_i; _i < _dataLength;) {
_feeDatum = _feeData[_i];
_dataToFee = _feeData[_i];

// If the token is ETH, transfer the funds to the receiver, otherwise transfer the tokens
if (_feeDatum.feeToken == _ETH) {
(_success,) = _feeDatum.feeRecipient.call{value: _feeDatum.fee}('');
if (_dataToFee.feeToken == _ETH) {
(_success,) = _dataToFee.feeRecipient.call{value: _dataToFee.fee}('');
if (!_success) revert AutomationVault_ETHTransferFailed();
} else {
IERC20(_feeDatum.feeToken).safeTransfer(_feeDatum.feeRecipient, _feeDatum.fee);
IERC20(_dataToFee.feeToken).safeTransfer(_dataToFee.feeRecipient, _dataToFee.fee);
}

// Emit the event
emit IssuePayment(msg.sender, _relayCaller, _feeDatum.feeRecipient, _feeDatum.feeToken, _feeDatum.fee);
emit IssuePayment(msg.sender, _relayCaller, _dataToFee.feeRecipient, _dataToFee.feeToken, _dataToFee.fee);

unchecked {
++_i;
Expand All @@ -247,7 +261,7 @@ contract AutomationVault is IAutomationVault {
*/
modifier onlyOwner() {
address _owner = owner;
if (msg.sender != _owner) revert AutomationVault_OnlyOwner(_owner);
if (msg.sender != _owner) revert AutomationVault_OnlyOwner();
_;
}

Expand All @@ -256,7 +270,7 @@ contract AutomationVault is IAutomationVault {
*/
modifier onlyPendingOwner() {
address _pendingOwner = pendingOwner;
if (msg.sender != _pendingOwner) revert AutomationVault_OnlyPendingOwner(_pendingOwner);
if (msg.sender != _pendingOwner) revert AutomationVault_OnlyPendingOwner();
_;
}

Expand Down
4 changes: 2 additions & 2 deletions solidity/contracts/AutomationVaultFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ contract AutomationVaultFactory is IAutomationVaultFactory {
}

/// @inheritdoc IAutomationVaultFactory
function deployAutomationVault(address _owner) external returns (IAutomationVault _automationVault) {
function deployAutomationVault(address _owner, uint256 _salt) external returns (IAutomationVault _automationVault) {
// Create the new automation vault with the owner
_automationVault = new AutomationVault(_owner);
_automationVault = new AutomationVault{salt: keccak256(abi.encodePacked(msg.sender, _salt))}(_owner);
Copy link
Member

Choose a reason for hiding this comment

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

This add a bit of extra gas cost, so it's optional but when a CREATE2 deployment fails, it returns address(0). You may want to add a check for it to avoid adding address(0) to the automation vaults mapping

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 didnt know nice one 👍

Copy link
Contributor Author

@ashitakah ashitakah Dec 29, 2023

Choose a reason for hiding this comment

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

@0xng are we sure that this happens?, im trying to test this situation and when the deployment fails reverts and not return address (0)

Copy link
Member

Choose a reason for hiding this comment

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

@ashitakah interesting, you may be right, I got this idea from the evm.codes CREATE2 line that says:

Deployment can fail due to:

  • A contract already exists at the destination address.
  • Insufficient value to transfer.
  • Sub context reverted.
  • Insufficient gas to execute the initialisation code.
  • Call depth limit reached.
  • Note that these failures only affect the return value and do not cause the calling context to revert (unlike the error cases below).

But also tested one of these cases (contract already exists at the destination address), and it also reverts. So then we should be good without adding the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 cool


// Add the automation vault to the list of automation vaults
_automationVaults.add(address(_automationVault));
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/Keep3rBondedRelay.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract Keep3rBondedRelay is IKeep3rBondedRelay {
/// @inheritdoc IKeep3rBondedRelay
function setAutomationVaultRequirements(
IAutomationVault _automationVault,
IKeep3rBondedRelay.Requirements memory _requirements
IKeep3rBondedRelay.Requirements calldata _requirements
) external {
if (_automationVault.owner() != msg.sender) revert Keep3rBondedRelay_NotVaultOwner();

Expand Down
Loading
Loading