Add Voting streak multiplier contract #128
Conversation
|
|
||
| /// @notice Mapping of user addresses to their multiplier validity period | ||
| mapping(address => uint256) public override userToValidity; | ||
| mapping(address => uint256) public userToValidity; |
There was a problem hiding this comment.
probably add these to IMultiplier and override them here
There was a problem hiding this comment.
Not sure we need to add them to the IMultiplier interface. It's unclear to me how they'd be used differently than the existing functions that are f(address) returns uint256 for multliplying factor and validity
src/YieldDistributor.sol
Outdated
| /// @notice Mapping of user addresses to their current multiplier factor | ||
| mapping(address => uint256) public userToMultiplier; | ||
| /// @notice Mapping of user addresses to their multiplier validity period | ||
| mapping(address => uint256) public userToValidity; | ||
| /// @notice The maximum multiplier incrementation | ||
| uint256 public maxVotingStreakMultiplier; | ||
| /// @notice The increment value for the multiplier | ||
| uint256 public votingStreakMultiplierIncrement; |
There was a problem hiding this comment.
confused on why this is in the yield distributor, this should be implemented in the multiplier no?
There was a problem hiding this comment.
this commit was exploring a design where the yielddistributor itself knows about the voting history of addresses.
i have some feedback from bagel to integrate that will change this significantly, so ignore this for now :)
src/YieldDistributor.sol
Outdated
|
|
||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| constructor(address _votingStreakMultiplier) { |
There was a problem hiding this comment.
we should need to add them through a setter, not through the constructor
also because these contracts are upgradeable, we actually don't use the constructor at all
https://docs.openzeppelin.com/upgrades-plugins/writing-upgradeable
| function finalizeCycle() internal { | ||
| cycles[currentCycle].voteDistribution = projectDistributions; | ||
| cycles[currentCycle].totalVotes = currentVotes; | ||
| cycles[currentCycle].endBlock = lastClaimedBlockNumber; | ||
| currentCycle++; | ||
| } | ||
|
|
||
| /// @notice Initialize the new cycle | ||
| function initializeCycle() internal { | ||
| cycles[currentCycle].startBlock = lastClaimedBlockNumber; | ||
| cycles[currentCycle].projects = projects; |
There was a problem hiding this comment.
why aren't we tracking the cycles in the multiplier itself per user instead of adding additional logic to the yd? I'm against this approach
There was a problem hiding this comment.
Can you elaborate on the solution you're proposing? Tracking the cycles in the multiplier would require updating storage, so it's not immediately clear to me how that would work with the existing implementation of the multipliers extension where all the functions are "view".
With this solution, yes, we modify the YieldDistributor contract, but we add only a single storage write or two (depending on implementation) with each cycle start/end and add additional data that can be useful to implementing other multipliers.
To be clear, I'm not necessarily endorsing this specific implementation, but this approach overall (modifying the YD vs tracking this data in the multiplier)
There was a problem hiding this comment.
This breaks the entire pattern and design of the multiplier spec, which is meant to not add multiplier logic to the YD but instead provide a way to add modular and decoupled voting power logic...
if we want this data on the YD in general that's another question, but as far as I can tell of those data structures can sit inside the multiplier and I can't think of other multipliers that would use this.
I'm very against adding any code to the YD in general,and I don't understand why we need all this data for the voting streak multiplier. Don't we just need a counter that goes up when the user votes?
My suggestion would be to hook into the multiplier when the user votes , update their last voted time and increment a counter. Next time they vote, we can use the CYCLE_LENGTH to determine whether they've broken their streak or not by referencing whether their last time voted is longer than CYCLE_LENGTH
There was a problem hiding this comment.
"My suggestion would be to hook into the multiplier when the user votes"
What exactly do you mean by this? How can we "hook" into the multiplier when the user votes without updating the YieldDistributor or adding an additional transaction? I guess we can add a contract that wraps the vote transaction and updates multipliers in the same transaction, but that just seems awkward. Feel free to DM me to discuss.
fwiw, this is not a suggestion to "add multiplier logic to the YD". It is a suggestion to add storage variables for data that didn't previously have a clear onchain use case (so it's emitted as an event).
There was a problem hiding this comment.
seems we'll have to align on a path forward to get this going.
@RonTuretzky from my perspective the updates here to the YD are not multiplier logic, they are just adding relevant Cycle data (meaning a YD distribution Cycle) which the new voting streak multiplier uses to implement its own functions.
if you see an alternate strategy, about hooking into the multiplier when the user votes, can you clarify how that could be implemented? the only way i can see that working is also adding logic to the YD in the castVote function 🤔
There was a problem hiding this comment.
I'm against adding anymore variables to the YD in general , it will require complex upgrade logic and add more variables I'm not sure are needed.
My suggestion would be to hook into the multiplier when the user votes , update their last voted time and increment a counter. Next time they vote, we can use the CYCLE_LENGTH to determine whether they've broken their streak or not by referencing whether their last time voted is longer than CYCLE_LENGTH
^ this is my suggestion
There was a problem hiding this comment.
As per my previous comment, we need clarification from you on what this means:
My suggestion would be to hook into the multiplier when the user votes"
What exactly do you mean by this? How can we "hook" into the multiplier when the user votes without updating the YieldDistributor or adding an additional transaction? I guess we can add a contract that wraps the vote transaction and updates multipliers in the same transaction, but that just seems awkward. Feel free to DM me to discuss.
|
After some discussion and advice we are going a different direction. Closing in favor of #129 |
[#127]