-
Notifications
You must be signed in to change notification settings - Fork 153
Remove pause functions #159
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
Conversation
contracts/UFragments.sol
Outdated
require(!tokenPaused); | ||
_; | ||
} | ||
bool public rebasePausedDeprecated; |
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.
As far I recall making this private should not affect anything. It will also remove these variable from the ABI.
But If we want to be extra-cautious, I'm okay with renaming. 👍
Leaving it to you to decide
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.
From the docs, only order and type determine storage layout: https://solidity.readthedocs.io/en/v0.4.24/miscellaneous.html#layout-of-state-variables-in-storage
Also for most recent v0.7.0: https://solidity.readthedocs.io/en/v0.7.0/internals/layout_in_storage.html?highlight=storage%20layout
This is corroborated by OpenZeppelin's high level guide: https://docs.openzeppelin.com/upgrades/2.6/writing-upgradeable
So I'll move to private
, good idea.
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.
If you want additional certainty on stability of storage layout it could be a good idea to run integration tests on forked mainnet state
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.
Good ideas thanks!
We have a script that dumps storage state so we can compare before and after.
We also have a mainnet-staging that we test with before mainnet production.
rebasePaused = false; | ||
tokenPaused = false; | ||
rebasePausedDeprecated = false; | ||
tokenPausedDeprecated = false; |
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.
the initialize function should be removed entirely when deploying a new template (since it is never called)
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.
Good to know.
There could be a future where we do a separate non-upgrade deployment. Could be on Ethereum or an alternate chain that works with solidity tools. In that case, it'd be useful to have it there already.
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 see - in that case I would recommend keeping as a comment since you want to avoid deploying unreachable code
https://github.com/ampleforth/RFCs/blob/master/RFCs/rfc-3.md