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-5192 - Minimal Soulbound NFTs #5192

Merged
merged 11 commits into from
Aug 1, 2022

Conversation

TimDaub
Copy link
Contributor

@TimDaub TimDaub commented Jun 30, 2022

  • I was looking at many of the current Soulbound token implementations and it upset me that they all revert upon transfer
  • At least it'd be nice if developers would allow wallets to understand that a token is permanently non-transferrable
  • With my previous attempt at Soulbound tokens (EIP-4973), we got a bit lost in the details
  • This attempt is much cleaner: Yes, it still permanently binds a token to an account. But that's fine: Users are doing this and it'd be dogmatic to make that a taboo around here
  • Rather, with this change, I think, if implementers adopt it, it'll actually make the life of wallet implementers easier.
  • I put status: Review right away. I think this EIP is done and can be peer-reviewed

@eth-bot
Copy link
Collaborator

eth-bot commented Jun 30, 2022

All tests passed; auto-merging...

(pass) eip-5192.md

classification
updateEIP
  • passed!

@TimDaub TimDaub changed the title Add EIP-5555 Add EIP-5192 Jun 30, 2022
@TimDaub TimDaub changed the title Add EIP-5192 Add EIP-5192 - Minimal Soulbound NFTs Jun 30, 2022
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.

Looks great! Here are a few suggestions.

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

Upon minting an EIP-5192 token, it must be permanently inseparable from the receiving account. All [EIP-721](./eip-721.md) functions of the contract that transfer the token from one account to another must throw.

To aid recognition that an [EIP-721](./eip-721.md) token implements soulbinding via `EIP-5192`, upon calling [EIP-721](./eip-721.md)'s `function supportsInterface(bytes4 interfaceID) external view returns (bool)` with EIP-5192's `SOULBOUND_VALUE=0x9e7d7f8` as `bytes4 interfaceID`, a contract implementing `EIP-5192` must return `true`.
Copy link
Member

Choose a reason for hiding this comment

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

I think perhaps something even simpler (and much more versatile!) is to add a function soulbound(uint256 tokenId) external view returns (bool tokenIdSoulbound), which would even allow for soulbounding to be managed on a per-tokenId basis. This is by no means necessary though.

Copy link

@colinnielsen colinnielsen Jul 7, 2022

Choose a reason for hiding this comment

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

I like the interface you proposed, a token's "soulboundness" can be known implicitly by calling ownerOf(tokenId) in the 4973 spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the interface you proposed, a token's "soulboundness" can be known implicitly by calling ownerOf(tokenId) in the 4973 spec.

no. Unless I'm missing something

EIPS/eip-5192.md Outdated Show resolved Hide resolved
EIPS/eip-5192.md Show resolved Hide resolved
EIPS/eip-5192.md Outdated Show resolved Hide resolved
EIPS/eip-5192.md Outdated Show resolved Hide resolved
EIPS/eip-5192.md Outdated Show resolved Hide resolved
EIPS/eip-5192.md Outdated Show resolved Hide resolved
TimDaub and others added 4 commits July 26, 2022 16:30
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
@github-actions
Copy link

The commit 7bc6ea7 (as a parent of aa08663) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit 8440917 (as a parent of 49e598a) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit f6b0b57 (as a parent of cee63ba) contains errors. Please inspect the Run Summary for details.

EIPS/eip-5192.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aramalipoor aramalipoor left a comment

Choose a reason for hiding this comment

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

Looks good to me 💯 Hopefully a good minimal step towards Soulboundedness (or Soulbinding) 😄

@TimDaub
Copy link
Contributor Author

TimDaub commented Jul 26, 2022

seems ready to merge

@TimDaub
Copy link
Contributor Author

TimDaub commented Aug 1, 2022

@MicahZoltu is there anything that we're missing to get this merged? Seems the automerge got stuck somehow

@MicahZoltu MicahZoltu closed this Aug 1, 2022
@MicahZoltu MicahZoltu reopened this Aug 1, 2022
@eth-bot eth-bot enabled auto-merge (squash) August 1, 2022 10:11
@eth-bot eth-bot merged commit 40617ab into ethereum:master Aug 1, 2022
@TimDaub
Copy link
Contributor Author

TimDaub commented Aug 1, 2022

thank you!

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

* 5555->5192

* Add discussions link

* Fix line break

* Update EIPS/eip-5192.md

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

* Update EIPS/eip-5192.md

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

* Update EIPS/eip-5192.md

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

* Update EIPS/eip-5192.md

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

* Add function locked(...)

* Attempt to fix tests

* Update EIPS/eip-5192.md

Co-authored-by: aram.eth <[email protected]>

Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: aram.eth <[email protected]>
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