Referral Rewards #274

jonathandiep · 2024-04-04T16:05:37Z

No description provided.

jonathandiep · 2024-04-05T08:46:28Z

contracts/QuestFactory.sol

@@ -166,7 +172,8 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
                actionType_,
                questName_,
                "erc20",
-                projectName_
+                projectName_,
+                referralRewardFee_


Are we going to need to update the QuestCreated event in order to get the referral reward fee value?

Can't we consistently derive it from stuff we're already throwing?

Hmm I think we can keep the same event. We may need to update our db model in order to store the value though

jonathandiep · 2024-04-05T08:52:51Z

contracts/QuestFactory.sol

@@ -419,7 +428,29 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
            emit QuestClaimed(claimer_, quest.questAddress, questId_, rewardToken_, rewardAmountOrTokenId);
        }
        if(ref_ != address(0)){
-            emit QuestClaimedReferred(claimer_, quest.questAddress, questId_, rewardToken_, rewardAmountOrTokenId, ref_, 3333, mintFee);
+            if (IQuestOwnable(quest.questAddress).startTime() > referralRewardTimestamp) {
+                emit QuestClaimReferred(


I think we're only going to want to emit this on erc20 quests (that way we also won't need to emit the "empty data"

SGTM - might be easier to just emit empty data though, keeps our data shaped more consistently. I don't have a strong opinion depends a lot more on ease-of-consumption.

Quazia

LGTM - only things blocking are the fuzzed test and not doing external calls using this.

Quazia · 2024-04-05T15:23:20Z

contracts/Quest.sol

-        if (ref_ != address(0)) ref_.safeTransferETH(_claimFee() / 3);
+        if (ref_ != address(0)) {
+            ref_.safeTransferETH(_claimFee() / 3);
+            _updateReferralTokenAmount(ref_);


This feels wrong to transfer some out and to account for the other internally

Quazia · 2024-04-05T15:23:56Z

contracts/Quest.sol

@@ -155,7 +167,7 @@ contract Quest is ReentrancyGuardUpgradeable, PausableUpgradeable, Ownable, IQue
        protocolFeeRecipient.safeTransferETH(protocolPayout);

        // transfer reward tokens
-        uint256 protocolFeeForRecipient = this.protocolFee() / 2;
+        uint256 protocolFeeForRecipient = (this.protocolFee() / 2) - referralClaimTotal;


Nice approach - clean

contracts/Quest.sol

Quazia · 2024-04-05T15:32:04Z

contracts/Quest1155.sol

@@ -180,6 +181,10 @@ contract Quest1155 is ERC1155Holder, ReentrancyGuardUpgradeable, PausableUpgrade
        return questId;
    }

+    function referralRewardAmount() external pure returns (uint256) {


I would confirm we wanna do 0 for these. Makes sense but let's confirm with product just so there are no surprises.

Will confirm, but don't think we can give a percentage of 1155s lol

Quazia · 2024-04-05T15:32:44Z

contracts/QuestFactory.sol

@@ -166,7 +172,8 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
                actionType_,
                questName_,
                "erc20",
-                projectName_
+                projectName_,
+                referralRewardFee_


Can't we consistently derive it from stuff we're already throwing?

Quazia · 2024-04-05T15:33:46Z

contracts/QuestFactory.sol

@@ -419,7 +428,29 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
            emit QuestClaimed(claimer_, quest.questAddress, questId_, rewardToken_, rewardAmountOrTokenId);
        }
        if(ref_ != address(0)){
-            emit QuestClaimedReferred(claimer_, quest.questAddress, questId_, rewardToken_, rewardAmountOrTokenId, ref_, 3333, mintFee);
+            if (IQuestOwnable(quest.questAddress).startTime() > referralRewardTimestamp) {
+                emit QuestClaimReferred(


SGTM - might be easier to just emit empty data though, keeps our data shaped more consistently. I don't have a strong opinion depends a lot more on ease-of-consumption.

Quazia · 2024-04-05T15:35:59Z

contracts/QuestFactory.sol

@@ -484,6 +515,10 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
        defaultMintFeeRecipient = mintFeeRecipient_;
    }

+    function setReferralRewardTimestamp(uint256 timestamp_) external onlyOwner {


Need to remember to call this as part of the deployment. Another option is to actually just set this in the initialize call.

It's a part of the initialize call already, but I think we are going to need to call this function on chains where we upgrade

Quazia · 2024-04-05T15:36:48Z

test/Quest.t.sol

+                        CLAIM REFERRAL FEES
+    //////////////////////////////////////////////////////////////*/
+
+    function test_claimReferralFees() public {


Can we fuzz this test just to check edges? Preference for fuzzed tests whenever there's accounting in play.

I updated this to be testFuzz_claimReferralFees() and based off the results when running forge test, I don't think it's actually running the fuzz tests

test_fuzz_claimReferralFees() doesn't work either -- lets sync up on this

The name isn't important, you have to pass in a fuzzable arg:

function test_someFuzzableFunction(uint256 fuzzableArg) { // ... contract.someMutativeFunctionWithArguments(fuzzableArg); // ... }

But it doesn't really make sense here because the only input is an address which only enables a binary logic branch: either the address has fees to claim or it does not. The purpose of fuzz testing is mainly to catch cases off the happy path that unexpectedly break the contract's invariants.

Generally, you'd want to either mock out something the function depends on to return a fuzzed value (rather than enforcing the correct execution path by performing the "normal" operations inline as pre-assertion setup), or accept arguments which can be manipulated by the fuzz test. The only thing that seems fuzzable to me in this specific function is the time, since you're warping around and expecting certain behavior depending on the time.

Less concerned with the address more concerned with the accounting around the referral payouts since this is the only test function that's covering those accounting paths.

Quazia

LGTM - we can add fuzzing for referralRewardFee_ as a follow-on, doesn't need to be blocking.

…s()`

…nction

Quazia · 2024-04-18T18:56:05Z

contracts/Quest.sol

@@ -170,7 +170,7 @@ contract Quest is ReentrancyGuardUpgradeable, PausableUpgradeable, Ownable, IQue
        uint256 protocolFeeForRecipient = (this.protocolFee() / 2) - referralClaimTotal;
        rewardToken.safeTransfer(protocolFeeRecipient, protocolFeeForRecipient);

-        uint256 remainingBalanceForOwner = rewardToken.balanceOf(address(this));
+        uint256 remainingBalanceForOwner = rewardToken.balanceOf(address(this)) - referralClaimTotal;


This doesn't account for the fact that some claims might have gone through already.

Gonna need to update the value in the claim too. I would maintain to variables:
referralClaimTotal and totalReferralsClaimed or something like this.

Here we want:
uint256 remainingBalanceForOwner = rewardToken.balanceOf(address(this)) - (referralClaimTotal - totalReferralsClaimed) ;

Quazia

LGTM

jonathandiep force-pushed the jon/referral-rewards branch from cf2b942 to ed12804 Compare April 5, 2024 08:38

jonathandiep marked this pull request as ready for review April 5, 2024 08:40

jonathandiep requested a review from a team as a code owner April 5, 2024 08:40

jonathandiep commented Apr 5, 2024

View reviewed changes

Quazia requested changes Apr 5, 2024

View reviewed changes

jonathandiep requested a review from Quazia April 5, 2024 23:40

jonathandiep force-pushed the jon/referral-rewards branch from 14e515b to 071ee85 Compare April 9, 2024 16:58

Quazia approved these changes Apr 9, 2024

View reviewed changes

jonathandiep and others added 9 commits April 16, 2024 12:17

feat: Add referral rewards

5a902f5

fix: ensure referral totals are deducted from `withdrawRemainingToken…

a93255b

…s()`

fix: emit QuestClaimReferred to include referral token rewards

4e09cfa

fix: prevent _updateReferralTokenAmount from calling an external fu…

5ff4460

…nction

test: Add tests for new Errors being emitted

0843742

test: Use fuzz test on claimReferralFees

be71dda

fix: Add questId to the claimReferrerFees

bef50eb

chore: Sepolia deployments

418cb23

Test(quest): test_fuzz_claimReferralFees

7500d39

jonathandiep force-pushed the jon/referral-rewards branch from bb209c6 to 7500d39 Compare April 16, 2024 19:18

jonathandiep added 2 commits April 17, 2024 13:38

chore: Upload Sepolia deployments

da7e09c

fix: Keep referral amounts even after withdrawals

33a7089

Quazia requested changes Apr 18, 2024

View reviewed changes

fix: track amount claimed to process correct withdrawal

10564e0

Quazia approved these changes Apr 18, 2024

View reviewed changes

jonathandiep added 2 commits April 18, 2024 14:26

chore: Upload broadcast

1f26922

chore: Update json abi files

25690cb

jonathandiep merged commit d0e7c24 into main Apr 29, 2024
2 checks passed

jonathandiep deleted the jon/referral-rewards branch April 29, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Referral Rewards #274

Referral Rewards #274

jonathandiep commented Apr 4, 2024

jonathandiep Apr 5, 2024 •

edited

Loading

Quazia Apr 5, 2024

jonathandiep Apr 5, 2024

jonathandiep Apr 5, 2024

Quazia Apr 5, 2024

Quazia left a comment

Quazia Apr 5, 2024

Quazia Apr 5, 2024

Quazia Apr 5, 2024

jonathandiep Apr 5, 2024

Quazia Apr 5, 2024

Quazia Apr 5, 2024

Quazia Apr 5, 2024

jonathandiep Apr 5, 2024

Quazia Apr 5, 2024

jonathandiep Apr 5, 2024

ccashwell Apr 8, 2024

Quazia Apr 9, 2024

Quazia left a comment

Quazia Apr 18, 2024

Quazia left a comment

Referral Rewards #274

Referral Rewards #274

Conversation

jonathandiep commented Apr 4, 2024

jonathandiep Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quazia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quazia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quazia left a comment

Choose a reason for hiding this comment

jonathandiep Apr 5, 2024 •

edited

Loading