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

NFT image: check nftChainId value & NFT ownership #5

Open
tempe-techie opened this issue May 24, 2022 · 3 comments
Open

NFT image: check nftChainId value & NFT ownership #5

tempe-techie opened this issue May 24, 2022 · 3 comments

Comments

@tempe-techie
Copy link
Member

tempe-techie commented May 24, 2022

If user has image data in domain custom data, check if imgChainId is present.

If it's not present, check the chain where domain is running on for NFT data.

If chainID is present, check that chain.

In any case also check if user really owns this NFT. If user owns it, we can add a verified checkmark somewhere (or vice-versa, if they don't own it, add a non-verified checkmark). The alternative is to use hexagon image placeholder (just like Twitter does).

@Mouradif
Copy link

Mouradif commented Mar 6, 2023

Ideally, there would also be a check on-chain at

  function editData(string calldata _domainName, string calldata _data) external {
    require(domains[_domainName].holder == msg.sender, "Only domain holder can edit their data");
    // TODO: check if imgAddress is a contract address and do:
    // require(IERC721(address).ownerOf(imgTokenId) == msg.sender)
    domains[_domainName].data = _data;
    emit DataChanged(msg.sender);
  }

But as far as I know, this would require that imgChainId is the same chain as the contract

But if the check is only implemented in the front-end, this should be both when trying to set the image (to protect users from committing a useless transaction) and when actually showing the image (to prevent people from committing the update on the contract directly and showing an NFT they don't own).

Any thoughts on this @tempe-techie ?

@tempe-techie
Copy link
Member Author

@Mouradif Yep, I agree, I think it's better to do these checks on the frontend, so that NFTs on other chains are also supported 🙂

@Mouradif
Copy link

Ok I'm on it then 😄

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

No branches or pull requests

2 participants