Voting Multipliers #100
Voting Multipliers #100
Conversation
…onfig with tests Changes to be committed:
RonTuretzky
force-pushed
the
feat/voting_multiplier
branch
from
3a0c818
to
daeb1ce
Compare
src/YieldDistributor.sol
Outdated
| uint256 breadVotingPower = this.getVotingPowerForPeriod(BREAD, lastCycleStart, lastClaimedBlockNumber, _account); | ||
| uint256 butteredBreadVotingPower = | ||
| this.getVotingPowerForPeriod(BUTTERED_BREAD, lastCycleStart, lastClaimedBlockNumber, _account); | ||
| return (breadVotingPower + butteredBreadVotingPower) * multiplier / PRECISION; |
There was a problem hiding this comment.
Ok I see what's going on. TBH I don't think PRECISION should be shared here since it's being used in two different calculations. I think the getTotalMultipliers function should probably return a tuple like (multiplier, multiplier_precision) and that should be used here. Ignore my previous comment about using "1 instead of 1e18"
There was a problem hiding this comment.
so you're saying each multiplier should have it's own precision potentially? can't we just assume they are all 1e18?
| @@ -20,7 +21,7 @@ import {IYieldDistributor} from "src/interfaces/IYieldDistributor.sol"; | |||
| * @custom:coauthor kassandra.eth | |||
| * @custom:coauthor theblockchainsocialist.eth | |||
| */ | |||
| contract YieldDistributor is IYieldDistributor, OwnableUpgradeable { | |||
| contract YieldDistributor is IYieldDistributor, OwnableUpgradeable, VotingMultipliers { | |||
There was a problem hiding this comment.
TBH I'm a little dubious of the upgrade safety here. It would probably be best to implement VotingMultipliers to use ERC7201 storage layouts (like they do with OwnableUpgradeable) to avoid storage layout collisions. It seems complicated but it's actually a pretty trivial one time calculation and some boilerplate code for accessing storage variables.
There was a problem hiding this comment.
I was thinking we should just deploy a new proxy for this version, and use the setter on the bread contract, as this would also help us resolve #96
| /// @title VotingMultipliers | ||
| /// @notice A contract for managing voting multipliers | ||
| /// @dev Implements IVotingMultipliers interface | ||
| contract VotingMultipliers is OwnableUpgradeable, IVotingMultipliers { |
There was a problem hiding this comment.
So I think this approach of just a single array is fine, but it's quite in inefficient. Consider most users will probably not have any multipliers. This approach requires every user to check every multiplier that was ever created for every calculation. An alternative approach could be:
- a mapping of approved multipliers (instead of the array to iterate over)
- a mapping of user addresses to a list of multipliers they'd like to apply
An explicit update step to a user's list of multipliers would be required (maybe there's a way to work around that in a way that doesn't tradeoff security? maybe let user's set an "autoapply new multipliers" setting?) and then the ecosystem would only apply those ones. It also gives people more control over their setup. This of course comes with the tradeoff of creating a UI for people to add/remove multipliers, but having a UI for this seems like it would actually be a nice thing to have regardless.
There was a problem hiding this comment.
Can def change it to mapping > list but I don't think the user should be able to control their multipliers. If it's a non provable multiplier (for example voting streaks) then it should just be activated \ deactivated without their interaction. If it's something they have to prove (like twitter likes for example) then they should have a UI for submitting the proof , but no further than that. I also can't imagine why a user would want to remove a multiplier?
There was a problem hiding this comment.
@bagelface why would a UI be needed to add/remove multipliers? Isn't it possible to check in the UI what multipliers from the allowed multipliers the user has and add them automatically?
I also suggest that the original design can be improved by making it possible to query multiple users at onces, thus reducing external calls to multipliers to be proportional to the amount of multipliers, and not a quadratic complexity of m external calls for n users.
Regarding memory concerns (since many users might not have any multipliers at all, returning 1 as a multiplier for most users) it should be noted memory in solidity is not as expensive as it seems:
x^2/512 + 3x (x is number of 32 byte words in memory)
This means that for 20,000 gas you can expand memory up to ~2500 EVM words, which is 80kb - compared to external calls this cost is negligible
There was a problem hiding this comment.
I think that this function is only supposed to be called in the context of a single voter voting (that is, it is called within the context of castVote).
But I'm not sure I understand what you mean, are you referring to making 1 function call to receive the total multiplier for all multipliers? Currently within the design, the multipliers are intended to be separate contracts...Do you mean making it so that multipliers are struct instead? That could def improve efficiency as you say....
However some of these multipliers would require adding custom logic for validation and integrations, which makes it inconvenient as we would need to upgrade the contract for each instance of that happening
There was a problem hiding this comment.
nvm, for some reason I thought this is called for each user in some function to calculate the total voting power, but on a secong look it seems this is called only when a user votes
Edit: yeah saw your edit now
There was a problem hiding this comment.
Check out #71 for more details :)
There was a problem hiding this comment.
@bagelface why would a UI be needed to add/remove multipliers? Isn't it possible to check in the UI what multipliers from the allowed multipliers the user has and add them automatically?
I also suggest that the original design can be improved by making it possible to query multiple users at onces, thus reducing external calls to multipliers to be proportional to the amount of multipliers, and not a quadratic complexity of m external calls for n users.
Regarding memory concerns (since many users might not have any multipliers at all, returning 1 as a multiplier for most users) it should be noted memory in solidity is not as expensive as it seems:
x^2/512 + 3x (x is number of 32 byte words in memory)This means that for 20,000 gas you can expand memory up to ~2500 EVM words, which is 80kb - compared to external calls this cost is negligible
If each multiplier is it's own token contract then we need to either iterate over a master list of token contracts for each voting power calculation or maintain a user => multipliers mapping, which would need to be updated explicitly with each new multiplier token minted. We could potentially build that into the multiplier design. OR we could have some kind of explicit "refresh" function.
Really all I'm suggesting is maybe there is a better way than just maintaining a single master list of all available multiplier token contracts and iterating over them every time a calculation for voting power is made because many users will only have one or two multipliers.
There was a problem hiding this comment.
The problem is that then we are forcing the user to perform some action in order to add the multiplier to their list. So for example,lets say we have a community call attendance multiplier, which is awarded as an NFT for someone for attending a community call.
In this design, the user would not have to do anything other than attend the call, next time he votes during the master list iteriation the YD will detect they are eligible for a voting multiplier and will apply it.
In the suggested design , the user will have to perform some action (or through account abstraction / refresh function breadchain will have to account for this) in order for the multiplier to be added to their list.
While this is more gas efficient, the tradeoff here is either offchain infrastructure and additional engineering to maintain a synchronized state , or additional friction on the user.
Because we are deploying on Gnosis , the actual additional cost on the user to iterate through the master list is negligible, so my opinion is to ignore it.
The current cost of voting is $0.000169425051222655 and I'm guessing that this iteriation will not increase it in any order of magnitude.
For reference, users can get 0.0005 / week through through the Gnosis Faucet
@rubydusa would love your opinion on this as well
There was a problem hiding this comment.
Im not necessarily suggesting any specific design, just pointing out a potentially issue with the current design. I don't think tx costs are a huge concern, but the block gas limits might (30,000,000). We should run a gas simulation on 1,000 and 10,000 multipliers and see how it looks. If it seems like it might be, maybe we can find some tradeoff that automatically pushes an update to a user's list when a new multiplier is issued.
There was a problem hiding this comment.
Added tests with 5b9b7b3
| Gas | $ Cost | |
|---|---|---|
| Current | 152_133 | 0.000228199501217064 |
| 100 Multipliers | 364_702 | 0.000547053002917616 |
| 1000 Multipliers | 2_272_819 | 0.0034092285181825522 |
It seems the increase is not substantial , and I'm doubting we ever get to 1000 multipliers.
|
Deprioritizing for now |
#71
This is not upgrade-safe due to new variables introduced in an inherited class. Suggesting to couple this PR with #96 and create a brand new YieldDistributor