# Quiver Protocol

First analysis of the smart contracts.

#### [QNFT](https://github.com/QuiverCommunity/quiver-contracts/blob/master/contracts/QNFT/QNFT.sol)

1. **[important]** Is every NFT must be unique? Can 2 NFTs have the same `_imageId, _bgImageId, _favCoinId, _lockOptionId`?
- If NFT are totally unique, then the upgrade functions should not exist. Users could just mint new NFT.
- If they are not unique, then a lot of simplification concerning the token's id should be done. It will simplify the codebase as well as reducing gas consumption.

2. **[important]** If NFT must be unique then `upgradeNftImage`, `upgradeNftBackground`, `upgradeNftCoin` does not release the ids of the previous version of the upgraded NFT. The functions should unset the previous NFT's id from the state variable `nftIds` so it can be minted by someone else.

3. **[important]** I saw you ask about how to make the NFT compatible with NFT Marketplace in the doc ["Emotional NFT Technical Spec #13"](https://docs.google.com/document/d/1tAnfPLfWV_P3-qaNuDdLCGsZ_mx3Di1B2tVhEaLGBBA).
- To do so, the NFT smart contract have to implement the [ERC721-Metadata standard](https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#IERC721Metadata). Basically: implement `tokenURI` function and make sure the returned an URI that is accessible from everywhere and contains the metadata as a JSON payload.
- You need to choose where to store the metadata. They have to be off-chain but there is no requirement where the metadata must be hosted. Most NFT marketplaces are compatible with HTTP and IPFS.
- HTTP provides more flexibility because the metadata can be changed at anytime but requires to run an HTTP server.
- IPFS provides a more decentralized solution but requires an onchain transaction to update the metadata. As data uploaded on public IPFS node are not guarantee to be available forever, running a private IPFS node or paying for a pinning IPFS service is more than advised.
- If you implement the metadata using HTTP and as the SC already implements `ERC721Upgradeable`, you just need to implement the function `_baseURI`. Check the preset [`ERC721PresetMinterPauserAutoIdUpgradeable`](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/e83c4774c447fc0cf927914d9bfb80e4021ac521/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoIdUpgradeable.sol#L71-L73). This make the function `tokenURI` to return a unique URI per token using the `baseURI + tokenId`.

The metadata system has been designed this way to reduce a maximum the gas related to data store on-chain. I think the QNFT could contains a lot less data on-chain (no more `string memory _name, string memory _creator_name, string memory _color, string memory _story`) and moved them off-chain on IPFS to keep some immutability property but reduced the gas.

OpenSea metadata implementation is compatible with what they call ["attributes"](https://docs.opensea.io/docs/metadata-standards#section-attributes) that are nicely displayed on the marketplace. That could be something QNFT implements easily.

4. **[important]** Function [`upgradeQstk`](https://github.com/QuiverCommunity/quiver-contracts/blob/092b91ca93008ea992ed3cd1d63b787563dd612a/contracts/QNFT/QNFT.sol#L571-L577)

function upgradeQStk(address _qstk) public onlyOwner {
  require(!unlocked, "QNFT: already unlocked");

  transferFrom(msg.sender, address(this), totalQstkBalance());

  qstk = _qstk;
}

I don't think the QStk token should be transfered from the `msg.sender` to the current smart contract. I think the `transferFrom` should be from `address(this)` and `_qstk` (current contract to new contract)?

5. Function [`mintNFT`](https://github.com/QuiverCommunity/quiver-contracts/blob/092b91ca93008ea992ed3cd1d63b787563dd612a/contracts/QNFT/QNFT.sol#L309-L320)
- [`nftCount` counter increment](https://github.com/QuiverCommunity/quiver-contracts/blob/092b91ca93008ea992ed3cd1d63b787563dd612a/contracts/QNFT/QNFT.sol#L357-L358) should be after using its value as a index. currently, the `nftCount` starts at 1. Moving the increment after will starts the count at 0.

- nftCount = nftCount.add(1);
 nftData[nftCount] = NFTData(
 // ...
+ nftCount = nftCount.add(1);

#### [QNFTSettings](https://github.com/QuiverCommunity/quiver-contracts/blob/master/contracts/QNFT/QNFTSettings.sol)
1. **[important]**`removeImageSet`, `removeBgImage`, `removeFavCoin` [change the id](https://github.com/QuiverCommunity/quiver-contracts/blob/092b91ca93008ea992ed3cd1d63b787563dd612a/contracts/QNFT/QNFTSettings.sol#L201-L202) of the last image by the image removed. This breaks existing NFTs as their image ids point to another one.
2. Why not using IPFS hash instead of `dataUrl` to link to images? It will be more "decentralized" and cheaper for gas.
3. Use better types. Example: no need to use `uint256` to store number from 0 to 100.

#### [QStk contract](https://github.com/QuiverCommunity/quiver-contracts/blob/master/contracts/QStk.sol)
- Why the smart contract is using both `Ownable` and `AccessControl`?
- It will be must simpler and cleaner to use only `AccessControl`
- `AccessControl` has the `DEFAULT_ADMIN_ROLE` that can be used as the owner. `DEFAULT_ADMIN_ROLE` can grant and revoke any roles.
- Functions `addMinter` and `removeMinter` can be removed because already available with `grantRole` and `revokeRole` functions.

#### All contract
- `SafeMath` is not needed since Solidity v0.8. OpenZeppelin keeps it for [backward compatibility](https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2465). It can be removed.

#### Nice to have for ERC20
- [ERC20Snapshot](https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20Snapshot): can be used to safely create mechanisms based on token balances such as trustless dividends or weighted voting
- [ERC20Permit](https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20Permit) *draft*: can be used to change an account’s ERC20 allowance by presenting a message signed by the account