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

Submit EIP-4973 - Account-bound tokens #4973

Merged
merged 37 commits into from
May 18, 2022
Merged

Conversation

TimDaub
Copy link
Contributor

@TimDaub TimDaub commented Apr 5, 2022

Changes made after initial submission based on discussion on Ethereum Magicians forum:

  • Consider name change to "Account-bound" tokens
  • In rationale, add the following about caveats of the implementation
    • tokens are essentially account bound and cannot be reclaimed in case of e.g. a wallet rotation
    • highlights that accounts bound property can be circumvented using contracts
  • Re-edit "Rationale" section
  • Address @will-holley feedback. Implement suggestion from Eth Magicians: https://ethereum-magicians.org/t/eip-4973-account-bound-tokens/8825/19?u=timdaub
  • Address @iainnash's feedback: Write about the spec's neutrality towards admin transfers/migrations
  • Address @SamWilsn's feedback on GitHub PR
  • Add "Raphael Roullet (@ra-phael)" as co-author
  • Address comments in implementation from call with @ra-phael from 2022-05-04
  • Test-mint an EIP4973 token and check what popular wallets display
  • Test-mint an EIP4973 token and check if indexing of event signatures is practical enough
  • Drive further studying existing wallets and how they handle EIP 4973 tokens
  • Address ETHMagician's feedback from @MicahZoltu about encouraging users to rotate keys.

@TimDaub TimDaub mentioned this pull request Apr 5, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 5, 2022

All tests passed; auto-merging...

(pass) eip-4973.md

classification
updateEIP
  • passed!

(pass) assets/eip-4973/src/ERC165.sol

classification
ambiguous
  • file assets/eip-4973/src/ERC165.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

(pass) assets/eip-4973/src/ERC4973.sol

classification
ambiguous
  • file assets/eip-4973/src/ERC4973.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

(pass) assets/eip-4973/src/ERC4973.t.sol

classification
ambiguous
  • file assets/eip-4973/src/ERC4973.t.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

(pass) assets/eip-4973/src/interfaces/IERC165.sol

classification
ambiguous
  • file assets/eip-4973/src/interfaces/IERC165.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

(pass) assets/eip-4973/src/interfaces/IERC4973.sol

classification
ambiguous
  • file assets/eip-4973/src/interfaces/IERC4973.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

(pass) assets/eip-4973/src/interfaces/IERC721Metadata.sol

classification
ambiguous
  • file assets/eip-4973/src/interfaces/IERC721Metadata.sol is associated with EIP 4973; because there are also changes being made to EIPS/eip-4973.md all changes to corresponding assets are also allowed

EIPS/eip-4966.md Outdated Show resolved Hide resolved
EIPS/eip-4966.md Outdated Show resolved Hide resolved
EIPS/eip-4966.md Outdated Show resolved Hide resolved
EIPS/eip-4966.md Outdated Show resolved Hide resolved
EIPS/eip-4966.md Outdated Show resolved Hide resolved
EIPS/eip-4966.md Outdated Show resolved Hide resolved
@TimDaub TimDaub changed the title Resubmit EIP-4966 Submit EIP-4973 Apr 10, 2022
@TimDaub
Copy link
Contributor Author

TimDaub commented Apr 10, 2022

@SamWilsn I've addressed all your comments and generally edited the text such that it now complies better with the structure outlined in ERC-1. I've also now renamed it finally to ERC4973, the number of this issue. Please let me know how to proceed.

EIPS/eip-4973.md Outdated Show resolved Hide resolved
EIPS/eip-4973.md Outdated Show resolved Hide resolved
EIPS/eip-4973.md Outdated Show resolved Hide resolved
EIPS/eip-4973.md Outdated Show resolved Hide resolved
assets/eip-4973/src/ERC4973.sol Outdated Show resolved Hide resolved
assets/eip-4973/src/ERC4973.t.sol Outdated Show resolved Hide resolved
assets/eip-4973/src/interfaces/IERC4973.sol Outdated Show resolved Hide resolved
assets/eip-4973/src/interfaces/IERC721Metadata.sol Outdated Show resolved Hide resolved
TimDaub and others added 9 commits April 18, 2022 10:21
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@TimDaub
Copy link
Contributor Author

TimDaub commented Apr 18, 2022

Updates from 2022-04-18:

  • Added section on revocations.
  • Changed name to "Account-bound tokens."
  • Added section on exception handling in cases of key loss.

TimDaub and others added 2 commits May 9, 2022 14:28
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
EIPS/eip-4973.md Outdated Show resolved Hide resolved
Copy link
Contributor

@SamWilsn SamWilsn 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 the remaining comments can be fixed while this EIP is a draft.

---
eip: 4973
title: Account-bound Tokens
description: A standard interface for non-transferrable non-fungible tokens, also known as "account-bound" or "soulbound tokens" or "badges".
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend removing "standard", but I think it's fine to leave in for the draft.

Suggested change
description: A standard interface for non-transferrable non-fungible tokens, also known as "account-bound" or "soulbound tokens" or "badges".
description: An interface for non-transferrable non-fungible tokens, also known as "account-bound" or "soulbound tokens" or "badges".

@eth-bot eth-bot enabled auto-merge (squash) May 13, 2022 21:04
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@MicahZoltu
Copy link
Contributor

MicahZoltu commented May 16, 2022

No idea why CI is failing... kicking to try to resolve it.

@MicahZoltu MicahZoltu closed this May 16, 2022
auto-merge was automatically disabled May 16, 2022 06:46

Pull request was closed

@MicahZoltu MicahZoltu reopened this May 16, 2022
@eth-bot eth-bot enabled auto-merge (squash) May 16, 2022 06:47
@MicahZoltu
Copy link
Contributor

CI is failing with this error, but that file does exist so I'm not sure what the problem is:

- ./_site/EIPS/eip-4973.html
  *  internally linking to ./eip-721.md`, which does not exist (line 237)
     <a href="./eip-721.md`"></a>

@MicahZoltu
Copy link
Contributor

If someone can help me track down exactly why CI is failing for this that would be super helpful! Maybe there is some non-ascii character somewhere or something?

EIPS/eip-4973.md Outdated Show resolved Hide resolved
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
auto-merge was automatically disabled May 18, 2022 10:56

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) May 18, 2022 10:57
@eth-bot eth-bot merged commit d5858d2 into ethereum:master May 18, 2022
PowerStream3604 pushed a commit to PowerStream3604/EIPs that referenced this pull request May 19, 2022
* Add EIP-1238.md

* Rename from 1238 to 4966

* Address feedback from PR

* Change name from 4966 to 4973

* Fix header property order

* Remove 79 character manual word wrapping

* Fix syntax error for incomplete link tag

* Remove Caveat section (for now)

* Clarify standards text

* Run through grammarly

* Update discussion link

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Relicense to CC0-1.0

* More copyediting

* Rename from "Soulbound" to "Account-bound" tokens

* Add section on "Exception handling"

* Add section on revocation

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Rename to 'Account-bound Tokens'

* Correct co-author section

* Correct TLA from non-sensical 'ACT' to 'ABT'

- ABT stands for "Account-bound token"

* Remove superfluous, explicit interface definitions

* Add 'Naming' section

* Consistently use EIP-XXXX format

* Remove reference to badge concept

* Correctly apply camelCase in tests

* Update reference implementation

- Vendor all third-party code (OZ)
- Allowing to mint to third-party address
- Rename `_bonds` to `_owners`
- Fix TLA from ACT to ABT
- Fix ABT name in tests

* Replace `event Transfer` w/ Attest/Revoke

* Fix username prefix in author field

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix broken link to eip-721.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix typo

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix link to reference implementation

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-4973.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
FilipLaurentiu added a commit to FilipLaurentiu/soulbound-cairo that referenced this pull request Jun 6, 2022
@inso-
Copy link

inso- commented Jun 12, 2022

@TimDaub I think it would be good to have hooks _beforeTokenTransfer and _afterTokenTransfer (on _mint and _burn) for extensibility (to allow extension like ERC4973Enumerable to be build in the future), like on ERC721.

@MicahZoltu
Copy link
Contributor

@inso- I recommend commenting on the discussions-to link in the EIP so your comment gets appropriate visibility.

@TimDaub
Copy link
Contributor Author

TimDaub commented Jun 13, 2022

@inso-

the reference implementation has _burn and _mint. We're tracking commits as a separate repo here: https://github.com/rugpullindex/ERC4973

nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Add EIP-1238.md

* Rename from 1238 to 4966

* Address feedback from PR

* Change name from 4966 to 4973

* Fix header property order

* Remove 79 character manual word wrapping

* Fix syntax error for incomplete link tag

* Remove Caveat section (for now)

* Clarify standards text

* Run through grammarly

* Update discussion link

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Relicense to CC0-1.0

* More copyediting

* Rename from "Soulbound" to "Account-bound" tokens

* Add section on "Exception handling"

* Add section on revocation

* Update EIPS/eip-4973.md

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Rename to 'Account-bound Tokens'

* Correct co-author section

* Correct TLA from non-sensical 'ACT' to 'ABT'

- ABT stands for "Account-bound token"

* Remove superfluous, explicit interface definitions

* Add 'Naming' section

* Consistently use EIP-XXXX format

* Remove reference to badge concept

* Correctly apply camelCase in tests

* Update reference implementation

- Vendor all third-party code (OZ)
- Allowing to mint to third-party address
- Rename `_bonds` to `_owners`
- Fix TLA from ACT to ABT
- Fix ABT name in tests

* Replace `event Transfer` w/ Attest/Revoke

* Fix username prefix in author field

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix broken link to eip-721.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix typo

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Fix link to reference implementation

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-4973.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-4973.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@rotcivegaf
Copy link
Contributor

rotcivegaf commented May 12, 2023

I played wow in my old days, the idea of soulbound items is that they couldn't be transferred and the only way to remove them was to destroy them

This goes against the specification that the unequip function says that the item can be re-equipable:
"Removes the uint256 tokenId from an account. At any time, an ABT receiver must be able to disassociate themselves from an ABT publicly through calling this function. After successfully executing this function, given the parameters for calling function give or function take a token must be re-equipable."

I don't think this should be re-equipable

Look this case:

  • Alice signs a give transaction to allow Tom to give her an ABT
  • Tom give this ABT to alice
  • Alice unequip the ABT
  • Tom haves the give transaction and can give the ABT to Alice again, and again
    Finally, Tom can spam Alice giving the ABT every time Alice unequip it

@TimDaub
Copy link
Contributor Author

TimDaub commented May 13, 2023

ok I‘m open to it

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.