ERC-6982: Default Lockable Proposal

Tokens
41

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
42

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.

43

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
44

The ERC moved today to Last Call. If there is any other comment, please, make it asap.

45

Some brief thoughts:

  1. I might reword the comment for the DefaultLocked event. In initially reading the ERC, I thought that emitting a new DefaultLocked event 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).

  2. 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 to Locked(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!

46

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.

47

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.
48

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.

49

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
50

@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.

51

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
52

I am notoriously bad at predicting what developers will find easy to understand :sweat_smile:

2 Likes