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

initial commit of the low-code nft marketplace #8

Merged
merged 21 commits into from
Aug 22, 2023

Conversation

bogacyigitbasi
Copy link

@bogacyigitbasi bogacyigitbasi commented Mar 23, 2023

Purpose

_Describe the purpose of the pull request, link to issue describing the problem, etc.

Changes

_Describe the changes that were needed.

Depends on

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

CLA acceptance

_Remove if not applicable.

By submitting the contribution I accept the terms and conditions of the
Contributor License Agreement v1.0

@abizjak abizjak self-requested a review March 30, 2023 12:14
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

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

Most of it looks pretty good, but there are a few things that need to be changed

  • I don't think we should have a duplicate copy of the CIS2-multi contract. We should work to get the one in rust-smart-contracts modified if practical.
  • There is a bunch of duplication for generic stuff for interacting with CIS2 contracts. That should be moved to the node SDK and the duplication should be removed.
  • The typescript projects should have linting as well.
  • Cargo.lock files should be committed for contracts.

.github/workflows/rust-fmt-lint.yaml Outdated Show resolved Hide resolved
low-code-nft-marketplace/README.md Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/.gitignore Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/cis2_client.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/cis2_client.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/mint-ui/src/models/Cis2Client.ts Outdated Show resolved Hide resolved
low-code-nft-marketplace/mint-ui/src/models/Cis2Types.d.ts Outdated Show resolved Hide resolved
@parv0888
Copy link
Contributor

parv0888 commented May 7, 2023

Most of it looks pretty good, but there are a few things that need to be changed

  • I don't think we should have a duplicate copy of the CIS2-multi contract. We should work to get the one in rust-smart-contracts modified if practical.
  • There is a bunch of duplication for generic stuff for interacting with CIS2 contracts. That should be moved to the node SDK and the duplication should be removed.
  • The typescript projects should have linting as well.
  • Cargo.lock files should be committed for contracts.

@abizjak : We have done our best efforts to fix the issues and to address the comments. Kindly review again

@abizjak abizjak self-requested a review May 9, 2023 11:04
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

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

This is looking OK now.

However I am wondering what level of scrutiny we should apply to the contracts. I have not done a full audit, but I'm thinking that if we are going to promote this we should do a thorough analysis of the marketplace contract to make sure we don't have problems.

@bogacyigitbasi Can you answer what the intended scope of this is?

low-code-nft-marketplace/cis2-market/src/cis2_client.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/state.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/state.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/state.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/.eslintrc Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/.prettierrc.json Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/README.md Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/src/Constants.ts Outdated Show resolved Hide resolved
@abizjak abizjak requested a review from DOBEN July 14, 2023 11:47
@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

I get errors running the mint-ui front end. Using a fixed version "@concordium/browser-wallet-api-helpers": "2.5.0" in both packages resolves it. There is some other error/warning output when running yarn build in mint-ui folder, please have a look.

everTS2345: Argument of type 'import(".../LowCodeMareketPlace/concordium-dapp-examples/low-code-nft-marketplace/mint-ui/node_modules/@concordium/browser-wallet-api-helpers/lib/wallet-api-types").WalletApi' is not assignable to parameter of type 'import(".../LowCodeMareketPlace/concordium-dapp-examples/low-code-nft-marketplace/common-ui/node_modules/@concordium/browser-wallet-api-helpers/lib/wallet-api-types").WalletApi'.
    22 |     setState({ ...state, processing: true });
    23 |
  > 24 |     initContract(props.provider, props.contractInfo, props.account)
       |                  ^^^^^^^^^^^^^^
    25 |       .then((address) => {
    26 |         setState({ ...state, processing: false });
    27 |         props.onDone(address, props.contractInfo);`

@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

Some suggestions for the future (in new PRs if we decide to incorporate it) once we get the first version merged and we discuss future plans:

  • Add versions somewhere visible at the front end
  • Add Dockerfile so the front ends can also run via a Docker container
  • Add Filter for the NFTs listed (e.g. user can search by creator, by collection, by price, by newest)

@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

Users can end up in a weird situation where they have uploaded something to pinata (adding pinata metadatHash) and also add a regular manual metaData hash. Consider disabling the button addNewMetaDataHash when the user already has successfully uploaded something to IPFS (is going down the IPFS route).
WeirdSituationWhereYouMintBothPinataAndRegularMetaDataHashAtTheSameTime

@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

When clicking the “Create My MarketPlace” page and deploying a new contract an empty white page appears. Consider adding some information for the user what to do next. It is fair enough to link the developer documentation and let them know where to continue reading or just explain in text form that they now need to run the whole front end again but with the new smart contract index set in the constant.ts file.
CreateNewMarketPlaceEmptyPage

@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

Suggestion: We need to explain what "primary seller royalties" are. It is not the creator but the person that first sells the NFT on this marketplace that can set the royalties. Maybe that is part of the developer documentation then that is fair enough. It would be super nice if there is an explanation at the front end explaining how the royalties work in this marketplace and how the sales price is calculated (aka what the seller, primary seller, and the marketplace get from a sold NFT).

PrimarySellerRoyalties

@DOBEN
Copy link
Member

DOBEN commented Jul 21, 2023

I am not a fan of hard-coding the pinata gateway/provider into the metaData in the smart contract when minting a token via the front end.
notHardCodingTheIPFSGateWayProviderIntoTheMetaDataHash

E.g. if Pinata decides to change its Gateway-URL one day then all the metadata links are broken or the company goes out of business. Generally, I wouldn’t want to hardcode third-party URLs into smart contract code that my NFT collection now depends on.
howIPFSMetaDataIsStoredInContractsUsually

Usually, only the IPFS hash (not the gateway) is hardcoded in the smart contract as metadata. The marketplace infrastructure does then resolve the IPFS hash and uses the gateway that they want to use. For e.g. big NFT marketplaces they can not rely on the free pinata gateway which is rate-limited but rather have their own gateway or a paid/high-performing one.

Copy link
Member

@DOBEN DOBEN left a comment

Choose a reason for hiding this comment

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

This project is a great addition to our tools. Well written and it can serve as a good start for people to create their own projects. There went a lot of work in this PR and this is appreciated. So depending on how polished we want to get it, I am fine if we decide to address some of the remaining comments in future PRs to get this massive PR merged.

I had some hiccups with the pinata/front-end flow so (the keys that I just generated didn’t work reliably but I got some borrowed keys that worked). Since users can skip that step, we are good to go for the first version. If we get some more feedback about the pinata flow, we can improve on it to make it easier for people to get through e.g. by improving the documentation how and which types of keys they need to generate.

  • Does the user have to wait after creating the pinata key (until it is activated)?
  • Can I upload the same picture (the ipfs hash has not changed) in the flow several times?

low-code-nft-marketplace/README.md Show resolved Hide resolved
low-code-nft-marketplace/README.md Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/cis2_client.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/cis2-market/src/cis2_client.rs Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/README.md Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/README.md Outdated Show resolved Hide resolved
low-code-nft-marketplace/market-ui/package.json Outdated Show resolved Hide resolved
low-code-nft-marketplace/mint-ui/README.md Outdated Show resolved Hide resolved
@bogacyigitbasi
Copy link
Author

Hey @DOBEN , here are some explanations about how it works/why?

  • The reason why we have both upload with Pinata/IPFS + Add New Metadata is to allow the minter to use both IPFS upload(metadata generator) and if he has metadata already. So he will mint multiple tokens.
  • For the UI improvements, we have some in the next versions, first one is limited in terms of the functionalities.
  • For the IPFS gateway for pinata is actually defined as a variable in the Constants.ts file. Users can choose it to store only IPFS hash, "gateway.pinata.ipfs" is optional. The same hash should also work with "https://ipfs.io/ipfs" but the IPFS servers sometimes require too much time to respond. For reuploading it, there is nothing implemented that prevents it. We decided that should be something the custom use case owner can decide to allow/disallow it.
  • Royalties are similarly explained in the documentation as far as I remember, we may update it if it is not enough though.
  • We will address the suggested changes regarding links, comments and rest.
    Overall, thank you for the comments. This tool is just for setting a baseline, when all the changes are addressed there will be a video showing how to use it effectively (in addition to the existing document)

@abizjak abizjak requested review from abizjak and DOBEN August 21, 2023 07:30
low-code-nft-marketplace/README.md Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
low-code-nft-marketplace/README.md Show resolved Hide resolved
@DOBEN DOBEN self-requested a review August 22, 2023 12:11
Copy link
Member

@DOBEN DOBEN left a comment

Choose a reason for hiding this comment

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

Well done. This project is a valuable contribution to our dApp examples. Thanks for your continued effort in working on it.

@abizjak abizjak merged commit 01481ca into Concordium:main Aug 22, 2023
4 checks passed
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.

4 participants