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

Move questNFT to a 1155 #141

Merged
merged 17 commits into from
Jun 28, 2023
Merged

Move questNFT to a 1155 #141

merged 17 commits into from
Jun 28, 2023

Conversation

waynehoover
Copy link
Contributor

@waynehoover waynehoover commented Jun 16, 2023

This changes the NFTQuest type from an ERC721 to an ERC1155 type. When an erc1155 is created we call this a collection. A collection can have many quests which map to token ids in the 1155. The flow is this:

Quest creator:

  • Deploy a new erc1155 "collection" QuestFactory.createCollection
  • Add an NFT to the collection QuestFactory.addQuestToCollection

Quest completer:

  • mint an NFT from the collection QuestFactory.mintQuestNFT

And we have some handy functions for the frontend to use:
QuestFactory.ownerCollectionsByOwner returns all the collections given an user address. This will be useful because a user can choose to either use an existing collection or create a new one when creating a NFT type quest.

contracts/QuestNFT.sol Fixed Show fixed Hide fixed
contracts/QuestNFT.sol Fixed Show fixed Hide fixed
Comment on lines +101 to +111
function mint(address to_, string memory questId_)
public
onlyMinter questChecks(questId_) whenNotPaused nonReentrant
{
QuestData storage quest = quests[questId_];

_mint(to_, quest.tokenId, 1, "");

(bool success, ) = protocolFeeRecipient.call{value: quest.questFee}("");
require(success, 'protocol fee transfer failed');
}

Check warning

Code scanning / Slither

Low-level calls

Low level call in QuestNFT.mint(address,string) (contracts/QuestNFT.sol#101-111): - (success) = protocolFeeRecipient.call{value: quest.questFee}() (contracts/QuestNFT.sol#109)
Comment on lines 74 to 99
function addQuest(
uint256 questFee_,
uint256 startTime_,
uint256 endTime_,
uint256 totalParticipants_,
string memory questId_,
string memory description_,
string memory imageIPFSHash_
) public onlyMinter returns (uint256) {
require (endTime_ > block.timestamp, 'endTime_ in the past');
require (startTime_ > block.timestamp, 'startTime_ in the past');
require (endTime_ > startTime_, 'startTime_ before endTime_');

_tokenIdCounter.increment();
QuestData storage quest = quests[questId_];
quest.startTime = startTime_;
quest.endTime = endTime_;
quest.totalParticipants = totalParticipants_;
quest.questFee = questFee_;
quest.imageIPFSHash = imageIPFSHash_;
quest.description = description_;
quest.tokenId = _tokenIdCounter.current();
tokenIdToQuestId[_tokenIdCounter.current()] = questId_;

return _tokenIdCounter.current();
}

Check notice

Code scanning / Slither

Block timestamp

QuestNFT.addQuest(uint256,uint256,uint256,uint256,string,string,string) (contracts/QuestNFT.sol#74-99) uses timestamp for comparisons Dangerous comparisons: - require(bool,string)(endTime_ > block.timestamp,endTime_ in the past) (contracts/QuestNFT.sol#83) - require(bool,string)(startTime_ > block.timestamp,startTime_ in the past) (contracts/QuestNFT.sol#84)
@waynehoover waynehoover marked this pull request as ready for review June 20, 2023 18:03
@@ -15,73 +16,53 @@ import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/se
/// @title QuestNFT
/// @author RabbitHole.gg
/// @notice This contract is the Erc721 Quest Completion contract. It is the NFT that can be minted after a quest is completed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should update this comment to use 1155

string memory imageIPFSHash_
) public onlyMinter returns (uint256) {
require (endTime_ > block.timestamp, 'endTime_ in the past');
require (startTime_ > block.timestamp, 'startTime_ in the past');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this check? I believe we removed it in Quest.sol so that we can start quests immediately

Copy link
Contributor Author

@waynehoover waynehoover Jun 23, 2023

Choose a reason for hiding this comment

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

Oh yes, we removed the start time checks when creating a quest here: 9a3556a

Good point, I'll remove it.

Copy link
Contributor

@jonathandiep jonathandiep left a comment

Choose a reason for hiding this comment

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

Left a couple of comments, but lgtm!

@waynehoover waynehoover mentioned this pull request Jun 28, 2023
@waynehoover waynehoover merged commit f7c6cf2 into main Jun 28, 2023
Comment on lines +100 to +110
function mint(address to_, string memory questId_)
public
onlyMinter questChecks(questId_) whenNotPaused nonReentrant
{
QuestData storage quest = quests[questId_];

_mint(to_, quest.tokenId, 1, "");

(bool success, ) = protocolFeeRecipient.call{value: quest.questFee}("");
require(success, 'protocol fee transfer failed');
}

Check warning

Code scanning / Slither

Unchecked low-level calls

QuestNFT.mint(address,string) (contracts/QuestNFT.sol#100-110) ignores return value by (success) = protocolFeeRecipient.call{value: quest.questFee}() (contracts/QuestNFT.sol#108)
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