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

Add EIP-223: Token standard with event handling implementation #6485

Merged
merged 61 commits into from
Mar 6, 2023
Merged

Add EIP-223: Token standard with event handling implementation #6485

merged 61 commits into from
Mar 6, 2023

Conversation

Dexaran
Copy link
Contributor

@Dexaran Dexaran commented Feb 9, 2023


title: Token Standard
description: ERC223 token implementing a communication model for interaction between contracts
author: <Dexaran (@Dexaran) [email protected]
discussions-to: #223
status: Draft
type: Standards Track
category: ERC
created: <2017-05-03>

@Dexaran Dexaran requested a review from eth-bot as a code owner February 9, 2023 21:55
@github-actions github-actions bot added c-new Creates a brand new proposal s-review This EIP is in Review t-erc labels Feb 9, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 9, 2023

All reviewers have approved. Auto merging...

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Feb 9, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 9, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Feb 9, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 9, 2023
@Dexaran
Copy link
Contributor Author

Dexaran commented Feb 9, 2023

Finally. I thought I will never defeat your bots.

btw the current state of EIP20 does not match your requirements. Most importantly it contains the prohibited word "standard" in its title. And now it will look like ERC20 is the only "standard" on Ethereum and everything else is non-standard.

@SamWilsn
Copy link
Contributor

Finally. I thought I will never defeat your bots.

Great! I hope the error messages were helpful, and if not, I'd love your feedback.

btw the current state of EIP20 does not match your requirements. Most importantly it contains the prohibited word "standard" in its title. And now it will look like ERC20 is the only "standard" on Ethereum and everything else is non-standard.

Yeah, that's an unfortunate side effect of treating "final" EIPs as nearly immutable. It's also a great example of why we prohibit "standard" in newer EIPs 🤣

@SamWilsn
Copy link
Contributor

SamWilsn commented Feb 10, 2023

Ah, yeah, I just fixed incorrect line numbers bug. 🤞 I'll be deploying it shortly.

EIPS/eip-223.md Show resolved Hide resolved
@@ -0,0 +1,389 @@
---
eip: 223
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to other editors: this eip is resurrecting #223

EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Show resolved Hide resolved
Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>
@github-actions github-actions bot removed s-review This EIP is in Review w-ci Waiting on CI to pass labels Mar 4, 2023
@Dexaran Dexaran requested a review from Pandapip1 March 4, 2023 03:07
EIPS/eip-223.md Outdated
Comment on lines 134 to 136
- [ERC-20](./eip-20.md) deposit: `approve` ~53K gas, `transferFrom` ~80K gas

- ERC-223 deposit: `transfer` and handling on the receivers side ~46K gas
Copy link
Member

Choose a reason for hiding this comment

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

How does the deposit + call use less gas than an approve? I feel like this gas usage might be outdated

Copy link
Contributor Author

@Dexaran Dexaran Mar 5, 2023

Choose a reason for hiding this comment

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

I have performed this tests https://github.com/Dexaran/Token_Deposits_GAS_testing using REMIX, solidity 0.8.19, optimization 200 runs

Updated the values:

  • approve >> 46K
  • deposit with transferFrom >> 75K
  • ERC223 deposit via transfer >> 53K

Copy link
Member

@Pandapip1 Pandapip1 Mar 6, 2023

Choose a reason for hiding this comment

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

Your transferFrom function is very unoptimized, with 7 total internal contract calls from the initial transferFrom, most of them with three parameters (two 20-byte addresses and a 64-byte unsigned integer). Your ERC-223 contract has two, which both only take a single 20-byte parameter. This skews the results a lot.

EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated

NOTE: A possible way to check whether the `_to` is a contract or an address is to assemble the code of `_to`. If there is no code in `_to`, then this is an externally owned address, otherwise it's a contract.

### Contract that is intended to receive [ERC-223](./eip-223.md) tokens
Copy link
Member

Choose a reason for hiding this comment

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

@SamWilsn have we decided to allow self-references?

Also, I feel like we should ignore EIP linking rules for section titles.

Copy link
Contributor Author

@Dexaran Dexaran Mar 6, 2023

Choose a reason for hiding this comment

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

In my opinion allowing self-references in EIPs would improve readability.

Imagine that we will have to compare one token (ERC-A) to three others (ERC-B, ERC-C, ERC-D).

We need to say that ERC-A is faster than ERC-B, more secure than ERC-C but cuter than ERC-D. At the same time it's cheaper than ERC-B, more expensive than ERC-C and still cuter than ERC-D.

With self-referencing:

"ERC-A is faster than ERC-B, more secure than ERC-C and cuter than ERC-D. ERC-A is cheaper than ERC-B, ERC-A is more expensive than ERC-C. ERC-A is still cuter than ERC-D."

Without self-referencing:

"The proposed token is faster than ERC-B, more secure than ERC-C and cuter than ERC-D. This token is cheaper than ERC-B, this token is more expensive than ERC-C. This token is still cuter than ERC-D."

Comparisons of ERCs/EIPs would be more readable if ALL EIPs (including the one being proposed) would be in the same format.

<disclaimer: English is not my native language so if anything is improperly phrased thats why>

Copy link
Member

@Pandapip1 Pandapip1 Mar 6, 2023

Choose a reason for hiding this comment

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

"The proposed token is faster than ERC-B, more secure than ERC-C and cuter than ERC-D. This token is cheaper than ERC-B, this token is more expensive than ERC-C. This token is still cuter than ERC-D."

Self-references would be replaced with "this EIP" or "this ERC." Basically, it means that the reader doesn't need to remember what the current EIP's number is while they read the EIP. So this would become:

This ERC is faster than ERC-B, more secure than ERC-C and cuter than ERC-D. While this ERC is cheaper than ERC-B, it is more expensive than ERC-C. This ERC is still cuter than ERC-D."

Nonetheless, I definitely don't think this rule needs to be applied to section titles, as I feel that "this E[IP/RC]" would be cumbersome there. This comment was just to notify Sam about some eipw changes I would like.

EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
EIPS/eip-223.md Outdated Show resolved Hide resolved
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

I think this is good now.

@eth-bot eth-bot enabled auto-merge (squash) March 6, 2023 13:11
@eth-bot eth-bot merged commit 5058c8a into ethereum:master Mar 6, 2023
fulldecent pushed a commit to fulldecent/EIPs that referenced this pull request Mar 13, 2023
…eum#6485)

* Add EIP 223: Token standard with event handling implementation

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Replaced `constant` function definitions with `view` to match modern Solidity

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited changes as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Adding a license.

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* re-added email

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Manually committing formatting change

Co-authored-by: Sam Wilson <[email protected]>

* Fix formatting stuff

Co-authored-by: Sam Wilson <[email protected]>

* Fix author list

* Fix more formatting that Sam missed

* Update EIPS/eip-223.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update the code of Transfer(...) event

* Fix function signature

* Moved the tokenReceived() paragraph directly below transfer() deifnition

* Backwards compatibility considerations

* Update eip-223.md

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* update GAS tested values

* updated the value of transferFrom GAS cost

* Apply suggestions from code review

* Move receiver to bottom

* Remove old function

* Lint

---------

Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Gavin John <[email protected]>
axelcabee pushed a commit to axelcabee/EIPs that referenced this pull request May 6, 2023
…eum#6485)

* Add EIP 223: Token standard with event handling implementation

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Replaced `constant` function definitions with `view` to match modern Solidity

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited changes as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Adding a license.

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* re-added email

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Manually committing formatting change

Co-authored-by: Sam Wilson <[email protected]>

* Fix formatting stuff

Co-authored-by: Sam Wilson <[email protected]>

* Fix author list

* Fix more formatting that Sam missed

* Update EIPS/eip-223.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update the code of Transfer(...) event

* Fix function signature

* Moved the tokenReceived() paragraph directly below transfer() deifnition

* Backwards compatibility considerations

* Update eip-223.md

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* update GAS tested values

* updated the value of transferFrom GAS cost

* Apply suggestions from code review

* Move receiver to bottom

* Remove old function

* Lint

---------

Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Gavin John <[email protected]>
GAEAlimited pushed a commit to GAEAlimited/EIPs that referenced this pull request Jun 19, 2024
…eum#6485)

* Add EIP 223: Token standard with event handling implementation

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Readability improvements as suggested by the EIP editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Replaced `constant` function definitions with `view` to match modern Solidity

* Update EIPS/eip-223.md

Commited readability improvements as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Commited changes as suggested by the EIPs editor

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Adding a license.

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* re-added email

* Update EIPS/eip-223.md

Co-authored-by: Sam Wilson <[email protected]>

* Manually committing formatting change

Co-authored-by: Sam Wilson <[email protected]>

* Fix formatting stuff

Co-authored-by: Sam Wilson <[email protected]>

* Fix author list

* Fix more formatting that Sam missed

* Update EIPS/eip-223.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update eip-223.md

* Update the code of Transfer(...) event

* Fix function signature

* Moved the tokenReceived() paragraph directly below transfer() deifnition

* Backwards compatibility considerations

* Update eip-223.md

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* Update EIPS/eip-223.md

Co-authored-by: Gavin John <[email protected]>

* update GAS tested values

* updated the value of transferFrom GAS cost

* Apply suggestions from code review

* Move receiver to bottom

* Remove old function

* Lint

---------

Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Gavin John <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants