ERC-6982: Default Lockable Proposal

[sullof]

I am not sure I got it. The proposal does not give any recommendation about how to lock/unlock tokens, leaving a lot of freedom to the user.

[sullof]

@SamWilsn

Personally I would not expect the status of already minted tokens to change when the default changes.

The default status does not apply only to un-minted tokens, it applies to any token. But a token is not in an undefined status, it is in the default status.
The idea is that at deployment, before than any token is minted, the default status is defined. Than, any minted token inherit that status. So, if initially the tokens are unlocked, all the minted tokens will be unlocked.
Let’s say that — like in the ERC721Lockable example — the tokens are stakable in a pool, which would mean that the pool is authorized to lock the token, if an owner “stakes” their token, that token is locked.
At that point, even if the default status would change, that specific token would remain locked, because it has been locked by the pool (which clear the doubt I had, actually).

Maybe, it makes sense to request that the default locked status can be changed only before than any single status changes.

[SamWilsn]

Ah, I think I misunderstood your comment then.

[sullof]

I will try to make the text of the ERC more clear. Maybe ChatGPT can help me edit it

[sullof]

I updated the comments in the interface. Now it looks like this and I think that the recommendations are more clear:

interface IERC6982 {
  // 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.
  // Note that emitting a new DefaultLocked event does not affect the lock 
  // status of any tokens for which a Locked event has previously been emitted.
  event DefaultLocked(bool locked);

  // This event MUST be emitted whenever the lock status of a specific token 
  // changes, effectively overriding the default lock status for this token.
  event Locked(uint256 indexed tokenId, bool locked);

  // This function returns the current default lock status for tokens.
  // It reflects the value set by the latest DefaultLocked event.
  function defaultLocked() external view returns (bool);

  // This function returns the lock status of a specific token.
  // If no Locked event has been emitted for a given tokenId, it MUST return 
  // the value that defaultLocked() returns, which represents the default 
  // lock status.
  // This function MUST revert if the token does not exist.
  function locked(uint256 tokenId) external view returns (bool);
}

[hiddenintheworld]

We still have a long way to go on this one, more concrete end goals are needed:

1. Where is the interoperability with another contract (what state should be mutable)
2. Any threshold state variable?
3. Does the owner control it all?

[sullof]

@hiddenintheworld

Where is the interoperability with another contract (what state should be mutable)

The contract has two main states: locked and unlocked. These states are mutable, but the exact rules of mutation, such as who can change them and under what conditions, are left to the discretion of the implementer.

As an example implementation of the interface, I utilized the ERC721Lockable contract by ndujaLabs.

interface IERC721Lockable is IERC6982 {
  event LockerSet(address locker);
  event LockerRemoved(address locker);
  event ForcefullyUnlocked(uint256 tokenId);

  function lockerOf(uint256 tokenID) external view returns (address);
  function isLocker(address _locker) external view returns (bool);
  function setLocker(address pool) external payable;
  function removeLocker(address pool) external;
  function hasLocks(address owner) external view returns (bool);
  function lock(uint256 tokenID) external;
  function unlock(uint256 tokenID) external;
  function unlockIfRemovedLocker(uint256 tokenID) external;
}

It extends ERC6982 and adds the necessary features to make the token “lockable.” The intention here is to allow tokens to be “staked” in a pool without the need for actual staking, instead, the tokens are simply locked in place leaving the ownership to the tokenId’s owner.

However, this is merely one possible scenario. The goal of this proposal is not to dictate the specifics of implementation, but rather to provide a basic framework. This framework aims to be as flexible as possible, allowing for various implementations of lockable tokens, similar to what ERC173 and ERC5313 do with ownership.

@hiddenintheworld

Does the owner control it all?

The role of the owner varies depending on the specific implementation and scenario. In the example of the ERC721Lockable contract, the owner of the NFT grants approval to a pool, which then has the ability to lock/unlock the token.

In other scenarios, such as soulbounds and badges, it’s likely that the contract owner is the one who locks them. Yet, other scenarios may present different criteria. This proposal does not prescribe a specific role for the owner. Instead, it aims to establish a common base that can be adapted to various needs and contexts.

[SamWilsn]

Your comment, @sullof, on the pull request was most illuminating:

About the apparent contradiction, there are tokens that are locked during the pre-reveal phase and so would emit a DefaultLocked(true) event, that will be unlocked after the reveal. In that case, forcing the contract to emit a Locked(tokenId, false) for every tokenId would go against the primary goal behind this proposal, i.e., saving gas when a locked/unlocked status is applicable to all the tokens. I think that if that happens, emitting a single DefaultLocked(false) event would be better.

Now I understand why changing the DefaultLocked status could be useful.

I’m still concerned that an application subscribing to the events from a ERC-6982 compatible contract cannot unambiguously know the lock state of a token. After the following events, what is the locked state of 0x1, 0x2, and 0x3, and which requirements from the EIP determine those states?

1. DefaultLocked(locked=true)
2. Transfer(_from=0x000...000, _to=0xaaa...aaa, _tokenId=0x1)
3. Transfer(_from=0x000...000, _to=0xaaa...aaa, _tokenId=0x2)
4. Transfer(_from=0x000...000, _to=0xaaa...aaa, _tokenId=0x3)
5. Locked(tokenId=0x1, locked=true)
6. Locked(tokenId=0x2, locked=false)
7. DefaultLocked(locked=false)

[sullof]

After 1, 2, 3, 4
0x1, 0x2 and 0x3 are locked

After 5
0x1 is locked because a specific event has been emitted for it

After 6
0x2 is unlocked because a specific event has been emitted for it

After 7
0x1 and 0x2 keeps their status, since a Locked event was emitted for them
0x3 moves from locked to unlocked because no specific event has been emitted for it and so it refers to default Locked that in the meantime has changed

In general, the rule is that DefaultLocked refers to any tokenId for which no Locked events has been emitted. In other words, the Locked event has priority on DefaultLocked

[hiddenintheworld]

lock and unlock can be written into

function flip(){
locked = !locked;
}

to always flip the state, so we have one function instead of two, but this may not be good if you want to have a smart contract that locks something, even if it is already locked, you don’t want to flip the state but still wants to lock is again(not changing the state).

[sullof]

That flip function make sense. However, the interface is supposed to include only generic events and views that work in most scenarios. Any function to manage the status is left to the specific implementation.

[sullof]

Contracts that may use ERC-6982 as a base for pools and other DeFi systems, may need to log the address of the locker/unlocker. I am considering the possibility of moving from

event Locked(uint256 indexed tokenId, bool locked);

to

event Locked(uint256 indexed tokenId, bool locked, address locker);

where the third parameter is optional and can be set to ZeroAddress when unnecessary.

Personally, since I have implemented the interface in production, this change would put my deployed contracts off-standard, but that may be solved by ERC-5269 by @xinbenlv

@urataps @SamWilsn @xtools-at @tbergmueller What do you think?

[urataps]

@sullof

Contracts that may use ERC-6982 as a base for pools and other DeFi systems, may need to log the address of the locker/unlocker

Personally, I think the third locker argument in the event is unnecessary, the proposal should be generic enough to cover all use-cases and not delve into implementation details. If some specific applications need to emit the address of the locker, they can create a separate event for that. Otherwise, other contracts are forced to emit the redundant zero address that may incur additional gas fees.

Also, I want to catch up on this previous comment.

@sullof

The primary area of uncertainty pertains to whether a newly emitted DefaultLocked event should supersede all previously emitted Locked events, or whether it should only apply to tokens that have not yet been impacted by a Locked event.

Now I understand there are two cases to be addressed: 1) The DefaultLocked event resets any token-specific Lock state or 2) individual token-specific Lock is not impacted by a default state change. I see prospective use-cases in both situations and this proposal shouldn’t be bound just to one. Therefore I consider the introduction of a second bool override argument to the DefaultLocked event as appropriate, since it eliminates this ambiguity.

[sullof]

@urataps

I think the third locker argument in the event is unnecessary, the proposal should be generic enough to cover all use-cases and not delve into implementation details.

I concur with your viewpoint. It’s indeed beneficial to maintain the proposal’s simplicity and generality, avoiding getting entangled in specific implementation details.

@urataps

Therefore I consider the introduction of a second bool override argument to the DefaultLocked event as appropriate, since it eliminates this ambiguity.

Upon testing various scenarios, I’ve observed that altering the default status post utilization of tokens is a rare occurrence. The usual pattern involves locking during a sale, followed by unlocking after the sale, with the default status generally remaining untouched thereafter.

If we consider a situation where numerous assets are locked while the default status is set to unlocked, can we identify a context where you’d desire to unlock all assets simultaneously? I find it hard to visualize.

Let’s consider the following sequence:

1. The default status is unlocked.
2. TokenId #3 gets locked.
3. defaultLocked is switched to locked (TokenId #3 remains unaffected).
4. defaultLocked is switched back to unlocked.

This presents an odd scenario that doesn’t seem to hold much practical significance. Consequently, I lean towards maintaining the current structure without the override feature.

[tbergmueller]

I tend to agree with @urataps that the address of locker may not be needed in many use-cases. Also for the DeFi-UseCase you referenced I believe that in many scenarios rather the address(es), who have the authority to unlock a token, would be more interesting.

Unlocking is often more complex, as more than one party may have permission to unlock and the permission to unlock may change during lock-time as well (e.g. you get a loan with an NFT as collateral, the loan may get sold to another DeFi account, so suddenly the new DeFi account can unlock it.

I think an implementers interested in recording who locked/unlocked a token would rather implement additional lockerOf(tokenId) and canUnlock(tokenId, address operator) views.

[hiddenintheworld]

Maybe add a function to lock and unlock individual tokens: While the interface allows for locking and unlocking of tokens through the Locked event, it might be useful to provide a function that enables contract owners to lock and unlock individual tokens programmatically. This function could take a tokenId parameter and a bool parameter to indicate the new lock status of the token.

1. You can also add a mapping function and a variable that allows all mapped NFT to this pointer could be toggled as it is mapped separately, this gives more flexibility.

2. When the mapping is disabled, then it will revert back to its initial state (assume it is False here, meaning no locking)

[urataps]

@sullof

If we consider a situation where numerous assets are locked while the default status is set to unlocked, can we identify a context where you’d desire to unlock all assets simultaneously?

I understand our problem now, for me it’s hard to visualize such a scenario as well, once a token’s lock status is individually changed then a default override should not make sense. The contract can also provide means to deactivate the individual status for the default one, but that is an implementation detail we should not pursue in this proposal.

So in my opinion, for your provided scenario, the step 4 would leave TokenId#3 untouched. But, just for the sake of curiosity, let’s assume step 4 would change the status of TokenId#3 to unlocked in some kind of strict marketplace, there still is the locked(uint256 tokenId) function to rely on in this case.

@hiddenintheworld

Maybe add a function to lock and unlock individual tokens: While the interface allows for locking and unlocking of tokens through the Locked event, it might be useful to provide a function that enables contract owners to lock and unlock individual tokens programmatically.

I see why such a function would make sense, but because of the vast set of possible implementations of the lock/unlock process, this proposal should not specify any particular locking function. Consider the token standards, they provide the events and the view methods for querying tokens, but not the minting function. It’s left at the discretion of the implementer of the standard, which is responsible only for emitting the events and set the state correspondingly.

@hiddenintheworld

1. You can also add a mapping function and a variable that allows all mapped NFT to this pointer could be toggled as it is mapped separately, this gives more flexibility.

2. When the mapping is disabled, then it will revert back to its initial state (assume it is False here, meaning no locking)

I’m sorry I don’t understand the idea here. What is mapped to what? What kind of flexibility would we get if we introduce this mapping? What do you mean by disable a mapping?

[sullof]

I appreciate your insights and I believe the consensus leans towards maintaining the existing interface as it is.

To address @urataps’ point, indeed, the locked(uint256 tokenId) function does provide a safety net. However, this interface was primarily designed to streamline the process for event loggers. It’s doubtful that platforms like OpenSea would regularly invoke locked(id) to verify a token’s status. For a more dynamic handling of the locking status, projects could consider adopting ERC6454.

As for @hiddenintheworld’s suggestion of a function that allows contract owners to manipulate the lock status of individual tokens programmatically, it’s a valid point. But introducing more functions could inadvertently favor certain implementations over others, which might not be ideal. It’s crucial to remember that these functions should be tailored to suit the specific objectives of the project. For instance, consider the interface used in ERC721Lockable (erc721lockable/contracts/IERC721Lockable.sol at main · ndujaLabs/erc721lockable · GitHub).
I did contemplate proposing it as a standard, but considering the vast array of unique use cases in this field, it dawned on me that standardizing it might not be the best course of action.

[sullof]

@urataps I am using this in production in many projects and I know of others that are doing the same. I am moving it to the review stage. Is that ok for you?

[urataps]

Sure it is @sullof, I think the proposal can be moved to review stage