ToB code quality 3: Refactor & comment _adjustTrove #258
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
RickGriff
commented
Jan 22, 2021
| { | ||
| uint newTCR = _getNewTCRFromTroveChange(_collChange, _isCollIncrease, _debtChange, _isDebtIncrease, _price); | ||
| require(newTCR >= CCR, "BorrowerOps: An operation that would result in TCR < CCR is not permitted"); | ||
| function _requireNewICRisAboveOldICR(uint _newICR, uint _oldICR) internal pure { |
There was a problem hiding this comment.
We now have four similar functions: requireICRisAbove... , requireNewICRisAbove... , requireNewTCRsAbove.. , etc.
Perhaps we should just change that to a single:
requireCRsAboveThreshold(uint CR, uint _threshold, string memory _error)
and pass the appropriate ICR, TCR, MCR, etc.
We'd have to pass different error strings in each call though if we want good information in tests, which makes the top-level code messy again, and the calls are no longer more concise than a naked require. Not sure which I prefer.
There was a problem hiding this comment.
yes, maybe it’s ok as it is, also I wonder if passing the strings may increase gas
RickGriff
changed the title
RickGriff
changed the title
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.
Addresses ToB's code quality recommendation 3: #245
"Consider refactoring the adjustTrove function. This adjustTrove function implements some important operations, but it is difficult to review because of thelack of inline documentation and their checks cover a large variety of corner cases. This will make the code easier to understand, maintain, and review."
I've extracted a couple of sub-functions, renamed some
requiresand added comments in an attempt to make the code clearer.There's only so much we can do though - it's still a complicated function, given the number of branches and different cases it covers.
(PS, ugh - I wrote "adjustLoan" in commit messages. That was its name for a long time, I have clearly not fully "adjusted"...)
Fixes #245