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

[WIP] Actor app, EVM #580

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@izqui
Member

izqui commented Nov 16, 2018

Spec and design discussion: https://forum.aragon.org/t/actor-app-arbitrary-actions-from-daos/275

  • 1. Vault inheritance
  • 2. Arbitrary call execution
  • 3. Forwarding interface
  • 4. Signature handling

TODO:

  • Roles in arapp

Closes #45.

@izqui izqui added wip new-app labels Nov 16, 2018

"network": "rinkeby"
}
},
"roles": [

This comment has been minimized.

@izqui

izqui Nov 16, 2018

Member

TODO: add roles from both vault and actor

// TODO: requires the @decodeData helper to be implemented in radspec
/**
* @notice Execute `@decodeData(target, data)` on `target` `ethValue == 0 ? '' : '(Sending' + @formatToken(ethValue, ETH) + ')'`.

This comment has been minimized.

@izqui

izqui Nov 16, 2018

Member

This radspec string won't evaluate until this helper is implemented in radspec or we decide on an alternative

This comment has been minimized.

@onbjerg

onbjerg Dec 2, 2018

Contributor

What is @decodeData supposed to do?

bytes memory input = ""; // no input
address[] memory blacklist = new address[](0); // no addr blacklist, can interact with anything
runScript(_evmScript, input, blacklist);
// We don't need to emit an event here as EVMScriptRunner will emit ScriptResult if successful

This comment has been minimized.

@izqui

izqui Nov 16, 2018

Member

There is a risk that the default script runner in the DAO won't emit this event and we wouldn't have visibility on Actor app script executions. Considering emitting an event here as well even though it will be redundant in most cases.

@@ -0,0 +1,238 @@
// Test that Actor is a fully functioning Vault by running the same tests against the Actor app

This comment has been minimized.

@izqui

izqui Nov 16, 2018

Member

Copy pasting this code from the Vault tests feels dirty. @sohkai suggestions on how to do this in a clean way?

}
function forward(bytes _evmScript)
authP(RUN_SCRIPT_ROLE, arr(getScriptACLParam(_evmScript)))

This comment has been minimized.

@izqui

izqui Nov 16, 2018

Member

Having an explicit role for this function rather than require(canForward()) to make the role more explicit (and help our role extraction helpers!)

@izqui izqui force-pushed the actor-app branch from 21955da to 9830981 Nov 16, 2018

@coveralls

This comment has been minimized.

coveralls commented Nov 16, 2018

Coverage Status

Coverage remained the same at 96.124% when pulling 9830981 on actor-app into f6df8d0 on master.

@coveralls

This comment has been minimized.

coveralls commented Nov 16, 2018

Coverage Status

Coverage increased (+0.8%) to 96.915% when pulling e132297 on actor-app into f6df8d0 on master.

@izqui izqui force-pushed the actor-app branch from 5576ad2 to abe3249 Nov 19, 2018

@izqui izqui force-pushed the actor-app branch from 3fea717 to af478c1 Nov 19, 2018

@sohkai sohkai requested review from sohkai and bingen and removed request for sohkai Nov 19, 2018

@izqui izqui added app: actor and removed new-app labels Nov 23, 2018

@bingen

bingen approved these changes Nov 27, 2018

I had a first glance and it looks good to me. I will try to do another one more thorough though.

bytes4 public constant ISVALIDSIG_INTERFACE_ID = 0xabababab; // TODO: Add actual interfaceId
string private constant ERROR_EXECUTE_ETH_NO_DATA = "VAULT_EXECUTE_ETH_NO_DATA";
string private constant ERROR_EXECUTE_TARGET_NOT_CONTRACT = "VAULT_EXECUTE_TARGET_NOT_CONTRACT";

This comment has been minimized.

@bingen

bingen Nov 27, 2018

Contributor

This is 33 chars long, I seem to recall we were using up to 32 to avoid using extra storage.

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