N-04 Multiple Optimizable Storage Operations #2322
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sparrowDom
requested review from
DanielVF,
naddison36,
shahthepro and
clement-ux
as code owners
clement-ux
approved these changes
Dec 5, 2024
DanielVF
approved these changes
Dec 5, 2024
sparrowDom
added a commit
that referenced
this pull request
Jan 8, 2025
* initial second implementation of rebasing to another account * update to core functionality * add gas fucntion * simplify execute * simplify mint and burn * initial version of Daniel's yield delegation implementation * prettier and linter fixes * Add slots to OUSD contract to align with existing deployments * Generated new OUSD contract diagrams * Added deploy scripts for OToken upgrades * Generated new OUSD storage diagram * some minor bug fixes * Prettier and fix spelling in comments * Unit test fixes * fix initialise function sigature and visibility of functions * fix some tests * add explanation for the alternativeCreditsPerToken check * prettier * reintroduce the original check * explicitly call rebaseOpt out if the yield delegating account hasn't opted out already * add one more test * add Readme of the token contract logic * force account to be rebasing on yield delegation * add explicit whitelist of allowed states when delegating yield * add more defensive checks where isNonRebasingAccount can not mistankingly be used to return true for yield delegation sources * remove increase/decrease allowance * Update docs with invarients * Use safecast for any downcasting (#2306) * use safecasts for any downcasting * use safecast when downcasting from int256. Also fix tests * prettier * clean up implementation * More invarients around outer functions * Format invarients * further simplify the code when handling different rebase states * remove nonreentrant modifiers from the toke code as they are not needed * allow empty conracts to rebaseOptIn without auto migration * Transfer unit tests for new token implementation (#2310) * remove nonreentrant modifiers from the toke code as they are not needed * add setup for all account types * check all relevant contract initial states * add test between any possible contract accounts * prettier * add some documentation and prettier * More invarients * Correct total supply docs * Transfer unit tests for new token implementation (#2310) * remove nonreentrant modifiers from the toke code as they are not needed * add setup for all account types * check all relevant contract initial states * add test between any possible contract accounts * prettier * add some documentation and prettier * fix test * remove redundant state checks * remove overwriting the same value to alternativeCreditsPerToken * add non zero checks in delegation functions * correct error * correct some contract view modifiers * add a readable error message when allowance is exceeded * reducing 2 functions to 1 * deprecate isUpgraded * rename totalSupply * improve initialisation checks * remove gas optimisation that would also allow for errorneously large transfers * remove underflow checks * simplify code check for rebaseOptIn and add a test for it * remove comments * move the balance and credits query above the rebaseState changes * use _adjustGlobals function to adjust globals in yield delegation * futher simplify the undelegateYield function * unify the variable names * add comment * a couple of more places to utilise the _adjustGlobals function * no need this being a separate variable * undo bug introduction * wrong use of msg.sender bug fix * function doesn't need to return any values * simplify * improve syntax * add events for yield delegation * remove var init * fix deploy file * add tests to catch possible incorrect rebaseOptIn / rebaseOptOut attributions * simplify changeSupply code * add storage slot gap * Comments update * Comments spelling update * correct comments * unify variable names * make credits calculation based of off balance for higher accuracy (in context of rounding errors) * minor gas optimisation * correct storage slot amount so it totals to 200 * Improve rebasing supply accuracy V2 (#2314) * accurate balance accounting v2 for nonRebasingSupply calculation * prettier * correct comment * minor gas optimisation * gas optimisation * better naming * add a test where multiple rebaseOptIn/OptOut calls do not result in increasing account balance * add a check for zero address with governanceRebaseOptIn tx * Update on rebasing * add a check for zero address with governanceRebaseOptIn tx * make an exception for balance exact non rebasing accounts (StdNonRebasing, YieldDelegationSource) * add test for the 1e27 cpt token exception * add a test to for creditsBalanceOf and creditsBalanceOfHighres * add nonRebasingCreditsPerToken to the test * add auto migration test and revert test for rebaseOptOut * prettier * add tests for missing requires in yield delegation * simplify code * revert to the previous implementation of the deprecated function * add OETH upgrade deployment file * change license to Business Source License * prettier * on changeSupply round up in the favour of the protocol * round down when calculating credits from balances * Revert "round down when calculating credits from balances" This reverts commit dc854bc. * fix typos (#2323) * gas optimisation (#2322) * add missing natspec (#2321) * L-02 Missing Docstrings (#2319) * add missing natspec * corrections to code comments * Correct globals storage * Only empty accounts can rebaseOptIn if already rebasing * gas optimisation * remove extra new line * optimise gas when setting alternativeCreditsPerToken (#2325) * Certora Formal Verification for OUSD (#2329) * Add OUSD spec files and running scripts * Small updates to running scripts. * Add invariants to SumOfBalances spec. * Fix certora spec. Starting with an invalid state would result in an invalid state --------- Co-authored-by: Nicholas Addison <[email protected]> Co-authored-by: Daniel Von Fange <[email protected]> Co-authored-by: Roy-Certora <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue by Open Zeppelin team:
Multiple optimizable storage reads and writes were identified in the OUSD contract:
Within the _adjustAccount function, the alternativeCreditsPerToken for non-rebasing accounts is always set to 1e18, even when the mapping already has that value.
To lower gas consumption, consider reducing unnecessary storage reads by caching these values in memory variables, and only write to storage when the value needs to be updated.
Origin comment:
_creditsPerTokenfixed as suggestedchangeSupplyis a called rarely and by the vault and we'd rather keep better code readability compared to gas optimisationundelegateYieldis a called rarely by the governor we'd rather keep better code readability compared to gas optimisation_adjustAccountneeds to stay as is, since current (on chain) state of the contract doesn't havealternativeCreditsPerTokenset to1e18(rather to a much larger number). Not adjusting for this value would result in critical errors greatly underreporting non rebasing user balances.