not considering `ERC20.transfer()` returning `false`

[daejunpark]

It seems to not consider the fact that transfer() and/or transferFrom() of some ERC20 token contracts could return false instead of throwing an exception in case of failure. Note that such a contract is still ERC20-compliant, as throwing an exception is recommended but not mandatory according to the ERC20 standard.

You may want to add the assertion on the return value, like (not sure about the exact syntax):

assert self.token.transfer(msg.sender, tokens)

[haydenadams]

@daejunpark Good point, thanks for the feedback. One potential issue is that many ERC20s are not fully ERC20 compliant (over 130 on EtherDelta) and do not return a bool at all, so this assertion might make those token transfers to fail. I'll write a test for it.

The only way I can think of that covers all cases is something like this:

user_balance: uint256 = Token(self.token).balanceOf(msg.sender)
self.token.transfer(msg.sender, tokens_purchased)
assert Token(self.token).balanceOf(msg.sender) == user_balance - tokens_purchased

Probably not even that bad a gas hit either!

[haydenadams]

I decided to go with assert self.token.transfer(msg.sender, tokens). Tokens that do not return True will need to be wrapped.

[BlinkyStitt]

Is there a recommend way of doing this wrapping in Vyper? I’ve seen some solutions in solidity.

[jacqueswww]

Just for info: Vyper has assert_modifiable for this purpose now.