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

Various gas saving measures for the claim function #229

Closed
wants to merge 4 commits into from

Conversation

waynehoover
Copy link
Contributor

@waynehoover waynehoover commented Nov 15, 2023

Some small refactoring that save about 3k gas.

  • reduce gas by

    • not doing the same checks in the factory as in the singleClaim function on each quest contract
    • removing the NFT fee from NFT quests as this is not used
  • remove numberMinted from questFactory and use redeemedTokens on quest instead.

  • renames some errors to be consistent on each quest contract

@waynehoover waynehoover requested a review from a team as a code owner November 15, 2023 01:21
@waynehoover waynehoover changed the title save gas by not doing double checks Various gas saving measures for the claim function Nov 15, 2023
@@ -605,12 +602,9 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
{
Quest storage currentQuest = quests[claimData_.questId];
IQuestOwnable questContract_ = IQuestOwnable(currentQuest.questAddress);
if (!questContract_.queued()) revert QuestNotQueued();
if (block.timestamp < questContract_.startTime()) revert QuestNotStarted();
if (block.timestamp > questContract_.endTime()) revert QuestEnded();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing this line will allow people to be able to claim past the quest end time

@waynehoover
Copy link
Contributor Author

Most of these changes have been implemented in: #228

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