Agent app: EVM #580

izqui · 2018-11-16T18:37:24Z

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.

future-apps/actor/arapp.json

future-apps/actor/contracts/Actor.sol

izqui · 2018-11-16T18:45:39Z

future-apps/actor/test/vault.js

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


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

Hmm, this is quite a special case, but we could change the Vault tests to take in a contract artifact as an argument and export it as part of the package.

izqui · 2018-11-16T18:49:44Z

future-apps/actor/contracts/Actor.sol

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


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

coveralls · 2018-11-16T21:40:12Z

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

coveralls · 2018-11-16T21:40:12Z

Coverage remained the same at 96.484% when pulling 2a293d1 on actor-app into 7fb014b on next.

bingen

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

bingen · 2018-11-27T22:32:02Z

future-apps/actor/contracts/Actor.sol

+    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 is 33 chars long, I seem to recall we were using up to 32 to avoid using extra storage.

jvluso · 2019-01-03T04:38:37Z

future-apps/actor/contracts/Actor.sol

+        authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(getSig(_data)))) // TODO: Test that sig bytes are the least significant bytes
+    {
+        require(_ethValue == 0 || _data.length > 0, ERROR_EXECUTE_ETH_NO_DATA); // if ETH value is sent, there must be data
+        require(isContract(_target), ERROR_EXECUTE_TARGET_NOT_CONTRACT);


Is there a reason we don't want the actor app to be able to send eth to someone without calling a function? Anyone able to call the execute function will be able to send funds anyway, and this makes compatibility with external dapps slightly more difficult.

Thanks to Actor inheriting from Vault, that can be done using the transfer function. I just tried to separate role concerns here.

izqui · 2019-01-11T09:35:25Z

@sohkai do you think you can take a look this next or the next one? It is important for a concierge project

future-apps/agent/contracts/SignatureValidator.sol

sohkai · 2019-02-13T17:06:15Z

future-apps/agent/contracts/SignatureValidator.sol

    /// @return Returns whether signature is from a specified user.
-    function isValidSignature(bytes32 hash, address signer, bytes signature) internal pure returns (bool) {
+    function isValidSignature(bytes32 hash, address signer, bytes signature) internal view returns (bool) {
        if (signature.length == 0) {


This should be <= 1 since mode is always required

Length being 1 is valid for ERC1271 mode signatures. We could perform the check before sending it over to the erc1271 contract, but that contract may have logic which allows empty signatures

future-apps/agent/test/agent.js

future-apps/agent/contracts/SignatureValidator.sol

sohkai · 2019-02-13T17:58:12Z

future-apps/agent/contracts/standards/ERC1271.sol

@@ -5,7 +5,7 @@ pragma solidity 0.4.24;
 // Rationale: https://github.com/ethereum/EIPs/issues/1271#issuecomment-462719728

 contract ERC1271 {
-    bytes4 constant public ERC1271_INTERFACE_ID = this.isValidSignature.selector;
+    bytes4 constant public ERC1271_INTERFACE_ID = 0xfb855dc9; // this.isValidSignature.selector


Note that this was giving a warning; I guess solidity doesn't know this is known at compile-time

The Vault now exports its tests via a sharable utility. See f41519a for the changes made to the original tests to make them sharable.

izqui · 2019-02-15T17:11:04Z

Branch ready. @sohkai PTAL and let's get this 🚢🚢🚢

sohkai

🌮 🎉

sohkai · 2019-02-15T19:47:07Z

apps/agent/contracts/SignatureValidator.sol

@@ -134,4 +131,24 @@ library SignatureValidator {

        return ret;
    }
+
+    // From: https://github.com/Arachnid/solidity-stringutils/blob/master/src/strings.sol
+    function memcpy(uint dest, uint src, uint len) private pure {


Let's use uint256 consistently 👍

sohkai · 2019-02-15T19:49:10Z

apps/agent/contracts/SignatureValidator.sol

+        assembly {
+            let srcpart := and(mload(src), not(mask))
+            let destpart := and(mload(dest), mask)
+            mstore(dest, or(destpart, srcpart))


Technically this could be optimized since we don't need to make sure the remaining bytes in dest are kept, but let's keep it the same as the library 👍.

Good observation, but I would rather not modify the code

sohkai · 2019-02-15T19:49:29Z

apps/agent/contracts/SignatureValidator.sol

-    function popFirstByte(bytes memory input) internal pure returns (bytes memory output, byte firstByte) {
+    function popFirstByte(bytes memory input) private pure returns (bytes memory output) {
+        uint256 inputLength = input.length;
+        require(inputLength > 0);


What do you think about adding an error reason here?

* Actor app: inherit Vault and test vault functionality * Actor app: implement execute action * Actor app: implement permissioned forwarding * Actor app: improve tests * Actor app: signatures * CI: travis wait on npm install * Actor app: lint and enable CI * Add max gas for static calls * chore: upgrade to [email protected] * Address review comments * Lint * test: add comments, cosmetic changes, and more tests * SignatureValidator: fix compile warnings * test: add context headers * Metadata: add roles to arapp, remove webapp artifacts * Radspec strings * Travis: allow actor coverage to fail, tracking fix in aragon#658 * Prevent infinite loop by prohibiting setting itself as the designated signer * Radspec * Actor -> Agent rename * Add status to arapp and rename constants * Update ERC1271 implementation * Add ERC1271 interfaceId * Rename constants * Rename calldata * Refactor SignatureValidator.sol * Refactor signature validation tests and test EIP712 signatures * Handle empty signatures and undefined signature modes * Refactor SignatureValidator: handle ERC1271 checks in library * Test ERC1271 signature wrapping * cosmetic: fix whitespace * Agent: always allow execute to transfer ETH (aragon#651) * cosmetic: fix whitespace and add EIPs hash * chore: upgrade aragonOS and use test-helper's contracts * cosmetic: fix compile errors * cosmetic: fix linter * chore: fix travis allowed to fail matrix * test: ignore all test contracts from coverage * Actor: reuse Vault tests (aragon#668) The Vault now exports its tests via a sharable utility. See f41519a for the changes made to the original tests to make them sharable. * Move agent from future-apps/ to apps/ * Refactor popFirstByte to copy to a new array * Address last review comments * Lint * Address very last review comments

izqui added 3 commits November 15, 2018 19:38

Actor app: inherit Vault and test vault functionality

0d01aaf

Actor app: implement execute action

1fa6386

Actor app: implement permissioned forwarding

7a4dd5b

izqui added status: wip labels Nov 16, 2018

izqui commented Nov 16, 2018

View reviewed changes

Actor app: improve tests

9830981

izqui force-pushed the actor-app branch from 21955da to 9830981 Compare November 16, 2018 18:53

Actor app: signatures

7457eb3

izqui force-pushed the actor-app branch from 5576ad2 to abe3249 Compare November 19, 2018 11:21

CI: travis wait on npm install

af478c1

izqui force-pushed the actor-app branch from 3fea717 to af478c1 Compare November 19, 2018 13:25

Actor app: lint and enable CI

ebb10aa

sohkai requested review from sohkai and bingen and removed request for sohkai November 19, 2018 16:31

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

Add max gas for static calls

e132297

bingen approved these changes Nov 27, 2018

View reviewed changes

sohkai mentioned this pull request Dec 1, 2018

Allow to perform actions as token holder with tokens held in Vault #45

Closed

sohkai mentioned this pull request Dec 14, 2018

EPIC: Actor app aragon/client#541

Closed

4 tasks

jvluso reviewed Jan 3, 2019

View reviewed changes

izqui requested a review from sohkai January 11, 2019 09:35

sohkai modified the milestone: A1 Sprint: 3.2 Jan 11, 2019

sohkai added 2 commits February 13, 2019 15:21

Merge branch 'next' into actor-app

98369d1

Agent: always allow execute to transfer ETH (#651)

a0bddef

sohkai reviewed Feb 13, 2019

View reviewed changes

future-apps/agent/contracts/SignatureValidator.sol Outdated Show resolved Hide resolved

izqui commented Feb 13, 2019

View reviewed changes

future-apps/agent/contracts/SignatureValidator.sol Outdated Show resolved Hide resolved

sohkai reviewed Feb 13, 2019

View reviewed changes

future-apps/agent/test/agent.js Outdated Show resolved Hide resolved

sohkai reviewed Feb 13, 2019

View reviewed changes

future-apps/agent/contracts/SignatureValidator.sol Outdated Show resolved Hide resolved

sohkai added 3 commits February 13, 2019 18:08

cosmetic: fix whitespace and add EIPs hash

e8eb268

chore: upgrade aragonOS and use test-helper's contracts

2225408

cosmetic: fix compile errors

2ed4a2a

sohkai reviewed Feb 13, 2019

View reviewed changes

cosmetic: fix linter

c482981

sohkai force-pushed the actor-app branch 4 times, most recently from 91ca782 to 3959ac0 Compare February 13, 2019 20:51

chore: fix travis allowed to fail matrix

cd9614b

sohkai force-pushed the actor-app branch from 3959ac0 to cd9614b Compare February 13, 2019 20:52

sohkai and others added 5 commits February 13, 2019 23:19

test: ignore all test contracts from coverage

970431a

Actor: reuse Vault tests (#668)

cff0689

The Vault now exports its tests via a sharable utility. See f41519a for the changes made to the original tests to make them sharable.

Move agent from future-apps/ to apps/

905c32d

Refactor popFirstByte to copy to a new array

0d3e2ec

Address last review comments

5b7a5fe

Lint

2a293d1

sohkai approved these changes Feb 15, 2019

View reviewed changes

Address very last review comments

687e29d

izqui merged commit a2a7ac5 into next Feb 15, 2019

sohkai deleted the actor-app branch September 10, 2019 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agent app: EVM #580

Agent app: EVM #580

izqui commented Nov 16, 2018 •

edited

Loading

izqui Nov 16, 2018

sohkai Jan 26, 2019

izqui Nov 16, 2018

coveralls commented Nov 16, 2018

coveralls commented Nov 16, 2018 •

edited

Loading

bingen left a comment

bingen Nov 27, 2018

jvluso Jan 3, 2019 •

edited

Loading

izqui Jan 3, 2019

izqui commented Jan 11, 2019

sohkai Feb 13, 2019

izqui Feb 15, 2019

sohkai Feb 13, 2019

izqui commented Feb 15, 2019

sohkai left a comment

sohkai Feb 15, 2019

sohkai Feb 15, 2019

izqui Feb 15, 2019 •

edited

Loading

sohkai Feb 15, 2019

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

Agent app: EVM #580

Agent app: EVM #580

Conversation

izqui commented Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 16, 2018

coveralls commented Nov 16, 2018 • edited Loading

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvluso Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui commented Feb 15, 2019

sohkai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui commented Nov 16, 2018 •

edited

Loading

coveralls commented Nov 16, 2018 •

edited

Loading

jvluso Jan 3, 2019 •

edited

Loading

izqui Feb 15, 2019 •

edited

Loading