(WIP) add extendable erc721r contract

[Amirh24]

Created a simple extendable version of ERC721R
WIP: need to handle owner mints differently

[elie222]

For owner mints I would work on what we already have merged in which fixes issues around that.

[elie222]

Note, I'm not sure the actual issue is owner mint. Because even if the owner can't mint, they can still buy 1 off the market and refund indefinitely. So I'm not sure we need it at all. We just need the fixes that were put in here:
#9

The core being:

    mapping(uint256 => bool) public hasRefunded; // users can search if the NFT has been refunded
    mapping(uint256 => bool) public isOwnerMint; // if the NFT was freely minted by owner

[0xsublime]

Could the notContract modifier be removed? I don't see why it should be included in the base contract, an inheriting contract could simply implement it themselves if they need it. (There is a reason you don't see it in the ERC721A contract, for example). Also, it's Bad Practice® (#5).

[Amirh24]

Note, I'm not sure the actual issue is owner mint. Because even if the owner can't mint, they can still buy 1 off the market and refund indefinitely. So I'm not sure we need it at all. We just need the fixes that were put in here: #9

The core being:

    mapping(uint256 => bool) public hasRefunded; // users can search if the NFT has been refunded
    mapping(uint256 => bool) public isOwnerMint; // if the NFT was freely minted by owner

I have a nice solution for all that I plan to implement to push tomorrow to fix this issue

[Amirh24]

Could the notContract modifier be removed? I don't see why it should be included in the base contract, an inheriting contract could simply implement it themselves if they need it. (There is a reason you don't see it in the ERC721A contract, for example). Also, it's Bad Practice® (#5).

yeah I see what you're saying. It may be removed soon

[elie222]

Could the notContract modifier be removed? I don't see why it should be included in the base contract, an inheriting contract could simply implement it themselves if they need it. (There is a reason you don't see it in the ERC721A contract, for example). Also, it's Bad Practice® (#5).

Yes, we will remove it

[elie222]

Note, I'm not sure the actual issue is owner mint. Because even if the owner can't mint, they can still buy 1 off the market and refund indefinitely. So I'm not sure we need it at all. We just need the fixes that were put in here: #9
The core being:

    mapping(uint256 => bool) public hasRefunded; // users can search if the NFT has been refunded
    mapping(uint256 => bool) public isOwnerMint; // if the NFT was freely minted by owner

I have a nice solution for all that I plan to implement to push tomorrow to fix this issue

a solution was already merged in for this btw. there was discussion around some different options too. may be worth adding comments to that discussion on what you're planning as a few options were already put out

[elie222]

remove

[Amirh24]

mapping(uint256 => bool) public hasRefunded; // users can search if the NFT has been refunded

Yes the mapping are included to the implementation.
I was thinking about adding another mapping for token id -> purchase price.
This will require modifying _safeMint to add purchase price after minting.
We then require that purchase price for this token id > 0 when requesting a refund for it.

[elie222]

So I pushed a version like that a few days ago:
#9 (comment)

The reason we didn't use it is discussed there. It increases mint price. The other versions increase ownermint price and refund which is probably better