lens replacement, closed sync accounting

[dapp-whisperer]

Close syncAccounting

The open syncAccounting was closed to just be callable by borrower operations.
This prevents using it to desort CDPs
It's called on adjust and close

We do rely on syncing CDPs on tests.
This seemed to work fine - take out and repay 1 debt unit as two actions

/// @dev sync cdp by adding and removing debt
function _syncIndividualCdp(bytes32 _cdpId) internal {
    address borrower = sortedCdps.getOwnerAddress(_cdpId);
    vm.startPrank(borrower);
    borrowerOperations.withdrawDebt(_cdpId, 1, bytes32(0), bytes32(0));
    borrowerOperations.repayDebt(_cdpId, 1, bytes32(0), bytes32(0));

    vm.stopPrank();
}

Replace CRLens Usage

CRLens relies on open syncAccounting.

Rather than create a CdpManagerTester with an open version, or replace syncAccounting with an open version that reinserts, I opted to remove CRLens usage for synced TCR, ICR, NICR.

These functions exist in the main code now.

CRLens method revamp

• ability to quote ICR, TCR, NICR by CRLens now works using the same method of withdraw and repay debt
• it requires positionManager() approval to do so

Invariants

invariant_GENERAL_12
invariant_GENERAL_13
invariant_GENERAL_14

were about confirming the CRLens values versus the native synced versions.
This was very useful to confirm synced implemenation worked.
They have been modified to reflect the above.

[rayeaster]

It may not always work in some test scenarios since BorrowerOperation has some require conditions like if the CDP is already below MCR

[dapp-whisperer]

Good point - agreed, but in practice it has worked for the foundry tests.

[GalloDaSballo]

Agree and would change to add 1 coll and remove 1 coll

[GalloDaSballo]

Sending

[GalloDaSballo]

The CR lens is the reference implementation

SyncedCR is being swapped as reference

But no test verifies that SyncedCR and adding and removing debt maintains the reference

IMO
CRlens MUST be kept
Remove and add coll should be tested vs the CR Lens as an invariant
Also the change is redundant as you could just use syncedCR which is a simpler refactor

I question the motive of this PR and believe it reduces the soundness of the codebase

[dapp-whisperer]

Great, I can see that perspective.

So your proposal is to:

• find a way for CRLens to call syncAccounting()
  • Is the "add and remove" one debt unit acceptable solution?
  • we also have to add it as positionManager to all tests that use it
  • we can also add a syncAccounting that reInserts
• keep CRLens and all invariants around it

[GalloDaSballo]

@dapp-whisperer

Suggestion:

• Keep invariants

• Have CRLens do the same operation as the test if need be

• Create comparison invariants from CrLens, to synced version, to new code
  If a chain of comparison is missed then you cannot assume the invariant and functionality is holding

[GalloDaSballo]

Intuitively:

• Keep all Properties instead of deleting them
• Add CRLens as Delegate of Actors
• Use CrLens to add and remove coll to cause the accrual

Alternatively:

• Build a separate Echidna Tester to only test the CrLens Like Behaviour
  -> Add Coll, Remove Coll, compare with Sync functions to verify they are valid

[GalloDaSballo]

Could also do some weirdness with HEVM to have the CR Lens prank BO

[dapp-whisperer]

scrapped