Prepare for post-launch NFT artwork #128
Conversation
| if (bond.status != BondStatus.active) { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we add the final accrued bLUSD at the time termination to bond data? Will the artwork/metadata need that information?
|
For the first pending issue, “Wrapper to terminate and transfer bond in one TX”, we decided not to do it, to prioritize security. The potential use cases of the wrapper don’t seem so important. |
|
A note about transfer fee: We have to make sure that pending bonds can be transferred without a fee, otherwise the sale of NFTs for the economical value doesn’t make sense (the fee value would be higher than the value of the time / opportunity cost of the pending bond). |
| @@ -1,4 +1,4 @@ | |||
| [default] | |||
| [profile.default] | |||
There was a problem hiding this comment.
Still a newbie with foundry. What’s that for?
There was a problem hiding this comment.
It was automatically changed by the latest version of forge after a foundryup. Something to do with deprecation.
LUSDChickenBonds/src/BondNFT.sol
Outdated
| } | ||
|
|
||
| // Tokens are never burnt, therefore total minted equals the total supply. | ||
| function totalMinted() external view returns (uint256) { |
There was a problem hiding this comment.
This doesn’t belong to any standard, right? Then I guess we can remove it.
|
|
||
| _burn(_tokenID); | ||
| } | ||
| function tokenURI(uint256 _tokenID) public view virtual override returns (string memory) { |
There was a problem hiding this comment.
That's what I tried initially, but it won't compile. I guess because OpenZeppelin already implements it as a public view virtual, the overriding implementation has to have the same visibility.
There was a problem hiding this comment.
This looks great to me! Really nice and clean.
A minor suggestion: we could add individual bond property getters to the NFT contract itself, e.g. getBondAmount, getBondStartTime, etc. Could potentially be useful for marketplaces like OpenSea. IIRC Luchadore had that in an example NFT contract.
| renounceOwnership(); | ||
| } | ||
|
|
||
| function mint(address _bonder) external returns (uint256) { | ||
| requireCallerIsChickenBondsManager(); | ||
| tokenSupply++; |
There was a problem hiding this comment.
I was confused a little here as to where we actually increase totalSupply. now. I see now it happens in _beforeTokenTransfer when we _mint.
| // the permanent increase should be N * NFT_RANDOMNESS_DIVISOR. | ||
| // It also means that as long as Permanent doesn’t change in that order of magnitude, attacker can try to manipulate | ||
| // only changing the event date. | ||
| uint256 constant public NFT_RANDOMNESS_DIVISOR = 1000e18; |
There was a problem hiding this comment.
Nice cost imposed on manipulators!
Addressed in this PR
This PR addresses the following points of #118.
Not burning
The NFT is no longer burnt when terminating the bond (either through chicken-in or chicken-out). As a consequence, bond supply only ever increases. Bond data is no longer deleted either. This allows bond data to be used in the generation of metadata/artwork even after the bond is terminated.
Since the bond is no longer burnt, we need an alternative way of preventing multiple termination (e.g. chickening out twice). A new field
statusis added to bond data, which is checked and updated by chicken-in/out. The same field can also be used by the artwork to determine which kind of visuals to render.Transfer lockout period
To prevent potential scams where the sale of an active bond (which holds additional economical value on top of the artistic aspect of the NFT) is frontrun by a chicken-in/out by the bond's owner, we disallow transfers of the bond NFT for a configurable period of time after chicken-in/out. To facilitate this, a new field
endTimeis added to bond data, holding the timestamp of the block which terminated the bond. The NFT artwork may also use this field as additional source of (weak) randomness in generating the chickened in/out visuals.Upgradeable NFT metadata
The computation of
tokenURI()is delegated byBondNFTto an external contract. When constructingBondNFT, an initial delegate may be provided as a constructor parameter. If left zero,BondNFTwill initially return empty string astokenURI(), until the final delegate is set. This lets us decide whether we want to provide some preliminary metadata even before artwork launch, or not.A final delegate can be set (once) through
setArtworkAddress(). Upon calling this, the owner ofBondNFTloses ownership over the contract.Left open
The following points are not addressed by this PR, as there are still open questions.
startTimeandendTimeof the bond, both of which come from block timestamps. If we want higher quality randomness, we should probably implement it ahead of artwork launch to avoid having a procedure for claiming the artwork.tokenURI()) so that bond NFTs show up with some description (or even some basic attributes like bonded amount, creation date, termination date) in wallets and marketplaces. Since this is implemented as a read-only function in an external contract, it may not need to be thoroughly audited.