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 ERC: Minimal Lockable Range NFTs #103

Closed
wants to merge 14 commits into from

Conversation

0xCLARITY
Copy link

@0xCLARITY 0xCLARITY commented Nov 15, 2023

Add ERC to support batch operations around soulbound/lockable 721 NFTs.

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Nov 15, 2023

File ERCS/erc-7558.md

Requires 1 more reviewers from @axic, @g11tech, @Pandapip1, @SamWilsn, @xinbenlv

@eip-review-bot eip-review-bot changed the title Add ERC: Batch Minimum SBTs Add ERC: Batch Minimal Soulbound NFTs Nov 15, 2023
@github-actions github-actions bot added the w-ci label Nov 15, 2023
@github-actions github-actions bot added w-ci and removed w-ci labels Nov 16, 2023
@0xCLARITY 0xCLARITY changed the title Add ERC: Batch Minimal Soulbound NFTs Add ERC-7558: Batch Minimal Soulbound NFTs Nov 16, 2023
@eip-review-bot eip-review-bot changed the title Add ERC-7558: Batch Minimal Soulbound NFTs Add ERC: Batch Minimal Soulbound NFTs Nov 16, 2023
@github-actions github-actions bot added w-ci and removed w-ci labels Nov 16, 2023
@eip-review-bot eip-review-bot changed the title Add ERC: Batch Minimal Soulbound NFTs Add ERC: Minimal Lockable Range NFTs Nov 22, 2023
@github-actions github-actions bot removed the w-ci label Nov 22, 2023
@github-actions github-actions bot added the w-ci label Nov 22, 2023
@0xCLARITY 0xCLARITY changed the title Add ERC: Minimal Lockable Range NFTs Add ERC 7558: Minimal Lockable Range NFTs Nov 28, 2023
@github-actions github-actions bot removed the w-ci label Nov 28, 2023
@eip-review-bot eip-review-bot changed the title Add ERC 7558: Minimal Lockable Range NFTs Add ERC: Minimal Lockable Range NFTs Nov 28, 2023
@github-actions github-actions bot added the w-ci label Nov 28, 2023
Copy link

The commit 1003b82 (as a parent of f50e2d1) contains errors.
Please inspect the Run Summary for details.

@0xCLARITY
Copy link
Author

@abcoathup - what are the next steps to get this merged in as a "Draft" or "Review" status?

There's been some discussion on the corresponding thread, and I've adopted some requested changes:
https://ethereum-magicians.org/t/erc-7558-minimal-lockable-range-nfts/16668

@abcoathup
Copy link
Contributor

Need to wait for an ERC editor to review.

Copy link

github-actions bot commented Jan 9, 2024

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added w-stale and removed w-stale labels Jan 9, 2024
@0xCLARITY
Copy link
Author

I have addressed all required feedback and am ready for a review

Copy link

github-actions bot commented Feb 2, 2024

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@@ -0,0 +1,69 @@
---
eip: 7558
title: Minimal Lockable Range NFTs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is minimal here describing the standard, or the range? I'd try to reduce the ambiguity in your title.

---
eip: 7558
title: Minimal Lockable Range NFTs
description: A minimal interface for soulbinding multiple ERC-721 NFTs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Including "minimal" here in your description doesn't really add a lot of information, nor would it help to distinguish this proposal from hypothetical others in a similar theme. Try to make your description highlight what makes this proposal special/interesting.

type: Standards Track
category: ERC
created: 2023-11-15
requires: 165, 721
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
requires: 165, 721
requires: 165, 721, 5192

EIP-5192 is definitely required reading if the interface here extends IERC5192.


## Abstract

This proposal extends [ERC-721](./eip-721.md). It proposes a minimal interface to make both individual tokens and a contiguous range of tokenIds soulbound (locked).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This proposal extends [ERC-721](./eip-721.md). It proposes a minimal interface to make both individual tokens and a contiguous range of tokenIds soulbound (locked).
This proposal extends [ERC-721](./eip-721.md) and [ERC-5192](./eip-5192.md). It proposes a minimal interface to make both individual tokens and a contiguous range of tokenIds locked.

Adding 5192 and removing "soulbound" since these can be unlocked.


The [ERC-165](./eip-165.md) interfaceId is `0x75587558`.

> NOTE: The ERC-165 identifier doesn't strictly follow ERC-165 calculations, but there is prior art to assigning a custom identifier to avoid conflict or for ERCs that do not include any functions, like [ERC-4906](./eip-4906.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this to Rationale.

}
```

The Locked/Unlocked or RangeLocked/RangeUnlocked event MUST be emitted when the locking status of a token, or a consecutive range of tokens, is changed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explicitly state when each type of event must be emitted. Do Locked / Unlocked events need to be emitted for every token in the range? I would assume not.


## Backwards Compatibility

This proposal is fully backward compatible with [ERC-721](./eip-721.md).
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should note the backwards incompatible changes to ERC-5192 here too. I assume software watching events would need to treat this kind of contract specially?

It might even be worth sketching out an algorithm for indexer software to follow to determine what types of events to expect.

@github-actions github-actions bot removed the w-stale label Feb 3, 2024
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants