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

Implement Metadata URI for Land NFTs #85

Open
fishonamos opened this issue Oct 23, 2024 · 9 comments · May be fixed by #109
Open

Implement Metadata URI for Land NFTs #85

fishonamos opened this issue Oct 23, 2024 · 9 comments · May be fixed by #109
Assignees
Labels
Milestone

Comments

@fishonamos
Copy link
Member

fishonamos commented Oct 23, 2024

Task 1.: Enhance the LandNFT contract to include a metadata URI for each NFT, allowing for off-chain storage of additional land information.

Definition of Done:
• The LandNFT contract is updated to include a metadata URI for each token.
• A function is added to update the metadata URI.
• The tokenURI function is implemented to return the metadata URI.
• Tests are written to verify the new functionality.

Task 2. Implement NFT Locking Mechanism for Land Transfers
Description:
Create a locking mechanism for Land NFTs to prevent transfers during pending land transactions or disputes.
Tasks:
Add a locked status to the NFT contract
Implement lock and unlock functions in the ILandNFT trait
Update the transfer function to check for locked status before allowing transfers
Modify the LandRegistry contract to lock NFTs when a land transfer is initiated
Implement unlocking mechanism after successful transfer or cancelled transaction
Add events for locking and unlocking NFTs
Write tests for the locking mechanism and its integration with land transfers

Note: Only applications through the OnlyDust platform will be considered.

@PoulavBhowmick03
Copy link
Contributor

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm Poulav Bhowmick, a Starknet Wolf. I am a software engineer at Invisible Studios, and a blockchain engineer with a robust background in TypeScript, Rust, Solidity Cairo, fullstack development and blockchain technology. My experience includes building robust applications, optimizing functionalities and blockchain integration. I have actively participated in events and open source contributions, enhancing my capability to tackle real-world tech challenges. My projects can be viewed on my GitHub Profile and OnlyDust Profile. Plus I´m active member of Starknet, Ethereum, Stellar ecosystem.

How I plan on tackling this issue

To address this issue, I will enhance the LandNFT contract by adding a storage field that maps each token_id to its corresponding metadata URI. I will implement a new function that allows only authorized entities (likely the land registry contract) to set or update the metadata URI for each token. The tokenURI function will then be implemented to return the URI for a given token_id. I will ensure that the implementation is gas-efficient and integrates smoothly with the existing ERC721 logic. Finally, I will update the test suite to verify that the metadata URIs are correctly set, retrieved, and that only authorized updates are allowed.

ETA - 2 days

@CollinsC1O
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello can I work on this

@manlikeHB
Copy link
Contributor

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hi, I am a skilled cairo dev with lots of experience contributing to amazing projects, my profile is a witness to that statement. this is issue is similar to something i've worked on in the past so, it's a perfect fit for me.

How I plan on tackling this issue

I'll be using the standard erc4906 to achieve this to ensure compatibility. then i'll write test to ensure it functions as expected.

@0xNeshi
Copy link

0xNeshi commented Oct 24, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Creator and maintainer of Cairo learning track on Exercism. Regular "Cairo Book" and "Starknet By Example" contributor. Developer with 6 years of experience, 3 of which in Web3 space.

How I plan on tackling this issue

  1. Add the required functions to LandNFT interface in https://github.com/NoshonNetworks/landver/blob/main/land_registry/src/land_nft.cairo
  2. Implement the required functions in the contract. Include validity assertions (e.g. metadata URI is not an empty string).
  3. Add tests for each of these functions (including any edge cases) to https://github.com/NoshonNetworks/landver/blob/main/land_registry/tests/test_land_nft.cairo

@od-hunter
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hi, please can I be assigned this please? This would be my first time contributing to this project and I would love to be the given the opportunity to contribute. I have experience in html, css, JavaScript,TypeScript and solidity, and Cairo.

How I plan on tackling this issue

To solve this issue, I’ll take the following the steps:
1.⁠ ⁠I’ll add metadata URI storage-that is I’ll introduce a mapping to store metadata URIs for each token.
2.⁠ ⁠I’ll create a function to update the URI, restricted to the owner or authorized role.
3.⁠ ⁠Next, I’ll implement the ⁠ tokenURI ⁠ function to retrieve the stored URI.
4.⁠ ⁠Then I’ll create tests to validate the setting and retrieval of metadata URIs.

Please assign me, I’m ready to work.

@fishonamos fishonamos added this to the Milestone 5 milestone Oct 24, 2024
@blessingbytes
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello, i'm a frontend and blockchain developer, pls i will love to work on this issue

How I plan on tackling this issue

  • I will enhance the LandNFT contract to include a metadataURI variable that stores the URI for each NFT.
  • i will Implement Lock and Unlock Functions
    -I will Update Transfer Function
  • i will Modify LandRegistry Contract
  • I will Implement Unlocking Mechanism

@saimeunt
Copy link
Contributor

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have contributed to several Cairo projects on OnlyDust and I've worked on very similar issues in the past.

How I plan on tackling this issue

I will carefully implement the feature as requested and add all the necessary tests.

@0xNeshi
Copy link

0xNeshi commented Oct 24, 2024

@fishonamos thanks for the opportunity to contribute.
I noticed the issue was labeled as "Blocked", should I hold off on working on it for now?

Regarding the 1st task

  1. How much URI validation is expected? Is any validation even necessary (e.g. ERC721_base_uri has no validation)?

  2. Presumably, metadata URI will originally be set in the constructor. Who is then later allowed to update the metadata URI?
    The Land registry is the straightforward choice, but that would mean that this contract now has an additional responsibility like managing LandNFT state. An alternative would be to make LandNFT ownable, and only let the owner update the metadata URI.

  3. I'm writing tests, and I noticed there is a circular dependency between LandNFT and LandRegistry, which makes deploying both contracts impossible - e.g. LandNFT can be deployed with a temporary mock address for land_registry, then actual LandRegistry can be deployed with the real nft_contract address, but there's no way to update the LandNFT.land_registry post-construction.
    There are a couple of ways to solve this, but the most straightforward way is to make LandNFT ownable, and add a LandNFT.set_land_registry function that can be used post-construction to set the actual LandRegistry address. This would also make the land_registry constructor param redundant, but we would need checks in each function that land_registry is initialized. Additionally, this would solve the "who can update the metadata URI" problem from "EDIT 2".
    An alternative is to make LandRegistry ownable, with a similar flow as previously. But this then leaves "EDIT 2" problem open.

Regarding the 2nd task

Given these task requirements:

Modify the LandRegistry contract to lock NFTs when a land transfer is initiated
Implement unlocking mechanism after successful transfer or cancelled transaction

What does "when a land transfer is initiated" mean, i.e. which function is it referring to?
LandRegistry.transfer_land is an atomic operation, so to make NFT locking useful, the transfer would have to be separated into three operations:

  1. initiate_transfer
    • can be called by the land NFT owner
    • locks the land NFT
    • the NFT can be unlocked only after the inspector (or someone else?) confirms and proceeds with the transfer or cancels the transfer
  2. complete_transfer
    • can be called by the inspector (or someone else?)
    • unlocks the NFT and transfers it to the target contract
    • bonus: the transfer completes only after both the inspector and the receiving address have both confirmed the transfer - this ensures that: 1) the receiver is a valid address (so the NFT is not lost); and 2) the receiver actually wants to accept the land NFT
  3. cancel_transfer
    • can be called by the inspector (or someone else?)
    • unlocks the NFT and stops the transfer
    • emit event with a reason for the cancellation
    • bonus: this operation can also be called by the receiver in case they do not wish to accept the NFT

Is my understanding correct?

Note: implementing task 2 will cause conflicts with #100

@0xNeshi
Copy link

0xNeshi commented Oct 28, 2024

@fishonamos pinging you in case you missed my inquires above, need your input to be able to continue the work.

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

Successfully merging a pull request may close this issue.

8 participants