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

ERC: Token standard #20

Closed
frozeman opened this Issue Nov 19, 2015 · 362 comments

Comments

Projects
None yet
@frozeman
Member

frozeman commented Nov 19, 2015

The final standard can be found here: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md


ERC: 20
Title: Token standard
Status: Draft
Type: Informational
Created: 19-11.2015
Resolution: https://github.com/ethereum/wiki/wiki/Standardized_Contract_APIs

Abstract

The following describes standard functions a token contract can implement.

Motivation

Those will allow dapps and wallets to handle tokens across multiple interfaces/dapps.

The most important here are, transfer, balanceOf and the Transfer event.

Specification

Token

Methods

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

totalSupply

function totalSupply() constant returns (uint256 totalSupply)

Get the total token supply

balanceOf

function balanceOf(address _owner) constant returns (uint256 balance)

Get the account balance of another account with address _owner

transfer

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

Send _value amount of tokens to address _to

transferFrom

function transferFrom(address _from, address _to, uint256 _value) returns (bool success)

Send _value amount of tokens from address _from to address _to

The transferFrom method is used for a withdraw workflow, allowing contracts to send tokens on your behalf, for example to "deposit" to a contract address and/or to charge fees in sub-currencies; the command should fail unless the _from account has deliberately authorized the sender of the message via some mechanism; we propose these standardized APIs for approval:

approve

function approve(address _spender, uint256 _value) returns (bool success)

Allow _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.

allowance

function allowance(address _owner, address _spender) constant returns (uint256 remaining)

Returns the amount which _spender is still allowed to withdraw from _owner

Events

Transfer

event Transfer(address indexed _from, address indexed _to, uint256 _value)

Triggered when tokens are transferred.

Approval

event Approval(address indexed _owner, address indexed _spender, uint256 _value)

Triggered whenever approve(address _spender, uint256 _value) is called.

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

recent discussion from https://gist.github.com/frozeman/090ae32041bcfe120824

@vbuterin said:
Yeah, createCheque and cashCheque as above, plus transferCheque(address _from, address _to, uint256 _value) sounds good. In that case, we should probably remove the _to argument from cashCheque; generally, you can only cash cheques from your own bank account.

We probably also want getChequeValue(address _from, address _for). We then have a choice of whether we want to keep the value argument in cashCheque rather than simply only allowing cashing in 100% of whatever is in there. If we want to fully follow the cheque analogy, this triad seems most intuitive to me:

function createCheque(address _for, uint256 _maxValue)
function cashCheque(address _from)
function getChequeValue(address _from, address _for)

Question: does running createCheque twice add the value of the two cheques together? Are there legitimate use cases for creating a cheque multiple times and then cashing either once or multiple times?

@nmushegian said:
All the functions that return uint should return (uint, bool) instead. You can easily make up scenarios where a 0 return value is ambiguous and significant. Is there any other simpler pattern for handling this?

@niran said:
I think the value parameter is useful in cashCheque. It absolves callers from having to verify that the amount they needed was provided, and from having to refund amounts greater than what was needed. cashCheque would only succeed if the provided value was remaining in the cheque.
Also, I think using createCheque(2**100) for the approve use case is going to lead to less clear code. It gets better if you make the magic number a constant, like createCheque(UNLIMITED_CHEQUE_VALUE), but lots of people won't do that. I think it's worth having a createBlankCheque or something for the approve scenario. Most tokens will use the TokenLib to handle all of the cheque logic, so it doesn't really make things worse for token authors.

@caktux
I also think there is a problem with the terminology of cheques since they imply one-offs. Cheques are also unique, and here cheques wouldn't return unique IDs or anything; those are merely approval methods for transfers using internal bookkeeping. I think the current approve/transfer terminology is accurate and simple enough, instead of ending up with a mix of transfer and cashCheque. Would we change unapprove to tearCheque? There's also that ambiguity of cheques adding up, where approvals more clearly override a previous one.

In the use case described by Vitalik of contracts charging fees in subcurrencies, it could easily cost more to have to call approveOnce each time (if we replace the current approve method with it) than the actual fee in subcurrency. For that reason I think we should keep both approve and approveOnce, but we could add the _maxValue argument to the former, that way subscriptions or fees could be taken in multiple calls but only up to a certain amount. Another reason to keep the approval terminology, as I think it's much simpler to describe approve and approveOnce than some createMultiCheque and createCheque. Regarding isApprovedFor, it would have to return the approved amount if we do add _maxValue, just as isApprovedOnceFor does.

Member

frozeman commented Nov 19, 2015

recent discussion from https://gist.github.com/frozeman/090ae32041bcfe120824

@vbuterin said:
Yeah, createCheque and cashCheque as above, plus transferCheque(address _from, address _to, uint256 _value) sounds good. In that case, we should probably remove the _to argument from cashCheque; generally, you can only cash cheques from your own bank account.

We probably also want getChequeValue(address _from, address _for). We then have a choice of whether we want to keep the value argument in cashCheque rather than simply only allowing cashing in 100% of whatever is in there. If we want to fully follow the cheque analogy, this triad seems most intuitive to me:

function createCheque(address _for, uint256 _maxValue)
function cashCheque(address _from)
function getChequeValue(address _from, address _for)

Question: does running createCheque twice add the value of the two cheques together? Are there legitimate use cases for creating a cheque multiple times and then cashing either once or multiple times?

@nmushegian said:
All the functions that return uint should return (uint, bool) instead. You can easily make up scenarios where a 0 return value is ambiguous and significant. Is there any other simpler pattern for handling this?

@niran said:
I think the value parameter is useful in cashCheque. It absolves callers from having to verify that the amount they needed was provided, and from having to refund amounts greater than what was needed. cashCheque would only succeed if the provided value was remaining in the cheque.
Also, I think using createCheque(2**100) for the approve use case is going to lead to less clear code. It gets better if you make the magic number a constant, like createCheque(UNLIMITED_CHEQUE_VALUE), but lots of people won't do that. I think it's worth having a createBlankCheque or something for the approve scenario. Most tokens will use the TokenLib to handle all of the cheque logic, so it doesn't really make things worse for token authors.

@caktux
I also think there is a problem with the terminology of cheques since they imply one-offs. Cheques are also unique, and here cheques wouldn't return unique IDs or anything; those are merely approval methods for transfers using internal bookkeeping. I think the current approve/transfer terminology is accurate and simple enough, instead of ending up with a mix of transfer and cashCheque. Would we change unapprove to tearCheque? There's also that ambiguity of cheques adding up, where approvals more clearly override a previous one.

In the use case described by Vitalik of contracts charging fees in subcurrencies, it could easily cost more to have to call approveOnce each time (if we replace the current approve method with it) than the actual fee in subcurrency. For that reason I think we should keep both approve and approveOnce, but we could add the _maxValue argument to the former, that way subscriptions or fees could be taken in multiple calls but only up to a certain amount. Another reason to keep the approval terminology, as I think it's much simpler to describe approve and approveOnce than some createMultiCheque and createCheque. Regarding isApprovedFor, it would have to return the approved amount if we do add _maxValue, just as isApprovedOnceFor does.

@ethers

This comment has been minimized.

Show comment
Hide comment
@ethers

ethers Nov 19, 2015

Member

decimals() doesn't seem needed. The Token itself represents an indivisible unit. A higher-level, like SubCurrency, should use Token, SubCurrency is where decimals() or other things like symbol() could be implemented.

Member

ethers commented Nov 19, 2015

decimals() doesn't seem needed. The Token itself represents an indivisible unit. A higher-level, like SubCurrency, should use Token, SubCurrency is where decimals() or other things like symbol() could be implemented.

@ethers

This comment has been minimized.

Show comment
Hide comment
@ethers

ethers Nov 19, 2015

Member

In all method descriptions, let's also remove "coin", eg "Get the total coin supply" -> "Get the total token supply"

Member

ethers commented Nov 19, 2015

In all method descriptions, let's also remove "coin", eg "Get the total coin supply" -> "Get the total token supply"

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

I disagree, as the token is the currency or whatever and to represent it properly in any kind of dapp you need to know what is the proper way to represent that token. Ideally the user has to add only one contract address and the dapp can derive everything from there. Otherwise you make every dapp implement the low level token, plus some high level currency contract API. And not knowing the decimal points can be dangerous, otherwise one user sends 100.00 and another 100 (equals 1.00)

Member

frozeman commented Nov 19, 2015

I disagree, as the token is the currency or whatever and to represent it properly in any kind of dapp you need to know what is the proper way to represent that token. Ideally the user has to add only one contract address and the dapp can derive everything from there. Otherwise you make every dapp implement the low level token, plus some high level currency contract API. And not knowing the decimal points can be dangerous, otherwise one user sends 100.00 and another 100 (equals 1.00)

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 19, 2015

I'm neither here not there about the terminology. I think "approve" OR "cheque" terminology is good enough.

At the end of the day, it seems we need both blanket approvals & once-offs. Or rather, it seems it would be useful to specify 3 things: 1) Total value that can be withdrawn, 2) How many times they can do that, & 3) How much at a time.

Spitballing another option:

Just one method, called approve (or setCustodian) that takes 2 parameters. How many times they are allowed to withdraw & how much each time?

approve(address _for, uint256 _withdraws, uint256 _max)

?

@frozeman Regarding names & other information. I agree with @ethers here. There will be tokens minted that don't have any requirement for names, symbols or decimals. Like prediction market outcomes or energy meter kwh tokens for example. This should not be at the token layer. All tokens are not automatically a sub-currency or coin (that uses additional information).

simondlr commented Nov 19, 2015

I'm neither here not there about the terminology. I think "approve" OR "cheque" terminology is good enough.

At the end of the day, it seems we need both blanket approvals & once-offs. Or rather, it seems it would be useful to specify 3 things: 1) Total value that can be withdrawn, 2) How many times they can do that, & 3) How much at a time.

Spitballing another option:

Just one method, called approve (or setCustodian) that takes 2 parameters. How many times they are allowed to withdraw & how much each time?

approve(address _for, uint256 _withdraws, uint256 _max)

?

@frozeman Regarding names & other information. I agree with @ethers here. There will be tokens minted that don't have any requirement for names, symbols or decimals. Like prediction market outcomes or energy meter kwh tokens for example. This should not be at the token layer. All tokens are not automatically a sub-currency or coin (that uses additional information).

@ethers

This comment has been minimized.

Show comment
Hide comment
Member

ethers commented Nov 19, 2015

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

Created it here #22

Member

frozeman commented Nov 19, 2015

Created it here #22

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

@simondlr @ethers i think divisibility belongs to the token contract, as even non currency token can need that. Im fine with putting the name and symbol in the registry tho.

Member

frozeman commented Nov 19, 2015

@simondlr @ethers i think divisibility belongs to the token contract, as even non currency token can need that. Im fine with putting the name and symbol in the registry tho.

@ethers

This comment has been minimized.

Show comment
Hide comment
@ethers

ethers Nov 19, 2015

Member

approve(address _address, uint256 _withdraws, uint256 _max) seems quite clean (suggested by @simondlr).
May tweak it as approve(address _address, uint256 _withdrawals, uint256 _maxValue)

(isApprovedFor then checks if there's at least 1 withdrawal approved for the amount)

EDIT: add address

Member

ethers commented Nov 19, 2015

approve(address _address, uint256 _withdraws, uint256 _max) seems quite clean (suggested by @simondlr).
May tweak it as approve(address _address, uint256 _withdrawals, uint256 _maxValue)

(isApprovedFor then checks if there's at least 1 withdrawal approved for the amount)

EDIT: add address

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

You mean function approve(address _address, uint256 _withdraws, uint256 _max)

Member

frozeman commented Nov 19, 2015

You mean function approve(address _address, uint256 _withdraws, uint256 _max)

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 19, 2015

@frozeman wrt decimals. Neither really here nor there about it. You can have multiple thresholds as is the case with Ether. The base token is still wei at the end of the day. You can't have decimal wei. You can write the wei in Ether, Finney, Shannon, etc. Each with their own decimal position. Should multiple thresholds be specified? Or only one? If so, is the token name specified to the base unit, or where the decimal point starts? ie Ether or Wei? It's a naming/usability convention, not a specification on how the token actually works.

Personally, for most new tokens that will be created, it doesn't really make sense to have them anymore. I do however see scenarios where it can be used. Thus, I'm neutral on this point. Don't mind. Perhaps it should be optional.

simondlr commented Nov 19, 2015

@frozeman wrt decimals. Neither really here nor there about it. You can have multiple thresholds as is the case with Ether. The base token is still wei at the end of the day. You can't have decimal wei. You can write the wei in Ether, Finney, Shannon, etc. Each with their own decimal position. Should multiple thresholds be specified? Or only one? If so, is the token name specified to the base unit, or where the decimal point starts? ie Ether or Wei? It's a naming/usability convention, not a specification on how the token actually works.

Personally, for most new tokens that will be created, it doesn't really make sense to have them anymore. I do however see scenarios where it can be used. Thus, I'm neutral on this point. Don't mind. Perhaps it should be optional.

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

I should be optional sure, but for tokens which will be used in the wallet or user facing stuff, its quite necessary, except you don't want them to be divisible.

Concerning multiple steps, i think only one basic step is necessary from there on you can go in /100steps as usual, thats not hard to guess.

But it surely makes a difference if a user accidentally sends 10000 vs 100.00 token units ;)

Member

frozeman commented Nov 19, 2015

I should be optional sure, but for tokens which will be used in the wallet or user facing stuff, its quite necessary, except you don't want them to be divisible.

Concerning multiple steps, i think only one basic step is necessary from there on you can go in /100steps as usual, thats not hard to guess.

But it surely makes a difference if a user accidentally sends 10000 vs 100.00 token units ;)

@ethers

This comment has been minimized.

Show comment
Hide comment
@ethers

ethers Nov 19, 2015

Member

No one wants many informational EIPs, but from a design perspective is it better to consider EIPs as composable and keep them modular and lean?

For example, an informational EIP for Wallet Tokens could be composed of EIP20, EIP22, EIPX?

Let's not forget https://gist.github.com/frozeman/090ae32041bcfe120824#gistcomment-1623513
As there's also been some discussion about the approve functionality, maybe approve APIs should also be its own EIP?

Member

ethers commented Nov 19, 2015

No one wants many informational EIPs, but from a design perspective is it better to consider EIPs as composable and keep them modular and lean?

For example, an informational EIP for Wallet Tokens could be composed of EIP20, EIP22, EIPX?

Let's not forget https://gist.github.com/frozeman/090ae32041bcfe120824#gistcomment-1623513
As there's also been some discussion about the approve functionality, maybe approve APIs should also be its own EIP?

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

I think its best, when somebody makes a change proposal he list nicely formatted exactly what he would change. In the same as above in the first comment, then these changes are easier to understand, and later move into the actual proposal. Simply:

function myFunc(address _address, ...) returns (bool)

etc.

Member

frozeman commented Nov 19, 2015

I think its best, when somebody makes a change proposal he list nicely formatted exactly what he would change. In the same as above in the first comment, then these changes are easier to understand, and later move into the actual proposal. Simply:

function myFunc(address _address, ...) returns (bool)

etc.

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 19, 2015

Agreed @frozeman. wrt to this method.

function approve(address _address, uint256 _withdraws, uint256 _max) returns (bool success)

It's still slightly clunky as opposed to true or false (as you mentioned previously @niran). ie having to specify 2**100. We could add a helper function that replicates this internally? ie

function approveAll(address _address) returns (bool success)

it internally calls approve(_address, 2**100, 2**100)

Any other comments from others on this method?

simondlr commented Nov 19, 2015

Agreed @frozeman. wrt to this method.

function approve(address _address, uint256 _withdraws, uint256 _max) returns (bool success)

It's still slightly clunky as opposed to true or false (as you mentioned previously @niran). ie having to specify 2**100. We could add a helper function that replicates this internally? ie

function approveAll(address _address) returns (bool success)

it internally calls approve(_address, 2**100, 2**100)

Any other comments from others on this method?

@niran

This comment has been minimized.

Show comment
Hide comment
@niran

niran Nov 19, 2015

Is there a use case for specifying the number of withdrawals? If we're sticking with the approval language, I think the function signatures we started with were fine. You're always granting access to a certain amount of funds, and I don't see a case for caring about the increments those funds are withdrawn in.

Display information about tokens, like where to put the decimal point when displaying the native integer value, belongs in a registry. Changing a token contract is hard. Changing a registry entry is easy. decimals wouldn't change how the token contract works, nor how any calling contract would work. It's not necessary, and the UI problem will still be solved.

niran commented Nov 19, 2015

Is there a use case for specifying the number of withdrawals? If we're sticking with the approval language, I think the function signatures we started with were fine. You're always granting access to a certain amount of funds, and I don't see a case for caring about the increments those funds are withdrawn in.

Display information about tokens, like where to put the decimal point when displaying the native integer value, belongs in a registry. Changing a token contract is hard. Changing a registry entry is easy. decimals wouldn't change how the token contract works, nor how any calling contract would work. It's not necessary, and the UI problem will still be solved.

@alexvandesande

This comment has been minimized.

Show comment
Hide comment
@alexvandesande

alexvandesande Nov 19, 2015

Contributor

@ethers decimals, name and symbol are important for displaying to the end user. If Ether was a token then the name would be "ether" with 8 decimal cases, and internally everything would be wei. Just like we do currently.

Regarding the approve/cheque discussion, I feel that we should always use focus on paving cow paths: implement what everyone is on absolute consensus as the basic "standard" and then allow real world usage dictate how to better define more advanced use cases.

Contributor

alexvandesande commented Nov 19, 2015

@ethers decimals, name and symbol are important for displaying to the end user. If Ether was a token then the name would be "ether" with 8 decimal cases, and internally everything would be wei. Just like we do currently.

Regarding the approve/cheque discussion, I feel that we should always use focus on paving cow paths: implement what everyone is on absolute consensus as the basic "standard" and then allow real world usage dictate how to better define more advanced use cases.

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 19, 2015

@niran true... If you have a subscription, then you someone can anyway withdraw all 12 months in 12 transactions (for example). A future layer could perhaps limit based on timestamps when new withdraws can be made. But let's leave that for later.

I agree with @alexvandesande. Let's keep it simple.

So we thus we only have 1 approve function?

function approve(address _for, uint256 _value) returns (bool success)

address _for can withdraw any amount up to _value. If approve is called again, it is not reset, it is simply added to that amount.

The transfer is difficult in this scenario, since it doesn't follow the cheque tango. Would it simple mean transferring ALL outstanding value to another custodian?

Perhaps we should leave that out for now & "pave the cowpaths" (love this saying). Find the "desire paths". :)


I understand why decimals are useful. It just won't matter in a substantial subset of tokens (you won't see them in wallet really). Thus it should be optional.

simondlr commented Nov 19, 2015

@niran true... If you have a subscription, then you someone can anyway withdraw all 12 months in 12 transactions (for example). A future layer could perhaps limit based on timestamps when new withdraws can be made. But let's leave that for later.

I agree with @alexvandesande. Let's keep it simple.

So we thus we only have 1 approve function?

function approve(address _for, uint256 _value) returns (bool success)

address _for can withdraw any amount up to _value. If approve is called again, it is not reset, it is simply added to that amount.

The transfer is difficult in this scenario, since it doesn't follow the cheque tango. Would it simple mean transferring ALL outstanding value to another custodian?

Perhaps we should leave that out for now & "pave the cowpaths" (love this saying). Find the "desire paths". :)


I understand why decimals are useful. It just won't matter in a substantial subset of tokens (you won't see them in wallet really). Thus it should be optional.

@aakilfernandes

This comment has been minimized.

Show comment
Hide comment
@aakilfernandes

aakilfernandes Nov 19, 2015

Just struck me, it would be great to have some kind of standardized system to pay miners with tokens.

You could imagine an user who holds a wallet of various coins, but doesn't know what Ether is. Rather than having to purchase Ether to make a transfer, there could be a _sendToMiner option in each function which pays the miner a fee in that token.

aakilfernandes commented Nov 19, 2015

Just struck me, it would be great to have some kind of standardized system to pay miners with tokens.

You could imagine an user who holds a wallet of various coins, but doesn't know what Ether is. Rather than having to purchase Ether to make a transfer, there could be a _sendToMiner option in each function which pays the miner a fee in that token.

@nmushegian

This comment has been minimized.

Show comment
Hide comment
@nmushegian

nmushegian Nov 19, 2015

@simondlr I like this minimal viable approval function and also agree we should see what people build with it before making too broad a standard API

nmushegian commented Nov 19, 2015

@simondlr I like this minimal viable approval function and also agree we should see what people build with it before making too broad a standard API

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 19, 2015

I think tokens created from the wallets (like what @frozeman demo-ed at Devcon) can include decimals. Because it will be needed in that context (for example), which "paves the cowpaths". :)

simondlr commented Nov 19, 2015

I think tokens created from the wallets (like what @frozeman demo-ed at Devcon) can include decimals. Because it will be needed in that context (for example), which "paves the cowpaths". :)

@ethers

This comment has been minimized.

Show comment
Hide comment
@ethers

ethers Nov 19, 2015

Member

decimals, name and symbol are important for displaying to the end user

Agree. Are we going to add them to EIP20, or leave them in EIP22?

Member

ethers commented Nov 19, 2015

decimals, name and symbol are important for displaying to the end user

Agree. Are we going to add them to EIP20, or leave them in EIP22?

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

I would add them to 20, but mark as optional

Member

frozeman commented Nov 19, 2015

I would add them to 20, but mark as optional

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 19, 2015

Member

@simondlr i add your suggestion and also changed the order and names for isApprovedFor so it matches the function name:

function isApprovedFor(address _allowed, address _for) constant returns (bool success)

e.g. _allowed isApprovedFor _for

And this should probably return the still allowed value, e.g. if you already used up a part.
We should then probably rename this one too

Member

frozeman commented Nov 19, 2015

@simondlr i add your suggestion and also changed the order and names for isApprovedFor so it matches the function name:

function isApprovedFor(address _allowed, address _for) constant returns (bool success)

e.g. _allowed isApprovedFor _for

And this should probably return the still allowed value, e.g. if you already used up a part.
We should then probably rename this one too

@nmushegian

This comment has been minimized.

Show comment
Hide comment
@nmushegian

nmushegian Nov 19, 2015

^^ it's still ambiguous:

_allowed ( has allowed or is an approved holder for ) isApprovedFor _for

or

_allowed (is allowed or is an approved spender for ) isApprovedFor _for

Maybe holder and spender is more clear: function isApprovedFor(address _spender, address _holder) constant returns (bool _success)

nmushegian commented Nov 19, 2015

^^ it's still ambiguous:

_allowed ( has allowed or is an approved holder for ) isApprovedFor _for

or

_allowed (is allowed or is an approved spender for ) isApprovedFor _for

Maybe holder and spender is more clear: function isApprovedFor(address _spender, address _holder) constant returns (bool _success)

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 20, 2015

Yes, I would say isApprovedFor should return uint256 value, not bool anymore. I like @nmushegian's suggestion of holder & spender.

simondlr commented Nov 20, 2015

Yes, I would say isApprovedFor should return uint256 value, not bool anymore. I like @nmushegian's suggestion of holder & spender.

@Georgi87

This comment has been minimized.

Show comment
Hide comment
@Georgi87

Georgi87 Nov 20, 2015

For Gnosis it would make sense to add an optional identifier, to handle multiple tokens within one contract. In our use case each outcome for each event represents one type of token. Without an identifier, one contract has to be created for each outcome. A lot of token contracts replicating the same functionality would be written on chain. We think there are many use cases for contracts controlling multiple tokens. E.g. also Weifund has to create a new contract for each campaign. Knowing that there are many use cases for both, we should define a standard for both. Since Solidity allows method overloading we can still use the same function names:

function transfer(address _to, uint256 _value) returns (bool _success)

function transfer(address _to, uint256 _value, bytes32 _identifier) returns (bool _success)

What do you think?

Georgi87 commented Nov 20, 2015

For Gnosis it would make sense to add an optional identifier, to handle multiple tokens within one contract. In our use case each outcome for each event represents one type of token. Without an identifier, one contract has to be created for each outcome. A lot of token contracts replicating the same functionality would be written on chain. We think there are many use cases for contracts controlling multiple tokens. E.g. also Weifund has to create a new contract for each campaign. Knowing that there are many use cases for both, we should define a standard for both. Since Solidity allows method overloading we can still use the same function names:

function transfer(address _to, uint256 _value) returns (bool _success)

function transfer(address _to, uint256 _value, bytes32 _identifier) returns (bool _success)

What do you think?

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 20, 2015

Member

@Georgi87 +1 we would need to do the same for balanceOf

Member

frozeman commented Nov 20, 2015

@Georgi87 +1 we would need to do the same for balanceOf

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 20, 2015

You'll need to add identifiers for each function method. How are we going to work with optional, but useful standards? ie decimals, naming & identifiers? We should emphasise this as separate.

simondlr commented Nov 20, 2015

You'll need to add identifiers for each function method. How are we going to work with optional, but useful standards? ie decimals, naming & identifiers? We should emphasise this as separate.

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 20, 2015

Member

Lets discuss this more.

I changed isApprovedFor to allowance:

function allowance(address _address, address _spender) constant returns (uint256 remaining)

// Returns true if `_spender ` is allowed to direct debit from `_address`

And renamed every occourance to _spender to make all variables consistent.

I also renamed the event, please @ALL take a look

Member

frozeman commented Nov 20, 2015

Lets discuss this more.

I changed isApprovedFor to allowance:

function allowance(address _address, address _spender) constant returns (uint256 remaining)

// Returns true if `_spender ` is allowed to direct debit from `_address`

And renamed every occourance to _spender to make all variables consistent.

I also renamed the event, please @ALL take a look

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 20, 2015

@frozeman +1 for allowance. Much more clear. Although description should be:

"Returns the remaining allowance that _spender is allowed to withdraw from _address"?

simondlr commented Nov 20, 2015

@frozeman +1 for allowance. Much more clear. Although description should be:

"Returns the remaining allowance that _spender is allowed to withdraw from _address"?

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman

frozeman Nov 20, 2015

Member

fixed, right?

Member

frozeman commented Nov 20, 2015

fixed, right?

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Nov 20, 2015

fixed, right?

👍

simondlr commented Nov 20, 2015

fixed, right?

👍

@koeppelmann

This comment has been minimized.

Show comment
Hide comment
@koeppelmann

koeppelmann Nov 20, 2015

I support the demand for an token identifier in the standard.
Lets think of tokens as tickets to a concert, m^3 freight in a ship, the right to consume an amount of energy at a specific place in a specific point of time.

In all these examples you have different tokens (different concert, different ships, different place) but other than that all others rules are the same and it makes a lot of sense to handle them in one contract. And very important - this is not only for gas saving reasons.

As a responsible DAPP creator that would allow other tokens to be used in the DAPP you want to check that the tokens do what they promise - it will not be enough that someone self registered the token at a registry. Therefore DAPPs would most likely have a whitelist of (token) contracts that have been reviewed manually.

Just for the pure fun I created a market on it: http://groupgnosis.com/dapp/#/markets/0x149b4a227ef92585d61211bd0a518b2e/0x1878ace41a67160c97a419b01f63b2b094d67ae54

If we had a good metric for the "success" of the token standard we could set of a futarchy. But I guess this is difficult to come up with - ether price is clearly not the right metric for such a minor decision.

koeppelmann commented Nov 20, 2015

I support the demand for an token identifier in the standard.
Lets think of tokens as tickets to a concert, m^3 freight in a ship, the right to consume an amount of energy at a specific place in a specific point of time.

In all these examples you have different tokens (different concert, different ships, different place) but other than that all others rules are the same and it makes a lot of sense to handle them in one contract. And very important - this is not only for gas saving reasons.

As a responsible DAPP creator that would allow other tokens to be used in the DAPP you want to check that the tokens do what they promise - it will not be enough that someone self registered the token at a registry. Therefore DAPPs would most likely have a whitelist of (token) contracts that have been reviewed manually.

Just for the pure fun I created a market on it: http://groupgnosis.com/dapp/#/markets/0x149b4a227ef92585d61211bd0a518b2e/0x1878ace41a67160c97a419b01f63b2b094d67ae54

If we had a good metric for the "success" of the token standard we could set of a futarchy. But I guess this is difficult to come up with - ether price is clearly not the right metric for such a minor decision.

@niran

This comment has been minimized.

Show comment
Hide comment
@niran

niran Nov 20, 2015

We should avoid optional parts of EIPs as a design goal. They make it much harder to convey what an implementer has actually implemented. If we separate optional components into separate EIPs, some tokens will claim to support EIP N, while others will say they support EIP N and EIP N+1. Everything becomes much clearer.

I still don't understand the opposition to using registries for names and decimal places. Separating this cosmetic data from the token itself allows dapps to make their own decisions about what data about the token is required to display in their interface. It also lets dapps add new requirements we didn't think of in the future. A token can specify an admin who can set registry entries on its behalf, ideally by forwarding transactions through the token contract so registries can blindly allow entries to be set for msg.sender.

Consider the Bitcoin community's desire to switch to "bits" as the common denomination. With decimals baked into the token contract, this is impossible without just ignoring what the token contract says. With decimals in registries, the admin can update the name of the BitcoinToken unit to "bits" and the decimal from 8 to 2. If people don't like admin control of this, they will use registries with different access control policies.

We should let this problem solve itself by keeping it out of the token contracts and letting dapp authors do whatever they want, starting with an admin-controlled token registry. TokenContract.decimals will be ignored by clients eventually. It is inevitable. We shouldn't include it here.

Identifiers should make it in this EIP so contracts that consume dynamic tokens don't have to implement separate code paths to implement a separate identifier EIP. They should not be optional: either all calls should have identifiers or we should create an identifier EIP. But more importantly, we should wrap this thing up. I don't think it's the end of the world if identifiers don't make it in.

niran commented Nov 20, 2015

We should avoid optional parts of EIPs as a design goal. They make it much harder to convey what an implementer has actually implemented. If we separate optional components into separate EIPs, some tokens will claim to support EIP N, while others will say they support EIP N and EIP N+1. Everything becomes much clearer.

I still don't understand the opposition to using registries for names and decimal places. Separating this cosmetic data from the token itself allows dapps to make their own decisions about what data about the token is required to display in their interface. It also lets dapps add new requirements we didn't think of in the future. A token can specify an admin who can set registry entries on its behalf, ideally by forwarding transactions through the token contract so registries can blindly allow entries to be set for msg.sender.

Consider the Bitcoin community's desire to switch to "bits" as the common denomination. With decimals baked into the token contract, this is impossible without just ignoring what the token contract says. With decimals in registries, the admin can update the name of the BitcoinToken unit to "bits" and the decimal from 8 to 2. If people don't like admin control of this, they will use registries with different access control policies.

We should let this problem solve itself by keeping it out of the token contracts and letting dapp authors do whatever they want, starting with an admin-controlled token registry. TokenContract.decimals will be ignored by clients eventually. It is inevitable. We shouldn't include it here.

Identifiers should make it in this EIP so contracts that consume dynamic tokens don't have to implement separate code paths to implement a separate identifier EIP. They should not be optional: either all calls should have identifiers or we should create an identifier EIP. But more importantly, we should wrap this thing up. I don't think it's the end of the world if identifiers don't make it in.

@nmushegian

This comment has been minimized.

Show comment
Hide comment
@nmushegian

nmushegian Nov 20, 2015

I'm in favor of leaving multi-token controllers out of this proposal. I think registries and multi-asset controllers are fundamentally related and can be tackled simultaneously once we see some real use cases.

nmushegian commented Nov 20, 2015

I'm in favor of leaving multi-token controllers out of this proposal. I think registries and multi-asset controllers are fundamentally related and can be tackled simultaneously once we see some real use cases.

@koeppelmann

This comment has been minimized.

Show comment
Hide comment
@koeppelmann

koeppelmann Nov 20, 2015

once we see some real use cases.

Gnosis is already real and Augur soon will be. Those tokens from this markets can be used for all kind of APPs since they are basically a payment that will be done if a specific event happens. In principal you could do a crowdfunding campaign with "Hillary Clinton Tokens" from a prediction market. This would mean: you contribute to this campaign but only under the condition that Hillary gets elected.

I am 100% sure there are more meaningful uses of prediction market tokens.

I don't think it's the end of the world if identifiers don't make it in.

Of course not. But why defining a standard that from the beginning excludes one of the very few already existing DAPPs.

koeppelmann commented Nov 20, 2015

once we see some real use cases.

Gnosis is already real and Augur soon will be. Those tokens from this markets can be used for all kind of APPs since they are basically a payment that will be done if a specific event happens. In principal you could do a crowdfunding campaign with "Hillary Clinton Tokens" from a prediction market. This would mean: you contribute to this campaign but only under the condition that Hillary gets elected.

I am 100% sure there are more meaningful uses of prediction market tokens.

I don't think it's the end of the world if identifiers don't make it in.

Of course not. But why defining a standard that from the beginning excludes one of the very few already existing DAPPs.

oryband added a commit to kinecosystem/kin-token that referenced this issue Sep 5, 2017

add basic token and ERC20 contracts
this is a basic erc20 token contract implementation
ethereum/EIPs#20

includes tests

jorlicki referenced this issue in jorlicki/liquid-tokens Sep 18, 2017

15chrjef referenced this issue in poanetwork/token-wizard Sep 28, 2017

@frozeman

This comment has been minimized.

Show comment
Hide comment
Member

frozeman commented Sep 29, 2017

@HaleTom

This comment has been minimized.

Show comment
Hide comment
@HaleTom

HaleTom Oct 11, 2017

@jooray your question is a few months old but I didn't see it answered, so:

ERC20 says under transfer:

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

Regarding burning, the spec is silent; you could burn to (a) designated burn address(es).

HaleTom commented Oct 11, 2017

@jooray your question is a few months old but I didn't see it answered, so:

ERC20 says under transfer:

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

Regarding burning, the spec is silent; you could burn to (a) designated burn address(es).

@HaleTom

This comment has been minimized.

Show comment
Hide comment
@HaleTom

HaleTom Oct 11, 2017

Regarding the wording in approve:

THOUGH The contract itself shouldn't enforce it, to allow backwards compatilibilty with contracts deployed before

Is there something missing here? Before what?

I note an unnecessary T at the start of the sentence and no full stop at the end.

I assume that the words SHOULD, MUST, and OPTIONAL have the meanings defined in RFC2119, but as written in the RFC itself, the RFC should be mentioned if this is the case.

Should the word shouldn't be interpreted as RFC2119's "SHOULD NOT"?

We still have no definition of the capitalised THOUGH.

@frozeman I believe you were the author of these lines, can you shed some light?

HaleTom commented Oct 11, 2017

Regarding the wording in approve:

THOUGH The contract itself shouldn't enforce it, to allow backwards compatilibilty with contracts deployed before

Is there something missing here? Before what?

I note an unnecessary T at the start of the sentence and no full stop at the end.

I assume that the words SHOULD, MUST, and OPTIONAL have the meanings defined in RFC2119, but as written in the RFC itself, the RFC should be mentioned if this is the case.

Should the word shouldn't be interpreted as RFC2119's "SHOULD NOT"?

We still have no definition of the capitalised THOUGH.

@frozeman I believe you were the author of these lines, can you shed some light?

@GriffGreen

This comment has been minimized.

Show comment
Hide comment
@GriffGreen

GriffGreen Oct 25, 2017

@jonnyhsy thats not how the nonce works.
Say if 1st approve()'s nonce is 96, (Called by bob) 2nd approve()'s nonce 97 (Called by bob). If tx with nonce 97 is still pending, the transferFrom() call can have any nonce, as the call would be coming from a different account (Evil Alice).

GriffGreen commented Oct 25, 2017

@jonnyhsy thats not how the nonce works.
Say if 1st approve()'s nonce is 96, (Called by bob) 2nd approve()'s nonce 97 (Called by bob). If tx with nonce 97 is still pending, the transferFrom() call can have any nonce, as the call would be coming from a different account (Evil Alice).

@jonnyhsy

This comment has been minimized.

Show comment
Hide comment
@jonnyhsy

jonnyhsy Oct 26, 2017

@GriffGreen after reading this lengthy (wow!) post, I think the attack scenario still exists, even after applying people's suggestion that "alice first set allowance to 0, check, then set allowance to N", the front running still exists and alice may not be aware of bob has transferred her M tokens already before she resets allowance to 0... Right? How's the final decision about dealing with this race?

jonnyhsy commented Oct 26, 2017

@GriffGreen after reading this lengthy (wow!) post, I think the attack scenario still exists, even after applying people's suggestion that "alice first set allowance to 0, check, then set allowance to N", the front running still exists and alice may not be aware of bob has transferred her M tokens already before she resets allowance to 0... Right? How's the final decision about dealing with this race?

@HaleTom

This comment has been minimized.

Show comment
Hide comment
@HaleTom

HaleTom Oct 26, 2017

@jonnyhsy @GriffGreen I tried to summarise the current state of play regarding the API attack in this separate issue since:

  1. It's security related - ensure it it not overlooked
  2. The thread is very hard to pick out here amongst everything else going on

I thought I'd offer a place to keep the discussion focused in a separate issue, given that there is no EIP20-compatible solution anyway.

HaleTom commented Oct 26, 2017

@jonnyhsy @GriffGreen I tried to summarise the current state of play regarding the API attack in this separate issue since:

  1. It's security related - ensure it it not overlooked
  2. The thread is very hard to pick out here amongst everything else going on

I thought I'd offer a place to keep the discussion focused in a separate issue, given that there is no EIP20-compatible solution anyway.

@CarlosRoldanx

This comment has been minimized.

Show comment
Hide comment
@CarlosRoldanx

CarlosRoldanx Oct 26, 2017

CarlosRoldanx commented Oct 26, 2017

@athlona64 athlona64 referenced this issue Nov 10, 2017

Closed

i got problem #541

mazvydasm pushed a commit to debitum/ico-contracts that referenced this issue Nov 24, 2017

Mazvydas Mackevicius
Add new implementation of OpenZeppelin StandardToken implementation i…
…n order to fix vulnerability described in ethereum/EIPs#20 (comment) (affects BV4.2.2)

@ethereum ethereum deleted a comment from lucianopel Dec 11, 2017

assistivereality added a commit to assistivereality/ico that referenced this issue Dec 11, 2017

// -------------------------------------------------
// Assistive Reality.io ICO token sale contract
// Final revision 22b
// Refunds integrated, full test suite passed
// -------------------------------------------------
// ERC Token Standard #20 Interface
// ethereum/EIPs#20
// -------------------------------------------------
// Recent changes:
// - Updates to comply with latest Solidity versioning (0.4.18):
// -   Classification of internal/private vs public functions
// -   Specification of pure functions such as SafeMath integrated
functions
// -   Conversion of all constant to view or pure dependant on state
changed
// -   Full regression test of code updates
// -   Revision of block number timing for new Ethereum block times
// - Removed duplicate Buy/Transfer event call in buyARXtokens
// - Burn event now records number of ARX tokens burned vs Refund event
Eth
// - Transfer event now fired when beneficiaryWallet withdraws
// -------------------------------------------------
// Price configuration:
// First Day Bonus    +50% = 1,500 ARX  = 1 ETH       [blocks: start ->
s+5959]
// First Week Bonus   +40% = 1,400 ARX  = 1 ETH       [blocks: s+5960
-> s+41710]
// Second Week Bonus  +30% = 1,300 ARX  = 1 ETH       [blocks: s+41711
-> s+83421]
// Third Week Bonus   +25% = 1,250 ARX  = 1 ETH       [blocks: s+83422
-> s+125131]
// Final Week Bonus   +15% = 1,150 ARX  = 1 ETH       [blocks: s+125132
-> endblock]
// -------------------------------------------------
@termslang

This comment has been minimized.

Show comment
Hide comment
@termslang

termslang Dec 23, 2017

Proposed improvement to erc20 protocol:
Add optional (or maybe required?) getOwner() method.

The need for this emerges in our case of decentralised token exchange where only token owner is able to change the token's metadata like name of the company, web link etc (subject to decentralised token exchange implementation). There is no other way for such exchange to learn if the information provided is correct without sacrificing decentralisation.

termslang commented Dec 23, 2017

Proposed improvement to erc20 protocol:
Add optional (or maybe required?) getOwner() method.

The need for this emerges in our case of decentralised token exchange where only token owner is able to change the token's metadata like name of the company, web link etc (subject to decentralised token exchange implementation). There is no other way for such exchange to learn if the information provided is correct without sacrificing decentralisation.

@lichuan

This comment has been minimized.

Show comment
Hide comment
@lichuan

lichuan Jan 23, 2018

is this closed?

lichuan commented Jan 23, 2018

is this closed?

@frozeman

This comment has been minimized.

Show comment
Hide comment
@frozeman
Member

frozeman commented Jan 23, 2018

@Souptacular

This comment has been minimized.

Show comment
Hide comment
@Souptacular

Souptacular Jan 23, 2018

Member

Yes this is closed as @frozeman says.

Member

Souptacular commented Jan 23, 2018

Yes this is closed as @frozeman says.

@LefterisJP

This comment has been minimized.

Show comment
Hide comment
@LefterisJP

LefterisJP Jan 23, 2018

Contributor

@frozeman @Souptacular It could then be beneficial to lock this thread as no further discussion should happen here. HOWTO: https://help.github.com/articles/locking-conversations/

Contributor

LefterisJP commented Jan 23, 2018

@frozeman @Souptacular It could then be beneficial to lock this thread as no further discussion should happen here. HOWTO: https://help.github.com/articles/locking-conversations/

@3esmit

This comment has been minimized.

Show comment
Hide comment
@3esmit

3esmit Jan 23, 2018

There is a lot of problems caused by ERC20, specifically the bad usage of transfer to contracts supporting usage of approval.
I think it would be interesting to block direct transfers to contracts, I mean, using the method transfer(address to, uint amount) to an address which codesizedata > 0. But this block would not exist in transferFrom(address from, address to, uint amount), or would be inverse (require codesizedata < 0) however not specifically important.
This restriction would prevent human errors from bad usage of ERC20 interface in wallets, where sending tokens directly to a contract.
However this changes could be also unnecessary if UI checked this stuff and would not enable send button, requiring a misleading action to be taken from terminal commands, this action should be coordinated between major wallets (and exchanges).

3esmit commented Jan 23, 2018

There is a lot of problems caused by ERC20, specifically the bad usage of transfer to contracts supporting usage of approval.
I think it would be interesting to block direct transfers to contracts, I mean, using the method transfer(address to, uint amount) to an address which codesizedata > 0. But this block would not exist in transferFrom(address from, address to, uint amount), or would be inverse (require codesizedata < 0) however not specifically important.
This restriction would prevent human errors from bad usage of ERC20 interface in wallets, where sending tokens directly to a contract.
However this changes could be also unnecessary if UI checked this stuff and would not enable send button, requiring a misleading action to be taken from terminal commands, this action should be coordinated between major wallets (and exchanges).

@Souptacular

This comment has been minimized.

Show comment
Hide comment
@Souptacular

Souptacular Jan 24, 2018

Member

Thanks @LefterisJP I'll lock it.

Member

Souptacular commented Jan 24, 2018

Thanks @LefterisJP I'll lock it.

@ethereum ethereum locked as resolved and limited conversation to collaborators Jan 24, 2018

zhangxi2097 referenced this issue in zhangxi2097/ether Aug 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.