Skip to content
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

AINFT contract template #1

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

AINFT contract template #1

wants to merge 15 commits into from

Conversation

jakepyo
Copy link
Contributor

@jakepyo jakepyo commented Mar 8, 2023

feat: AINFT contract template structure implementation

  • AINFT721.sol, AINFT721Upgradeable.sol
  • Support upgradeable contract(apply UUPS pattern)
    • proxy contract is AINFT721Upgradeable.sol, the example logic contract is AINFT721LogicV2.sol.
  • Update metadata scheme is on proxy contract - IAINFT.sol interface

deploy

  • deploy proxy and logic contract

test

  • If I've missed something to test, feel free to comment or mention

@jakepyo jakepyo requested review from clu3xi and ChanComcom March 8, 2023 03:48
@jakepyo jakepyo force-pushed the feature/jakepyo/contract branch from dffd9c9 to a0c29a1 Compare March 8, 2023 03:54
feat: AINFT contract template structure impl.
@jakepyo jakepyo force-pushed the feature/jakepyo/contract branch from a0c29a1 to 24861dd Compare March 8, 2023 03:55
@jakepyo jakepyo requested a review from minho-comcom-ai March 10, 2023 02:42
contracts/AINFT721Upgradeable.sol Outdated Show resolved Hide resolved
contracts/AINFT721Upgradeable.sol Outdated Show resolved Hide resolved
contracts/AINFT721Upgradeable.sol Outdated Show resolved Hide resolved
);

baseURI = newBaseURI;
return true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should emit BatchMetadataUpdate(start, end) with counter's help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it before. However the setBaseURI() call occurs after the someone calls updateTokenURI(), the baseURI could be ignored when calling tokenURI(). I will leave it behind, with remaining TODO comment

Copy link

@minho-comcom-ai minho-comcom-ai Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MetadataUpdate / BatchMetadataUpdate is for the NFT marketplaces or other 3rd parties, so I think this should be emitted on here.
If not, we don't have to implement the ERC-4906 at first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of ERC721 contracts set the baseURI in initialization phase and no implementation for setBaseURI(). In the perspective of structuring the base template, setBaseURI() don't need to be necessary. However, the versioning of metadata is important concept of template and it'd better to put ERC4906 inside the template.

As a alternative way, I'm trying to add pre-condition check of setBaseURI(): if the number of having been called updateTokenURI() is equal or more than once, setBaseURI() cannot be executed. In this case, the BatchMetadataUpdate can be added.

What do you think of this way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants