Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Voting Multipliers #100
base: dev
Are you sure you want to change the base?
Voting Multipliers #100
Changes from 22 commits
47bdbff
0aa29f4
2534725
bc9abf8
12fa7ea
faf437f
8c89ed0
8dd45db
e0d2527
3906049
3f06af0
0cc8661
6f95cc5
1c6c8c0
fffe13b
fc1221b
daeb1ce
cf53a4f
dce84d8
3d92685
bf324f1
5b9b7b3
12bfd60
50787c1
47333b3
967d6cc
706cd8e
2d04f5c
6e78550
ec07fb1
f867e06
80b7345
de0309d
1c5f5cd
0d4836e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ensureYieldDistributor
andVotingMultipliers
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realizing this will be invalid if
cycleLength
is changed. We need a "lastCycleLength" variable. Maybe it'd be good to create aCycle
struct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should open a separate issue for this ... also mark it as bug so we don't upgrade changes without considering this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line break