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

Jump to bottom

feat: add tests and refactor #100

Merged
merged 5 commits into from
Aug 22, 2024
Merged

feat: add tests and refactor #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79ca4aa
feat: add test for checking the ExecuteFailed is properly emitted
clauBv23 Aug 21, 2024
9e9cddc
feat: add new case in mock executor to simulate the tests
clauBv23 Aug 21, 2024
8135b8f
feat: refactor code to avoid duplications
clauBv23 Aug 21, 2024
99f5cef
ci: fix solhint warnings
clauBv23 Aug 21, 2024
154a8b7
ci: fix lint issues
clauBv23 Aug 21, 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
3 changes: 3 additions & 0 deletions contracts/src/mocks/plugin/CustomExecutorMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ contract CustomExecutorMock {
) external returns (bytes[] memory execResults, uint256 failureMap) {
if (callId == bytes32(0)) {
revert Failed();
} else if (callId == bytes32(uint256(123))) {
// solhint-disable-next-line reason-string, custom-errors
revert();
} else {
emit Executed(msg.sender, callId, _actions, allowFailureMap, failureMap, execResults);
}
Expand Down
1 change: 1 addition & 0 deletions contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ contract ProposalMock is Proposal {
// We don't need to test these below functions as they will be tested in the actual plugins.
// This mock contract is only used to test `supportsInterface` function.

// solhint-disable no-empty-blocks
function createProposal(
bytes memory data,
IDAO.Action[] memory actions,
Expand Down
1 change: 1 addition & 0 deletions contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ contract ProposalUpgradeableMock is ProposalUpgradeable {
// We don't need to test these below functions as they will be tested in the actual plugins.
// This mock contract is only used to test `supportsInterface` function.

// solhint-disable no-empty-blocks
function createProposal(
bytes memory data,
IDAO.Action[] memory actions,
Expand Down
34 changes: 8 additions & 26 deletions contracts/src/plugin/Plugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,35 +106,14 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
IDAO.Action[] memory _actions,
uint256 _allowFailureMap
) internal virtual returns (bytes[] memory execResults, uint256 failureMap) {
Operation op = currentTargetConfig.operation;

if (op == Operation.DelegateCall) {
bool success;
bytes memory data;

(success, data) = currentTargetConfig.target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
}
} else {
revert ExecuteFailed();
}
}

(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(currentTargetConfig.target).execute(
return
_execute(
currentTargetConfig.target,
_callId,
_actions,
_allowFailureMap
_allowFailureMap,
currentTargetConfig.operation
);
}
}

/// @notice Forwards the actions to the `target` for the execution.
Expand All @@ -155,12 +134,14 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
bool success;
bytes memory data;

// solhint-disable-next-line avoid-low-level-calls
(success, data) = _target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
// solhint-disable-next-line no-inline-assembly
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
Expand All @@ -169,6 +150,7 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
revert ExecuteFailed();
}
}
(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(_target).execute(_callId, _actions, _allowFailureMap);
}
Expand Down
34 changes: 8 additions & 26 deletions contracts/src/plugin/PluginCloneable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,35 +121,14 @@ abstract contract PluginCloneable is
IDAO.Action[] memory _actions,
uint256 _allowFailureMap
) internal virtual returns (bytes[] memory execResults, uint256 failureMap) {
Operation op = currentTargetConfig.operation;

if (op == Operation.DelegateCall) {
bool success;
bytes memory data;

(success, data) = currentTargetConfig.target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
}
} else {
revert ExecuteFailed();
}
}

(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(currentTargetConfig.target).execute(
return
_execute(
currentTargetConfig.target,
_callId,
_actions,
_allowFailureMap
_allowFailureMap,
currentTargetConfig.operation
);
}
}

/// @notice Forwards the actions to the `target` for the execution.
Expand All @@ -170,12 +149,14 @@ abstract contract PluginCloneable is
bool success;
bytes memory data;

// solhint-disable-next-line avoid-low-level-calls
(success, data) = _target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
// solhint-disable-next-line no-inline-assembly
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
Expand All @@ -184,6 +165,7 @@ abstract contract PluginCloneable is
revert ExecuteFailed();
}
}
(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(_target).execute(_callId, _actions, _allowFailureMap);
}
Expand Down
34 changes: 8 additions & 26 deletions contracts/src/plugin/PluginUUPSUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,35 +136,14 @@ abstract contract PluginUUPSUpgradeable is
IDAO.Action[] memory _actions,
uint256 _allowFailureMap
) internal virtual returns (bytes[] memory execResults, uint256 failureMap) {
Operation op = currentTargetConfig.operation;

if (op == Operation.DelegateCall) {
bool success;
bytes memory data;

(success, data) = currentTargetConfig.target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
}
} else {
revert ExecuteFailed();
}
}

(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(currentTargetConfig.target).execute(
return
_execute(
currentTargetConfig.target,
_callId,
_actions,
_allowFailureMap
_allowFailureMap,
currentTargetConfig.operation
);
}
}

/// @notice Forwards the actions to the `target` for the execution.
Expand All @@ -185,12 +164,14 @@ abstract contract PluginUUPSUpgradeable is
bool success;
bytes memory data;

// solhint-disable-next-line avoid-low-level-calls
(success, data) = _target.delegatecall(
abi.encodeCall(IDAO.execute, (_callId, _actions, _allowFailureMap))
);

if (!success) {
if (data.length > 0) {
// solhint-disable-next-line no-inline-assembly
assembly {
let returndata_size := mload(data)
revert(add(32, data), returndata_size)
Expand All @@ -199,6 +180,7 @@ abstract contract PluginUUPSUpgradeable is
revert ExecuteFailed();
}
}
(execResults, failureMap) = abi.decode(data, (bytes[], uint256));
} else {
(execResults, failureMap) = IDAO(_target).execute(_callId, _actions, _allowFailureMap);
}
Expand Down
1 change: 0 additions & 1 deletion contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ pragma solidity ^0.8.8;
import {CountersUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";

import {IDAO} from "../../../dao/IDAO.sol";
import {IProposal} from "./IProposal.sol";

/// @title ProposalUpgradeable
Expand Down
2 changes: 0 additions & 2 deletions contracts/test/plugin/extensions/proposal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
DAOMock,
DAOMock__factory,
IDAO,
IProposal__factory,
ProposalMock,
ProposalUpgradeableMock,
Expand All @@ -13,7 +12,6 @@ import {getInterfaceId} from '@aragon/osx-commons-sdk';
import {IProposal__factory as IProposal_V1_0_0__factory} from '@aragon/osx-ethers-v1.0.0';
import {loadFixture} from '@nomicfoundation/hardhat-network-helpers';
import {expect} from 'chai';
import {BigNumberish, BytesLike} from 'ethers';
import {ethers} from 'hardhat';

describe('Proposal', async () => {
Expand Down
39 changes: 30 additions & 9 deletions contracts/test/plugin/plugin-clonable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ describe('PluginCloneable', function () {
const {implementation} = await loadFixture(fixture);
const iface = PluginCloneableMockBuild1__factory.createInterface();

let interfaceId = ethers.BigNumber.from(
const interfaceId = ethers.BigNumber.from(
iface.getSighash('setTargetConfig')
)
.xor(ethers.BigNumber.from(iface.getSighash('getTargetConfig')))
Expand All @@ -147,7 +147,7 @@ describe('PluginCloneable', function () {
it('reverts if caller does not have the permission', async () => {
const {deployer, proxy, daoMock} = await loadFixture(fixture);

let newTarget = proxy.address;
const newTarget = proxy.address;

await expect(proxy.setTargetConfig({target: newTarget, operation: 0}))
.to.be.revertedWithCustomError(proxy, 'DaoUnauthorized')
Expand All @@ -165,10 +165,10 @@ describe('PluginCloneable', function () {
// Set the `hasPermission` mock function to return `true`.
await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests

let newTarget = proxy.address;
const newTarget = proxy.address;

let targetConfig = {target: newTarget, operation: 0};
let previousTargetConfig = {
const targetConfig = {target: newTarget, operation: 0};
const previousTargetConfig = {
target: ethers.constants.AddressZero,
operation: 0,
};
Expand Down Expand Up @@ -205,14 +205,15 @@ describe('PluginCloneable', function () {
const executorFactory = new CustomExecutorMock__factory(deployer);
executor = await executorFactory.deploy();

var abiA = CustomExecutorMock__factory.abi;
var abiB = PluginCloneableMockBuild1__factory.abi;
const abiA = CustomExecutorMock__factory.abi;
const abiB = PluginCloneableMockBuild1__factory.abi;
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
mergedABI = abiA.concat(abiB);
});

beforeEach(async () => {
let data = await fixture();
const data = await fixture();
const [deployer] = await ethers.getSigners();

proxy = new ethers.Contract(
Expand Down Expand Up @@ -291,6 +292,20 @@ describe('PluginCloneable', function () {
)
).to.emit(proxy, 'Executed');
});

it('reverts with `ExecuteFailed`error', async () => {
await proxy.setTargetConfig({
target: executor.address,
operation: Operation.delegatecall,
});
await expect(
proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](
123,
[],
0
)
).to.be.revertedWithCustomError(proxy, 'ExecuteFailed');
});
});
});

Expand Down Expand Up @@ -339,7 +354,13 @@ describe('PluginCloneable', function () {
).to.emit(proxy, 'Executed');
});

// TODO: one more test that catches `ExecuteFailed` revert message.
it('reverts with `ExecuteFailed`error', async () => {
await expect(
proxy[
'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)'
](executor.address, 123, [], 0, Operation.delegatecall)
).to.be.revertedWithCustomError(proxy, 'ExecuteFailed');
});
});
});
});
Expand Down
Loading
Loading