# Kelvin's LexDAO/MolochDAO Notes

## General Feedback

* Revert comments could be expanded. Although the comments are relatively clear if you're familiar with the codebase, they might be confusing for others (ex.: "not delegate" in the `onlyDelegate` modifier).

**LexDAO Response:** *Yes, we agree that more fulsome revert statements would help molDAO users without strong UI. Due to eip-170 size limits and our desire to fold in more functionality, we saw shortened reverts as 'low hanging fruit' to save code space.*

* Pattern of using fixed addresses to represent temporary token states is interesting (`GUILD`, `ESCROW`, etc.). Not necessarily bad, though somewhat unusual. Could be worth hiding this by adding internal functions (e.g., `getGuildBalance(token)`)

**LexDAO Response:** *In v2x we are opting to maintain this fixed address pattern for consistency with v2, and also see some value in how it clearly segregates states of `ESCROW` during a contribution proposal and ultimate `GUILD` token balances. We will reconsider for v3 and future implementations.*

* Several places where logic could probably be reused (noted in below comments).

**LexDAO Response:** *Addressed below (e.g., creation of `registerMember` internal function logic).*

* Any particular reason why the guild logic + token logic need to be in the same contract? Feels like should be two separate contracts for simplicity.

**LexDAO Response:** *This implementation uses basic ERC20 as overlay on molDAO shares and loot balances. It felt simpler to compact this into base molDAO contract, as we use some ERC20 functions, such as `totalSupply()` as sum total balance check in some areas. Also, in terms of deployment UX, and saving codespace, contract tracking, it seemed optimal to reduce creation of molDAO and corresponding token balances to a single summoning TX.*

* Comments could generally be improved/added to functions.

**LexDAO Response:** *Agreed. It is a balance of brevity and making things clear for users. We reworked comments to make things a bit more informative.*

* API could be simplified (fewer external methods, send data to the right method internally. e.g., a single `submitProposal` function).

**LexDAO Response:** *Yes, this has been tricky. We moved many functions into `external` visibility for gas savings (in light of new network cost paradigm). We will likely do large revisions of proposal track for v3, but will maintain current rails for better compatability with existing molDAO UIs.*

## Function-Specific Feedback

### `constructor`

* Summoner initialization loop might want to check that the `_summoner` and `_summonerShares` arrays are the same length. Would prevent accidental reverts when this is not the case.

**LexDAO Response:** *Per recommendation, we have updated constructor to include this check:* `require(_summoner.length == _summonerShares.length, "summoner & shares must match");`

* Summoner initialization loop could use the explicit struct format (`Member({ field: value, ... })`) for clarity.

**LexDAO Response:** *Per recommendation, we have updated `registerMember` function with explicit struct format:*

function registerMember(address newMember, uint256 shares) internal [...] {
members[newMember] = Member({
delegateKey : newMember,
exists : 1, // 'true'
shares : shares,
loot : 0,
highestIndexYesVote : 0,
jailed : 0});

### `submitProposal`
* Is `sharesRequested.add(lootRequested) <= MAX_GUILD_BOUND` correct? Seems like should be including current guild balances too.

**LexDAO Response:** *Yes, correct, though unclear from [v2 codebase](https://github.com/MolochVentures/moloch/blob/master/contracts/Moloch.sol)*.

*This first check on `submitProposal` allows fluid guild share/loot accounting format, where an applicant can technically ask for shares/loot that would exceed guild max bound, but conceivably, an existing member might ragequit and preserve bound before such share proposal passes. Therefore, the first check pretty much asks that they at least don't request a sum that itself exceeds entire bound. Ultimately, there is a second check on `processProposal` that enforces rules:*

if (totalSupply().add(proposal.sharesRequested).add(proposal.lootRequested) > MAX_GUILD_BOUND) {
didPass = false;
}

* Includes a check that `totalGuildBankTokens < MAX_TOKEN_GUILDBANK_COUNT`. I think it'd be easier to simply have a maximum number of whitelisted tokens instead of a maximum number of nonzero balances. Not sure if there's a specific reason for this. Would remove the need for this check.

**LexDAO Response:** *We are planning fulsome revision of bank accounting/whitelisting scheme for v3. We will maintain current internal patterns and this check for v2x.*

* Proposal flag system could be made more efficient by using a single uint and setting specific bits. Would cut down on gas and probably avoid `stack too deep` in certain places.

**LexDAO Response:** *We swapped v2 `bool` flag for `uint8` to take advantage of struct gas/size savings; curious if you can elaborate on using simpler uint/bits.*

### `submitWhitelistProposal`
* Might be worth having a single entrypoint for all proposals instead of requiring the client to call a specific proposal function.

**LexDAO Response:** *Yes, agreed. We are maintaining seperate functions that feed values into `submitProposal` track for compatability with v2 UIs, but expect to compress (single entry) to take advantage of code size/gas savings. However, there is something to be said in seperating out functions to affirm caller intent, as each are material DAO operations (membership, payment, whitelisting, guildkick, arbitrary call action).*

* `approvedTokens.length < MAX_TOKEN_WHITELIST_COUNT` check is probably unnecessary here. It's possible for `approvedTokens.length` to be incremented before the proposal is completed (i.e., by another proposal). Useful for user feedback in certain cases, though.

**LexDAO Response**: *As noted above, molDAO involves two checks for bounds. In case of whitelisting, it merely confirms on `submitWhitelistProposal` that suggested sum total doesn't break whitelist bounds; on `processWhitelistProposal`, there is a second check, similar to regular `processProposal` check for guild bound (shares/loot) that confirms molDAO state (due to variances with other props) and makes sure bound is ultimately respected:*

if (approvedTokens.length >= MAX_TOKEN_WHITELIST_COUNT) {
didPass = false;
}

### `_submitProposal`
* Lots of data being stored on-chain within the Proposal struct. Could consider hashing the data, emitting as an event, requiring users to provide the data when voting. Depends on personal determination of value of reduced gas cost.

**LexDAO Response:** *Yes, that seems like an efficient option, though might induce more reliance on UIs, offchain data (which lacks persistence guarantees of molDAO struct). We will explore this in overall proposal track revamp in v3 research pile.*

### `sponsorProposal`
* Same check with `MAX_TOKEN_GUILDBANK_COUNT`.

**LexDAO Response:** *Per above: We are planning fulsome revision of bank accounting/whitelisting scheme for v3. We will maintain current internal patterns and this check for v2x.*

* Some checks are repeated here (whitelist checks for instance). Could be pulled into a separate function.

**LexDAO Response:** *Yes, we agree there are likely good effiency gains in stripping out repeat bounds and other checks. In this case, there seems to be enough variability that it makes sense to kick this for now into v3 redesign over proposal/whitelist tracks.*

### `submitVote`
* `uintVote < 3, "not < 3"` is a little confusing. Could make this parameter a boolean since you're only allowing yes/no votes anyway (and removes need for `vote == Vote.Yes || vote == Vote.No` check).

**LexDAO Response:** *Indeed, it is confusing without UI helper. In this case, we believe that value of using `Enum` struct to reflect null/abstention on proposals. Having three options allows us to maintain the ragequitting mechanic as-is and also avoid quorum requirements for the votes. Thus, we think the potential risks caution against deviating for now.*

### `processProposal`
* Similar comment for being able to process proposals at a single entrypoint instead of separate functions for different proposal types.

**LexDAO Response:** *Per above: We are planning fulsome revision proposal track and related logic in anticipation of v3.*

* `didPass` pattern feels a little off, since `_didPass` doesn't actually tell you if the proposal passed or not (only partly passing). Maybe `_didPass` could be renamed to something like `_didVotePass`.

**LexDAO Response:** *Yes, there is a slight intuitive gap here between votes 'passing' and successfully 'processing' to change state, mint shares, move money, etc. However, to keep deviation minimal, it seems helpful for now to maintain `_didPass` moniker, simplicity, as votes in typical orgs. often 'pass', but still require followup back-office and bank processing, which may ultimately require 'reverting' voted-for txs.*

* Same `MAX_TOKEN_GUILDBANK_COUNT` check here.

**LexDAO Response:** *Per above: We are planning fulsome revision of bank accounting/whitelisting scheme for v3. We will maintain current internal patterns and this check for v2x.*

* Member initialization logic seems like something that could be pulled out into a separate function.

**LexDAO Response:** *Yes, agreed. We have pulled this logic out as a new `registerMember` internal function*:

function registerMember(address newMember, uint256 shares) internal {
// if the sender is already taken by a member's delegateKey, reset it to their member address
if (members[memberAddressByDelegateKey[newMember]].exists == 1) {
address memberToOverride = memberAddressByDelegateKey[newMember];
memberAddressByDelegateKey[memberToOverride] = memberToOverride;
members[memberToOverride].delegateKey = memberToOverride;
}

members[newMember] = Member({
delegateKey : newMember,
exists : 1, // 'true'
shares : shares,
loot : 0,
highestIndexYesVote : 0,
jailed : 0
});

memberAddressByDelegateKey[newMember] = newMember;
}

### `_ragequit`
* Inherently limited by the number of `approvedTokens`. If too many tokens are approved, gas cost could be higher than the block gas limit (in theory). Could have this be a separate function so this limit doesn't exist anymore (just call as many times as necessary to fully ragequit).

**LexDAO Response:** *The `MAX_TOKEN_WHITELIST_COUNT` may work well enough here for now to maintain sensible limit on `approvedTokens` count for guild bank and ragequit purposes. We agree that iterative ragequit function seems sensible to avoid these harcoded bound limits seeming arbitrary, too restrictive, and we will research for v3 implementation.*

### `collectTokens`
* A little confused about this method. Where are the tokens actually being collected?

**LexDAO Response:** *This function would seem to allow members or their delegates to update guild bank state to reflect claimable tokens that may have been transferred 'off the books' of proposal track. We are confirming this purpose among our members to add to this comment/review.*

### `cancelProposal`
* Any reason why the proposal can't be cancelled once sponsored?

**LexDAO Response:** *Yes, molDAO is designed to essentially lock in the value of a proposal once sponsored by a member in accelerated offer/acceptance pattern;`ESCROW` and `GUILD` fixed addresses help reflect that execution and deposit into molDAO is contingent on a full vote among members. We could consider extending the cancellation period until the voting period for a proposal begins, thus avoiding any user wasting gas on a proposal that gets cancelled; however, this might only extend the cancellation period by an four hours or less in most molDAOs. *

### `updateDelegateKey`
* Could be modified so that it's usable within both `constructor` and `processProposal` when new members are added.

**LexDAO Response:** *Useful recommendation, but we will kick this for later, in anticipation of v3 designs aiming to makes guild votes and other member action delegation behave more like an ERC20 token.*