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

ERC-777 A New Advanced Token Standard #907

Merged
merged 16 commits into from
Apr 6, 2018
Merged

Conversation

jbaylina
Copy link
Contributor

@jbaylina jbaylina commented Feb 27, 2018

Tjis is the PR discussed in #777

Pending issues required for the standard to be approved

  • Approve eip820
  • Decide if include listOperators(address account) constant public returns(address[]) (Will not be implemented).
  • Decide if include a revokeAllOperators(address account) public (Will not be implemented)
  • Approve

@jbaylina jbaylina mentioned this pull request Feb 27, 2018
@0xjac
Copy link
Contributor

0xjac commented Feb 27, 2018

@jbaylina
I'm currently working on revokeAllOperators: 0xjac/ERC777#35
Last time we agreed not to include listOperators. Any reason why the change of mind?

@jbaylina jbaylina changed the title ERC777 A New Advanced Token Standard ERC-777 A New Advanced Token Standard Feb 28, 2018
@nicksavers nicksavers added the ERC label Mar 8, 2018
@fulldecent
Copy link
Contributor

revokeAllOperators requires a for loop. Here is a way to implement deletable mappings where you can delete them with O(1).

https://ethereum.stackexchange.com/a/42540/26015

It seems ridiculous, but I'm not sure what exactly the gas cost or practicality is.

@fulldecent
Copy link
Contributor

This EIP now includes a logo. And the logo is not licensed CC0 as per EIP-1.

@0xjac
Copy link
Contributor

0xjac commented Mar 19, 2018

@fulldecent The reason the logo is under CC is to add a restriction to prevent the use of the logo for non ERC777 tokens and other technologies (such as wallets). I'm happy to change it to CC0 if the community think it's better that way.

Also technically the logo is only referenced in the EIP, we should include it in this repo as well. D0 you know what the best approach is? Maybe similar to #858: add an eip-777 folder containing the logo?

@MicahZoltu
Copy link
Contributor

I very much prefer CC0; attribution makes everything hard if you actually follow it (which 99% of people don't, so they all violate the copyright anyway). I am also not personally a fan of copyrights in the first place.

@0xjac
Copy link
Contributor

0xjac commented Mar 19, 2018

@fulldecent @MicahZoltu Alright I will move it to CC0 and add the logo in the repo. The idea was really not to do attribution or to restrict the use of the logo in any way except for false advertising.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Mar 19, 2018

Unfortunately, there are very few licenses that don't require attribution. CC0, Unlicense, WTFPL are the common ones. All of them are basically "release into public domain". There are no licenses (unfortunately) that release into public domain with exceptions. There is also no license for "release into public domain and also release all patents", which is really needed IMO.

@jbaylina
Copy link
Contributor Author

Please comment on this: #777 (comment)

@0xjac
Copy link
Contributor

0xjac commented Mar 19, 2018

@fulldecent @MicahZoltu Done, the logo is now under CC0 and is included in this repo. (Before it was just referenced from jacquesd/erc777 which contains the reference implementation)

Note that the images do appear broken, that's normal they are links to where the image will be once merged into the master branch of /ethereum/EIPs. After merging the images will appear.

EIPS/eip-777.md Outdated
<table>
<tr>
<th>Image</th>
<th><img src="https://github.com/ethereum/EIPs/blob/master/EIPs/eip-777/logo/png/ERC-777-logo-beige-192px.png?raw=true" height="46px" align="top"></img></th>
Copy link
Contributor

@MicahZoltu MicahZoltu Mar 20, 2018

Choose a reason for hiding this comment

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

I recommend changing this to use markdown rather than HTML and use relative links instead of absolute links. This way the images work both in the PR and on any branch.

I think the syntax is something like this for the image link, though you should look at the GitHub Markdown docs for specifics.

![Beige](EIPs/eip-777/logo/png/ERC-777-logo-beige-192px.png)

@Arachnid
Copy link
Contributor

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

@0xjac
Copy link
Contributor

0xjac commented Mar 29, 2018

@Arachnid Thanks for the notice, the eip draft has been adapted to the new format.

@MicahZoltu thanks for your feedback. I have updated the links to be relative and they seem to work, but the new travis hook is complaining about them.

I have run the travis jobs locally and I noticed that links on the generated page for the EIP work fine and the images with the relative links are displayed. So I'm guessing there is something wrong with the way the links to the local eip-777 folder with the logo are validated with htmlproofer. Maybe some config I'm missing?

@Arachnid
Copy link
Contributor

You need to use relative URIs - instead of /EIPS/eip-777/..., use eip-777/....

@0xjac
Copy link
Contributor

0xjac commented Mar 29, 2018

@Arachnid

You need to use relative URIs - instead of /EIPS/eip-777/..., use eip-777/....

Sorry I was unclear. I did fix the links, and Jordi should update this PR soon with the fix, but at least with my fork, the build still fails because of the links related to the folder.

@Arachnid
Copy link
Contributor

Sorry I was unclear. I did fix the links, and Jordi should update this PR soon with the fix, but at least with my fork, the build still fails because of the links related to the folder.

Ah, right.

htmlproofer does not like the fact that there is a directory with the same name as the file.

We probably need a better way to handle this in general. I'm open to suggestions.

@0xjac
Copy link
Contributor

0xjac commented Mar 29, 2018

I based the eip-777 folder on previous eips which did the same like 107 or 858.
I would suggest having a single folder named something like static or assets and moving all the eip-X folders in there.

If the folder is named assets then it should be merged with the generated assets folder of jekyll.
I think this might be a bit cleaner and if each eip has it's own folder in there named properly, it should not create any conflict with other assets

EIPS/eip-777.md Outdated
---
eip: 777
title: A New Advanced Token Standard
author: Jordi Baylina @jbaylina, Jacques Dafflon @jacquesd, Thomas Shababi @tshabs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put usernames in parens, so they'll be automatically linkified and the bot can determine they're authors.

EIPS/eip-777.md Outdated
category: ERC
created: 2017-11-20
requires: 820
replaces: 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really replace ERC20?

@Arachnid Arachnid merged commit 547af9d into ethereum:master Apr 6, 2018
@fulldecent
Copy link
Contributor

@Arachnid Have the "EIP collaborators" approved this to be accepted as draft? I believe your merging here was premature.

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

@fulldecent To be accepted as a draft, an EIP only needs to be syntactically correct and not obviously stupid; it doesn't imply approval. An author asked me to merge it OOB, so I did.

@jbaylina
Copy link
Contributor Author

jbaylina commented Apr 8, 2018

@fulldecent I prefer for discussion to have individual concrete PRs than discuss them in the long thread.

@h3ph4est7s
Copy link

h3ph4est7s commented Apr 20, 2018

One question, imagine the scenario that a malicious person in any of both parties implement a gas exhausting function and consume deliberately the remaining gas. In the case of the sending party even if the owner gave operator permission to a third party the call will keep remaining out of gas and the operator will lose value by trying to execute the transaction in behalf of the sending party. In the other case the sending party will lose value by trying to send value to the receiving party. A standardized gas limit should be provided for tokensReceived and tokensToSend. Something like .gas(n) here and here

Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request May 2, 2018
* ERC777 A New Advanced Token Standard

* ERC777: Add ERC777 Logo

* ERC777: Normalize EIP/ERC names

* ERC777: Spec for tokensToSend when burning tokens

* ERC777: Update official repository

* ERC777: Clarification and corrections

- Clarify unclear sections of the spec
- Fix typos and grammar mistakes
- Improve aesthetics of the text

* ERC777: Change terminology of the repo, small fix

- Don't refer to the repo as the official repo but the repo of the
  reference implementation
- Fix a small typo in the AuthorizedOperator event

* ERC777: logo & release to public domain (CC0)

* ERC777: Use markdown not html & relative links

* ERC777: Adapt to new EIP template

* ERC777: Use solidity syntax and fix relative links

* ERC777: Add discussions-to link

* ERC777: Fix link in discussion-to

* ERC777: Fix image links

* ERC777: Fix eip type

* ERC777: Update header
@yuwiggin
Copy link

Is ERC777 vulnerable from short address attack? If it is, how to avoid it?

This pull request was closed.
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.

8 participants