somish / govblocks-protocol Goto Github PK

We either need to maintain this variable at proposal level rather than global level or remove it completely.

Right now, no matter if it's true or false, winning voters get the same reward. It only decides if pool keeps the reward of losing voters or gives them the reward. We either need to store this variable as well as vote value of winning solution in proposal data so that we can give more reward to the winning voters. It doesn't make sense that pool keeps the reward. Implementing this way will increase the gas cost for closing proposal and claiming reward, though.

I personally believe that voters should not be punished for having a different opinion and we should remove this option completely.

Doesn't look like the GovBlocks UI repo is open-source, so posting this here instead. I got this error when visiting https://app.nexusmutual.io/#/Governance in the Brave browser:

Token holding time per sub category has been used in 2 different ways:

1. As number of seconds for which tokens should be locked under "GOV" - Validation of number of tokens locked until tokenHoldingTime in validateStake(Governance)
2. Extra time for which tokens should be locked apart from voting period - functions getMaxCategoryTokenHoldTime and getRemainingClosingTime

An ideal approach would be tokenHoldingTime = votingTime + extraHoldingTime
and function getRemainingClosingTime = tokenHoldingTime - voting time spent

Also function getMaxCategoryTokenHoldTime() needs to be rechecked, remove if not being used.

Shifting methods like updateProposalDetails in Governance to GovernanceData can help improve performance and modularity.

We should add reason along with all the require statements so that curious users can debug on their own.

This was not done earlier as it would've increased the deployment cost significantly but as we are using proxies now, It doesn't matter. A contract is deployed only once.

Currently, a category is identified as external as long as the 1st member role in _allowedToCreateProposal is 0. However, there is no check in function addNewCategory() to ensure that _allowedToCreateProposal is non-0 at all other index, say[1,0]. While this can be avoided via UI, it should be handled at the contract level as well.

As dappName needs to be set in MemberRoles, users can mistakenly set it to something incorrect and replace their current member role contract with it. It will cause them to lose the control over their organisation. This is unlikely to happen but we can add a check in Master.sol to make sure that only MemberRoles with correct dApp name are added in new versions. This can be done by creating an instance of the member role contract using the Governed Interface and then checking the dApp name.

The functions proposalVoting() and initialVote() do have a few differences, but a lot of code that is common. I suggest we create an internal function handling the reusable part. The function proposalVoting() can pass msg.sender as a parameter to the internal function.

We are using a combination of Master-slave and eternal storage to maintain upgradeability. This means that a new set of contracts is deployed for every app which costs about 30m gas in total. This also means that we can not change the storage structure in future if we want to.

I propose that we change the deployment process to governed + master-slave + unstructured proxy storage.

I just noticed that there are no mappings which record proposals/solutions created by a user. While this is great as we save on gas, the claim reward function becomes very costly as we iterate through all proposals.

It is even worse for solutions, where we iterate via nested for-loops.

I suggest we do the following:

1. Take proposal numbers pending for reward as a parameter, instead of computing on blockchain. This can easily be done off-chain and we can calculate the reward to be given on-chain.
2. Maintain the proposal start variable per address, off chain.
3. Since we have all proposal ids in an array, we just raise 1 event for increasing reputation.

This shall reduce the overall complexity of the contract to a great extent.

I think we should add instructions for manually deploying an instance of the GovBlocks protocol, including a custom GBM, EventCaller etc for testing.

When completing the data inputs for automatic actions it can be easy to get the data formats incorrect.

To reduce risk it would be great to show the current value for the parameter/s being changed when values are about to be entered.

Note: I don't seem to be able to tag this for some reason. It's a enhancement, nice-to-have.

I have created a flat file from GBTStandardToken with their dependences in the hierachical order and you will see that many contracts are inherited many times.

ERC20 is inherited 3 times and imported 2 times, SafeMath is imported 2 times, StandardToken is inherited 3 times,

Function callCloseEvent() is called from openProposalForVoting() in Governance which has categoryId against proposal id. We can reuse data here.

As all the contracts are deployed my master, byte code hash is no longer needed in GBM and should be removed in next version.

I suggest the following:

1. Function validateStake should be marked internal and use address as a parameter to validate stake.
2. We make a separate function to calculate vote value.
3. The functions used to caste vote use 1 and 2 separately.

I realized that because of duplicating the code we missed the condition if (subCatThenMinStake == 0) in validateStakeAndReturnVoteValue() and unnecssarily computed locked values even in case no minimum stake is required.

In my opinion, we need to weigh the amount of gas being saved vs code complexity and reusability.

We can create a function in master that returns dApp token address or Token Proxy address based on if dAppSupportsLocking. This will reduce the complexity in other contracts as they will just have to call this function to get the locking address rather than first checking if dAppSupportsLocking and then fetching token or token proxy address from different functions.

The function categorizeProposal in Governance can be called either via AB or proposal owner. Both cases, should be handled in a different manner, for example validateStake() is not required in case of AB.

Also, allowedToCreateProposal looks like a duplicate check.

Currently the solutions event in GovernanceData contract does not have a solution id. The only way to access final verdict details is by fetching member address of final verdict, followed by filtering of the Solutions event.
Addition of a parameter in Solutions event could hence reduce complexity.

rather than storing bytes32[] memberRoles and using it's length, we can just store a uint to store the number of memberRoles. The member role names can be read form events.

Default category and subcategory parameters like incentive need to be updated to more practical values rather than the current dummy ones.

Function checkProposalVoteClosing in Governance.sol has restricted access because of modifier onlyInternal.

The function needs to be accessed via API and hence the modifier should be removed.

dApps are currently dependent on GBM for fetching some data like EventCaller. As we are planning on making GBM upgradable, we should eliminate this dependency and have the master of every dApp hold all such data.