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
| @@ -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
There was a problem hiding this comment.
Is there a foreseeable problem deploying as a new proxy, thus changing the address? if there is not, i would prefer we deploy fresh, therefore do not need to worry about upgrade-safety, and not over-complicated things. but again, this is assuming there is no major problem with changing the address.
There was a problem hiding this comment.
Deploying a new contract is definitely the move in this instance but I mean in future instances where we maybe want to make some changes to the VotingMultipliers.sol contract. Namespacing the storage layouts ensure YieldDistributor and VotingMultipliers are using their own storage locations that won't clash, so if we want to add a variable to one or the other, we can do so safely. I can make a quick branch so show what it would look like.
There was a problem hiding this comment.
I also think there is a strong case to be made for VotingMultipliers to be its own contract.
There was a problem hiding this comment.
What are the pros/cons here?
I believe we should redeploy fresh to avoid upgrade safety issues and address #96 at some point
There was a problem hiding this comment.
#110 is also opened but I would really like to merge this and then maybe hand it off to someone else at a later point
…dev, reintroducing
|
@daopunk @bagelface all issues have been resolved except the issue of inheritance vs modularity. As we plan to migrate to dough-kit sometime in the future, I would suggest we postpone dealing with this design question since we strongly agree that this should be modular and decoupled. tldr: lets go with the path of least resistance for now as it will get reworked eventually anyways #110 has been opened to address this concern, but I suggest we hand that off to someone at a later point and unblock this pr which has been open since oct at this point |
#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