How to assert transfer() and transferFrom() are successful for all implementations

[haydenadams]

There are several ways I have seen contracts call transfer() and transferFrom() and they all have issues.

1)

token: address(ERC20)
total_deposits: uint256

def deposit(amount: uint256):
    total_deposits += amount
    self.token.transferFrom(msg.sender, self, amount)

Issue: Although most tokens return an exception if the transaction fails, BAT and several other tokens return false. For these tokens, total_deposits can be increased without tokens ever being transferred.

2)

token: address(ERC20)
total_deposits: uint256

def deposit(amount: uint256):
    total_deposits += amount
    assert self.token.transferFrom(msg.sender, self, amount) == True

Issue: Safer for most ERC20s, but over 130 existing ERC20s including OMG do not return bools. These tokens can never be deposited, since all function calls will fail.

3) assert self.token.transfer(msg.sender, amount) != False

See above. I have a new solution I've been thinking about:

4)

token: address(ERC20)
total_deposits: uint256

def deposit(amount: uint256):
    total_deposits += amount
    balance: uint256 = self.token.balanceOf(self)
    self.token.transferFrom(msg.sender, self, amount)
    assert self.token.balanceOf(self) == balance + amount

This seems to guarantee desired behavior under all implementations differences. Unfortunately, it adds ~4000 gas. Are there any other catch-all methods?

[fubuloubu]

So, you are replicating the assertions that the token may not be implementating as part of a proper implemetation? This is probably one of the only ways to do this, so I don't see an opportunity for improvement.

The better solution is many of these tokens should investigate moving to newer standards and/or better implemetations. We definitely need a more comprehensive standard than ERC20 that accommodates various issues working with tokens. We especially need to stop using the older paradigms of returning false on a revert, at least for interface suggestions. How do we upgrade?

These are all philosophical opinions of course. Not sure there is a suggestion here to add to the Vyper language.

[haydenadams]

I don't think its an issue with the ERC20 standard. If every token returned a boolean value, it would be as simple as:

assert self.token.transferFrom(msg.sender, self, amount) == True

And politically I agree that tokens that did not follow the standard should create a simple wrapping contract which could become the "real" token, or get left out of decentralized exchanges.

As someone currently building an automated market maker its frustrating because I know they prob wont and I have to make the choice between increasing the gas cost for all other tokens, not supporting certain tokens there is demand for, or building multiple versions of my smart contracts and figuring out a way for them to trade between each other. But yeah I agree not a ton that can be done on your end, was just checking if anyone had any ideas or alternate solutions.

[jacqueswww]

@haydenadams can I close this issue now? I don't think there is anything that can be changed from the vyper side?

[haydenadams]

Don't think theres anything to change. BNB, OMG, and others should reissue their tokens.

Is something like this possible in Vyper? Not that I support it lol.

https://github.com/sec-bit/badERC20Fix#solution-for-dapp-and-dex

[fubuloubu]

Lol, just say no to bad tokens.

Maybe someone should make WOMG (Wrapped OmiseGO) that is a real implementation that isn't crap, or issue a new token (using Vyper!) and port over balances (a la Augur)