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: Efficient Default Lockable Tokens #6982

Merged
merged 46 commits into from
Jun 8, 2023

Conversation

sullof
Copy link
Contributor

@sullof sullof commented May 3, 2023

We suggest a solution that is applicable in most scenarios, saving gas.

pragma solidity ^0.8.9;

// ERC165 interfaceId 0x6b61a747
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);
}

The primary limit in existing proposal like EIP-5192 is that

  1. it has 2 events for Locked and Unlocked, which is not optimal.
    To make a comparison, it's like in the ERC721 instead of Transfer(from, to, id) used for mints, transfers and burns, there were Transfer(from, to, id), Mint(to, id), Burn(from, id), etc.

  2. it forces you to emit an event even when the token is minted, causing a waste of gas when a token borns with a status and dies with it.

Take for example most soulbounds and non-transferable badges. They will be locked forever and it does not make sense to emit an extra event for all the tokens.

@sullof sullof requested a review from eth-bot as a code owner May 3, 2023 17:25
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels May 3, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 3, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title Proposing a new standard for lockable tokens Add EIP: Minimalistic Efficient Lockable tokens May 3, 2023
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels May 3, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label May 3, 2023
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed w-ci Waiting on CI to pass labels May 3, 2023
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed w-ci Waiting on CI to pass labels May 3, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label May 3, 2023
@poojaranjan
Copy link
Contributor

@sullof
With the allocated time of EIP Editing Office Hour, the editor could review 14/19 PRs added to the agenda. Most likely, the rest of the proposals will be reviewed and merged before the next meeting by one of the editors. If not, we will add them for review to the next agenda.

EIPS/eip-6982.md Outdated Show resolved Hide resolved
EIPS/eip-6982.md Outdated Show resolved Hide resolved
EIPS/eip-6982.md Outdated Show resolved Hide resolved
assets/eip-6982/README.md Outdated Show resolved Hide resolved
assets/eip-6982/contracts/ERC721LockableUpgradeable.sol Outdated Show resolved Hide resolved
assets/eip-6982/hardhat.config.js Outdated Show resolved Hide resolved
assets/eip-6982/pnpm-lock.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot added s-draft This EIP is a Draft and removed s-review This EIP is in Review labels May 30, 2023
EIPS/eip-6982.md Outdated

## Rationale

This standard aims to minimize gas usage by reducing the emission of events. The **DefaultLocked** event sets the initial lock status for all tokens, thus eliminating the need for an event upon the minting of each token. Some tokens may change their behavior (for example, after a reveal), in that case, the **DefaultLocked** event may be emitted again to set up the new default behavior when no token-specific event has been emitted yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're defining a requirement here:

the DefaultLocked event may

Requirements should go in the specification section.

Plus I believe this is directly contradicting the comment above the DefaultLocked event definition.

Copy link
Contributor Author

@sullof sullof Jun 2, 2023

Choose a reason for hiding this comment

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

Thanks, I will move that requirements it the specification section.

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.

@github-actions github-actions bot added s-review This EIP is in Review and removed s-draft This EIP is a Draft labels Jun 3, 2023
@github-actions github-actions bot added s-draft This EIP is a Draft and removed s-review This EIP is in Review labels Jun 3, 2023
@eth-bot eth-bot enabled auto-merge (squash) June 8, 2023 21:05
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 33f5b65 into ethereum:master Jun 8, 2023
@sullof sullof deleted the efficient-lockable-nft branch June 10, 2023 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants