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

Add revert reasons to proxies #495

Merged
merged 4 commits into from Oct 16, 2018

Conversation

@bingen
Copy link
Contributor

bingen commented Oct 16, 2018

Closes #490.

@izqui

izqui approved these changes Oct 16, 2018

Copy link
Member

izqui left a comment

🚀

@@ -50,12 +58,12 @@ contract Vault is EtherTokenConstant, AragonApp, DepositableStorage {
external
authP(TRANSFER_ROLE, arr(_token, _to, _value))
{
require(_value > 0);
require(_value > 0, ERROR_TRANSFER_VALUE_ZERO);

if (_token == ETH) {
_to.transfer(_value);

This comment has been minimized.

@sohkai

sohkai Oct 16, 2018

Member

Wondering if we should use a require(_to.send(_value)) here so we can customize the error message.

string private constant ERROR_TRANSFER_VALUE_ZERO = "VAULT_TRANSFER_VALUE_ZERO";
string private constant ERROR_VALUE_MISMATCH = "VAULT_VALUE_MISMATCH";
string private constant TOKEN_TRANSFER_FROM_REVERTED = "VAULT_TOKEN_TRANSFER_FROM_REVERT";
string private constant TOKEN_TRANSFER_REVERTED = "VAULT_TOKEN_TRANSFER_REVERTED";

This comment has been minimized.

@sohkai

sohkai Oct 16, 2018

Member

These last two should have the ERROR_ prefix :).

This comment has been minimized.

@bingen

bingen Oct 16, 2018

Contributor

I was not using the prefix when it's a _REVERT, but I'll add it.

@@ -176,7 +186,7 @@ contract Voting is IForwarder, AragonApp {
* @param _evmScript Start vote with script
*/
function forward(bytes _evmScript) public {
require(canForward(msg.sender, _evmScript));
require(canForward(msg.sender, _evmScript), ERROR_CAN_NOT_FORWARD);
_newVote(_evmScript, "", true, true);
}

This comment has been minimized.

@sohkai

sohkai Oct 16, 2018

Member

Can't comment far enough down, but there should be a message for the require(vote_.totalVoters > 0) later.

This comment has been minimized.

@bingen

bingen Oct 16, 2018

Contributor

Oh, I missed that one when merging master, thanks!

@@ -162,7 +172,7 @@ contract Voting is IForwarder, AragonApp {
* @param _voteId Id for vote
*/
function executeVote(uint256 _voteId) external voteExists(_voteId) {
require(canExecute(_voteId));
require(canExecute(_voteId), ERROR_CAN_NOT_EXCUTE);

This comment has been minimized.

@sohkai

sohkai Oct 16, 2018

Member

EXECUTE

@sohkai

sohkai approved these changes Oct 16, 2018

Copy link
Member

sohkai left a comment

🏅 👏

bingen and others added some commits Oct 16, 2018

Add revert reasons to proxies
Address PR #495 comments.
Fix

@izqui izqui merged commit cdaee27 into master Oct 16, 2018

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@izqui izqui deleted the issue490 branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment