Add Whitelist contract

[medvedev1088]

Implements #743

🚀 Description

Added Whitelist contract and tests.

[AugustoL]

The owner can add himself to whitelist if he wants to buy tokens, what about checking only msg.sender?

[medvedev1088]

@AugustoL I think this will be more flexible because the owner can add and remove himself if he wants. On the other hand, I can't think of a situation when an owner doesn't want himself to be whitelisted for some operation. I guess it will be more for preventing errors than for authorization in this case, because the owner can whitelist himself at any time?

[AugustoL]

I never saw the owner of the contract receiving tokens. I guess we can leave it but in my experience the owner of the ICO not always receives tokens or ETH, they are forwarded to another wallets, multisig, vestedPayments, etc. That is why O would not assume the owner will want any tokens, and in that case he can whitelist himself.

[martriay]

I wouldn't think of this in terms of what the owner wants, I would adjust the behavior to the semantics instead. This modifier should only check whether an address is in the whitelist or not as its name promises.

[medvedev1088]

Done

[AugustoL]

What about using an array of addresses instead of just one? if I want to add 100 whitelisted addresses I have to make 100 txs,

[medvedev1088]

Good idea. I'll do it. Will do the same for removeFromWhitelist()

[martriay]

Would you mind setting a different function for it? Creating an array of 1 element seems clunky for this particular action.

[medvedev1088]

sure.

[medvedev1088]

Done

[AugustoL]

can you use require instead of returning bools?

[medvedev1088]

@martriay had a good point about returning bool: "the contract can be used programmatically by other contracts and react accordingly". Original discussion in this issue #743. What do you think @AugustoL ?

[AugustoL]

Intaht case we can still return bool and only leave the return true at the end of the function,

  function addToWhitelist(address[] addrs) onlyOwner public returns(bool) {
    for(uint256 i = 0; i < addrs.length; i ++) {
      require(!whitelist[addrs[i]]);
      whitelist[addrs[i]] = true;
      WhitelistedAddressAdded(addrs[i]);
    }
  return true;
  }

We should also check that the receiver address is not 0x.

[martriay]

@AugustoL I see no point in that revert. Sending a list of 100 addresses where only 1 is already whitelisted would result in a tremendous gas waste with no upside.

[AugustoL]

@martriay ture, and we dont even need to check that the address is not 0x, the address 0x it wont be able to make any call to the contract, so is the same to have it or not. So no requires needed in the add/remove whitelist functions?

[martriay]

I would call the return value success to make this easier to think. I would say that if 99 addresses were whitelisted and only 1 skipped, it should return true.

[medvedev1088]

@martriay any objections against returning successCount?

[martriay]

It would differ in style with the one-element whitelisting (that would return bool) and make the caller compare the result with the original amount sent, having little knowledge about which addresses have failed (events cannot be seen within the blockchain). As I don't see what can the caller do with that number, I prefer the success bool for being simple enough, and the if clausule will be nicer 😊.

[medvedev1088]

Guys looks like truffle doesn't support overloaded functions (we need it for addToWhitelist that accepts a single address and an array). Right now I get intermittent errors "TypeError: value.forEach is not a function" and "Error: new BigNumber() not a base 16 number: f17f52151ebef6c7334fad080c5704d77216b732,0xc5fdf4076b8f3a5357c5e395ab970b5b54098fef"

There is a PR here https://github.com/trufflesuite/truffle-contract/pull/75 but it's still open. There is a workaround here but it's bulky https://beresnev.pro/test-overloaded-solidity-functions-via-truffle/.

Should I just leave a TODO for now in tests and wait until it's fixed in truffle? Another option is to disambiguate addToWhitelist to addAddressToWhitelist and addAddressesToWhitelist

[medvedev1088]

Done. Decided to disambiguate methods.

[redsquirrel]

Was using 0.4.8 intentional here?

[medvedev1088]

@redsquirrel Not really.
Should I use pragma solidity ^0.4.18;? I found most contracts here use this pragma, however some contracts are different. Do you have any guidelines for the compiler version?

[AugustoL]

Yes please, use .18 :)

[medvedev1088]

Done

[shrugs]

I feel like this is a specific case of RBAC—would it be reasonable to inherit from RBAC and store a ROLE_WHITELISTED label for users? And then continue to provide the isWhitelisted modifiers and access for the ROLE_ADMIN?

[martriay]

@shrugs Indeed, this is a specific case, but a very common one. I think it's a good idea to have them separated since otherwise people would have to extend RBAC with unnecessary additional logic that's only increasing the attack surface.

[shrugs]

Seems reasonable to me 👍

[medvedev1088]

@martriay any plans on merging this PR?

[shrugs]

nitpick: sorry about that. otherwise LGTM

[shrugs]

let's add an @dev prefix here for multiple-line dev comments

[medvedev1088]

Done.