-
Notifications
You must be signed in to change notification settings - Fork 6
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
feature: targets and standardized proposal #99
base: develop
Are you sure you want to change the base?
Conversation
* feat: add test for checking the ExecuteFailed is properly emitted * feat: add new case in mock executor to simulate the tests * feat: refactor code to avoid duplications * ci: fix solhint warnings * ci: fix lint issues
function proposalCount() public view override returns (uint256) { | ||
return proposalCounter.current(); | ||
function proposalCount() public pure override returns (uint256) { | ||
return type(uint256).max; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should add here a comment to explain why we are doing this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description is added in IProposal
which is clear as this function has inheritdoc
above which dev should go and check in IProposal anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we added a @dev
comment, this function would be returning a 100% incorrect value. Any contract/UI depending on it would incorrectly assume that things are still working while getting 2^256-1
can lead to unpredictable consequences.
In terms of reliability, I would either:
- Remove the function
- Make it
revert()
to signal that this function is no longer valid
This is a breaking change anyway
/// @param _allowFailureMap Bitmap-encoded number. TODO: | ||
/// @return execResults address of the implementation contract. | ||
/// @return failureMap address of the implementation contract. | ||
function _execute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have quite some code duplication over all different Plugin* contracts.. probably worth refactoring it to have those methods only in one place..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* add params byte approach * add simple executor (#103) * add simple executor * action as free level export
/// Most useful use-case is to deploy as non-upgradeable and call from another contract via delegatecall. | ||
/// If used with delegatecall, DO NOT add state variables in sequential slots, otherwise this will overwrite | ||
/// the storage of the calling contract. | ||
contract Executor is IExecutor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this contract be abstract?
error TooManyActions(); | ||
|
||
/// @notice Thrown if an action has insufficient gas left. | ||
error InsufficientGas(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error InsufficientGas(); | |
error InsufficientGasLeft(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already using InsufficientGas
name in DAO.sol so better to stick with this name.
/// @notice Checks if this or the parent contract supports an interface by its ID. | ||
/// @notice Returns the currently set target contract. | ||
/// @return TargetConfig The currently set target. | ||
function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { | |
function getRawTargetConfig() public view virtual returns (TargetConfig memory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative:
getTargetConfig()
(raw)getEffectiveTargetConfig()
type(IProposal).interfaceId ^ | ||
IProposal.createProposal.selector ^ | ||
IProposal.canExecute.selector ^ | ||
IProposal.createProposalId.selector ^ | ||
IProposal.createProposalParamsABI.selector || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a versioned IProposalv1
and IProposalv2
interface?
It makes little sense that we are checking 2 different subsets of one interface
_interfaceId == | ||
type(IProposal).interfaceId ^ | ||
IProposal.createProposal.selector ^ | ||
IProposal.canExecute.selector ^ | ||
IProposal.createProposalId.selector ^ | ||
IProposal.createProposalParamsABI.selector || | ||
_interfaceId == type(IProposal).interfaceId || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same (versioned)
* metadata contracts * rename * diamond storage for metadata * remove revert
function proposalCount() public view override returns (uint256) { | ||
return proposalCounter.current(); | ||
function proposalCount() public pure override returns (uint256) { | ||
return type(uint256).max; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we added a @dev
comment, this function would be returning a 100% incorrect value. Any contract/UI depending on it would incorrectly assume that things are still working while getting 2^256-1
can lead to unpredictable consequences.
In terms of reliability, I would either:
- Remove the function
- Make it
revert()
to signal that this function is no longer valid
This is a breaking change anyway
uint256 internal constant MAX_ACTIONS = 256; | ||
|
||
// keccak256("osx-commons.storage.Executor") | ||
bytes32 private constant ReentrancyGuardStorageLocation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this value be computed? We are doing this for permission ID's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can but then assembly's sstore
fails with:
TypeError: Only direct number constants and references to such constants are supported by inline assembly.
contracts/src/executors/Executor.sol
Outdated
if (!hasBit(_allowFailureMap, uint8(i))) { | ||
// Check if the call failed. | ||
if (!success) { | ||
revert ActionFailed(i); | ||
} | ||
} else { | ||
// Check if the call failed. | ||
if (!success) { | ||
// Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)). | ||
// In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit | ||
// where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call. | ||
if (gasAfter < gasBefore / 64) { | ||
revert InsufficientGas(); | ||
} | ||
|
||
// Store that this action failed. | ||
failureMap = flipBit(failureMap, uint8(i)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving an "if" check when successful
if (!hasBit(_allowFailureMap, uint8(i))) { | |
// Check if the call failed. | |
if (!success) { | |
revert ActionFailed(i); | |
} | |
} else { | |
// Check if the call failed. | |
if (!success) { | |
// Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)). | |
// In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit | |
// where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call. | |
if (gasAfter < gasBefore / 64) { | |
revert InsufficientGas(); | |
} | |
// Store that this action failed. | |
failureMap = flipBit(failureMap, uint8(i)); | |
} | |
// Check if the call failed. | |
if (!success) { | |
if (!hasBit(_allowFailureMap, uint8(i))) { | |
revert ActionFailed(i); | |
} | |
// Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)). | |
// In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit | |
// where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call. | |
if (gasAfter < gasBefore / 64) { | |
revert InsufficientGas(); | |
} | |
// Store that this action failed. | |
failureMap = flipBit(failureMap, uint8(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 2f31a61
contracts/src/plugin/Plugin.sol
Outdated
event TargetSet(TargetConfig newTargetConfig); | ||
|
||
/// @notice Thrown when `delegatecall` fails. | ||
error ExecuteFailed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error ExecuteFailed(); | |
error DelegateCallFailed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 2f31a61
contracts/src/plugin/Plugin.sol
Outdated
enum Operation { | ||
Call, | ||
DelegateCall | ||
} | ||
|
||
struct TargetConfig { | ||
address target; | ||
Operation operation; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to IPlugin
. They produce no code when not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 2f31a61
/// @notice Checks if this or the parent contract supports an interface by its ID. | ||
/// @notice Returns the currently set target contract. | ||
/// @return TargetConfig The currently set target. | ||
function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative:
getTargetConfig()
(raw)getEffectiveTargetConfig()
|
||
/// @notice Allows to update only the metadata. | ||
/// @param _metadata The utf8 bytes of a content addressing cid that stores plugin's information. | ||
function updateMetadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggesting setMetadata()
unless it brings breaking changes on existing code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 2f31a61
Co-authored-by: Jør∂¡ <[email protected]>
…ithub.com/aragon/osx-commons into feature/targets-and-standardized-proposal
* create proposal id not enforced * new changes
Description
A couple of things remaining:
targets
andexecute
functions are duplicated in abstract contracts. Either:_execute
functions but it might be desirable to leave this choice to consumer, but if we do so, consumer will have to do this decode stuff manually which is also not ideal. Considering how OZ does such stuff, the current way seems better than others.Task ID: OS-1481
Checklist:
CHANGELOG.md
file in the root folder.