search

exo-digital-labs / ERC721R

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

(WIP) add extendable erc721r contract #16

Open Amirh24 opened 2 years ago

Amirh24 commented 2 years ago

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

elie222 commented 2 years ago

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

elie222 commented 2 years ago

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: https://github.com/exo-digital-labs/ERC721R/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Amirh24 commented 2 years ago

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 commented 2 years ago

So I pushed a version like that a few days ago: https://github.com/exo-digital-labs/ERC721R/pull/9#issuecomment-1097823682

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