mcchan1/adminFee-5.sol

Looks mostly ok to me...

Remarks:

1. Missing ; at the end of line 39.
2. setAdminFee should also be nonReentrant.
3. Although it's not really wrong the way it is, it would make more sense to have the check for >= 200 when a new denominator is set, not every time the set value is used. So I'd remove line 52, and insert require(_adminFeeDenominator >= 200); at the beginning of function setAdminFee. In this case, adminFeeDenominator and laoFundAddress should be initialized and/or you mustn't forget to call setAdminFee before the first payment period is over.
   If this seems too complicated, you can just leave it as it is...
4. A disadvantage of this solution compared to the previous one (giving a recipient address in each call) is that it is now easier to accidentally send the fees to an "old" address which is now considered wrong (for whatever reason). For example, if the owner changes, the new owner must remember to set a new laoFundAddress, or the old one will still receive the fees. If and when a change of the laoFundAddress is desired, it is also important to do it in a timely manner, because everyone can call withdrawAdminFee as soon as the payment period is over.

thanks! @HeikoFisch. Made corrections 1 & 2.

@adridadou regarding point 3. I'm ok with either... setAdminFee would be add a line: require(_adminFeeDenominator >= 200); and removing it from line 52 in withdrawAdminFee I didn't really want to add more to the constructor, so just set adminFeeDenominator and laoFundAddress after deployment. For point 4, I can live with this, but lmk what you think.

@HeikoFisch - quick fyi, spoke with @adridadou. Made recommended change for point 3. Point 4 - we can live with.

Ok, @mcchan1, thanks for the feedback.
You forgot the underscore, though. (Or move the test after the next line.)

Also, note that if you don't initialize adminFeeDenominator and you forget to call setAdminFee before the first call to withdrawAdminFee, the latter will not only revert, but also consume all available gas. I recommend to initialize adminFeeDenominator to 200 and laoFundAddress to a suitable value.

@HeikioFisch - Thanks again...put that underscore back in. For initializing adminFeeDenominator and laoFundAddress , do you think just adding into the Moloch's constructor is best...so that way it forces the summoner to declare at deployment? Also, one unrelated question -- if the adminFeeDenominator is at an odd number, for example 377, do you see any issues?

@mcchan1

For initializing adminFeeDenominator and laoFundAddress , do you think just adding into the Moloch's constructor is best...so that way it forces the summoner to declare at deployment?

Adding laoFundAddress to the constructor is, in my opinion, a good idea. Of course, you could just use the summoner as the initial laoFundAddress if that suits your needs (which I don't know). Forcing someone to choose an initial laoFundAddress -- what you proposed -- seems a bit safer, but ultimately that's a choice you have to make, not so much a technical question.

As to the adminFeeDenominator, I'd just set that to 200 when the variable is declared, because 200 is the default value anyway. That is, I'd replace the current line 39 with uint256 public adminFeeDenominator = 200;.
If you choose to set it in the constructor, that's fine too, but you have verify that it is at least 200, like in the setAdminFee function.

Also, one unrelated question -- if the adminFeeDenominator is at an odd number, for example 377, do you see any issues?

No, it doesn't make a difference whether the number is odd or even. In general, this is an integer division, that is, the fractional part of the result is cut off. So, for example,
9999 / 200 = 49,
10000 / 200 = 50,
10001 / 200 = 50, etc.
Nothing special happens if the denominator is an odd number:
123456 / 377 = 327.

@mcchan1
At the risk of belaboring the obvious, I'd like to point out that this function needs to be tested -- in particular, because it is one of the gas-critical functions that iterate over all tokens. You have to make sure that it stays within the block gas limit when the token counts hit their limits.
Note also that if the event (which should currently result in a compile-time error) is supposed to be emitted in the loop body, this will increase gas costs significantly, so I'd recommend figuring out first what event(s) you want...

Maybe this is all obvious to you, but because you wrote in the chat that the admin fee function just needs to be added, I became a little bit concerned about this and thought that it was better to be explicit about it...

@HeikoFisch -Thanks for making this explicit since I wasn’t clear in the chat about this function being production ready w/o testing @awrigh01. @adridadou - Definitely will need your help writing the tests before actual deployment. Also, I don’t think we need the Event, since this will not be used very often and I don’t see a need to surface in the UI.

@mcchan1, let me add 2 things:

1. A single event outside of the loop is no problem at all, in case you'd like to have that.

2. One of the last-minute changes in the Moloch makes it advisable to replace Line 61 (unsafeInternalTransfer(GUILD, laoFundAddress, token, amount); with the following two lines:

userTokenBalances[GUILD][token] -= amount;
userTokenBalances[laoFundAddress][token] += amount;

Essentially, we're doing the transfer manually, like in _ragequit, because unsafeInternalTransfer has become more expensive recently.

With these changes and the current token limits, I'd be very surprised if this function was able to exceed the block gas limit. Nevertheless, testing is always a good idea... ;)

thank you for the update @HeikoFisch - very much appreciated! The changes have been made, I'll keep the event out for the moment, unless our UI devs demand one. Also, I like that the the token limit was added in the new version.

👍

One (I hope) final remark on this, @mcchan1.
Contrary to my expectation, it looks like the compiler is not able to optimize the SLOADs for the adminFeeDenominator and the laoFundAddress out of the loop. This means that withdrawAdminFee could be up to ~480k gas more expensive than a ragequit. With the Moloch token limits, you'd be at around 82% of the current block gas limit (ragequit ~77%). If you'd like to play it safe(r) or just want to avoid wasting gas, you could add

uint256 denominator = adminFeeDenominator;
address recipient = laoFundAddress;

at current line 56, and then replace

• adminFeeDenominator in current line 59 with denominator and
• laoFundAddress in current line 62 with recipient.

So, rather than reading from storage in every iteration of the loop, you'd only read from storage once -- into local variables -- and then use these variables in the loop body.
Again, these are mostly theoretical considerations; testing the code is recommended...

It might be obvious, but let me add that the state variables lastPaymentTime and laoFundAddress must be set in the constructor, not where the variables are declared, because the summoningTime and the initial value for the laoFundAddress (no matter whether explicitly given or the summoner) will only be available in the constructor.

@HeikoFisch thanks again! always interested in safer and more gas efficiency. On your last note - I made it explicit with lastPaymentTime = now in the constructor, rather than inherit if from the constructor's summoningTime = now and same for laoFundAddress