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

add xNFT_milestone1.md #984

Closed
wants to merge 1 commit into from
Closed

add xNFT_milestone1.md #984

wants to merge 1 commit into from

Conversation

anshulWeb3
Copy link
Contributor

@anshulWeb3 anshulWeb3 commented Aug 28, 2023

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • The invoice form 📝 has been filled out for this milestone.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, the payment will be transferred to the BTC/ETH/fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#1798

@semuelle semuelle self-assigned this Aug 28, 2023
Copy link
Member

@semuelle semuelle left a 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.

@anshulWeb3
Copy link
Contributor Author

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.
Functions changed:
a) Create_delete_collection.rs: this file code was reused to create and delete the NFTs with some modifications of access specifier.
b) CollectionDetails:- https://github.com/antiers-solutions/xNFT/blob/f4b5a9387a24bfc4be0fd4dc79872a07902a22bb/nfts/src/types.rs#L157,
ItemDetails:- https://github.com/antiers-solutions/xNFT/blob/f4b5a9387a24bfc4be0fd4dc79872a07902a22bb/nfts/src/types.rs#L134,
CollectionMetadata:- https://github.com/antiers-solutions/xNFT/blob/f4b5a9387a24bfc4be0fd4dc79872a07902a22bb/nfts/src/types.rs#L157
structs removed super from them to access them in xNFT pallet.

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.

Copy link
Member

@semuelle semuelle left a 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.

@anshulWeb3
Copy link
Contributor Author

anshulWeb3 commented Sep 1, 2023

Hi @semuelle , thank you for your feedback.

  • While strategizing the tasks, we have also planned to do it without the changes in NFT pallet but as we proceeded further with the development we realized that we cannot access some of the structs and modules of NFT pallet because of access restriction. Therefore, we had to make them public in order to use them within our pallet.
  • We are aware that having a docker file is necessary but in our scenario we require a relay parachain environment which is not possible with docker for now, but we can provide you with a docker file of a parachain with our implemented code (if possible) along with the required versions of rust and substrate. We'll add the needed files to the repository as soon as possible.
  • As for the Licence, it has been added to the https://github.com/antiers-solutions/xNFT.

Copy link
Member

@semuelle semuelle left a 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.

@anshulWeb3
Copy link
Contributor Author

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.

@semuelle
Copy link
Member

semuelle commented Sep 8, 2023

As I said, @anshulWeb3, the agreement was to build a pallet that works with the FRAME NFT pallet, not with a fork of it.

@anshulWeb3
Copy link
Contributor Author

anshulWeb3 commented Sep 11, 2023

Hi @semuelle , we understand your concern, we have added a testing branch that contains files from the trappist repo and the name of this repo is trappist. Follow this link. Follow the readme for help on setup.

@semuelle
Copy link
Member

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.

@anshulWeb3
Copy link
Contributor Author

anshulWeb3 commented Sep 14, 2023

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.
We are looking into a way to access private fields of structs without making them public. Meanwhile I encourage you to please take a closer look at the changes we made, as they are documented within the provided files. If you have any further questions or concerns, please feel free to discuss them. Thanks for your understanding.

@semuelle
Copy link
Member

@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.

@semuelle
Copy link
Member

I am closing this delivery due to inactivity. Please let me know if you want to continue working on it.

@semuelle semuelle closed this Sep 26, 2023
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.

2 participants