Discuss SafeERC20 usage

[0x-r4bbit]

In #41 (comment) I've fixed a bug where there was a usage of transferFrom where it should've been transfer instead.

As part of the fix, I've also introduced SafeERC20 so that we can rely on safeTransfer() and safeTransferFrom() respectively.

The reason I've made that change is because I wanted to ensure, if for whatever reason we allow for staking ERC20 token other than SNT which we know behaves as intended, safeTransfer() catches the cases where tokens don't behave as expected.

@3esmit brought up that this is rather unnecessary, and he's right - we can also just do a return value check ourselves or just expect that we only ever deal with tokens that behave as expected.

See the discussion here: #41 (comment)

This issue is a place to discuss whether or not we want to do this longterm.

For now the consensus was to remove SafeERC20 again from this PR and then revisit this discussion here if necessary.

[3esmit]

Why SafeTransfer is necessary:

ERC20 was desgined with backwards compatability with older tokens that used return false to signal error in transfer.
Initially, it was being discussed how to handle with errors. and ended up that throw/revert when errors is what makes sense and all new tokens begin using this. So, to support all tokens, they made it as ERC20 standard always return true, or throws, and demand for true check in the spec.

So now, we have 3 types of tokens that implements transfer(address,uint256) transferFrom(address,address,uint256)

• ERC20 good tokens (99% of all tokens?)
  • abi: transfer(address,uint256) returns (bool)
  • Success -> return true
  • Error -> throw
• ERC20 behavior, no return (0.99% of all tokens?)
  • abi: transfer(address,uint256) returns ()
  • Success -> "does nothing"
  • Error -> throw
• Not ERC20 behavior: Return TRUE on success, return false on error, might throw as well (0.01% of all tokens?)
  • abi: transfer(address,uint256) returns (bool)
  • Success -> return true
  • Error -> return false

Safe Transfer do a low level call to the ERC20 transfer, reads the output, throws if returnsize > 0 && returndata[0] != 0x01.

Slight more gas used when transfer vs. Not supporting tokens that do not throw on error

[0x-r4bbit]

I'm going to close this discussion for now.
I guess the conclusion is that the staking contracts are only meant to work with SNT which is behaving correctly per ERC20.

If there's any objections or reasons to discuss this further, ping here, and we can reopen the issue.