exo-digital-labs / ERC721R

https://erc721r.super.site/
MIT License
240 stars 60 forks source link

Features added on bug fixed versions #10

Open emirhansirkeci opened 2 years ago

emirhansirkeci commented 2 years ago

ERC721ChangablePrice.sol

mintPrice can be set for different cases (like presale, publicsale). Every NFT's mint price storing with these lines of code,

mapping(uint256 => uint256) public tokenPrices;

    function setTokenPrices(uint256 quantity) private {
        for(uint256 i = 0; i < quantity; i++){
            tokenPrices[totalSupply() + i ] = mintPrice;
        }
    }

We have to call this function after mint functions called.

ERC721RefundLock.sol

We might dont want to let users refund their NFTs immediately.

uint256 public constant refundLock = 30 days;

    function refundLockActive() public view returns (bool) {
        return (block.timestamp >= refundLock);
    }

We have to use this function with require() inside of refund function.

require(!refundLockActive(), "Refund will be able in 30 days");

ERC721Full.sol

This contract is contains both of these files.

elie222 commented 2 years ago

We might dont want to let users refund their NFTs immediately. In which scenarios do you think this should be the case?

I'm wary of making it part of the standard as I don't think we want to encourage this. It makes it harder for someone to request the refund and there doesn't seem to be a benefit for forcing this behaviour (other than making it easier for users to forget to refund). Happy to hear if you think there is a good reason for this.

qbig commented 2 years ago

All features look super useful but I think we should rectify the critical bug first

lawweiliang commented 2 years ago

@qbig, thumbs up.

emirhansirkeci commented 2 years ago

We might dont want to let users refund their NFTs immediately. In which scenarios do you think this should be the case?

I'm wary of making it part of the standard as I don't think we want to encourage this. It makes it harder for someone to request the refund and there doesn't seem to be a benefit for forcing this behaviour (other than making it easier for users to forget to refund). Happy to hear if you think there is a good reason for this.

I thought if people who minted nfts are refund their nfts immediately that action can be trigger other users to refund their nfts immediately too. My goal is giving a chance to the project to prove themselves to their community that they are really doing their best. (At least first 5-10 days.) This is my thoughts if you want to i can delete this feature. Do you want me to do that? Also do you want me to edit ERC721ChangablePrice.sol with current bug fixed version?

elie222 commented 2 years ago

We might dont want to let users refund their NFTs immediately. In which scenarios do you think this should be the case?

I'm wary of making it part of the standard as I don't think we want to encourage this. It makes it harder for someone to request the refund and there doesn't seem to be a benefit for forcing this behaviour (other than making it easier for users to forget to refund). Happy to hear if you think there is a good reason for this.

I thought if people who minted nfts are refund their nfts immediately that action can be trigger other users to refund their nfts immediately too. My goal is giving a chance to the project to prove themselves to their community that they are really doing their best. (At least first 5-10 days.) This is my thoughts if you want to i can delete this feature. Do you want me to do that? Also do you want me to edit ERC721ChangablePrice.sol with current bug fixed version?

I'd be happy for the delayed refund feature to not be part of Full. I think it mostly hurts consumers. And doesn't solve the challenge of many people refunding. It would only delay it to the start of the refund period. I even think it might cause more panic that way with many people then all refunding at the same time as soon as it becomes liquid again. You also lose some benefits of the refund mechanism where it protects floor prices post mint.

In practice btw we haven't seen the mass refunds happen where it would cascade. Although it's possible this would happen in the future, I'm not sure it will as the refund period is quite long and you don't lose anything by waiting a little longer.

elie222 commented 2 years ago

Also do you want me to edit ERC721ChangablePrice.sol with current bug fixed version?

Yes, we shouldn't definitely keep the bug fixes in

elie222 commented 2 years ago

so i think what could be great here is creating contracts that we can extend. so similar to erc721 contracts on openzeppelin. you can choose which portions you want to extend to add each piece of functionality rather than us having lots of copy paste code (especially where a lot of it isnt related to ERC721R)

lawweiliang commented 2 years ago

@elie222 , I agree with your approach. Keep the core clean. Additional features put it over extended features. Stick with this approach.

elie222 commented 2 years ago

Work on splitting it into a contract people can inherit is happening here: https://github.com/exo-digital-labs/ERC721R/pull/16 btw