Make 0-valued transfers valid semantics (fire the event and return true)

[nmushegian]

EDIT: Replace "revert" with "returned false" in this issue; I realized StandardToken does not throw on error but then realized the issue is practically the same and still just as harmful

Reverting on 0-valued transfer and approval is an anti-pattern that pops up repeatedly in code reviews. I realized it's because many people are copying your StandardToken.

These two lines should have && _value > 0 removed:

        if (balances[msg.sender] >= _value && _value > 0) {

        if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && _value > 0) {

0-valued transfers and approvals should be valid and behave almost like no-ops, without reverting state. This is so that consumer contracts do not have to take care to work around this edge case.

[simondlr]

Thanks for the issue @nmushegian. I'm trying to think of examples where you would want a no-op and have zero transfers?

Can you elaborate more? Are there specific cases where zero transfers are issued (purposefully or accidentally) and what the issue of reverting is causing?

[nmushegian]

Here's a contrived example:

contract Thing {
   uint end; // end of fee era
   StandardToken coin1, coin2;
   // fee decays linearly to 0, then your app is free
   function fee() constant returns (uint) {
        return now < end ? end - now : 0;
   }
   function something() {
       coin1.transfer(msg.sender, their_money);
       coin2.transfer(dapp_owner, fee());
   }
}

This is a simple case, I'm more wary of transfers where the result is the result of computations the dev thinks shouldn't/can't be 0 but actually can be made to get stuck at 0

Another way to look at it, would you agree thatTransfer(0) event is more likely to represent actual intent than say, Transfer(uint256(-1)) ? Why not block uint256(-1)?

[nmushegian]

OMG I just realized that your tokens don't throw on failure at all

Please ignore this issue!

[nmushegian]

Actually I'm going to reopen this just to discuss the above but specifically for whether the events should still fire edit: and what the return value should be!

I have to say I strongly prefer throw-oriented error handling, but given the choice of return-oriented errors, edit: nvm ~~~the value>0 check could be reasonable, if you explicitly choose not to fire Transfer on 0 and so on~~

EDIT: and also the return value! the return value is the important part

[ethers]

value>0 check could be reasonable, if you explicitly choose not to fire Transfer on 0

The code is currently not firing Transfer on 0...
Maybe rename the title of this issue and we'll see what other comments are made.

[nmushegian]

I'm realizing now that the return value is nearly as critical as the reversion issue. I've updated title/OP for clarity, but now want to discuss Transfer and return.

The code is currently not firing Transfer on 0...

Yes I know, I think this is bad, and I think returning false makes it particularly bad. I think my original point is nearly 100% valid still because I expect the most common pattern people will try to use is assert( transfer(...) ), which should work with 0-valued transfers.

[nmushegian]

Claim: This is very dangerous and is only 1 level of indirection above the token contract; think of all the constraints imposed on all control contracts that might ever want to own this

uint _reward = 1000;
// suppose something can lower the reward
function lowerReward(uint what) auth {
    assert( what < _reward );
    _reward = what;
}
function claimReward() {
    assert( coin.transfer(recipient, amount) && coin.transfer(msg.sender, _reward) ) );
}

EDIT: not the best example because there's no reason to enforce atomicity

[simondlr]

I'm trying to figure out why in some circumstances you are allowing code to get to the point where it is transferring zero. It's a completely useless event.

For example, here:

contract Thing {
   uint end; // end of fee era
   StandardToken coin1, coin2;
   // fee decays linearly to 0, then your app is free
   function fee() constant returns (uint) {
        return now < end ? end - now : 0;
   }
   function something() {
       coin1.transfer(msg.sender, their_money);
       coin2.transfer(dapp_owner, fee());
   }
}

Why not include a check to not transfer zero when the fees get to zero?

Like, I'm trying to figure out whose responsibility it is.

I think the one thing that helps, is that if the standard is that you are allowed to transfer on zero, it just removes the requirements for checks everywhere and complexities of that (eg, who should be checking for zero transfers?).

Will this end up saving gas in the long run if there's no checks, but you do "transfer" on zero?

wrt it being dangerous, it's only dangerous if the patterns above are the norm, which it seemed to have moved towards.

Regarding return vs throw. I'm still in the return camp until revert arrives, since throws have never been good/descriptive errors.

[nmushegian]

The event is not very significant, the semantics of success and failure are, which in this standard is whether it returns true or false. Throw vs return and the event discussion were both tangents I shouldn't have gone on.

who should be checking for zero transfers?

Usually nobody because it's usually intended semantics. And also when higher-level operations on your dapps manipulate tokens internally, arguments which cause one or more internal token transfers to have 0 value are also valid. Having consistent semantics then requires working around token contract edge cases.

Also let me try another approach: would you agree that a call to transfer(uint256(-1)) is LESS likely to be caller's intent than transfer(0) is? Why not block transfer(uint256(-1))?

[dbrock]

I agree with @nmushegian that having these kinds of arbitrary tripwires in a contract is intrinsically dangerous, and not even just a little bit dangerous but "oops we lost all your money" dangerous.

The burden of proof is on whoever wants to have these to explain the rationale for them. I can't see any benefit whatsoever and multiple drawbacks: added gas cost... possibility of funds getting stuck in unknown horrible ways... and maybe most obnoxiously, the fact that now everybody has to add these checks to every contract that uses the tokens as well.

As a general principle, if we can't justify the existence of a line of code, it's our responsibility as programmers to get rid of it.

All else being equal, simple is always better than complicated.

[simondlr]

Usually nobody because it's usually intended semantics.

That I don't get? It literally does nothing. What does it help? Shouldn't the burden be to NOT run code that does nothing?

transfer(uint256(-1))

I don't get the point/argument here?

if we can't justify the existence a line of code

It has a good justification. Why should the user pay gas costs for something that does nothing?

most obnoxiously, the fact that now everybody has to add these checks to every contract that uses the tokens as well

I still don't understand what apps are doing zero transfers? Why is it designed to do this?

I feel like there's context here that's missing?

---

To me it seems the issue can be drawn along the following lines:

• It IS possible to do a zero transfer (accidental or possible), which means that from a contract code perspective, even if it does nothing, it is extra burden to work around this edge case.

• A user could accidentally transfer zero and you are then incurring them gas costs (if the front-end isn't protecting them from doing this).

So, you could have the possibility of incurring extra gas costs for screw-ups that would be cheaper to protect with a simple if (and this is if you believe the smart contract should protect you).

However, for developers there's less burden for them in terms of having to code with work-arounds, and could perhaps even end up being MORE costly due to extra gas cost checks vs the screw-up gas cost checks.

I'm okay with removing this to reduce complexities and shift the burden for protecting the user elsewhere.

...but I still want to know. Why and how are you writing code that does zero transfers? I still don't get that.

[mbrock]

I also agree that it's more parsimonious to have 0-valued transfers be harmless no-ops rather than failures.

It's kind of like how append(x, []) is a harmless no-op rather than something which causes an EmptyAppendException.

From an algebraic perspective, zero being the identity for additive operations is an ancient and fundamental meme; returning an error on "addition by zero" is surprising and unusual.

We don't yet know which contract will be the first to be surprised by this behavior. We've already seen some possible examples by @nmushegian.

For example a system that interacts with any number of external tokens and imposes some kind of configurable fees denominated in tokens. That fee is changed to 0 for some reason, which causes the contract to attempt to transfer 0 * x, which then fails unexpectedly, and now some user's funds are locked.

That kind of problem seems much more serious than the risk of accidentally wasting a user's gas because of a glitchy frontend. Such gas costs are a very limited risk, whereas the 0 fee example is an unbounded risk.

Someone could ask "why are you writing code that appends an empty string?", and maybe the examples I came up with would sound contrived -- but I still don't want the string library to fail in such cases.

[nmushegian]

It IS possible to do a zero transfer (accidental or possible), which means that from a contract code perspective, even if it does nothing, it is extra burden to work around this edge case.

No, it's less work.

0 is a natural number. uint is not a type approximating positive integers, it is a type approximating natural numbers. Without 0 there is no additive identity. The alarm bell is so loud it hurts. I'm honestly out of explanations, I just need to invent an example that is (1) not too "obvious" to be dismissed, (2) something you think is a good design pattern, and (3) accidentally loses all your money.

I'm okay with removing this to reduce complexities and shift the burden for protecting the user elsewhere.

You're not protecting the user from anything except extra gas, which should never feed back to introduce edge cases into your semantics.

edit: nice post @mbrock 👍

[dbrock]

A user could accidentally transfer zero

Which is harmless.

and you are then incurring them gas costs

Which you can't help but do anyway.

(if the front-end isn't protecting them from doing this).

Which it should.

Not only are you still incurring gas costs even in the unlikely case of a user accidentally transferring zero (how could you not? it's a transaction, isn't it? you can never avoid this), but more importantly the check adds unnecessary gas costs to every single normal token transfer that anyone will ever do.

So you're trying (and failing) to optimize for a totally harmless case happening 0.00001% of the time while adding bloat and extra gas cost to the 99.99999% of normal cases (as well as not to mention introducing a potentially totally disastrous edge case).

Again, there is absolutely no justification for this check and multiple drawbacks, including the not insignificant probability of a catastrophic loss of funds at some point in the future.

[ethers]

Perhaps ERC-20 should be more explicit about the behavior of transfer and this discussion temporarily moved there ethereum/EIPs#610

Note https://github.com/ethereum/EIPs/pull/610/files#diff-04cb92b536599f96697d5066f4c1bbb1R32

NOTE: An important point is that callers should handle false from returns (bool success). Callers should not assume that false is never returned!

[nmushegian]

https://s-media-cache-ak0.pinimg.com/736x/fa/18/02/fa18027a74a6f573cf2e1463c1c139b6.jpg

[simondlr]

Great points and well put @mbrock. Agreed.

Thanks for the comments/discussions.

Will change/fix soon.

[simondlr]

Soon (tm). It's coming. ;)