remove raffle tickets from prize pool, replace with pp #246
Conversation
Summary by OctaneNew ContractsNo new contracts were added. Updated Contracts
🔗 Commit Hash: 652b917 |
There was a problem hiding this comment.
Pull request overview
This PR aims to remove raffle tickets as a reward from the spin prize pool and increase the probability of receiving PP (Plume Points) rewards. However, the implementation contains critical bugs that prevent the contract from functioning correctly.
Changes:
- Set
raffleTicketThresholdequal toplumeTokenThreshold(both at 200,000) to effectively disable raffle ticket rewards - Updated comments to reflect the intended change in probability distribution
- Reformatted multiple function signatures to single-line format
- Minor formatting change to call syntax
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raffleTicketThreshold: 200_000, // Used to be up to 600,000 (Approx 40%). We have discontinued this tier by | ||
| // setting it equivalent to plumeTokenThreshold. |
There was a problem hiding this comment.
The existing tests in Spin.t.sol expect raffle ticket rewards to be awarded for probability values in the range 200_001 to 600_000 (see testRaffleTicketReward at line 719-739 and testBrokenStreakRaffleReward at line 598-640). With raffleTicketThreshold now set equal to plumeTokenThreshold, these tests will fail because the raffle ticket reward path is unreachable. The tests need to be updated to reflect the new probability distribution, either by removing raffle ticket test cases or updating them to test PP rewards instead.
| function _safeTransferPlume(address payable _to, uint256 _amount) internal { | ||
| require(address(this).balance >= _amount, "insufficient Plume in the Spin contract"); | ||
| (bool success,) = _to.call{ value: _amount }(""); | ||
| (bool success,) = _to.call{value: _amount}(""); |
There was a problem hiding this comment.
The spacing in the call syntax has been changed from "call{ value: _amount }" to "call{value: _amount}". This is inconsistent with the codebase convention where other files use a space after the opening brace (e.g., PlumeStakingRewardTreasury.sol:179, ManagementFacet.sol:156, StakingFacet.sol:416, AddressUtils.sol:27, MockPUSD.sol:101). For consistency, this should remain as "call{ value: _amount }".
There was a problem hiding this comment.
@ungaro do i have to change my forge version or something? whe ni run forget fmt it changes the existing format of the file
| raffleTicketThreshold: 200_000, // Used to be up to 600,000 (Approx 40%). We have discontinued this tier by | ||
| // setting it equivalent to plumeTokenThreshold. |
There was a problem hiding this comment.
Setting raffleTicketThreshold equal to plumeTokenThreshold breaks the validation logic in the setRewardProbabilities function (line 527), which requires strict inequality (plumeTokenThreshold < raffleTicketThreshold). This will prevent administrators from updating reward probabilities after deployment. Additionally, with these values being equal, the condition on line 299 (probability <= rewardProbabilities.raffleTicketThreshold) will be evaluated for probabilities where probability <= 200_000, which are already captured by line 296-298. This means the "Raffle Ticket" reward path becomes unreachable. If the intention is to discontinue raffle tickets, consider using a value slightly greater than plumeTokenThreshold (e.g., 200_001) to satisfy the validation while ensuring the range is effectively empty, or remove the raffle ticket logic entirely.
| raffleTicketThreshold: 200_000, // Used to be up to 600,000 (Approx 40%). We have discontinued this tier by | |
| // setting it equivalent to plumeTokenThreshold. | |
| raffleTicketThreshold: 200_001, // Previously up to 600,000 (Approx 40%). We have effectively discontinued | |
| // this tier by setting it just above plumeTokenThreshold to satisfy validation while leaving the | |
| // practical raffle range empty. |
| }); | ||
| raffleTicketThreshold: 200_000, // Used to be up to 600,000 (Approx 40%). We have discontinued this tier by | ||
| // setting it equivalent to plumeTokenThreshold. | ||
| ppThreshold: 900_000 // Up to 900,000 (Approx 70%) |
There was a problem hiding this comment.
The comment states PP threshold is "Up to 900,000 (Approx 70%)" but this calculation appears incorrect. If the raffle ticket threshold is effectively discontinued by setting it equal to plumeTokenThreshold at 200_000, then the PP range would be from 200_001 to 900_000, which is 700_000 out of 1_000_000, or exactly 70%. However, the jackpot probabilities also consume part of the range (varying by day from 1 to 20 based on jackpotProbabilities). The actual PP probability would be approximately (700_000 - avg_jackpot_threshold) / 1_000_000. Consider clarifying this comment to account for the dynamic jackpot threshold.
Overview
Detailed findings
|
1 participant
What's new in this PR?
In bullet point format, please describe what's new in this PR.
Why?
What problem does this solve?
Why is this important?
What's the context?