Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERC20 Token Standard #610

Merged
merged 13 commits into from
Sep 11, 2017
Merged

ERC20 Token Standard #610

merged 13 commits into from
Sep 11, 2017

Conversation

frozeman
Copy link
Contributor

The following is moving the ERC 20 token standard from #20 to a PR.

With the small addition of mentioning the attack vector on approve function and suggesting to set to 0 before allowing a new value.

@frozeman frozeman mentioned this pull request Apr 24, 2017
@frozeman frozeman added the ERC label Apr 24, 2017
@Souptacular
Copy link
Contributor

Thank you @frozeman for bringing this to the new EIP format. Those in the community are welcome to look over and comment on this ERC. To be clear, this is NOT a new version of ERC-20. This PR is simply meant to formally define ERC-20 version 1 since there are slight deviations within the community. Once community consensus on version1 is reached, we will mark this ERC as accepted and it will become official with regards to the EIP repo.

@Dexaran
Copy link
Contributor

Dexaran commented Apr 24, 2017

I want to draw attention to the fact that there are two ways to transfer tokens in the ERC20 standard:

  1. transfer
  2. approve + transferFrom

There is no way to handle transfer function calls. As a result, choosing the wrong way to transfer tokens will result in a loss of money.

The most common mistake is sending tokens to token contract itself. At least $16000 are already lost.
Stuck GNT inside Golem contract ~ $5600 lost.
Stuck REP inside Augur contract ~ $1500 lost.
Stuck DGD inside DigixDAO contract ~$7000 lost
Stuck 1ST inside FirstBlood contract ~ $2700 lost

If there is a possibility of error, there will always be users making an error. If you leave the way to transfer tokens to a contract without handling by the recipient, it will lead to the loss of tokens.

I developed ERC #223 , trying to solve this problem.

@frozeman
Copy link
Contributor Author

I do find this idea very useful, but at the same time we also should get the ERC 20 finalised as many tokens already us it. We can then make an ERC 23 for a new version based on ERC 20

@Souptacular
Copy link
Contributor

@frozeman @Dexaran and others.

Should the next version be a whole new ERC# or just ERC20v2? I'm leaning towards versions of ERC20 so there aren't 20 ERC numbers to keep up with.

@SilentCicero
Copy link

Can I also say, we need a balanceOf(address _sender, uint256 _blockNumber) method to be standard. It's extremely useful for voting, transferring and copying tokens.

Initially stated in the MiniMe token, for reference.

@Arachnid
Copy link
Contributor

@Souptacular ERC20 is deployed widely, so backwards-incompatible changes and significant extensions should absolutely be new EIPs. Otherwise, tracking the actual support in a given contract will become near impossible.

Take for example HTTP; different HTTP versions are standardised in different RFCs; you can refer to different RFC numbers, of to a specific version of HTTP as standardised in the relevant RFC.

@Arachnid
Copy link
Contributor

@SilentCicero Changes to ERC20 are out-of-scope for this PR. Also, the method you're asking for is already available offchain from archive clients, who can call any contract at any time in history.

@Arachnid
Copy link
Contributor

name, symbol and decimals are used near-universally in conjunction with ERC20, but I haven't seen them clearly documented anywhere. Would it be appropriate to extend the EIP with a description of these, since they're already de-facto standardised?

@Dexaran
Copy link
Contributor

Dexaran commented Apr 25, 2017

I can suggest to add constant returns functions getName / getSymbol / getDecimals if this variables would be standardized. This can be useful in some cases.
For example to access variables directly from Contracts MEW UI.
Also this will allow to calculate decimal units automatically inside contracts. For example token exchange that needs to trade tokens with different decimals can set a dynamic price using decimals difference coefficient:
uint coefficient = token1.getDecimals() / token2.getDecimals();

@Arachnid
Copy link
Contributor

Yes, if included those properties definitely need to be constant.

@christoph2806
Copy link
Contributor

christoph2806 commented Apr 26, 2017

I'd like to discuss an extension of ERC 20 - hope this is the right place.
As tokens become ubiquitous, there arises the demand to "pay" with tokens. Paying for something means "transfering a token and getting a service in return". This means basically:

  1. Transfer token to some contract
  2. Trigger execution of a function of that contract.

With the current ERC 20 token, this can't be done in one transaction. But for many reasons it would be desirable to trigger the execution of code with a token transfer. For example, paying for an insurance contract with a token and at the same time applying for the insurance policy with the same transaction.

This could be easily accomplished with an extension of the transfer function, giving it an optional third argument (the same applies for the transferFrom function respectivly):
function transfer(address _to, uint256 _value, bytes _data) returns (bool success)
The third parameter _data would then trigger a call(_data) in the referenced receiving contract.
In case the call fails, the transfer is rolled back.
I'd like to discuss such an extension and possible security issues which could be connected with this; everything under the assumption that a token contract is usually something which is audited anyway and therefore can be "trusted".

@Arachnid
Copy link
Contributor

I'd like to discuss an extension of ERC 20 - hope this is the right place.

This is not the correct place. Please comment on #20, open a new issue, or open a PR.

@frozeman
Copy link
Contributor Author

@christoph2806 @Dexaran takes a different approach in its #223 where he wants the token contract to call a tokenFallback fund. I think those a re great ideas but need to be in a new standard and discussed, e.g. in his issue.

@Souptacular
Copy link
Contributor

@Arachnid I was not commenting on re-opening the EIP, but how the wider public should refer to updates to the ERC-20 standard - either as a new ERC # or ERC 20 version x. Are you saying if any change does not support backwards compatibility it should be a whole new token standard (so ERC 23, 24, 118, etc.)?

@Arachnid
Copy link
Contributor

@Souptacular Yes, that's what I'm suggesting. Just like with RFCs, any significant change, including new features, should be a new EIP; tokens can then say they're compliant with EIPs x, y, and z (y and z could say "requires x", in which case it's sufficient to say you're compliant with z.

If we want a more 'stable' name, we should give the broader token standard a name independent of its EIP number, such as "token standard".

Title: <EIP title>
Author: Fabian Vogelsteller <fabian@ethereum.org>, Vitalik Buterin <vitalik.buterin@ethereum.org>
Type: Informational
Category ERC
Copy link
Member

@ethers ethers Apr 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Category" line can be removed


## Abstract

The following standard allows for people to implement a token standard API withing their smart contracts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling: within

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also revise this abstract:

"The following standard allows for the implementation of a standard API for tokens within their smart contracts: it primarily provides basic functionality to transfer tokens and allow them to be approved to be spent by another on-chain third party."


The following standard allows for people to implement a token standard API withing their smart contracts.

This standard provides basic functionality for sending and approving tokens to be spend by a third party.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar: use "spent" instead of spend

- https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/StandardToken.sol
- https://github.com/ConsenSys/Tokens/blob/master/Token_Contracts/contracts/StandardToken.sol

Implentation adding the force 0 before calling approve again:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling: implementation

@ethers
Copy link
Member

ethers commented Apr 29, 2017

How will it be decided whether name, symbol and decimals should be added here?

@Dexaran
Copy link
Contributor

Dexaran commented Apr 29, 2017

I would say it's a good idea to standardize name , symbol and decimals .

@simondlr
Copy link

The original reason that name, decimals & symbol (which is near universal) was not included was that it was considered that these are for mostly for human consumption. We will likely still see tokens that are only really for machine to machine consumption and this won't really be relevant. In other words, no real UI.

With that being said, I think if people want to drop them (say an IoT decive wanting to save gas costs when interacting in an only-m2m market), it's fine since UIs won't exist anyway for these tokens.

So yeah. I'd say, add this to erc20. Additionally, yip, any extensions should be new issues. I don't think we need to necessarily create a new naming scheme for erc20 extensions.

A one I want to add discussion on soon is a standard API for increasing and decreasing supply.

@@ -0,0 +1,129 @@
This is the suggested template for new EIPs.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this preamble also be included?


## Simple Summary

Token standard interface.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe revise a bit? "A standard interface for tokens." ?


## Motivation

Following the same standard interface allows those tokens to be used in many wallets and exchanges.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another potential revision:

"A standard interface allows any tokens on Ethereum to be re-used by other applications: from wallets to decentralized exchanges."

function transfer(address _to, uint256 _value) returns (bool success)
```

Send `_value` amount of tokens to address `_to`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "transfer" vs "Send"?

@3sGgpQ8H
Copy link

3sGgpQ8H commented Aug 24, 2017

@MicahZoltu

You can have balanceOf be constant, and still return the correct result even when the correct result requires state mutations. All you need to do is make it so balanceOf calculates pending state mutations as part of computing its return value, without actually applying those mutations.

This is not possible if correct result depends on current time, because constant functions do not have access to current time.

@3sGgpQ8H
Copy link

3sGgpQ8H commented Aug 24, 2017

@JoshCason

I think having balanceOf marked constant is a bad idea

Obtaining return value of non-constant function is non-trivial, when such function is called directly, i.e. not from other smart contract.

@MicahZoltu
Copy link
Contributor

constant functions do not have access to current time

@mikhail-vladimirov Can you clarify this and/or link to some documentation or code where this is asserted? Constant functions not having access to block.timestamp doesn't make any sense to me and I find it somewhat hard to believe that it is true. Especially since constant right now is not actually enforced in any way.

@cbdotguru
Copy link
Contributor

Just my two cents. Functions should be properly scoped in practice regardless of the current system implementation. balanceOf is clearly not meant to be a state changing function. It should be on the developer to configure workflow with balanceOf as a constant function. If that's not the case then that should be changed, not the standard.

@3sGgpQ8H
Copy link

3sGgpQ8H commented Aug 24, 2017

@MicahZoltu

Constant functions not having access to block.timestamp doesn't make any sense to me and I find it somewhat hard to believe that it is true

I didn't say they do not have access to block.timestamp, they actually do. I said they do not have access to current time when called directly. It is not specified what value block.timestamp should have when constant function is being executed locally, so you may not rely on any particular behavior. Different implementations may (and actually do) behave differently.

@MicahZoltu
Copy link
Contributor

Hmm, interesting. That sounds like a bug or something that should be well defined.

@d10r
Copy link

d10r commented Sep 5, 2017

It is not specified what value block.timestamp should have when constant function is being executed locally, so you may not rely on any particular behavior.

In the Solidity docs I couldn't find anything I would interpret that way. It just mentions current block - which in the context of a locally executed function I would interpret as the newest block my node has seen.
On EVM level according to my understanding (after taking a look at the Yellow Paper, Appendix H) there's no notion of local (or not) execution.

@mikhail-vladimirov is your statement based on other sources of information or on a different interpretation of that docs? Am I right assuming that you mean different implementations of the Solidity compiler?

@MicahZoltu
Copy link
Contributor

@d10r I believe the issue is that when you do eth_call, it is undefined what value the node that executes the contract will use for block.timestamp. For example, Geth may provide 0 while Parity may provide <now>.

@3sGgpQ8H
Copy link

3sGgpQ8H commented Sep 6, 2017

@d10r There are no problems with Solidity, it just translates block.timestamp into TIMESTAMP opcode. The problem is that this opcode is supposed to return timestamp of the block current transaction is being mined within, but when constant functions is called locally, it is not mined at all, so there is no current transaction nor current block. Using things like block.coinbase, block.difficulty, block.gaslimit, block.number, block.timestamp, tx.gasprice, or tx.origin in constant functions is not a good idea, because it is unspecified what EVM will return, and different implementations, or even different versions of the same implementation, may behave differently. Check this old thread: https://forum.ethereum.org/discussion/2452/block-timestamp-a-k-a-now-in-constant-functions for details.

Modern EVMs seem to substitute timestamp of last block as block.timestamp for constant functions being executed locally, but last block != current block, and timestamp of last block != current time, though for some applications this could be good enough estimate. In my contracts, when I need constant functions to access current time, I always pass current time to the function as an argument.

@Arachnid
Copy link
Contributor

Arachnid commented Sep 6, 2017

I think this is a red herring and besides the point; removing constant is a nonstarter for several reasons:

  1. It breaks the existing standard, which this PR seeks to formalise.
  2. There's no practical way at present to retrieve the return value of a transaction.
  3. Expecting people to submit a transaction and wait for it to be mined in order to retrieve their balance is crazy.

Is there anything holding up this PR being merged?


Returns the symbol of the token. E.g. "HIX".

OPTIONAL - This method can be used to improve useability,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useability> usability

Transfers `_value` amount of tokens to address `_to`, and MUST fire the `Transfer` event.
The function SHOULD `throw` if the `_from` account balance does not have enough tokens to spend.

A token contract which creates new tokens SHOULD trigger a Transfer event with the `_from` address set to `0x0` when tokens are created.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a recommendation to throw if the sender value is 0, as it's most likely an error. If the token wants to have a explicit burn function it should implement so or it should recommend users to send it to any other invalid address.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fine having explicit burn and mint functions but they should call Transfer events as defined in the standard and monitored on etherscan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anybody any thoughts on that?
I really want to merge that ASAP

Copy link
Contributor

@veox veox Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frozeman The token implementations I've seen so far (e.g. OpenZeppelin's, but also many others) seem to make no check that msg.sender is not 0x00..00, or 0xff..ff, or some other magic number.

Personally, I'd leave this out of the standard's text, instead of adding such provisions retroactively, since it was never widely discussed.

EDIT: That is, instead of adding another possibly contentious point that will keep discussion running ad infinitum, just leave it undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it is contentious because it's already there and block scanners use it. Whether individual dev teams like it or not, the block scanners need to find the tokens, they find the tokens through transfer events.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's changing what the spec requires - eg, introducing backwards incompatibilities or new features - and there's adding advice to implementers, or reflecting defacto standardised behaviour. I think recommending a transfer event on minting falls into the latter category rather than the former.

Copy link
Contributor

@MicahZoltu MicahZoltu Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Arachnid See my argument here: #610 (comment)
TL;DR: Without this being a MUST (which it can't be in this PR), tooling can't leverage it meaningfully without creating bugs elsewhere.

I believe etherscan tries to interpret transfer from 0 as mint and it results in some tokens having incorrect token allocation charts which leads to significant user confusion. As a SHOULD, I think it encourages other applications to do what etherscan has done (they would be operating under the assumption that most tokens will follow the SHOULD) when in fact I'm unconvinced that most do. So we end up with people building applications/interfaces that are wrong some non-trivial portion of the time with no clear mechanism for resolving that. If this were left out, the theory is that interfaces would stop trying to build against something that isn't widely adopted and therefore they would be less likely to build features that are permanently in a half broken state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MicahZoltu has the important point which is clarity, defining the standard, and closing. Maybe even just you three @frozeman @Arachnid and @MicahZoltu since you are the most reputable with investment in this discussion, decide and close. I say we all agree now to go with that decision rather than try to agree on the decision itself. Anyone that disagrees should present a valid argument against deciding and closing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just about the least reputable person I know. I recommend not including me in any list of "reputable persons". 😄

I'm 👍 on merging this with or without the SHOULD. I just wanted to make sure my arguments against it were clear and well understood before those that make the merge decisions do so. If @Arachnid / @frozeman (or whoever make the final merge decision) believe they understand my arguments but still disagree with them then consider the issue resolved (unless further discussion/clarification is desired, in which case I'll happily oblige).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR, at the initial submission of #20, the creation (minting) of tokens was not taken into consideration. The proposal aimed to standartise an interface for transfer of tokens.

In retrospect, I'd say considerations for creation of tokens should have been kept out of this standard proposal, seeing how it has caused a lot of uncertainty.

My comment above:

EDIT: That is, instead of adding another possibly contentious point that will keep discussion running ad infinitum, just leave it undefined.

... was meant to highlight this fact: creation of tokens shouldn't have been within scope of this standard's text.

It's still not too late to leave a Minted event outside the scope of this particular standard.


Allows `_spender` to withdraw from your account multiple times, up to the `_value` amount. If this function is called again it overwrites the current allowance with `_value`.

**NOTE**: To prevent attack vectors like the one [described here](https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/) and discussed [here](https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a pity that approve sets the approval amount and doesn't add or remove to it, it was implemented without much thought but now became a standard already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change that in another standard, but this one needs to be formalised

@frozeman
Copy link
Contributor Author

Im fine with the current standard and suggest merging.

@Souptacular
Copy link
Contributor

Going to merge this. Thank you all for the thoughtful discussion. Now we move on to working on an updated, better token standard now that this is formalized 💸 🤓

@Georgi87
Copy link

Regarding name, symbol and decimals: According to the style guide all constants should be capitalized (http://solidity.readthedocs.io/en/latest/style-guide.html#constants). Should they be renamed to NAME, SYMBOL and DECIMALS?

@jamesray1
Copy link
Contributor

jamesray1 commented Nov 11, 2017

@Georgi87, name, symbol and decimals are functions as well as input variables to the eponymous function (however, the latter input variables are just placeholders for the actual input variables) that returns a constant, respectively: a name of type string, a symbol of type string, and decimals of type uint8. The convention in programming languages for variables and function names is that the first word in the variable is lower case (separating words can use an underscore e.g. this_variable or Camel case, e.g. thisVariable; variables can also have numbers, but not as the first character). Constant variables use all caps, e.g. ACCELERATION_DUE_TO_GRAVITY_ON_EARTH = 9.8. So, the function names should be kept as they are (name, symbol and decimals). The input variables could be changed to NAME, SYMBOL and DECIMALS and I guess that that they ought to be changed to comply with convention and provide clarity.

@veox
Copy link
Contributor

veox commented Nov 11, 2017

Personally, I use ALL_CAPS for compile-time constants.

Also, although name/symbol/decimals are unlikely to change during the life cycle of a contract, they can be deployment-time parameters, e.g. when used with a token Factory.

@frozeman frozeman deleted the erc20 branch January 12, 2018 08:01
@bitcoinwarrior1
Copy link
Contributor

@frozeman and all.

I suggest adding this optional function because it will allow p2p atomic trading with low fees, it is already implemented in this standard: #875.

function trade(uint256 expiryTimeStamp, uint amount, uint8 v, bytes32 r, bytes32 s) public payable

A function which allows a user to sell a token without paying for the gas fee (only the buyer has to) with a p2p atomic swap. This is achieved by signing an attestation containing the token to sell, the contract address, an expiry timestamp, the price and a prefix containing the ERC spec name and chain id. A buyer can then pay for the deal in one transaction by attaching the appropriate ether to satisfy the deal.

This design is also more efficient as it allows orders to be done offline until settlement as opposed to creating orders in a smart contract and updating them. The expiry timestamp protects the seller against people using old orders.

This opens up the gates for a p2p atomic swap but should be optional to this standard as some may not have use for it.

Some protections need to be added to the message such as encoding the chain id, contract address and the ERC spec name to prevent replays and spoofing people into signing message that allow a trade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.