Core: Code review by Facu

notes.md

@@ -0,0 +1,31 @@
+### 2020-10-09 Review
+
+#### Usage
+- who would you say it is a target user?
+- what about providing an optional extending including a settlement period?
+- how do we expect people to be able to submit proposals to govern in terms of tools?
+
+#### Documentation
+- add inline doc and natspec descriptions, also inline comments could help in some cases
+- would be good to have a state-machine diagram showing the actions lifecycle
+- we can also add a funds diagram to show how the money flow in the system  
+
+#### Security
+- Possible attack vector submit action and challenge it immediately to avoid being vetoed
+- AFAIK there is nothing that can't be executed by the executors right?  
+
+#### Repos
+
+#### SC
+- use `_` prefix for internal functions
+- use a consistent function ordering
+- use different naming strategy for scope-declared variables and function params
+- unused declared return variables
+
+##### ERC3k JS
+- outdated abi, it is probably better to point to the exported ABIs directly
+- not exactly sure how this library should be used
+
+##### Subgraph
+- outdated abi, it is probably better to point to the exported ABIs directly
+- rename entities following the current naming strategy 

packages/erc3k/contracts/ERC3000Data.sol

@@ -9,13 +9,15 @@ import "./IERC3000Executor.sol";
 
 library ERC3000Data {
     // TODO: come up with a non-shitty name
+    // Note: Maybe payload is a better fit for this while renaming the current "payload" struct to something else
     struct Container {
         Payload payload;
         Config config;
     }
 
     struct Payload {
         uint256 nonce;
+        // Note: what could be a use case that would require an extra delay?
         uint256 executionTime;
         address submitter;
         IERC3000Executor executor;
@@ -31,9 +33,11 @@ library ERC3000Data {
 
     struct Config {
         uint256 executionDelay;
+        // Note: I'd call these variables "xCollateral" or rename the struct to "Deposit", I prefer the first option
         Collateral scheduleDeposit;
         Collateral challengeDeposit;
         Collateral vetoDeposit;
+        // Note: why not arbitrator?
         address resolver;
         bytes rules;
     }
@@ -44,6 +48,7 @@ library ERC3000Data {
     }
 
     function containerHash(bytes32 payloadHash, bytes32 configHash) internal view returns (bytes32) {
+        // Note: Split ERC name from version name
         return keccak256(abi.encodePacked("erc3k-v1", this, payloadHash, configHash));
     }
 
@@ -52,6 +57,7 @@ library ERC3000Data {
     }
 
     function hash(Payload memory payload) internal pure returns (bytes32) {
+        // Note: I'd try to follow the same order defined in the struct to be consistent
         return keccak256(
             abi.encodePacked(
                 payload.nonce,

packages/erc3k/contracts/IERC3000.sol

@@ -8,20 +8,25 @@ pragma experimental ABIEncoderV2;
 import "./ERC3000Data.sol";
 import "./IERC3000Executor.sol";
 
+// Note: why not defining functions as external?
 abstract contract IERC3000 {
     function schedule(ERC3000Data.Container memory container) virtual public returns (bytes32 actionHash);
+    // Note: why logging just the collateral? wouldn't be better to log a Config hash or identifier?
     event Scheduled(bytes32 indexed containerHash, ERC3000Data.Payload payload, ERC3000Data.Collateral collateral);
 
+    // Note: why enforcing this in the interface? it could be confused with the executor interface
     function execute(ERC3000Data.Container memory container) virtual public returns (bytes[] memory execResults);
     event Executed(bytes32 indexed containerHash, address indexed actor, bytes[] execResults);
 
     function challenge(ERC3000Data.Container memory container, bytes memory reason) virtual public returns (uint256 resolverId);
+    // Note: same here about the collateral vs config logging
     event Challenged(bytes32 indexed containerHash, address indexed actor, bytes reason, uint256 resolverId, ERC3000Data.Collateral collateral);
 
     function resolve(ERC3000Data.Container memory container, uint256 resolverId) virtual public returns (bytes[] memory execResults);
     event Resolved(bytes32 indexed containerHash, address indexed actor, bool approved);
 
     function veto(bytes32 payloadHash, ERC3000Data.Config memory config, bytes memory reason) virtual public;
+    // Note: same here about the collateral vs config logging
     event Vetoed(bytes32 indexed containerHash, address indexed actor, bytes reason, ERC3000Data.Collateral collateral);
 
     function configure(ERC3000Data.Config memory config) virtual public returns (bytes32 configHash);

packages/erc3k/contracts/IERC3000Executor.sol

@@ -8,7 +8,12 @@ pragma experimental ABIEncoderV2;
 import "./ERC3000Data.sol";
 
 abstract contract IERC3000Executor {
+    // Note: how is this expected to treat failing actions? especially when having multiple actions but one of them fails
+    // does it worth to allow the user specify whether it should ignore failures or execute in strict mode?
     function exec(ERC3000Data.Action[] memory actions) virtual public returns (bytes[] memory);
 
+    // Note: "actor" sounds a bit confusing, maybe executor, caller, or sender?
+    // Note: not sure how efficient it is to log the whole actions array, is it actually necessary if these are
+    // already being logged when the payload is scheduled? storing the hash should be enough
     event Executed(address indexed actor, ERC3000Data.Action[] actions, bytes[] execResults);
 }

packages/govern-core/contracts/ERC3000Registry.sol

@@ -7,14 +7,20 @@ pragma solidity 0.6.8;
 import "erc3k/contracts/ERC3000Executor.sol";
 import "erc3k/contracts/ERC3000.sol";
 
+
+// Note: should this be part of the standard?
+// Why not leveraging ENS for this?
 contract ERC3000Registry {
     bytes4 internal constant ERC3000_INTERFACE_ID = 0x9abf68a8;
 
     mapping (string => bool) public nameUsed;
 
+    // Note: I'd try to avoid the "dao" term here
     event Registered(ERC3000Executor indexed dao, ERC3000 queue, address indexed registrant, string name);
     event SetMetadata(ERC3000Executor indexed dao, bytes metadata);
 
+    // Note: does this mean that the same queue can be shared between multiple DAOs?
+    // or that the same DAO can have multiple queues?
     function register(ERC3000Executor _dao, ERC3000 _queue, string calldata _name, bytes calldata _initialMetadata) external
     {
         require(!nameUsed[_name], "registry: name used");
@@ -24,6 +30,7 @@ contract ERC3000Registry {
         nameUsed[_name] = true;
 
         emit Registered(_dao, _queue, msg.sender, _name);
+        // Note: This seems like DAOs metadata can be overwritten easily
         _setMetadata(_dao, _initialMetadata);
     }
 

packages/govern-core/contracts/GovernFactory.sol

@@ -29,6 +29,7 @@ contract GovernFactory {
         registry = _registry;
     }
 
+    // Note: This will be moved somewhere else right?
     function newDummyGovern(string calldata _name) external returns (Govern govern, OptimisticQueue queue) {
         ERC3000Data.Collateral memory noCollateral;
         ERC3000Data.Config memory config = ERC3000Data.Config(

packages/govern-core/contracts/OptimisticQueue.sol

@@ -13,6 +13,8 @@ import "./lib/MiniACL.sol";
 import "./lib/OptimisticQueueStateLib.sol";
 import "./lib/SafeERC20.sol";
 
+
+// Note: Not super fan of this name tbh
 contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
     using ERC3000Data for *;
     using DepositLib for ERC3000Data.Collateral;
@@ -22,12 +24,14 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
     // Permanent state
     bytes32 public configHash;
     uint256 public nonce;
+    // Note: this are actually queue items right?
     mapping (bytes32 => OptimisticQueueStateLib.Item) public queue;
 
     // Temporary state
     mapping (bytes32 => address) public challengerCache;
     mapping (IArbitrator => mapping (uint256 => bytes32)) public disputeItemCache;
 
+    // Note: Why not re-using the
     constructor(address _aclRoot, ERC3000Data.Config memory _initialConfig)
         public
         MiniACL(_aclRoot)
@@ -76,13 +80,15 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
             OptimisticQueueStateLib.State.Executed
         );
 
+        // Note: isn't this being release twice? (see executeApproved)
         _container.config.scheduleDeposit.releaseTo(_container.payload.submitter);
 
         return _execute(_container.payload, containerHash);
     }
 
     function challenge(ERC3000Data.Container memory _container, bytes memory _reason) auth(this.challenge.selector) override public returns (uint256 disputeId) {
         bytes32 containerHash = _container.hash();
+        // Note: Move this SSTORE bellow
         challengerCache[containerHash] = msg.sender;
         queue[containerHash].checkAndSetState(
             OptimisticQueueStateLib.State.Scheduled,
@@ -97,10 +103,12 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
         require(feeToken.safeTransferFrom(msg.sender, address(this), feeAmount), "queue: bad fee pull");
         require(feeToken.safeApprove(recipient, feeAmount), "queue: bad approve");
         disputeId = arbitrator.createDispute(2, abi.encode(_container));
+        // Note: This should be done before the initial approval
         require(feeToken.safeApprove(recipient, 0), "queue: bad reset"); // for security with non-compliant tokens (that fail on non-zero to non-zero approvals)
 
         emit EvidenceSubmitted(arbitrator, disputeId, _container.payload.submitter, _container.payload.proof, true);
         emit EvidenceSubmitted(arbitrator, disputeId, msg.sender, _reason, true);
+        // Note: This should be optional allowing to close it at a later point in time
         arbitrator.closeEvidencePeriod(disputeId);
 
         disputeItemCache[arbitrator][disputeId] = containerHash;
@@ -132,6 +140,7 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
             OptimisticQueueStateLib.State.Cancelled
         );
 
+        // Note: what happens with the collateral?
         emit Vetoed(containerHash, msg.sender, _reason, _config.vetoDeposit);
     }
 
@@ -154,6 +163,7 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
             OptimisticQueueStateLib.State.Executed
         );
 
+        // Note: make one single transfer
         _container.config.scheduleDeposit.releaseTo(_container.payload.submitter);
         _container.config.challengeDeposit.releaseTo(_container.payload.submitter);
 
@@ -168,6 +178,7 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
         );
 
         address challenger = challengerCache[containerHash];
+        // Note: make one single transfer
         _container.config.scheduleDeposit.releaseTo(challenger);
         _container.config.challengeDeposit.releaseTo(challenger);
         challengerCache[containerHash] = address(0); // refund gas, no longer needed in state
@@ -183,6 +194,8 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
         );
         disputeItemCache[arbitrator][_disputeId] = bytes32(0); // refund gas, no longer needed in state
 
+        // Note: why not settling
+
         emit Ruled(arbitrator, _disputeId, _ruling);
     }
 
@@ -191,6 +204,7 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
         bytes calldata,
         bool
     ) external override {
+        // Note: this will be implemented right?
         revert("queue: evidence");
     }
 
@@ -207,6 +221,7 @@ contract OptimisticQueue is ERC3000, IArbitrable, MiniACL {
         emit Executed(_containerHash, msg.sender, execResults);
     }
 
+    // Note: Rename to `_configure`
     function _setConfig(ERC3000Data.Config memory _config)
         internal
         returns (bytes32)

packages/govern-core/contracts/lib/DepositLib.sol

@@ -11,10 +11,12 @@ import "./SafeERC20.sol";
 library DepositLib {
     using SafeERC20 for ERC20;
 
+    // Note: why not lock/unlock? I think it's been widely adopted by the community
     event Collected(address indexed token, address indexed from, uint256 amount);
     event Released(address indexed token, address indexed to, uint256 amount);
 
     function collectFrom(ERC3000Data.Collateral memory _collateral, address _from) internal {
+        // Note: should this allow ETH deposits too?
         if (_collateral.amount > 0) {
             ERC20 token = ERC20(_collateral.token);
             require(token.safeTransferFrom(_from, msg.sender, _collateral.amount), "queue: bad get token");

packages/govern-core/contracts/lib/MiniACL.sol

@@ -6,15 +6,18 @@ pragma solidity 0.6.8;
 pragma experimental ABIEncoderV2;
 
 library MiniACLData {
+    // Note: Rename to op code
     enum BulkOp { Grant, Revoke, Freeze }
 
+    // Note: Rename to ACL action, change, or instruction
     struct BulkItem {
         BulkOp op;
         bytes4 role;
         address who;
     }
 }
 
+// Note: what about using simply "ACL"? let's try to forget about previous implementations
 contract MiniACL {
     bytes4 public constant ROOT_ROLE =
         this.grant.selector
@@ -33,6 +36,7 @@ contract MiniACL {
     event Frozen(bytes4 indexed role, address indexed actor);
 
     modifier auth(bytes4 _role) {
+        // Note: We can figure out a minimal ACL oracle interface
         require(
             roles[_role][msg.sender] ||  // sender authorized
             roles[_role][ANY_ADDR],      // or anyone allowed
@@ -53,6 +57,7 @@ contract MiniACL {
         _revoke(_role, _who);
     }
 
+    // Note: I'm a bit hesitant about this, if the root is trustless enough we should be able to get rid of this
     function freeze(bytes4 _role) external auth(ROOT_ROLE) {
         _freeze(_role);
     }

packages/govern-core/scripts/sync-abis.js

@@ -8,6 +8,7 @@ const KNOWN_ABIS = [
   "OptimisticQueueFactory.json",
 ];
 
+// Note: Why not linking to exported ABIs instead?
 async function main() {
   KNOWN_ABIS.map((abiName) => {
     console.log(`Syncing ${abiName}`);

packages/govern-subgraph/schema.graphql

@@ -1,3 +1,4 @@
+# Note: In general I'd try to add double-links to avoid having to loop over children entities every time you want to look for one single entity
 type Govern @entity {
   id: ID!
   address: Bytes!
@@ -27,6 +28,7 @@ type Config @entity {
 
 type Collateral @entity {
   id: ID!
+  # Note: Let's define a token entity to hold its metadata as well
   token: Bytes!
   amount: BigInt!
 }

packages/govern-subgraph/src/Eaglet.ts

@@ -15,6 +15,7 @@ export function handleFrozen(event: FrozenEvent): void {
   const govern = Govern.load(event.address.toHexString())
   let id = 0
   const roles = govern.roles
+  // Note: we may want to only flag this change and reduce it from the connector instead
   for (id = 0; id < roles.length; id++) {
     const currentRole = roles[id]
     const funcSelector = currentRole.split('-')[1]
@@ -44,6 +45,7 @@ export function handleRevoked(event: RevokedEvent): void {
     role.who = event.params.who
   }
   role.revoked = true
+  // Note: we can define a unique key for roles instead
   // Check if role exists in roles array and if not,
   // push it
   const roles = govern.roles

packages/govern-subgraph/src/OptimisticQueue.ts

@@ -164,6 +164,7 @@ export function handleRevoked(event: RevokedEvent): void {
     }
   }
 
+  // Note: Same here, it is possible to identify these uniquely
   if (!exists) {
     roles.push(roleId)
     queue.roles = roles
@@ -173,6 +174,8 @@ export function handleRevoked(event: RevokedEvent): void {
 }
 
 export function handleScheduled(event: ScheduledEvent): void {
+  // Note: let's always use a findOrCreate logic instead, the graph presented
+  // issues tracking events in order, not kidding
   let queue = OptimisticQueue.load(event.address.toHexString())
   if (!queue) {
     throw new Error('Didnt find queue')