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

A New Advanced Token Standard #777

Open
jbaylina opened this Issue Nov 20, 2017 · 397 comments

Comments

Projects
None yet
@jbaylina
Contributor

jbaylina commented Nov 20, 2017

Please, see https://eips.ethereum.org/EIPS/eip-777 for further discussion.


@tjayrush

This comment has been minimized.

tjayrush commented Nov 22, 2017

Was there discussion of adding a Mint/Burn pair of events and/or mint/burn functions to this proposed standard?

If this was discussed and rejected, what are the reasons for rejecting it? If it was not discussed, should it have been?

While not foolproof (because a contract may neglect to call these events), it would make the automated accounting of ICO sales for token contracts that do comply a lot easier. To accurately account for existing ERC 20 token sales, one must read and understand the contract's code.

@3esmit

This comment has been minimized.

3esmit commented Nov 30, 2017

What is the use of _to while is obvious that is the TokenFallback reciever itself (the contract address(this)), and why is needed _ref if we can store a _ref data inside _data if application needs it?

I would find better to stick to the needed stuff, such as:

    /**
    * @notice ERC223 and ERC667 Token fallback 
    * @param _from sender of token
    * @param _amount value sent
    * @param _data data sent
    **/    
    function tokenFallback(
        address _from,
        uint _amount,
        bytes _data
    )

Can you describe situations where _ref and _to are important, or crucial?

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Dec 1, 2017

@3esmit The _to is because the proxy that handles the interface for a specific address can be a different contract. Please see EIP #672 .

For the _ref, this should act as a reference, for example a check number, or an invoice number. In general the ref will be set by the operator and the data will be set for the sender and will be the equivalent to the data in an ethereum transaction.

May be a good alternative would be to integrate this 2 parameters in data and define a standard for data This way we would maintain current compatibility with EIP223...

@3esmit

This comment has been minimized.

3esmit commented Dec 2, 2017

I suggest also adding a boolean return to tokenFallback, and token contract require a true return to accept transaction, in order to avoid this scenario: dapperlabs/cryptokitties-bounty#3

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Dec 2, 2017

@3esmit This is problematic. This function is called after the transfer is done. So returning false would mean to rollback the transfer. This can add a lot of reentrance issues, so I decided that the function ether executes or throws the full transaction.
The nice thing of this standard is that if the tokens a sent via send it eans the the receiver must register the interface in EIP672 way. If not, it fails. Of course you can use the old transfer method for backwards compatibility.

@izqui

This comment has been minimized.

Contributor

izqui commented Jan 4, 2018

I propose renaming operatorData to logData to make more explicit that the purpose of that data is no other than being part of a log. The ability of adding context to token transfers is powerful, and the gas hit is minimal when they are not used.

Really like and support this proposal, exactly the vision that made me excited about ERC223 10 months (!!!) ago. We are considering making ERC777 the base standard for all the tokens issued on @aragon!

@onbjerg

This comment has been minimized.

onbjerg commented Jan 4, 2018

This is an interesting proposal, but I worry about the entire ecosystem having to migrate to new multisig wallets in order to be able to receive ERC777 tokens.

It seems like there was an attempt made to create a whitelist of contracts that one can safely transfer to even if they do not implement ITokenReceipient:

The function MUST throw if:

  • to is a contract that is not prepared to receive tokens. That is it is a contract that does not implements ITokensReceived interface and the hash of the sourcecode is not in between the whitilisted codes listed in the appendix of this code.

But there is no such appendix, I would love to see it 😊

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 4, 2018

@onbjerg We are working on it. We are thinking in keeping this list open for a while (centralized) and close the list at some point (make it decentralized).

@sohkai

This comment has been minimized.

sohkai commented Jan 4, 2018

Was there any consideration over allowing users to specify how much an operator can control, e.g. changing authorizeOperator() to:

function authorizeOperator(address operator, uint authorizedAmount) public?

One could use 2^256 - 1 (or hypothetically the totalSupply() if that never grows) to simulate the previous true behaviour and 0 for false.


The only difference for new contracts implementing ERC20 is that registration of ITokenRecipient via EIP-672 takes precedence over ERC20. This means that even with on a ERC20 transfer call, the token contract MUST check via EIP-672 if the to address implements tokensReceived and call it if available.

I find this somewhat confusing and unexpected. We'll have a dichotomy of "ERC20" tokens: ones that will never call the tokensReceived() callback, even if ITokenRecipient is registered; and ones that will always check. Even if the ERC20 functions are only supposed to be called via old contracts, I think there'll be lots of confusion about this since the meaning of what an "ERC20" token will have essentially changed depending on if your token also supports EIP777.

It also feels odd because you don't have to support the ERC20 interface with EIP777, but you most likely will to support prior contracts expecting that standard.

What if EIP777 was instead a superset of ERC20's interface but overrided specific parts, e.g. transfer() and transferFrom(), to support the ITokenRecipient interface?


I kind of like and dislike the send() nomenclature. On one hand, it's nice how it parallel's ETH's transfer() and send() nomenclature. On the other, it's confusing because these two terms are now both overloaded with different meanings for ETH and tokens. It's confusing enough that we have both for ETH, but it's going to be even more confusing when there's the same names for tokens. I do like the naming for transferAndCall() because it's really obvious what it's probably going to do.

I guess an alternative could be transferToRecipient().

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 4, 2018

@sohkai:
1.- The idea o authorizeOperator is mainly to authorise a contract.
The maximum allowed limitation and many others limitations, like a daily limits, should be implemented in the operator contract and keep this standard as clean as possible.

2.- The idea is that the receiver should have the warranty that the tokensReceived() method is ALWAYS called. Even if it is called via an obsolete ERC20 transfer() or transferFrom() method. This way, for example, allows a recipient to NEVER accept a specific token. or forward some tokens to a specific charity.

3.- The big problem of maintaining transfer() name in the new standard is that if you use transfer() in an ERC20 only token, you will end up locking a lot of tokens. This mistake might become very common in a moment where 50% of the tokens are ERC20Only and 50%ERC777.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jan 4, 2018

As I have mentioned in other threads, I strongly recommend removing decimals. Here is a cross post of what I have said elsewhere:

Decimals are easily the number one source of confusion for both token authors and users of ERC20. I strongly recommend removing this as a variable and instead asserting that tokens must have a certain "humanizing divisor". Reasonable choices IMO are:

  • 0 - The purpose of decimals is to humanize a very large number, nothing more. If you issue a bunch of your tokens, then people can work with gigatokens instead of tokens. People are used to this already with hard drives (no one talks about hard drive size in bytes, it's gigabytes or terrabytes). This scales with the system and allows it to easily change with time.
  • 10^24 - This Allows the token to center on a range that is maximally within the accepted SI prefixes, ranging all the way from yoctotokens to yottatokens. From a scientific/mathematics standpoint, this is probably the best option.
  • 10^18 - 10^18 is the most common humanizing divisor, and it is what ETH used. In order to limit confusion, there may be value in asserting that everyone should just use this. While this isn't a particularly optimal choice, it is fairly compelling due to ETH choosing it.
  • 10^2 - Most fiat currencies use cents, in general, population is more used to currencies with 2 decimals than 0 or more than 2. I'm including this for completeness, but it ends up being effectively the same as 0.

I think the worst option is to continue to allow for variable humanizing divisors. This doesn't actually solve any real problems, since any chosen unit is very likely to be a wrong choice at some point in time (too big or too small). Also, since the token author can pick the token supply, allowing them to also choose the humanizing divisor doesn't give them any more/less power to try to target a "nice human-scale number".

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jan 4, 2018

You mention function send(address to, uint256 value, bytes userData, bytes operatorData) public; in the interface but it doesn't appear in the function descriptions below. Perhaps it was meant to be replaced by operatorSend but you forgot to delete it from the interface?

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jan 4, 2018

I recommend splitting function authorizeOperator(address operator, bool authorized) public; into:

function authorizeOperator(address operator) public;
function revokeOperator(address operator) public;

At the callsite, this will provide a lot more clarity as to what is happening.

@bwheeler96

This comment has been minimized.

bwheeler96 commented Jan 5, 2018

This is rad. Its going to be a long, slow journey to move away from ERC20 but this is a good first step. Couple things:

  1. Why has spender authorization been moved to a boolean? I personally haven't found a use-case for allowing a spender to access a specific amount, but it seems like a nice feature to have since its already part of an existing standard.
  2. Why use the noun operator? I understand this is stupid-picky and certainly hair-splitty, but the work spender is, IMO, a really good descriptor of that particular actor. Operator just sounds like the person has more capability than they do (they aren't really "operating" on the tokens).

Anyways, big 👍. ERC20 needs an upgrade.

@nepalbitcoin

This comment has been minimized.

nepalbitcoin commented Jan 5, 2018

Public state variable for decimal is string public decimals;?
I think that should be uint8 public decimals; based on function decimals() public constant returns (uint8). Prolly a typo.

@GoldenDave

This comment has been minimized.

GoldenDave commented Jan 5, 2018

As I have mentioned in other threads, I strongly recommend removing decimals. Here is a cross post of what I have said elsewhere:

Unfortunately quite a few coins have a very good reason for selecting a different number of decimals. Many of them are in the wild already. Forcing all 10 n decimals would require internal restrictions that would, for example, force rounding of values or revert if an incorrect amount is specified.

Our objective is seldom to expect people to interact directly with the blockchain but, as an example, MEW does a good job of removing the decimal confusion.

@alexvandesande

This comment has been minimized.

Contributor

alexvandesande commented Jan 5, 2018

Should the ITokenRecipient contract also have a function that always returns true stating it's capable of this? It's a way to allow wallet implementers to know which function to use, and therefore save gas.

function isITokenRecipient() returns (bool) { return true};
@lyricalpolymath

This comment has been minimized.

lyricalpolymath commented Jan 5, 2018

Great stuff!

1- initially I too thought as @sohkai that authorizeOperator() would need a form of limiting the amount. In the end the ERC20 approve (which is a confusing name) does have a value up to which the spender is allowed.

I understand and share what you say

The idea o authorizeOperator is mainly to authorise a contract.
The maximum allowed limitation and many others limitations, like a daily limits, should be implemented in the operator contract and keep this standard as clean as possible

But I also think that it's an interesting addition to remind implementers to include optional limitation logic.


2- operatorSend userData vs operatorData
what is the scenario you are imagining for userData?
in any case it's a data that the operator has to input when calling the operatorSend function. Why couldn't both data points be contained in one?


3- Backwards Compatibility
I also found this a bit confusing

The only difference for new contracts implementing ERC20 is that registration of ITokenRecipient via EIP-672 takes precedence over ERC20. This means that even with on a ERC20 transfer call, the token contract MUST check via EIP-672 if the to address implements tokensReceived and call it if available.

I understand that new smart contracts will detect the right function to call (right?)
but what about users interacting directly with the contract? It will be confusing to see 2 functions that supposedly do more or less the same thing but have different names.
confusing UX and a potential source of problems if you say that "tokens will probably be locked"

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 5, 2018

@lyricalpolymath
3- New contracts that use new tokens must use send() and not transfer(). transfer() is just for backwards compatibility. mainly old smart contracts, as I expect that UI will be upgraded at some point.
Stay tunned for (1 & 2)

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 5, 2018

@alexvandesande To know if a contract implements ITokenRecipient, the reverseENS is used (EIP672) which will never throw and you will know if it implements or not the Interface. The gas cost should be the same as the one you propose.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jan 5, 2018

@GoldenDave Others have made the same argument in the past but were unable to provide (IMO) a compelling argument as to why forcing the humanizing divisor to the same for all tokens is bad. The most common cited example is "what if I have a token that is pegged to USD (or similar), which only has 2 decimals?" In this case, you can still have 24 decimals (or whatever the standard defines) exposed to the user and the contract can internally store however it likes. In this case, you would simply multiply whatever internal value you have by 10^22 when it is returning to the user. In all cases I have seen people come up with (including the USD peg) nothing is hurt by having a token be more divisible. There is really nothing fundamentally wrong with having 1 attousd.

@alexvandesande

This comment has been minimized.

Contributor

alexvandesande commented Jan 5, 2018

@jbaylina I support reverse ENS, but I don't see why not also add this to the contract itself. Is simpler to build, will work on any network, including test networks etc. Also, to check ens resolver you need to have multiple calls (see if there's a resolver, then check the resolver etc) AND to have an extra function on the constructor function to set the ens resolver info.

Again, I'm all for ENS, but why not add on the contract simple info like that? Reminds me of the debate on either tokens should have symbol and names on the contract or on a token registry: in contract won by the simplicity of it.


Also: I'd like to propose to add a provable standard to this token. One of the most requested features I get from token creators is how to send tokens without having ether and I think it makes sense that should be a core function of whatever is the next big token version.

@DaveAppleton

This comment has been minimized.

DaveAppleton commented Jan 5, 2018

Others have made the same argument in the past but were unable to provide (IMO) a compelling argument as to why forcing the humanizing divisor to the same for all tokens is bad.

During the HelloGold token sale, contributors received HGT which entitled them to a share of a reward token GBT (our gold backed token) which is related to the amount of management fees that we receive for storing clients' gold pro rated to the person's HGT holding.

In order that anybody holding the minimum amount of HGT should receive GBT during a distribution we calculated that GBT would work with 18 decimals but as a result HGT would need to have 8 d.p. Any more precision would be pointless and misleading.

It it rather dictatorial to say that everybody needs to normalise everything to meet a number of decimal points that do not particularly agree with them, especially when we already have a method of handling it.

@DaveAppleton

This comment has been minimized.

DaveAppleton commented Jan 5, 2018

Should the ITokenRecipient contract also have a function that always returns true stating it's capable of this? It's a way to allow wallet implementers to know which function to use, and therefore save gas.

It is great idea - but when I ran a quick test on remix, a contract with a simple fallback function would falsely satisfy your requirements.

function(){
}

appears to return true when a non existent function xyz() returns (bool) is called.

https://gist.github.com/DaveAppleton/ef44e9745b1f57c7ae0d6744a15bc5c6

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 6, 2018

@alexvandesande One of the nicest think of this standard is that not only you can have functionality in smart contract recipients, but also in any regular account. You can program for examle that you don't accept tokens sent to your public regular account. Or that you send half of it to a charity.
I agree that using EIP672 is a little complicated, and what's the worst, ENS still is centralised in some way. So that is why we plan to use EIP #820 which is equivalent to EIP672 but much more simpler and pure decentralised contract. (It still is a work in progress).

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 6, 2018

@alexvandesande Regarding the provable functionality, the idea is to do that via an operator. The operator can, for example, accept signed checks, which they are very much provable transfers.

This standard should allow for token contract creators to set some default operators to be authorised for everybody.

@izqui izqui referenced this issue Jan 7, 2018

Closed

ERC777 #163

@alexvandesande

This comment has been minimized.

Contributor

alexvandesande commented Jan 8, 2018

@DaveAppleton I just tested your code and got

{
	"0": "bool: false"
}

So it seems it should work.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Sep 19, 2018

@rainbreak The thing that convinced me to allow for special casing of 0x000... address is that it is the default value for all variables in Solidity/EVM thus it is very easy to accidentally interface with 0x000... when you misuse the contract. While I tend to agree with you that it isn't really the job of the spec to tell people how to write good code, it also doesn't hurt the spec by adding it since there isn't any good use case for allowing 0x000... address to have tokens.

@rainbreak

This comment has been minimized.

rainbreak commented Sep 19, 2018

@mcdee @MicahZoltu I think this only speaks to point 1. I can see the argument for special casing 0x00 on the basis of it being the default value, but otherwise it doesn't seem any more special than 0x1 0x2 etc.

@gitcnd yes, but don't take it too seriously as it's just a few hours messing around trying to capture the essence of this spec. Feel free to fork it or whatever.

@mcdee

This comment has been minimized.

Contributor

mcdee commented Sep 24, 2018

@jacquesd @jbaylina I know that it is late in the day, but I've been working on some contracts with tokensToSend() for common use cases and I'm finding an area that the current spec cannot handle without major gas and effort implications.

Take a simple example: an address holding a number of tokens as part of a "friends and family" deal (or any generic whitelisting). The acquirer is given a signature by the holder, for example S(acquiring address, number of tokens, cost), and the acquirer obtains the token through an operatorSend() with the signature in the operatorData section. The address has a tokensToSend() attached to it that carries out the authority decision: if the signature matches then the transfer goes ahead. Very easy to understand, very clean, and very extensible.

All well and good, except that the acquirer isn't listed as an operator so fails in operatorSend() before tokensToSend() comes in to play. Short of the holding address sending an authoriseOperator for each participant there is no way in the current spec to provide this type of functionality. What is needed is for some way for the token contract to know that it should delegate authority decisions to tokensToSend().

The way that I would implement this would be to assign 0xffffffffffffffffffffffffffffffffffffffff to be the "any" operator, so a single call by the address to authorizeOperator(0xffffffffffffffffffffffffffffffffffffffff) would be all that would be necessary. In the reference implementation this would require a change of:

function isOperatorFor(address _operator, address _tokenHolder) public constant returns (bool) {
    return _operator == _tokenHolder || mAuthorized[_operator][_tokenHolder];
}

to

address OPERATOR_ANY = 0xffffffffffffffffffffffffffffffffffffffff;
function isOperatorFor(address _operator, address _tokenHolder) public constant returns (bool) {
    return _operator == _tokenHolder || mAuthorized[_operator][_tokenHolder] || mAuthorized[OPERATOR_ANY][_tokenHolder];
}

(another value could, of course, be used if considered preferable.)

As I say, I know that it's late in the day. I've avoided mentioning a bunch of potential improvements because I'd rather see this standard finalised. But the problem this one solves isn't one that I've found a sensible way around, and it does appear to bring something to the standard that significantly increases its ability for a very minor change.

UPDATE: ah yes, forgot the other piece of this. send(), operatorSend(), burn() and tokensToSend() would need to become payable for the more advanced scenarios. This would, however, allow all of the commonly non-standard transfers (purchasing tokens for ICOs, for example) to go through the ERC-777 standard send() or operatorSend() functions.

@mcdee

This comment has been minimized.

Contributor

mcdee commented Sep 24, 2018

To provide some idea of the power that the above change provides, here is a sample contract that allows any token holder to sell their tokens at a given cost per token. This could easily be expanded to create a full ICO handler, an OTC offering system, and many other uses.

The potential intelligence that could be put behind tokensToSend() could kickstart a new wave of agent-style transactions on Ethereum. I do think that this would move the usefulness of ERC-777 to a new level.

contract Payed is ERC777TokensSender, ERC820Implementer {
    using SafeMath for uint256;

    // Mapping is token=>holder=>cost per token
    mapping(address=>mapping(address=>uint256)) costPerToken;

    event CostPerToken(address token, address holder, uint256 costPerToken);

    function setCostPerToken(address _token, uint256 _costPerToken) public {
        costPerToken[_token][msg.sender] = _costPerToken;
        emit CostPerToken(_token, msg.sender, _costPerToken);
    }

    function getCostPerToken(address _token, address _holder) public view returns (uint256) {
        return costPerToken[_token][_holder];
    }

    /**
     * This ensures that the correct value is present for the transfer
     */
    function tokensToSend(address operator, address holder, address recipient, uint256 amount, bytes data, bytes operatorData) public payable {
        (operator, recipient, data, operatorData);

        require(msg.value.div(amount) == costPerToken[msg.sender][holder]);
        holder.transfer(msg.value);
    }

    function canImplementInterfaceForAddress(address addr, bytes32 interfaceHash) public pure returns(bytes32) {
        (addr);
        if (interfaceHash == keccak256("ERC777TokensSender")) {
            return keccak256("ERC820_ACCEPT_MAGIC");
        } else {
            return 0;
        }
    }
}
@AdrianClv

This comment has been minimized.

AdrianClv commented Oct 5, 2018

I was integrating it with other contracts and I think the names "operator" and "operatorData" for the Minted event can be a bit confusing. It looks like it is must be a default operator, but I don't think that that's the case.

I suggest to change it for "minter" and "minterData"

@lrgeoemtry

This comment has been minimized.

lrgeoemtry commented Oct 16, 2018

hi all,
getting ready to implement the erc777 as my token for oct 29 release.
wondering why im running into an error encoding the arguements when trying to create the contract

screenshot 2018-10-16 at 10 42 30 am

any help is amazing at this point. here is a gist of what im compiling.
https://gist.github.com/lrgeoemtry/c8e67e188a152fc826875935d695c085

thanks fam
~ a stressed out sol dev

@mcdee

This comment has been minimized.

Contributor

mcdee commented Oct 16, 2018

@lrgeoemtry given the code and the error I suspect that the first parameter to your constructor is empty, when it should be [].

@lrgeoemtry

This comment has been minimized.

lrgeoemtry commented Oct 16, 2018

@mcdee hmm, this is contract im feeding the rest of the erc777 interfaces into.
first constructor param is [] and when i try to add [] to the burnOperator ( or to any other param) it no longer compiles. =\

screenshot 2018-10-16 at 2 38 41 pm

@jbaylina , @jacquesd thoughts? i think we're really close but this has been racking my brain for weeks.

@mcdee

This comment has been minimized.

Contributor

mcdee commented Oct 17, 2018

@lrgeoemtry the issue is with whatever code is attempting to construct the token rather than the token code itself; the solidity code is fine. If you send me a link to your github repository with the tests/depoyment code I can take a look to see where the problem is (but this isn't really the place to discuss the issue so please use gitter or similar to discuss further)

@lrgeoemtry

This comment has been minimized.

lrgeoemtry commented Oct 17, 2018

ill email you at your Jim@mcdee.net. thanks @mcdee :)

@catageek

This comment has been minimized.

catageek commented Oct 27, 2018

There is a security issue with this contract. This contract does not protect against reentrancy regarding tokensSend() and tokensReceived().

Let say a malicious contract implements tokensSend() and calls back the token contract using delegatecall. The resulting call will have address(this) as msg.sender and will act on behalf on the contract itself.
So the malicious user can add or remove operators for the contract token account, send tokens on behalf of the token contract, or operates on any account for which the token contract is an operator.

The simplest fix would be to forbid address(this) to be an operator, even for itself, so prevent the insertion of address(this) in defaultOperators in constructor and in authorizeOperator()

@mcdee

This comment has been minimized.

Contributor

mcdee commented Oct 27, 2018

@catageek the address holder would need to set the malicious contract themselves, so there is a level of protection there. Also, if the malicious tokensToSend()/tokensReceived() uses delegatecall won't it retain the storage of the calling contract and hence not be able to carry out the actions you are suggesting?

If you could put together a proof of concept malicious tokensReceived() this could be looked at in more detail.

@catageek

This comment has been minimized.

catageek commented Oct 27, 2018

@mcdee You're right, I missed the point that the token contract storage would not be accessible in a delegate call. Thanks.

@lrgeoemtry

This comment has been minimized.

lrgeoemtry commented Oct 28, 2018

are there any successful deployments of 777 ?
https://gcalliance.io/ these guys are pretending to be but there's 55 static warnings on their code on etherscan and its deprecated.
I can successfully deploy to roosten, but then transactions start to fail.
I'm at wits end,
I know @mcdee told me to just deploy a 20 but I feel like I'll be giving up a lot of potential with this.

currently Rinkeby won't deploy and the main net is certainly not going to take this.
https://gist.github.com/lrgeoemtry/f32c615a1e39474bb3f906bdcd52707a
the code I'm attempting to deploy

HALP q_q

@lrgeoemtry

This comment has been minimized.

lrgeoemtry commented Oct 29, 2018

I'm deploying an ERC20 and putting this to rest until you wizards can solidify this standard
thank you @mcdee for the help and advisement to do this as I was at wits end trying to make 777 work

@irdd

This comment has been minimized.

irdd commented Oct 29, 2018

@lrgeoemtry
Please before you try to discredit something do the proper homework. Your code is not working not because ERC777 is fault but rather because you did not understand how this standard works.
The Call token code is already deployed in the mainnet and working properly also the codebase has been through a code “audit” with non-alarming results. As of the errors, there are no errors during the compilation of the code nor the deployment if you take into account the proper parameter tweaking based on the target ethereum network.

@bluerosss

This comment has been minimized.

bluerosss commented Oct 29, 2018

@lrgeoemtry : CALL token from GCAlliance works perfectly

@mcdee

This comment has been minimized.

Contributor

mcdee commented Oct 29, 2018

@irdd @bluerosss I would suggest that the issue around implementing ERC777 at the moment is that it has not been finalised. Perhaps more importantly, ERC820 has not been finalised and due to its deployment method any changes to that codebase will result in a different contract address. It is, of course, up to each party to decide if they want to deploy at this time but personally I'd be wary until at least ERC820 is locked down.

(And @jbaylina @jacquesd if you're at Devcon it would be good to discuss my proposed tweak to ERC777 so that I can answer any questions you have about it).

@irdd

This comment has been minimized.

irdd commented Oct 29, 2018

@mcdee
We will update (644 ftw) our token contract with the final erc820 address when it’s deployed/finalized.

@AdrianClv

This comment has been minimized.

AdrianClv commented Oct 29, 2018

@lrgeoemtry We have an erc777 deployed on Kovan and it works fine. It will also be on Mainnet soon.

@chompomonim

This comment has been minimized.

chompomonim commented Oct 30, 2018

@lrgeoemtry We have an erc777 deployed on Kovan and it works fine. It will also be on Mainnet soon.

There is recommendation to not deploy into mainnet until ERC820 will be finalised. ERC777 is closely coupled with ERC820, and official address of ERC820 will be changes after it will be finalised.

@catageek

This comment has been minimized.

catageek commented Nov 1, 2018

Hi, it's me again. I think there is a possibility of a front run attack that should be documented or fixed.

Here is how I think the attack could be performed : An operator authorized for a token holder can watch the transactions from this token holder, and if they see a revokeOperator() transaction that remove their rights, they send a transaction executing operatorSend() to withdraw an amount of tokens from the token holder's balance, with a high gas price to be executed before the token holder transaction.

One can argue that the token holder can implement tokensToSend() interface and revert when the transaction is sent by the operator that has to be revoked, but again the operator can watch the implementer contract transactions, detect that a change will make its own transactions reverted and can act as described above.

I think this attack is plausible if an operator becomes malicious and a token holder wants to remove their operator rights but get their balance emptied as a punishment.

So there should be a way for any token holder to remove any operator in a way that prevent the operator to punish the token holder, but I am not sure it should be considered in this standard or if it's the token holder responsibility.

At least it should be documented that the token holder may be punished by an operator if it revokes it before making tokensToSend() revert, and that a front run attack protection must be implemented on the tokensToSend() function.

What is your opinion about it ?

@mcdee

This comment has been minimized.

Contributor

mcdee commented Nov 6, 2018

@catageek your attack is predicated on the idea that the operator has been authorised. It is incumbent on the token holder to consider the ramifications of authorising an operator. Note that most operators will be either pseudonyms of the holding address (i.e. another address that is controlled by the same entity) or a smart contract with well-defined functionality, so the chance of an operator "becoming malicious" is 0.

@lsaether

This comment has been minimized.

Contributor

lsaether commented Nov 7, 2018

Heads up -- it looks like Vyper reserves send as a keyword for its built-in function to send ether. This means that under the current specification, ERC777 would not be possible to implement in Vyper. Thoughts?

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Nov 7, 2018

@lsaether Thanks for your comment I'll definitely look into it. We already removed function overloading to make it compatible with Vyper and we don't want ERC777 to be Solidity only.

@mudgen

This comment has been minimized.

Contributor

mudgen commented Nov 7, 2018

What do you think about using the ERC1538 contract architecture to implement ERC777 tokens?

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Nov 8, 2018

I feel like Vyper, which came second, should stop using incredibly common words as reserved words. I argue that we should pressure Vyper to choose better reserved words that aren't incredibly common in this space (like send).

@mcdee

This comment has been minimized.

Contributor

mcdee commented Nov 8, 2018

@lsaether @jbaylina @MicahZoltu looking at the Vyper docs it appears that sending Ether uses a keyword style send <address> whereas contract methods use an object style <contract>.<method>(), in which case I would hope that send would be considered a valid method. There doesn't appear to be an clash here, although admittedly i haven't tried it.

@lsaether

This comment has been minimized.

Contributor

lsaether commented Nov 8, 2018

@mcdee Should have mentioned before that I have tried it since I was trying out to implement ERC777 in Vyper. The vyper compiler will not allow a function to be defined within a contract that is named send and throws the following error:

vyper.exceptions.FunctionDeclarationException: Function name invalid: send

I'll go ahead and open an issue on the Vyper repository as well so that they are aware of the issue and hopefully a simple resolution can be found!

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