Multipletroves #51
Multipletroves #51
Conversation
… into multipletroves
|
Let's merge latest master and see test pipeline again |
Bring package and CI cleanup into multipletroves
|
alright sers. we're passing CI tests. @SHAKOTN can you look into the solhint failure. |
Codecov Report
@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 97.60% 94.52% -3.08%
==========================================
Files 28 28
Lines 1918 1973 +55
Branches 290 293 +3
==========================================
- Hits 1872 1865 -7
- Misses 20 69 +49
- Partials 26 39 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
We can ignore solhint CI for now, since I will do a repo cleanup after this PR is merged. Another point is to check that coverage decreased by 3% on some contracts. Is it expected or shall we add some more tests? |
There was a problem hiding this comment.
Agreed with @dapp-whisperer that coverage will be bumped from another PR.
Lgtm
packages/contracts/contracts/Interfaces/IBorrowerOperations.sol
Outdated
Show resolved
Hide resolved
| return serialized; | ||
| } | ||
|
|
||
| function getOwnerAddress(bytes32 troveId) public pure returns (address) { |
There was a problem hiding this comment.
What is the purpose of this function? I don't see it being used anywhere.
There was a problem hiding this comment.
I'm not sure tbh, @rayeaster did you write this one?
There was a problem hiding this comment.
I'm not sure tbh, @rayeaster did you write this one?
Yep, that should be added by me. Thought it is a handy function to retrieve owner address from generated bytes32 ID.
There was a problem hiding this comment.
Review
Notes
- Adjusted the BorrowingOperations.sol contract and all of its dependencies accordingly to allow for multiple CDPs per user.
- CDPs are tracked by their individual ID which is unique for the system (based on owner, creation block and the global CDPs count.
- All global operations are performed on individual CDPs and based on their ids
Testing
- All tests were adjusted to reflect the changes introduced
- Coverage was maintained for all of the contracts touched (Coverage deficiencies are only seen on
BorrowerOperationsScript.sol,StabilityPoolScript.sol,LQTYToken.sol,LUSDToken.solandTokenScript.solwhich will be adjusted or removed in a separate PR. - Testing contracts were also adjusted and a new contract to test the multiCDP functionality from a non-EOA actor was introduced. Note that this contract assumes that there are no positions opened for it as it starts the CDP count from 0. It may need to be adjusted as needed for integration tests - LG
- Further fuzzing integration tests could be implemented to check proper handling of multiple users opening and closing multiple troves without any issues (special attention to proper handling of ID's generation, sorting and removals, CDP counts for users in extreme cases, etc) - To be developed with Foundry.
LGTM in general, just dropped a couple of nitty comments.
|
Closes #58 |
Thank you ser for taking time to review the change #143 |
multiple troves will serve as a base to build all other features on top, as it affects more or less every test due to the widespread interface changes.
once we've merged this, all new "feature sets" (liquidations, interest rate, redemptions) should go in their own branches for easier review.