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

Refactoring DID minting #434

Merged
merged 16 commits into from
Jan 18, 2023
Merged

Refactoring DID minting #434

merged 16 commits into from
Jan 18, 2023

Conversation

aaitor
Copy link
Member

@aaitor aaitor commented Jan 12, 2023

Description

This PR implements:

  • Remove the option of minting/burning NFTs via DIDRegistry. These functions must go through the NFT contracts
  • Simplify initialization of NFTs
  • Pre-configuration of NFT contracts when clonning

Is this PR related with an open issue?

Related to Issue #

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Follows the code style of this project.
  • Tests Cover Changes
  • Documentation

Funny gif

Put a link of a funny gif inside the parenthesis-->

@aaitor aaitor marked this pull request as ready for review January 15, 2023 16:54
@aaitor aaitor requested review from a team as code owners January 15, 2023 16:54
@aaitor
Copy link
Member Author

aaitor commented Jan 15, 2023

@mrsmkl , could you review the OpenGSN test? It seems to rely transactions fine for the DIDRegistry but there is a problem with the NFT minting

Copy link
Contributor

@mrsmkl mrsmkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot figure out how to mint an erc-721 token if it's not minted whyen registering or enabling the nft (except transfer condition w/ mint).
If possible, there shouldn't be overloaded method names, they just cause problems on JS side...

test/int/agreement/AccessAgreement_withAuction.Test.js Outdated Show resolved Hide resolved
test/int/nft/NFT721_e2e.Test.js Outdated Show resolved Hide resolved
test/int/nft/NFTAccessProofAgreement.Test.js Outdated Show resolved Hide resolved
test/unit/conditions/rewards/EscrowPayment.Test.js Outdated Show resolved Hide resolved
test/unit/conditions/rewards/MultiEscrowPayment.Test.js Outdated Show resolved Hide resolved
test/unit/conditions/rewards/wrapper.js Outdated Show resolved Hide resolved
test/unit/conditions/rewards/wrapper.js Outdated Show resolved Hide resolved
test/unit/registry/CurveRoyalties.Test.js Outdated Show resolved Hide resolved
test/unit/registry/MintableDIDRegistry.js Outdated Show resolved Hide resolved
@aaitor
Copy link
Member Author

aaitor commented Jan 17, 2023

I cannot figure out how to mint an erc-721 token if it's not minted whyen registering or enabling the nft (except transfer condition w/ mint).

The intention is to protect the contract in such a way only an operator (i.e TransferCondition or the contract owner can mint)

@mrsmkl
Copy link
Contributor

mrsmkl commented Jan 18, 2023

I cannot figure out how to mint an erc-721 token if it's not minted whyen registering or enabling the nft (except transfer condition w/ mint).

The intention is to protect the contract in such a way only an operator (i.e TransferCondition or the contract owner can mint)

The case I was wondering was if the owner of the DID isn't the owner of the contract, but would want to mint it without TransferCondition ...

@aaitor
Copy link
Member Author

aaitor commented Jan 18, 2023

The case I was wondering was if the owner of the DID isn't the owner of the contract, but would want to mint it without TransferCondition ...

ERC-1155 can represent a multi-token where different DIDs (from different owners) are minted under the same contract (because of that in the NFT1155Upgreadable contract the DID ownership is validated.

I think for ERC-721 that shouldn't be a normal case. For ERC-721 a user typically will require to deploy/clone a contract that keeps the different tokenIds representing the "mints" from different users for the same asset (DID).

In our ERC-1155 implemention the tokenId is the DID so the ownership can be validated. In our ERC-721 the tokenId is by default a counter and the owner of the DID is the owner of the contract.

@aaitor aaitor merged commit 3c0c56b into develop Jan 18, 2023
@aaitor aaitor deleted the fix/code_simplification branch January 18, 2023 18:13
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.

None yet

2 participants