-
Notifications
You must be signed in to change notification settings - Fork 526
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
add xNFT_milestone1.md #984
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anshulWeb3. Thanks for the delivery. Could you explain why each of the changes to the NFT pallet were necessary? I know that there are some open issues with XCM transfers of NFTs, but ideally we'd be working against the stock pallet.
Hi @semuelle . To deliver the functionality of cross chain NFT transfer we had to use some of the existing implementation. As the NFT and xNFT pallet are tightly coupled we need to use its default implementation and structs that were private in NFT pallet. To make them accessible in our pallet we have changed its access to public. Please refer to our readme (https://github.com/antiers-solutions/xNFT/blob/master/pallet-xnft/README.md) and technical docs (https://github.com/antiers-solutions/xNFT/tree/master/pallet-xnft/docs) for better understanding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, @anshulWeb3. The reason I am asking about these changes is that I expected your pallet to work with the FRAME
NFT pallet. Not a custom one. These changes mean that the xNFT pallet is of no use to any chain using the FRAME pallet. You are also referencing "the NFT pallet" in your application:
This pallet will not be able to transfer any NFT that is not registered using the NFT pallet for Substrate
Also:
- Can you add a license to the repo (https://github.com/antiers-solutions/xNFT)? I see that Apache 2.0 is mentioned here, but it's useful to have a global repo license.
- Please provide a Dockerfile as outlined in the application. The prerequisites listed in the user guide are quite complex, so having a Dockerfile to automate at least some of these steps would be extremely helpful.
Hi @semuelle , thank you for your feedback.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anshulWeb3. Sorry for the late reply.
Trappist actually contains a relay chain and three parachains with HRMP channels between them for local testing. You could you provide a tutorial how to integrate the pallets into it instead of just listing all that as a prerequisite.
However, the bigger issue is that the pallet was supposed to work with the stock NFT pallet. You pointed this and the fact that you would have to wait for changes to the pallet out yourself. I'm sure you are aware that this pallet is integrated into the runtime of several parachains today. It seems highly unlikely to me that anyone would switch to your fork to enable NFT cross-chain transfers. Therefore, I must insist that the pallet be compatible with the FRAME pallet, not just a fork of it.
Hi @semuelle , thanks for your response. We plan to add these pallets to Trappist's code and make it publicly available for thorough testing. We'd also like to suggest a potential solution to the main issue. When users add the xNFT pallet into their runtime, they might consider updating the NFT pallet at the same time. Your thoughts on this would be appreciated. Thank you for your cooperation. |
As I said, @anshulWeb3, the agreement was to build a pallet that works with the FRAME NFT pallet, not with a fork of it. |
Thanks for the update, @anshulWeb3, but unless the xnft pallet works with the stock NFT pallet, there is no point in continuing this milestone review. |
Hi @semuelle , I understand your concern about the changes to the 'FRAME' NFT Pallet. However, I want to clarify that we made minimal alterations to the pallet, primarily by making certain private structs public. These adjustments were necessary to enable the transfer of NFT metadata from one chain to another. I'd like to reassure you that the pallet is functioning as intended and is vital for the seamless transfer of NFT metadata. Without these changes, achieving this level of interoperability would be extremely challenging. |
@anshulWeb3, the extent of changes made to the pallet is irrelevant. What matters is that your pallet is compatible with the original FRAME pallet. The necessity to wait for changes to be made to the pallet has been pointed out by multiple people in your application PR. Your responses also indicate that you were aware at the time. A cross-chain NFT pallet that's incompatible with the FRAME NFT pallet is not of interest. Please let me know how you would like to proceed. But I recommend we close this PR and you revisit the milestone when the necessary changes have been made upstream. |
I am closing this delivery due to inactivity. Please let me know if you want to continue working on it. |
Milestone Delivery Checklist
Link to the application pull request: w3f/Grants-Program#1798