-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
EIP5007: TimeNFT, ERC-721 Time Extension #5007
Conversation
All tests passed; auto-merging...(pass) eip-5007.md
(pass) assets/eip-5007/.gitignore
(pass) assets/eip-5007/README.md
(pass) assets/eip-5007/contracts/ERC5007.sol
(pass) assets/eip-5007/contracts/ERC5007Demo.sol
(pass) assets/eip-5007/contracts/IERC5007.sol
(pass) assets/eip-5007/migrations/1_initial_migration.js
(pass) assets/eip-5007/package.json
(pass) assets/eip-5007/test/test.js
(pass) assets/eip-5007/truffle-config.js
|
Thanks for this, it is a great idea. Here are some initial things to consider.
|
EIPS/eip-5007.md
Outdated
|
||
## Abstract | ||
|
||
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional property( `startTime`, `endTime`,`originalTokenId`) to help with the on-chain time management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional property( `startTime`, `endTime`,`originalTokenId`) to help with the on-chain time management. | |
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional properties (`startTime`, `endTime`,`originalTokenId`) to help with on-chain time management. |
EIPS/eip-5007.md
Outdated
|
||
## Motivation | ||
|
||
Some NFTs have a defined usage period and cannot be used when they are not at a specific time. If you want to make NFT invalid when it is not in use period, or make NFT enabled at a specific time, while the NFT does not contain time information, you often need to actively submit the chain transaction, this process is both cumbersome and a waste of gas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include an example of what kind of NFTs have a time component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include an example of what kind of NFTs have a time component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant in the EIP itself 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good standard for utility tokens as we move towards more decentralization. Access can be token gated for a period of time. Hence, no need to trust the party to burn or transfer them.
EIPS/eip-5007.md
Outdated
|
||
## Motivation | ||
|
||
Some NFTs have a defined usage period and cannot be used when they are not at a specific time. If you want to make NFT invalid when it is not in use period, or make NFT enabled at a specific time, while the NFT does not contain time information, you often need to actively submit the chain transaction, this process is both cumbersome and a waste of gas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some NFTs have a defined usage period and cannot be used when they are not at a specific time. If you want to make NFT invalid when it is not in use period, or make NFT enabled at a specific time, while the NFT does not contain time information, you often need to actively submit the chain transaction, this process is both cumbersome and a waste of gas. | |
Some NFTs have a defined usage period and cannot be used outside of that period. With traditional NFTs that do not include time information, if you want to mark a token as invalid or enable it at a specific time, you need to actively submit a transaction—a process both cumbersome and expensive. |
EIPS/eip-5007.md
Outdated
|
||
Some NFTs have a defined usage period and cannot be used when they are not at a specific time. If you want to make NFT invalid when it is not in use period, or make NFT enabled at a specific time, while the NFT does not contain time information, you often need to actively submit the chain transaction, this process is both cumbersome and a waste of gas. | ||
|
||
There are also some NFTs contain time functions, but the naming is different, third-party platforms are difficult to develop based on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also some NFTs contain time functions, but the naming is different, third-party platforms are difficult to develop based on it. | |
Some existing NFTs contain time functions, but their interfaces are not consistent, so it is difficult to develop third-party platforms for them. |
EIPS/eip-5007.md
Outdated
/// @notice Emitted when the `startTime` or `endTime` of a NFT is changed | ||
/// @param tokenId The tokenId of the NFT | ||
/// @param startTime The new start time of the NFT | ||
/// @param endTime The new end time of the NFT | ||
event TimeUpdate(uint256 tokenId, uint64 startTime, uint64 endTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this emitted when tokens are minted/burned? Is the behaviour in constructors different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this emitted when tokens are minted/burned? Is the behaviour in constructors different?
TimeUpdate
will not be emitted when tokens are minted/burned. I will change this event to MetadataUpdate
of EIP4906
function merge(uint256 firstTokenId, uint256 secondTokenId, address newTokenOwner) external returns(uint256); | ||
} | ||
``` | ||
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
EIPS/eip-5007.md
Outdated
/// @notice Get the token id which this NFT mint from | ||
/// @dev Throws if `tokenId` is not valid NFT | ||
/// @param tokenId The tokenId of the NFT | ||
/// @return The token id which this NFT mint from | ||
function originalTokenId(uint256 tokenId) external view returns (uint256); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this does from this comment alone.
I'd guess that if you had a token 0x01
, and you split it into 0x01
and 0x02
, then originalTokenId(0x02) == 0x01
?
What happens if you then merged 0x02
with 0x03
, which happened to be adjacent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess that if you had a token
0x01
, and you split it into0x01
and0x02
, thenoriginalTokenId(0x02) == 0x01
?
Yes, originalTokenId(0x02) == 0x01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you then merged
0x02
with0x03
, which happened to be adjacent?
I'm still curious about this case.
EIPS/eip-5007.md
Outdated
|
||
### Time Data Type | ||
|
||
The max value of `uint64` is 18446744073709551615, timestamp 18446744073709551615 is about year 584942419325. `uint64` is sufficient and saves gas compared to `uint256`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually save gas? I thought all integer types encoded to 32 bytes: https://docs.soliditylang.org/en/develop/abi-spec.html#examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually save gas? I thought all integer types encoded to 32 bytes:
struct TimeNftInfo { uint256 originalTokenId; uint64 startTime; uint64 endTime; }
encoded to 64 bytes.
struct TimeNftInfo { uint256 originalTokenId; uint256 startTime; uint256 endTime; }
encoded to 96 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason,uint256
is too big for some software such as MySQL, Elastic Search. uint64
is natively supported on many software and programming languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason,uint256 is too big for some software such as MySQL, Elastic Search. uint64 is natively supported on many software and programming languages.
That's still completely arbitrary.
I just ran some tests on a project that already makes heavy use of time-based NFTs to confirm whether it was even worth it to switch to uint64 for storage. Our gas costs actually ended going up...
I highly recommend switching to uint256 completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran some tests on a project that already makes heavy use of time-based NFTs to confirm whether it was even worth it to switch to uint64 for storage. Our gas costs actually ended going up...
Could you please send the code link of this time-based NFTs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's private at the moment, but the reasoning for can be elaborated on. There is still some minimal amount of arithmetic and comparisons against other uint256's, and the uint64 => uint256 transitions adds gas everytime. For example, a common use-case is to add or compare against now (block.timestamp
) which is natively uint256
.
EIPS/eip-5007.md
Outdated
|
||
## Backwards Compatibility | ||
|
||
As mentioned in the specifications section, this standard can be fully ERC721 compatible by adding an extension function set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the specifications section, this standard can be fully ERC721 compatible by adding an extension function set. | |
As mentioned in the specifications section, this standard can be fully ERC-721 compatible by adding an extension function set. |
EIPS/eip-5007.md
Outdated
|
||
## Security Considerations | ||
|
||
Implementors of the `ERC5007` standard must consider the condition of `merge` function. The `originalTokenId` of two NFTs should be same and the time of two NFTs should be adjacent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add more test code later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more the English description of the security consideration. I don't understand what you're trying to convey. I think it just needs to be reworded a bit to be more understandable.
An old token is not support this EIP could be wrapped to this standards. |
Extending existing NFTs can be an optional addon / separate interface later, since it nothing to do with a general interface for time-based NFTs. See how OpenZeppling wraps ERC20 tokens to make them compatible with voting / governance: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Wrapper.sol Remember, small interfaces allow for better composability and modularity (good software design). My suggestion is that you want |
Thanks for your suggestion, I will discuss this with my team. |
EIPS/eip-5007.md
Outdated
|
||
## Abstract | ||
|
||
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional properties( `startTime`, `endTime`,`originalTokenId`) to help with on-chain time management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional properties( `startTime`, `endTime`,`originalTokenId`) to help with on-chain time management. | |
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional properties (`startTime`, `endTime`,`originalTokenId`) to help with on-chain time management. |
EIPS/eip-5007.md
Outdated
|
||
## Motivation | ||
|
||
Some NFTs have a defined usage period and cannot be used when they are not at a specific time. If you want to make NFT invalid when it is not in use period, or make NFT enabled at a specific time, while the NFT does not contain time information, you often need to actively submit the chain transaction, this process is both cumbersome and a waste of gas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant in the EIP itself 🤣
EIPS/eip-5007.md
Outdated
|
||
Some existing NFTs contain time functions, but their interfaces are not consistent, so it is difficult to develop third-party platforms for them. | ||
|
||
By introducing (`startTime`, `endTime`) and unifying the naming, it is possible to enable and disable NFT automatically on chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By introducing (`startTime`, `endTime`) and unifying the naming, it is possible to enable and disable NFT automatically on chain. | |
By introducing these properties (`startTime`, `endTime`) and unifying the naming, it is possible to enable and disable NFT automatically on chain. |
/// @notice Get the start time of the NFT | ||
/// @dev Throws if `tokenId` is not valid NFT | ||
/// @param tokenId The tokenId of the NFT | ||
/// @return The start time of the NFT | ||
function startTime(uint256 tokenId) external view returns (uint64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, note that in the EIP!
/// @dev Throws if `tokenId` is not valid NFT | ||
/// @param tokenId The tokenId of the NFT | ||
/// @return The end time of the NFT | ||
function endTime(uint256 tokenId) external view returns (uint64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note this too. I know it's in the comment for isValidNow
, but it could be more visible.
EIPS/eip-5007.md
Outdated
|
||
## Security Considerations | ||
|
||
Implementors of the `ERC5007` standard must consider the condition of `merge` function. The `originalTokenId` of two NFTs should be same and the time of two NFTs should be adjacent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more the English description of the security consideration. I don't understand what you're trying to convey. I think it just needs to be reworded a bit to be more understandable.
assets/eip-5007/README.md
Outdated
@@ -0,0 +1,15 @@ | |||
# EIP5007 | |||
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional functions (`startTime`, `endTime`, `isValidNow`) to help with on-chain time management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This standard is an extension of [ERC-721](./eip-721.md). It proposes some additional functions (`startTime`, `endTime`, `isValidNow`) to help with on-chain time management. | |
This standard is an extension of [ERC-721](../EIPS/eip-721.md). It proposes some additional functions (`startTime`, `endTime`, `isValidNow`) to help with on-chain time management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be ../../EIPS/eip-721.md
? assets/eip-5007/../EIPS == assets/EIPS
and /assets/eip-5007/../../EIPS == /EIPS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be
../../EIPS/eip-721.md
?assets/eip-5007/../EIPS == assets/EIPS
and/assets/eip-5007/../../EIPS == /EIPS
.
Yes, it should be ../../EIPS/eip-721.md
.
|
||
### Time Data Type | ||
|
||
The max value of `uint64` is 18446744073709551615, timestamp 18446744073709551615 is about year 584942419325. `uint256` is too big for some software such as MySQL, Elastic Search, and `uint64` is natively supported on mainstream programming languages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should really show a more detailed analysis of gas-savings with a real use case if you are going to take a non-uint256
opinionated view. This seems like the main property of the current EIP that needs further discussion.
/// @dev Throws if `tokenId` is not valid NFT | ||
/// @param tokenId The tokenId of the NFT | ||
/// @return The the NFT is valid now | ||
/// if(startTime <= now <= endTime) {return true;} else {return false;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCurrent
is more clear than isValidNow
, since the term valid has more generic implications about the property of the NFT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCurrent
is more clear thanisValidNow
, since the term valid has more generic implications about the property of the NFT.
Thanks. Different project may has different definition of 'valid', so I removed isValidNow
function.
/// @dev Throws if `tokenId` is not valid NFT | ||
/// @param tokenId The tokenId of the NFT | ||
/// @return The the NFT is valid now | ||
/// if(startTime <= now <= endTime) {return true;} else {return false;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I am sure you know, there will be many use cases where you will specifically not want overlapping times between NFTs. startTime <= now <= endTime
means that, if NFT 1 has seconds startTime: 0
, endTime: 10
, and NFT 2 has startTime: 10
, endTime: 20
, both will be current on second 10.
Best approach for this EIP is to simply not take a view on whether these ranges are doubly-inclusive, only inclusive for startTime
, only inclusive for endTime
, or neither. Leave that up to the implementation to specify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see final analysis and discussion of the choice uint64
instead of uint256
.
This opinionated approach to how timestamps are stored will be the determining factor of whether this EIP will gain popular adoption. It is taking a hard stance that uint64
is the canonical way for time storage on Ethereum, which may be correct, but should be backed up by:
-
Evidence of how popular projects are currently handling timestamps (e.g. OZ Timelock Controller using
uint256
). -
Gas saving in a real world use case, keeping in mind that non-trivial projects:
- Do not store just an array of timestamps (mappings tend to be far more common).
- Have arithmetic mostly based on other
uint256
, such asblock,timestamp
and other project-specific values.
Other than that, this looks good to me. Thank you for your contribution.
EIPS/eip-5007.md
Outdated
No security issues found. | ||
|
||
## Copyright | ||
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). | |
Copyright and related rights waived via [CC0](../LICENSE.md). |
assets/eip-5007/README.md
Outdated
@@ -0,0 +1,15 @@ | |||
# EIP5007 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# EIP5007 | |
# EIP-5007 |
* add eip-TimeNFT.md * add Reference Implementation * update code * update Implementation code * add test code * update test code * update code * update test code * rename file name * remove spaces * update Reference Implementation * update title * update by SamWilsn's suggestion * update Backwards Compatibility * update code * simplify EIP5007 * remove empty lines * udpate readme * update readme * update Test Cases * fix link in readme.md * fix link in readme.md * remove function isValidNow * update Copyright * update copyright * change EIP5007 to EIP-5007 Co-authored-by: anders <> Co-authored-by: Anders <anders@emojidao.org>
This standard is an extension of ERC-721. It proposes some additional property(
startTime
,endTime
,originalTokenId
) to help with the on-chain time management.