encodeParam and encodeParams functions #456

leftab · 2018-11-09T20:17:48Z

Fixes #427

Adds the following functions:

encodeParam(Param param) internal pure returns (uint256)
encodeParams(Param[] params) internal pure returns (uint256[])

I didn't add the encodeParam(uint8 id, uint8 op, uint240 value) function since I think encodeParam(Param param) covers pretty much the same use cases and is easier to use.

TODO:

Sanity check gas usage (these changes shouldn't cause any gas increases)

coveralls · 2018-11-09T20:38:58Z

Coverage increased (+0.008%) to 99.548% when pulling 7366642 on leftab:dev into 37f8eff on aragon:dev.

sohkai

Looking really good! Just thinking we could expose this as its own contract separate from ACLSyntaxSugar.

sohkai · 2018-11-19T11:30:26Z

contracts/acl/ACLSyntaxSugar.sol

@@ -86,6 +86,31 @@ contract ACLSyntaxSugar {


 contract ACLHelpers {
+
+    enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types


What do you think about putting this in another contract, e.g. ACLParams?

Good idea! I agree they fit more inside ACLParams

sohkai · 2018-11-19T11:33:05Z

contracts/acl/ACLSyntaxSugar.sol

+
+    function encodeParam(Param param) internal pure returns (uint256) {
+        return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value;
+    }


We could also expose the helpers in ACLHelpers.

I exposed all the functions except those having struct params since they would require the experimental ABI encoder.

sohkai · 2018-11-19T11:37:53Z

contracts/test/TestACLHelpers.sol

+        Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal");
+        Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal");        
+    }    
+


Let's test encodeOperator and encodeIfElse as well :)

I noticed that the encodeOperator and encodeIfElse functions are in a separate file in the test folder. Do you think it would be a good idea if I move them to the ACLHelpers contract?

Yes, I think it's a good idea, but let's wait for @sohkai opinion, maybe there is a reason I don't know for them to be there.

Ah I didn't notice they were only in the tests.

I think moving them could be helpful; AFAIK they're not simply because we only used them in test functionality. We should either add comments to explain how they work or make encodeOperator() more explicit (it's a bit confusing at first why it builds off of encodeIfElse()).

leftab · 2018-11-20T15:01:55Z

Thanks for the review @sohkai ! I applied the changes but somehow the Travis tests seems to fail now. I'm a bit perplexed since everything looks fine when I run the tests locally.

Allows a frontend to follow all `SetApp()` events emitted from the start of a KernelProxy's lifespan.

sohkai · 2019-01-11T08:24:43Z

@leftab Sorry for the hiatus on this—will get back to looking at this shortly :).

I think the CI broken when there was a ganache bug with account values or gas used; if you merge with master it should be fixed now :).

bingen · 2019-01-14T18:04:44Z

contracts/acl/ACLSyntaxSugar.sol

-contract ACLHelpers {
-    function decodeParamOp(uint256 _x) internal pure returns (uint8 b) {
+contract ACLHelpers is ACLParams {
+


Why are we adding 2 newlines here? Not a big deal, but we don't usually do that.

Oh I didn't know that, sorry. It's fixed.

bingen · 2019-01-14T18:14:45Z

contracts/acl/ACLSyntaxSugar.sol

+contract ACLHelpers is ACLParams {
+
+
+    function decodeParamOp(uint256 _x) public pure returns (uint8 b) {


Why are we converting these 3 functions from internal to public?

So it can be used by composition instead of only by inheritance. Do you think it would impact gaz usage and we should keep them internal?

Most likely wouldn't impact gas by very much (more or less only on deployment), but wondering how useful this would be. Do you think outside contracts might want access to this functionality, which is only accessible on the ACL?

We should also make sure encodeParams() and encodeParam() are the same visibility as their decode counterparts.

Sorry for the delay. hmm.. it’s been so long that I don’t even remember haha! I guess I wanted the encodeParam and encodeParams functions public, but it’s not supported yet anyway. I converted them back to internal and exposed the encodeOperator and encodeIfElse functions. The two functions are used in the ACL Interpreter tests and I made encodeOperator a little bit more explicit.

What do you think if I move the ACLHelpers contract into its own ACLHelpers.sol file? It would be a breaking change however.

Let's mark it as something we should do in the future 👍 .

bingen · 2019-01-14T18:26:58Z

contracts/test/TestACLHelpers.sol

+    }
+
+    function testEncodeParams() public {
+        Param[] memory params = new Param[](6);


Why 6? Only 4 are being used, right? (not a big deal)

bingen · 2019-01-14T18:27:45Z

contracts/test/TestACLHelpers.sol

+
+        uint256[] memory encodedParam = encodeParams(params);
+
+        (uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]);


Maybe we could use a loop here.

leftab · 2019-01-21T02:35:47Z

@sohkai @bingen Sorry guys for the delay, we've been quite busy working on our demo for Aracon and the end of the Drive app milestone. I should be able to post an update this week.

* APMRegistry: fix tests checking APMRegistry doesn't init without required permissions * EVMScriptRegistry: add more tests for coverage * ENSSubdomainRegistrary: fix initialization test

leftab · 2019-02-15T05:38:19Z

As for the gas usage sanity check, since the new functions are not used in aragonOS and are internal, there should be no impact on gas usage.

sohkai · 2019-02-15T11:08:13Z

@leftab Will check the compiled bytecode of the ACL. I don't see why it would change with this change, but need to be sure (don't want it to accidentally change from our initial deployment).

sohkai · 2019-02-15T11:10:05Z

contracts/test/helpers/ACLHelper.sol

-    }
-}
-
-
 contract AcceptOracle is IACLOracle {


Could we rename this file, since it only contains test ACLOracles now?

Renamed it to ACLOracleHelper

sohkai · 2019-02-15T11:10:41Z

contracts/acl/ACLParams.sol

@@ -0,0 +1,15 @@
+pragma solidity ^0.4.24;


Could we add the MIT header here (e.g. see https://github.com/aragon/aragonOS/blob/dev/contracts/acl/IACL.sol#L1)?

Done! ;) Is there a rule of thumb on which file an MIT header should be added?

I need to write more documentation on this, but on all the unpinned contracts (since they're the only ones meant to be used by apps, which frees them up to use any licensing model they want).

sohkai · 2019-03-07T18:54:17Z

@leftab I've pushed a commit to split ACLHelpers from ACLSyntaxSugar and clean up some formatting.

Unfortunately though, it looks like the changes do cause a bytecode difference against the old contracts :(. Going between this PR and dev, there's a bit of difference at the very end of the deployed bytecode:

Will have to investigate further to see what's exactly causing this, but it does mean it's a bigger change than expected. As far as I can tell it happens immediately as the struct and enum are moved out, which is quite odd behaviour for the compiler.

sohkai · 2019-05-18T10:11:42Z

@leftab Because this wasn't pulled into some of the minor aragonOS 4.x releases, due to the incompatible bytecode (and because we didn't want to risk re-deploying the ACL for 0.7), we'll roll this into aragonOS 5 :).

CLAassistant · 2019-08-08T12:09:09Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ usetech-llc
✅ leftab
✅ sohkai
❌ a33bcn
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

leftab added 9 commits November 5, 2018 15:38

Add encodeParam to ACLHelpers

37b2612

Move Op and Param structs from ACL to ACLHelpers

85dda17

Add encoreParams and encodeParam for Param structs

e3b49ea

Add TestACLHelpers

bc0e525

Remove encodeParam with uint params

a97c86d

Add AclHelpers js launch test

fee7fc1

Add encodeParams test

ecf2f5c

Add brackets around for loop

8542235

Remove ACL import and inheritance

a85c5aa

sohkai reviewed Nov 19, 2018

View reviewed changes

leftab added 5 commits November 19, 2018 19:00

Move Op and Param to ACLParams contract

b3ba05d

Add encodeOperator and encodeIfElse tests

79100ce

Compact params

9b4f8f3

Expose ACLHelpers functions

b056828

Fix linter issues

0aa8d3c

sohkai added 3 commits December 12, 2018 17:41

chore: update license text (aragon#467)

e96f7c0

Merge branch 'master' into dev

1fa2b95

KernelProxy: emit SetApp event on construction (aragon#466)

da862e2

Allows a frontend to follow all `SetApp()` events emitted from the start of a KernelProxy's lifespan.

sohkai added this to the A1 Sprint: 3.2 milestone Jan 14, 2019

bingen reviewed Jan 14, 2019

View reviewed changes

sohkai assigned leftab Jan 18, 2019

leftab added 5 commits February 3, 2019 00:03

Add encodeParam to ACLHelpers

480b2f6

Move Op and Param structs from ACL to ACLHelpers

bdeee78

Add encoreParams and encodeParam for Param structs

04c2048

Add TestACLHelpers

2530b15

Remove encodeParam with uint params

45727e3

sohkai and others added 8 commits February 7, 2019 17:03

chore: pin ganache-cli to 6.2.3 (aragon#472)

c4e0fb1

feat: Add SafeERC20 (aragon#469)

a026e22

tests: Coverage improvements (aragon#474)

5c61d10

* APMRegistry: fix tests checking APMRegistry doesn't init without required permissions * EVMScriptRegistry: add more tests for coverage * ENSSubdomainRegistrary: fix initialization test

4.1.0-rc.1

c37d4fd

chore: upgrade solium (aragon#476)

3d8c54f

Merge with dev

9734f9c

Convert back ACLHelpers functions to internal

8cf6039

Expose encodeOperator and encodeIfElse in ACLHelpers

3c909ef

sohkai reviewed Feb 15, 2019

View reviewed changes

leftab and others added 10 commits February 18, 2019 02:58

Add MIT license to ACLParams

584f4bc

Rename ACLHelper to ACLOracleHelper

05d3d26

VaultRecoverable: emit event on successful recovery (aragon#480)

d2268c8

test: remove accidentally placed .only() in tests (aragon#483)

722e25e

feat: Add radspec strings to ENSSubdomainRegistrar

dbb0e06

fix: update radspec strings (aragon#489)

db9342f

Create CONTRIBUTING.md

b18c1a5

chore: update readme.md

37f8eff

Merge branch 'dev' into leftab-dev

9ff0bda

fix: move ACLHelpers to its own file

032f577

feat: move ACL param IDs to ACLParams

7366642

sohkai removed this from the A1 Sprint: 4.1 milestone Mar 7, 2019

sohkai force-pushed the dev branch from a75ce92 to 76addef Compare April 5, 2019 11:53

sohkai added this to the aragonOS 5.0 milestone May 18, 2019

sohkai changed the base branch from dev to next July 11, 2019 09:29

		@@ -86,6 +86,31 @@ contract ACLSyntaxSugar {


		contract ACLHelpers {

		enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types

		contract ACLHelpers is ACLParams {


		function decodeParamOp(uint256 _x) public pure returns (uint8 b) {


		uint256[] memory encodedParam = encodeParams(params);

		(uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]);

encodeParam and encodeParams functions #456

Are you sure you want to change the base?

encodeParam and encodeParams functions #456

Conversation

leftab commented Nov 9, 2018 • edited Loading

coveralls commented Nov 9, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leftab commented Nov 20, 2018

sohkai 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leftab commented Jan 21, 2019

leftab commented Feb 15, 2019

sohkai commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Mar 7, 2019 • edited Loading

sohkai commented May 18, 2019

CLAassistant commented Aug 8, 2019 • edited Loading

leftab commented Nov 9, 2018 •

edited

Loading

coveralls commented Nov 9, 2018 •

edited

Loading

sohkai commented Mar 7, 2019 •

edited

Loading

CLAassistant commented Aug 8, 2019 •

edited

Loading