fix(TwabLib): prevent account.cardianlity from exceeding max cardinality #153
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
POOL-1678 TwabLib: prevent incrementation of cardinality above MAX_CARDINALITY
Add a condition to avoid incrementing above MAX_CARDINALITY. |
asselstine
requested changes
Oct 5, 2021
| // AccountDetails.cardinality will continue to be equal to max cardinality. | ||
| if(_accountDetails.cardinality < MAX_CARDINALITY) { | ||
| _accountDetails.cardinality += 1; | ||
| } |
There was a problem hiding this comment.
I know this is simple code, but I'd love to have this tested. It's mission-critical.
Can you:
- extract lines 362 - 373 into a separate pure function that's called
pushObservation(AccountDetails, uint256 cardinality)that returns a new Account details. - expose the function in the harness
Test that it correctly wraps and caps the cardinality
There was a problem hiding this comment.
if it's not clear: also use the pure function instead of the code at 362 - 373!
There was a problem hiding this comment.
Affirmative @asselstine - great feedback for this one. Will test'ify this bit of code.
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.
Prevent the Account specific cardinality from exceeding the MAX_CARDINALITY. The ring buffer length is limited by MAX_CARDINALITY. IF the account.cardinality exceeds the max cardinality, new observations would be incorrectly set or the
observation would be out of "bounds" of the ring buffer.
Once reached the AccountDetails.cardinality will continue to be equal to max cardinality.