Skip to content

Commit 7e00893

Browse files
orenyodfatorenyodfat
authored
genericCall is not revert (#575)
* genericCall should not require success * fix forwarder test * fix genericscheme test * more tests * Add descriptionHash to all proposing schems * fix genericScheme * naming
1 parent e03d8c6 commit 7e00893

23 files changed

+302
-147
lines changed

‎contracts/controller/Avatar.sol‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ contract Avatar is Ownable {
1717
DAOToken public nativeToken;
1818
Reputation public nativeReputation;
1919

20-
event GenericCall(address indexed _contract, bytes _params);
20+
event GenericCall(address indexed _contract, bytes _params, bool _success);
2121
event SendEther(uint256 _amountInWei, address indexed _to);
2222
event ExternalTokenTransfer(address indexed _externalToken, address indexed _to, uint256 _value);
2323
event ExternalTokenTransferFrom(address indexed _externalToken, address _from, address _to, uint256 _value);
@@ -45,14 +45,16 @@ contract Avatar is Ownable {
4545
* @dev perform a generic call to an arbitrary contract
4646
* @param _contract the contract's address to call
4747
* @param _data ABI-encoded contract call to call `_contract` address.
48-
* @return the return bytes of the called contract's function.
48+
* @return bool success or fail
49+
* bytes - the return bytes of the called contract's function.
4950
*/
50-
function genericCall(address _contract, bytes memory _data) public onlyOwner returns(bytes memory) {
51-
emit GenericCall(_contract, _data);
52-
// solhint-disable-next-line avoid-low-level-calls
53-
(bool success, bytes memory returnValue) = _contract.call(_data);
54-
require(success);
55-
return returnValue;
51+
function genericCall(address _contract, bytes memory _data)
52+
public
53+
onlyOwner
54+
returns(bool success, bytes memory returnValue) {
55+
// solhint-disable-next-line avoid-low-level-calls
56+
(success, returnValue) = _contract.call(_data);
57+
emit GenericCall(_contract, _data, success);
5658
}
5759

5860
/**

‎contracts/controller/Controller.sol‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,14 +373,15 @@ contract Controller is ControllerInterface {
373373
* @param _contract the contract's address to call
374374
* @param _data ABI-encoded contract call to call `_contract` address.
375375
* @param _avatar the controller's avatar address
376-
* @return bytes32 - the return value of the called _contract's function.
376+
* @return bool -success
377+
* bytes - the return value of the called _contract's function.
377378
*/
378379
function genericCall(address _contract, bytes calldata _data, Avatar _avatar)
379380
external
380381
onlyGenericCallScheme
381382
onlySubjectToConstraint("genericCall")
382383
isAvatarValid(address(_avatar))
383-
returns (bytes memory returnValue)
384+
returns (bool, bytes memory)
384385
{
385386
return avatar.genericCall(_contract, _data);
386387
}

‎contracts/controller/ControllerInterface.sol‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,12 @@ interface ControllerInterface {
105105
* @param _contract the contract's address to call
106106
* @param _data ABI-encoded contract call to call `_contract` address.
107107
* @param _avatar the controller's avatar address
108-
* @return bytes32 - the return value of the called _contract's function.
108+
* @return bool -success
109+
* bytes - the return value of the called _contract's function.
109110
*/
110111
function genericCall(address _contract, bytes calldata _data, Avatar _avatar)
111112
external
112-
returns(bytes memory);
113+
returns(bool, bytes memory);
113114

114115
/**
115116
* @dev send some ether

‎contracts/controller/UController.sol‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,13 +366,14 @@ contract UController is ControllerInterface {
366366
* @dev perform a generic call to an arbitrary contract
367367
* @param _contract the contract's address to call
368368
* @param _data ABI-encoded contract call to call `_contract` address.
369-
* @return bytes32 - the return value of the called _contract's function.
369+
* @return bool -success
370+
* bytes - the return value of the called _contract's function.
370371
*/
371372
function genericCall(address _contract, bytes calldata _data, Avatar _avatar)
372373
external
373374
onlyGenericCallScheme(address(_avatar))
374375
onlySubjectToConstraint("genericCall", address(_avatar))
375-
returns (bytes memory returnValue)
376+
returns (bool, bytes memory)
376377
{
377378
return _avatar.genericCall(_contract, _data);
378379
}

‎contracts/test/ActionMock.sol‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ contract ActionMock {
77

88
event WithoutReturnValue(address _addr);
99

10+
uint public activationTime;
11+
1012
function test(uint256 _a, address _b, bytes32 _c) public view returns(uint256) {
1113
require(_a == 7);
1214
require(_b == address(this));
@@ -24,4 +26,13 @@ contract ActionMock {
2426
emit WithoutReturnValue(_addr);
2527
}
2628

29+
function setActivationTime(uint _activationTime) public {
30+
activationTime = _activationTime;
31+
}
32+
33+
function test3() public view {
34+
// solhint-disable-next-line not-rely-on-time
35+
require(now > activationTime, "now should be greater than the activation time");
36+
}
37+
2738
}

‎contracts/test/UniversalSchemeMock.sol‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import "../controller/ControllerInterface.sol";
77
contract UniversalSchemeMock is UniversalScheme {
88

99
function genericCall(Avatar _avatar, address _contract, uint256 _a, address _b, bytes32 _c)
10-
public returns(bytes memory)
10+
public returns(bool, bytes memory)
1111
{
1212

1313
address controller = _avatar.owner();
@@ -16,7 +16,7 @@ contract UniversalSchemeMock is UniversalScheme {
1616
}
1717

1818
function genericCallDirect(Avatar _avatar, address _contract, uint256 _a, address _b, bytes32 _c)
19-
public returns(bytes memory)
19+
public returns(bool, bytes memory)
2020
{
2121
return _avatar.genericCall(_contract, abi.encodeWithSignature("test(uint256,address,bytes32)", _a, _b, _c));
2222
}

‎contracts/universalSchemes/GenericScheme.sol‎

Lines changed: 54 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,22 +15,29 @@ contract GenericScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu
1515
event NewCallProposal(
1616
address indexed _avatar,
1717
bytes32 indexed _proposalId,
18-
bytes callData
18+
bytes _callData,
19+
bytes32 _descriptionHash
1920
);
2021

2122
event ProposalExecuted(
2223
address indexed _avatar,
2324
bytes32 indexed _proposalId,
24-
int256 _param,
2525
bytes _genericCallReturnValue
2626
);
2727

28+
event ProposalExecutedByVotingMachine(
29+
address indexed _avatar,
30+
bytes32 indexed _proposalId,
31+
int256 _param
32+
);
33+
2834
event ProposalDeleted(address indexed _avatar, bytes32 indexed _proposalId);
2935

3036
// Details of a voting proposal:
3137
struct CallProposal {
3238
bytes callData;
3339
bool exist;
40+
bool passed;
3441
}
3542

3643
// A mapping from the organization (Avatar) address to the saved data of the organization:
@@ -48,25 +55,52 @@ contract GenericScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu
4855
/**
4956
* @dev execution of proposals, can only be called by the voting machine in which the vote is held.
5057
* @param _proposalId the ID of the voting in the voting machine
51-
* @param _param a parameter of the voting result, 1 yes and 2 is no.
58+
* @param _decision a parameter of the voting result, 1 yes and 2 is no.
59+
* @return bool success
5260
*/
53-
function executeProposal(bytes32 _proposalId, int256 _param) external onlyVotingMachine(_proposalId) returns(bool) {
61+
function executeProposal(bytes32 _proposalId, int256 _decision)
62+
external
63+
onlyVotingMachine(_proposalId)
64+
returns(bool) {
65+
Avatar avatar = proposalsInfo[_proposalId].avatar;
66+
CallProposal storage proposal = organizationsProposals[address(avatar)][_proposalId];
67+
require(proposal.exist, "must be a live proposal");
68+
require(proposal.passed == false, "cannot execute twice");
69+
70+
if (_decision == 1) {
71+
proposal.passed = true;
72+
execute(_proposalId);
73+
} else {
74+
delete organizationsProposals[address(avatar)][_proposalId];
75+
emit ProposalDeleted(address(avatar), _proposalId);
76+
}
77+
78+
emit ProposalExecutedByVotingMachine(address(avatar), _proposalId, _decision);
79+
return true;
80+
}
81+
82+
/**
83+
* @dev execution of proposals after it has been decided by the voting machine
84+
* @param _proposalId the ID of the voting in the voting machine
85+
*/
86+
function execute(bytes32 _proposalId) public {
5487
Avatar avatar = proposalsInfo[_proposalId].avatar;
5588
Parameters memory params = parameters[getParametersFromController(avatar)];
56-
// Save proposal to memory and delete from storage:
57-
CallProposal memory proposal = organizationsProposals[address(avatar)][_proposalId];
89+
CallProposal storage proposal = organizationsProposals[address(avatar)][_proposalId];
5890
require(proposal.exist, "must be a live proposal");
59-
delete organizationsProposals[address(avatar)][_proposalId];
60-
emit ProposalDeleted(address(avatar), _proposalId);
91+
require(proposal.passed, "proposal must passed by voting machine");
92+
proposal.exist = false;
6193
bytes memory genericCallReturnValue;
62-
// Check decision:
63-
if (_param == 1) {
64-
// Define controller and get the params:
65-
ControllerInterface controller = ControllerInterface(Avatar(avatar).owner());
66-
genericCallReturnValue = controller.genericCall(params.contractToCall, proposal.callData, avatar);
94+
bool success;
95+
ControllerInterface controller = ControllerInterface(Avatar(avatar).owner());
96+
(success, genericCallReturnValue) = controller.genericCall(params.contractToCall, proposal.callData, avatar);
97+
if (success) {
98+
delete organizationsProposals[address(avatar)][_proposalId];
99+
emit ProposalDeleted(address(avatar), _proposalId);
100+
emit ProposalExecuted(address(avatar), _proposalId, genericCallReturnValue);
101+
} else {
102+
proposal.exist = true;
67103
}
68-
emit ProposalExecuted(address(avatar), _proposalId, _param, genericCallReturnValue);
69-
return true;
70104
}
71105

72106
/**
@@ -108,9 +142,10 @@ contract GenericScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu
108142
* The function trigger NewCallProposal event
109143
* @param _callData - The abi encode data for the call
110144
* @param _avatar avatar of the organization
145+
* @param _descriptionHash proposal description hash
111146
* @return an id which represents the proposal
112147
*/
113-
function proposeCall(Avatar _avatar, bytes memory _callData)
148+
function proposeCall(Avatar _avatar, bytes memory _callData, bytes32 _descriptionHash)
114149
public
115150
returns(bytes32)
116151
{
@@ -121,14 +156,15 @@ contract GenericScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu
121156

122157
organizationsProposals[address(_avatar)][proposalId] = CallProposal({
123158
callData: _callData,
124-
exist: true
159+
exist: true,
160+
passed: false
125161
});
126162
proposalsInfo[proposalId] = ProposalInfo({
127163
blockNumber:block.number,
128164
avatar:_avatar,
129165
votingMachine:address(params.intVote)
130166
});
131-
emit NewCallProposal(address(_avatar), proposalId, _callData);
167+
emit NewCallProposal(address(_avatar), proposalId, _callData, _descriptionHash);
132168
return proposalId;
133169
}
134170

‎contracts/universalSchemes/GlobalConstraintRegistrar.sol‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,16 @@ contract GlobalConstraintRegistrar is UniversalScheme, VotingMachineCallbacks, P
1818
address indexed _intVoteInterface,
1919
address _gc,
2020
bytes32 _params,
21-
bytes32 _voteToRemoveParams
21+
bytes32 _voteToRemoveParams,
22+
bytes32 _descriptionHash
2223
);
2324

2425
event RemoveGlobalConstraintsProposal(
2526
address indexed _avatar,
2627
bytes32 indexed _proposalId,
2728
address indexed _intVoteInterface,
28-
address _gc
29+
address _gc,
30+
bytes32 _descriptionHash
2931
);
3032

3133
event ProposalExecuted(address indexed _avatar, bytes32 indexed _proposalId, int256 _param);
@@ -124,10 +126,16 @@ contract GlobalConstraintRegistrar is UniversalScheme, VotingMachineCallbacks, P
124126
* @param _gc the address of the global constraint that is being proposed
125127
* @param _params the parameters for the global constraint
126128
* @param _voteToRemoveParams the conditions (on the voting machine) for removing this global constraint
129+
* @param _descriptionHash proposal's description hash
127130
* @return bytes32 -the proposal id
128131
*/
129132
// TODO: do some checks on _voteToRemoveParams - it is very easy to make a mistake and not be able to remove the GC
130-
function proposeGlobalConstraint(Avatar _avatar, address _gc, bytes32 _params, bytes32 _voteToRemoveParams)
133+
function proposeGlobalConstraint(
134+
Avatar _avatar,
135+
address _gc,
136+
bytes32 _params,
137+
bytes32 _voteToRemoveParams,
138+
bytes32 _descriptionHash)
131139
public
132140
returns(bytes32)
133141
{
@@ -150,7 +158,8 @@ contract GlobalConstraintRegistrar is UniversalScheme, VotingMachineCallbacks, P
150158
address(intVote),
151159
_gc,
152160
_params,
153-
_voteToRemoveParams
161+
_voteToRemoveParams,
162+
_descriptionHash
154163
);
155164
proposalsInfo[proposalId] = ProposalInfo({
156165
blockNumber:block.number,
@@ -164,9 +173,10 @@ contract GlobalConstraintRegistrar is UniversalScheme, VotingMachineCallbacks, P
164173
* @dev propose to remove a global constraint:
165174
* @param _avatar the avatar of the organization that the constraint is proposed for
166175
* @param _gc the address of the global constraint that is being proposed
176+
* @param _descriptionHash proposal's description hash
167177
* @return bytes32 -the proposal id
168178
*/
169-
function proposeToRemoveGC(Avatar _avatar, address _gc) public returns(bytes32) {
179+
function proposeToRemoveGC(Avatar _avatar, address _gc, bytes32 _descriptionHash) public returns(bytes32) {
170180
Controller controller = Controller(_avatar.owner());
171181
require(controller.isGlobalConstraintRegistered(_gc, address(_avatar)));
172182
Parameters memory params = parameters[getParametersFromController(_avatar)];
@@ -186,7 +196,7 @@ contract GlobalConstraintRegistrar is UniversalScheme, VotingMachineCallbacks, P
186196
});
187197

188198
organizationsProposals[address(_avatar)][proposalId] = proposal;
189-
emit RemoveGlobalConstraintsProposal(address(_avatar), proposalId, address(intVote), _gc);
199+
emit RemoveGlobalConstraintsProposal(address(_avatar), proposalId, address(intVote), _gc, _descriptionHash);
190200
proposalsInfo[proposalId] = ProposalInfo({
191201
blockNumber:block.number,
192202
avatar:_avatar,

‎contracts/universalSchemes/SchemeRegistrar.sol‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@ contract SchemeRegistrar is UniversalScheme, VotingMachineCallbacks, ProposalExe
1818
address indexed _intVoteInterface,
1919
address _scheme,
2020
bytes32 _parametersHash,
21-
bytes4 _permissions
21+
bytes4 _permissions,
22+
bytes32 _descriptionHash
2223
);
2324

2425
event RemoveSchemeProposal(address indexed _avatar,
2526
bytes32 indexed _proposalId,
2627
address indexed _intVoteInterface,
27-
address _scheme
28+
address _scheme,
29+
bytes32 _descriptionHash
2830
);
2931

3032
event ProposalExecuted(address indexed _avatar, bytes32 indexed _proposalId, int256 _param);
@@ -115,14 +117,16 @@ contract SchemeRegistrar is UniversalScheme, VotingMachineCallbacks, ProposalExe
115117
* @param _scheme the address of the scheme to be registered
116118
* @param _parametersHash a hash of the configuration of the _scheme
117119
* @param _permissions the permission of the scheme to be registered
120+
* @param _descriptionHash proposal's description hash
118121
* @return a proposal Id
119122
* @dev NB: not only proposes the vote, but also votes for it
120123
*/
121124
function proposeScheme(
122125
Avatar _avatar,
123126
address _scheme,
124127
bytes32 _parametersHash,
125-
bytes4 _permissions
128+
bytes4 _permissions,
129+
bytes32 _descriptionHash
126130
)
127131
public
128132
returns(bytes32)
@@ -149,7 +153,8 @@ contract SchemeRegistrar is UniversalScheme, VotingMachineCallbacks, ProposalExe
149153
proposalId,
150154
address(controllerParams.intVote),
151155
_scheme, _parametersHash,
152-
_permissions
156+
_permissions,
157+
_descriptionHash
153158
);
154159
organizationsProposals[address(_avatar)][proposalId] = proposal;
155160
proposalsInfo[proposalId] = ProposalInfo({
@@ -164,10 +169,10 @@ contract SchemeRegistrar is UniversalScheme, VotingMachineCallbacks, ProposalExe
164169
* @dev propose to remove a scheme for a controller
165170
* @param _avatar the address of the controller from which we want to remove a scheme
166171
* @param _scheme the address of the scheme we want to remove
167-
*
172+
* @param _descriptionHash proposal description hash
168173
* NB: not only registers the proposal, but also votes for it
169174
*/
170-
function proposeToRemoveScheme(Avatar _avatar, address _scheme)
175+
function proposeToRemoveScheme(Avatar _avatar, address _scheme, bytes32 _descriptionHash)
171176
public
172177
returns(bytes32)
173178
{
@@ -179,7 +184,7 @@ contract SchemeRegistrar is UniversalScheme, VotingMachineCallbacks, ProposalExe
179184

180185
organizationsProposals[address(_avatar)][proposalId].proposalType = 2;
181186
organizationsProposals[address(_avatar)][proposalId].scheme = _scheme;
182-
emit RemoveSchemeProposal(address(_avatar), proposalId, address(intVote), _scheme);
187+
emit RemoveSchemeProposal(address(_avatar), proposalId, address(intVote), _scheme, _descriptionHash);
183188
proposalsInfo[proposalId] = ProposalInfo({
184189
blockNumber:block.number,
185190
avatar:_avatar,

0 commit comments

Comments
 (0)