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

Basic Modifications to IP Asset Registry register #69

Closed
wants to merge 2 commits into from

Conversation

jdubpark
Copy link

  • Remove check for IERC721 interface support since we check forIERC721Metadata interface support, which inherits IERC721.
  • Remove chainId from naming since contracts and offchain apps can already know the chain an IP is on.

@kingster-will
Copy link

Remove chainId from naming since contracts and offchain apps can already know the chain an IP is on.

For cross-chain scenarios, the offchain app might NOT be able to directly to know the original chain of an IP.

@jdubpark
Copy link
Author

For cross-chain scenarios, the offchain app might NOT be able to directly to know the original chain of an IP.

Using onchain name as a reference is not ideal as the underlying NFTs can be multi-chain as well, such as y00ts or DeGods migrating chains.

I agree that providing clarity is nice but I don't see IP's name as a good place for clarity.

@jdubpark
Copy link
Author

Will just keep Chain ID for potential offchain usage, but at the cost of gas overhead to store extra string.

@LeoHChen
Copy link

LeoHChen commented Apr 11, 2024

Will just keep Chain ID for potential offchain usage, but at the cost of gas overhead to store extra string.

maybe remove the id from the name for local chain registration, and add id for cross-chain registration?

but this will create inconsistency though. makes it harder to consume by dapp.

@jdubpark jdubpark marked this pull request as draft April 11, 2024 23:11
@LeoHChen LeoHChen closed this Apr 12, 2024
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