Hey I really appreciate the focus on simplicity and gas consciousness in this EIP. I’ve seen many lockable NFT proposals and this one is my favorite for the reasons that motivated you. I’m most concerned with the clashing of interfaceId with ERC-5192 that was mentioned already in the thread. I’m close to wanting to put this in my own contracts but am holding off because of that point. Is there a final decision if an interfaceId clash is accepted or are we going to change the naming to “seal” or something else?
1 Like
There was an initial clash because in the initial proposal there was only a locked(tokenId) function, like in ERC5192. Later we added the (actually very useful) defaultLocked() function that solved the issue, altering the interfaceId.
Added small readability improvements Update EIP-6982: Minor Improvements EIP6982 by urataps · Pull Request #7713 · ethereum/EIPs · GitHub before moving to Last Call @sullof
1 Like
The ERC moved today to Last Call. If there is any other comment, please, make it asap.
Some brief thoughts:
-
I might reword the comment for the
DefaultLockedevent. In initially reading the ERC, I thought that emitting a newDefaultLockedevent would only change the locked status for “tokens that will be minted in the future” (and not change the locked status for tokens that had already been minted). -
It is nicer for listeners to only have a single event to subscribe to, but it is slightly more expensive in terms of gas to have two arguments in the event, and have one of them
indexed(as compared toLocked(uint256 tokenId)in ERC-5192). Since one of the goals here is gas efficiency, perhaps that should be considered.
Aside: I’ve been thinking about this as well and put together ERC-7558 - Batch Minimal Soulbound NFTs. Since you’ve been thinking about this problem longer than I have I would appreciate any thoughts / feedback!
Thanks, I will clarify that.
The reason for having a single Locked event is to reduce the amount of code you write in the smart contract. Also to keep the interface as minimalistic as possible. I agree that the locked event consume a bit more gas, but it sounded like a good compromise since the boolean adds only 280 gas.
About your EIP, you could have posted here your suggestions and we could have worked together on it. I think that it is always a good practice to participate in the existing proposal, instead of making a new one just for a detail. Initially, I asked for changes in the ERC5192, but it was already in final state when I saw it, and than I was forced to start a new one, but if not I would have definitely tried to improve 5192 if it was not finalized.
The current description says:
// This event MUST be emitted upon deployment of the contract, establishing
// the default lock status for any tokens that will be minted in the future.
// If the default lock status changes for any reason, this event
// MUST be re-emitted to update the default status for all tokens.
I thought that last line is clear enough specifying that must be re-emitted to update the status for all tokens. I modified it this way:
// This event MUST be emitted upon deployment of the contract, establishing
// the default lock status for any tokens that will be minted in the future.
// If the default lock status changes for any reason, this event
// MUST be re-emitted to update the default status for all tokens, including the
// ones already minted.
I think that’s sufficient to clear it up - but I might even go with something like this (but realize this is a minor nit)
// This event MUST be emitted upon deployment of the contract,
// establishing the default lock status for this contract's tokens.
// If the default lock status changes for any reason, this event
// MUST be re-emitted to update the default status for all tokens,
// including the ones already minted.
//
Makes sense - I wasn’t sure what the best practice was here. Since this proposal is incompatible with ERC-5192 though, I do think it might still make sense to have a separate proposal that merely extends that, especially since 5192 has some adoption already (supported by OpenSea).
You proposed this in the other thread - but perhaps it should be a part of the core ERC-6982 proposal? I think having all of [defaultStatus, individualTokenStatus, tokenRange] would make this proposal even more flexible.
Actually, it’s not incompatible. In fact, it simply adds two more events to ERC-5192, so the type(ERC7558).interfaceId would remain the same. I’m assuming you’re suggesting a unique interfaceId for it, perhaps something like 0x75887588. For all these reasons, I believe that extending ERC-5192 is quite reasonable.
The issue here is that ERC-6982 is intended to be a generic proposal. If we were to add anything to this proposal, it should be treated as an extension. I think it is better to keep the proposal as it is.
Moreover, ERC-6982 is expected to be finalized by the end of the month. I’m not sure if it’s appropriate to include an extension at this stage.
@urataps, what are your thoughts?
1 Like
@SamWilsn I would appreciate your opinion about the point in the previous comment about adding an extension like this to the proposal (with a magic interfaceId like in ERC4906)
interface ERC6892Range is ERC6982 {
event RangeLocked(uint256 fromTokenId, uint256 toTokenId, boolean locked);
}
My primary doubt is that it would make harder for the listeners to understand what the events mean.
I tried to improve the comments in the interface, but the PR produces errors I can’t fix, and that could stuck the process. If so, I may close it and leave the description as it is now. Let’s hope for the best.
1 Like
I am notoriously bad at predicting what developers will find easy to understand 
2 Likes
ERC6982 has been finalized. Thanks everyone for the contributions.
2 Likes
Waiting for Open Zeppelin to (hopefully) add the interface in its contracts, you can get it with
npm install @ndujalabs/erc721lockable
@ndujalabs/erc721lockable is the implementation of an NFT lockable in pools. You don’t need to implement it, of course, just import the IERC6982 interface as
import {IERC6982} from "@ndujalabs/erc721lockable/IERC6982.sol";
Hi,
I’m glad to see this moved through! We’re a bit slower with the Asset-Bound NFTs (ERC-6956), as it’s a quite complex proposal.
ERC-6982 duplicates essentially parts of the mechanics of our optional IERC6956Floatable interface, in particular the events FloatingStateChange, FloatingAllStateChange.
To avoid standard-duplication and also motivate external platforms (e.g. OpenSea) to implement interfaces, I’m inclined to drop our two events from IERC6956Floatable and RECOMMEND implementing ERC-6982 together with IERC6956Floatable. I think this could also be included in the Full reference implementation of ERC-6956 (which implements all optional interfaces).
The logic would be:
- Implement Events from ERC-6982 instead of
FloatingStateChange,FloatingAllStateChange locked(tokenId)returns!floating(anchorByToken(tokenId))defaultLocked()is implemented additionally.
Questions
- Do you see any conflicts between ERC-6956 and your ERC-6982?
- This marries IERC6956Floatable somewhat to ERC-6982, although ERC-6956 cannot formally list ERC-6982 as required for ERC-6956, since only an optional extension is affected. Anybody seeing issues with this?
Thanks!
I don’t see problems with the functions, but you may have issues with the events. For example
event FloatingStateChange(bytes32 indexed anchor, uint256 indexed tokenId, FloatState isFloating, address operator);
expects 4 parameters, instead of 2, which means that if you replace it with the Locked events in IERC6982, you would either lose information or having to rethink the logic .
Maybe @SamWilsn can respond to that.
If you’re talking about the required preamble header, think of it more as “required reading” and not “required to implement”.
1 Like
Thanks for your comments @sullof and @SamWilsn!
Were considering to simplify our whole proposal and make the mapping tokenId <> anchor optional. A much simpler way with identical security is when anchor (bytes32) is used directly as a tokenId (uint256 == bytes32).
This would revert our rationale “Why do you use an anchor<>tokenId mapping and not simply use tokenIds directly?”. Consequently, we’ll need to change all ERC-6956 events and omit the anchor from events. Since I do not know of any large adoptions besides ours yet, this breaking change seems acceptable. dApps interested in the mapping anchor <> tokenId will need to call anchorByTokenId().
Yes I was, thanks for the clarification! If it is only “required to read” it’s fine IMHO to list ERC-6982 there.
1 Like