Approve increments the allowance instead of setting it

[jeisses]

Hi all,

Thanks for providing this template, it's very helpful.

I noticed in do_approve, that the new approved value is added to the old approved value. I think that do_approve should set the value instead of incrementing it. NEP-5 states:

Remarks: "approve" will approve the to account to transfer amount tokens from the originator acount. 

Also in ERC20 it states that calling approve overwrites the value. Am I correct that this should be the behavior?

[nickfujita]

I am also curious about this. While running through do_approve, it seems that it also allows for negative amounts to be sent it. While this could be useful to adjusting the approval amount, it also allows for amounts to be set to negative. Reviewing it's usage in do_transfer_from, a negative amount stored by do_approve would not cause any problems since the correct checks are in place. However, it seems like it would be a waste to have a bunch of negative amounts being stored by do_approve.

So in addition to the suggestion above to set the value, could we also add a pre-check to ensure that the amount requested is greater than zero?

Is there possibly a use-case where incremental additions and subtractions need to be made to these approved amounts? If so, I kind of understand wanted this functionality (minus allowing for negative amounts to be stored). The reason would be that constantly updating the approve amount would require two calls to the contract instead of a one. First a call to do_allowance to check the current approve amount in the contract and calculate the new amount, and a second to do_approve to update the amount stored in the contract.

[brianlenz]

To add to what @nickfujita said, since do_approve is additive, the following check isn't all that useful:

if from_balance >= amount:

Say I have 100 NEP-5 tokens in my wallet. I could easily get around this check by doing multiple approvals of 100 tokens, effectively making this check meaningless.

Additionally, a common use case would be to clear an allowance (similar to the last scenario @nickfujita mentioned). If you want to clear an allowance, you have to first check the current balance, then do an approval of the negative version. This definitely all seems quite upside down. If an approve always does a replace, then you can just set it to 0 and be done.

I've created a PR with a proposal to address these issues, which should make it consistent with ERC20 standards.

[shargon]

This issue could be closed @localhuman

[localhuman]

closed via https://github.com/neonexchange/neo-ico-template/pull/14