-
Notifications
You must be signed in to change notification settings - Fork 246
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
encodeParam and encodeParams functions #456
base: next
Are you sure you want to change the base?
Changes from 14 commits
37b2612
85dda17
e3b49ea
bc0e525
a97c86d
fee7fc1
ecf2f5c
8542235
a85c5aa
b3ba05d
79100ce
9b4f8f3
b056828
0aa8d3c
e96f7c0
1fa2b95
da862e2
480b2f6
bdeee78
04c2048
2530b15
45727e3
97327f3
c44a025
b647866
aa08e27
b05c010
9c84dc3
dbce97e
dd2f8f2
22ec764
93e5ba1
ad1b7f7
3a8fb64
c4e0fb1
a026e22
5c61d10
c37d4fd
3d8c54f
9734f9c
8cf6039
3c909ef
584f4bc
05d3d26
d2268c8
722e25e
dbb0e06
db9342f
b18c1a5
37f8eff
9ff0bda
032f577
7366642
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
pragma solidity ^0.4.24; | ||
|
||
|
||
contract ACLParams { | ||
|
||
enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types | ||
|
||
struct Param { | ||
uint8 id; | ||
uint8 op; | ||
uint240 value; // even though value is an uint240 it can store addresses | ||
// in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal | ||
// op and id take less than 1 byte each so it can be kept in 1 sstore | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
|
||
pragma solidity ^0.4.24; | ||
|
||
import "./ACLParams.sol"; | ||
|
||
|
||
contract ACLSyntaxSugar { | ||
function arr() internal pure returns (uint256[]) {} | ||
|
@@ -85,18 +87,34 @@ contract ACLSyntaxSugar { | |
} | ||
|
||
|
||
contract ACLHelpers { | ||
function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { | ||
contract ACLHelpers is ACLParams { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we adding 2 newlines here? Not a big deal, but we don't usually do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I didn't know that, sorry. It's fixed. |
||
|
||
function decodeParamOp(uint256 _x) public pure returns (uint8 b) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we converting these 3 functions from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the delay. hmm.. it’s been so long that I don’t even remember haha! I guess I wanted the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think if I move the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's mark it as something we should do in the future 👍 . |
||
return uint8(_x >> (8 * 30)); | ||
} | ||
|
||
function decodeParamId(uint256 _x) internal pure returns (uint8 b) { | ||
function decodeParamId(uint256 _x) public pure returns (uint8 b) { | ||
return uint8(_x >> (8 * 31)); | ||
} | ||
|
||
function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { | ||
function decodeParamsList(uint256 _x) public pure returns (uint32 a, uint32 b, uint32 c) { | ||
a = uint32(_x); | ||
b = uint32(_x >> (8 * 4)); | ||
c = uint32(_x >> (8 * 8)); | ||
} | ||
|
||
function encodeParams(Param[] params) internal pure returns (uint256[]) { | ||
uint256[] memory encodedParams = new uint256[](params.length); | ||
|
||
for (uint i = 0; i < params.length; i++) { | ||
encodedParams[i] = encodeParam(params[i]); | ||
} | ||
|
||
return encodedParams; | ||
} | ||
|
||
function encodeParam(Param param) internal pure returns (uint256) { | ||
return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
pragma solidity 0.4.24; | ||
|
||
import "./helpers/Assert.sol"; | ||
import "./helpers/ACLHelper.sol"; | ||
import "../acl/ACLSyntaxSugar.sol"; | ||
import "../acl/ACL.sol"; | ||
|
||
|
||
contract TestACLHelpers is ACL, ACLHelper { | ||
|
||
function testEncodeParam() public { | ||
Param memory param = Param(2, uint8(Op.EQ), 5294967297); | ||
|
||
uint256 encodedParam = encodeParam(param); | ||
|
||
(uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); | ||
|
||
Assert.equal(uint256(param.id), uint256(id), "Encoded id is not equal"); | ||
Assert.equal(uint256(param.op), uint256(op), "Encoded op is not equal"); | ||
Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); | ||
} | ||
|
||
function testEncodeParams() public { | ||
Param[] memory params = new Param[](6); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why 6? Only 4 are being used, right? (not a big deal) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True! :) |
||
|
||
params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); | ||
params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); | ||
params[2] = Param(2, uint8(Op.EQ), 1); | ||
params[3] = Param(3, uint8(Op.NEQ), 2); | ||
|
||
|
||
uint256[] memory encodedParam = encodeParams(params); | ||
|
||
(uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could use a loop here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally! |
||
|
||
Assert.equal(uint256(params[0].id), uint256(id0), "Encoded id is not equal"); | ||
Assert.equal(uint256(params[0].op), uint256(op0), "Encoded op is not equal"); | ||
Assert.equal(uint256(params[0].value), uint256(value0), "Encoded value is not equal"); | ||
|
||
(uint32 id1, uint32 op1, uint32 value1) = decodeParamsList(encodedParam[1]); | ||
|
||
Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); | ||
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"); | ||
|
||
(uint32 id2, uint32 op2, uint32 value2) = decodeParamsList(encodedParam[2]); | ||
|
||
Assert.equal(uint256(params[2].id), uint256(id2), "Encoded id is not equal"); | ||
Assert.equal(uint256(params[2].op), uint256(op2), "Encoded op is not equal"); | ||
Assert.equal(uint256(params[2].value), uint256(value2), "Encoded value is not equal"); | ||
|
||
(uint32 id3, uint32 op3, uint32 value3) = decodeParamsList(encodedParam[3]); | ||
|
||
Assert.equal(uint256(params[3].id), uint256(id3), "Encoded id is not equal"); | ||
Assert.equal(uint256(params[3].op), uint256(op3), "Encoded op is not equal"); | ||
Assert.equal(uint256(params[3].value), uint256(value3), "Encoded value is not equal"); | ||
|
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const runSolidityTest = require('./helpers/runSolidityTest') | ||
|
||
runSolidityTest('TestACLHelpers') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the MIT header here (e.g. see https://github.com/aragon/aragonOS/blob/dev/contracts/acl/IACL.sol#L1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! ;) Is there a rule of thumb on which file an MIT header should be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).