Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/more cleanup #695

Merged
merged 5 commits into from
Oct 17, 2023
Merged

Feat/more cleanup #695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09d00eb
_updateDebtRedistributionIndex naming consistency
dapp-whisperer Oct 14, 2023
f7c4f40
syncaccounting cdpupdated event + small refactor
dapp-whisperer Oct 15, 2023
ca863c6
lint
dapp-whisperer Oct 15, 2023
ddbbf6d
MultiCdpGetter uses synced values
dapp-whisperer Oct 15, 2023
8a465fc
sync coll index regardless of fee split
Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/contracts/contracts/CdpManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,8 +877,8 @@ contract CdpManager is CdpManagerStorage, ICdpManager, Proxy {
Cdps[_cdpId].status = Status.active;
Cdps[_cdpId].liquidatorRewardShares = _liquidatorRewardShares;

_applyAccumulatedFeeSplit(_cdpId);
_updateRedistributedDebtSnapshot(_cdpId);
cdpStEthFeePerUnitIndex[_cdpId] = systemStEthFeePerUnitIndex; /// @audit We critically assume global accounting is synced here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is ok here since BorrowerOperations already call syncGlobalAccounting() before

_updateRedistributedDebtIndex(_cdpId);
uint256 stake = _updateStakeAndTotalStakes(_cdpId);
uint256 index = _addCdpIdToArray(_cdpId);

Expand Down
141 changes: 72 additions & 69 deletions packages/contracts/contracts/CdpManagerStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,52 +332,77 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo
return (cdpDebtRedistributionIndex[_cdpId] < systemDebtRedistributionIndex);
}

function _updateRedistributedDebtSnapshot(bytes32 _cdpId) internal {
uint256 _L_EBTCDebt = systemDebtRedistributionIndex;
function _updateRedistributedDebtIndex(bytes32 _cdpId) internal {
uint256 _systemDebtRedistributionIndex = systemDebtRedistributionIndex;

cdpDebtRedistributionIndex[_cdpId] = _L_EBTCDebt;
emit CdpDebtRedistributionIndexUpdated(_cdpId, _L_EBTCDebt);
cdpDebtRedistributionIndex[_cdpId] = _systemDebtRedistributionIndex;
emit CdpDebtRedistributionIndexUpdated(_cdpId, _systemDebtRedistributionIndex);
}

// Add the borrowers's coll and debt rewards earned from redistributions, to their Cdp
/// @dev Calculate the new collateral and debt values for a given CDP, based on pending state changes
function _syncAccounting(bytes32 _cdpId) internal {
(, uint _newDebt, , uint _pendingDebt, uint256 _debtIndexDelta) = _applyAccumulatedFeeSplit(
_cdpId
);
// Ensure global states like systemStEthFeePerUnitIndex get updated in a timely fashion
// whenever there is a CDP modification operation,
// such as opening, closing, adding collateral, repaying debt, or liquidating
_syncGlobalAccounting();

uint256 _oldPerUnitCdp = cdpStEthFeePerUnitIndex[_cdpId];
uint256 _systemStEthFeePerUnitIndex = systemStEthFeePerUnitIndex;

// Update if there is a debt redistribution index delta
if (_debtIndexDelta > 0) {
_updateRedistributedDebtSnapshot(_cdpId);
(
uint256 _newColl,
uint256 _newDebt,
uint256 _feeSplitDistributed,
uint _pendingDebt,
uint256 _debtIndexDelta
) = _calcSyncedAccounting(_cdpId, _oldPerUnitCdp, _systemStEthFeePerUnitIndex);

// If any collShares or debt changes occured
if (_feeSplitDistributed > 0 || _debtIndexDelta > 0) {
Cdp storage _cdp = Cdps[_cdpId];

uint prevCollShares = _cdp.coll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint256 for prevCollShares as well?

uint256 prevDebt = _cdp.debt;
if (prevDebt != _newDebt) {
{
// Apply pending debt redistribution to this CDP
_cdp.debt = _newDebt;
_emitCdpUpdatedEventForSyncAccounting(_cdpId, _cdp, _newDebt, _cdp.coll);

// Apply Fee Split
if (_feeSplitDistributed > 0) {
_applyAccumulatedFeeSplit(
_cdpId,
_newColl,
_feeSplitDistributed,
_oldPerUnitCdp,
_systemStEthFeePerUnitIndex
);
}

Comment on lines +368 to +377
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change conditionally updates the global fee index

This will cause people to pay fees for collateral they added later

Rounding error for fees should favor users as a way to avoid undercollateralizing their position by mistake

Copy link
Contributor

@rayeaster rayeaster Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GalloDaSballo this _applyAccumulatedFeeSplit() is updating collShare & feePerUnit tracker for an individual CDP, not global fee index.

// Apply Debt Redistribution
if (_debtIndexDelta > 0) {
_updateRedistributedDebtIndex(_cdpId);

if (prevDebt != _newDebt) {
{
// Apply pending debt redistribution to this CDP
_cdp.debt = _newDebt;
}
}
}
emit CdpUpdated(
_cdpId,
ISortedCdps(sortedCdps).getOwnerAddress(_cdpId),
msg.sender,
prevDebt,
prevCollShares,
_newDebt,
_newColl,
_cdp.stake,
CdpOperation.syncAccounting
);
}
}

function _emitCdpUpdatedEventForSyncAccounting(
bytes32 _cdpId,
Cdp storage _cdp,
uint256 _newDebt,
uint256 _newColl
) internal {
emit CdpUpdated(
_cdpId,
ISortedCdps(sortedCdps).getOwnerAddress(_cdpId),
msg.sender,
_cdp.debt,
_cdp.coll,
_newDebt,
_newColl,
_cdp.stake,
CdpOperation.syncAccounting
);
// sync per stake index for given CDP
if (_oldPerUnitCdp != _systemStEthFeePerUnitIndex) {
cdpStEthFeePerUnitIndex[_cdpId] = _systemStEthFeePerUnitIndex;
}
}

// Remove borrower's stake from the totalStakes sum, and set their stake to 0
Expand Down Expand Up @@ -560,44 +585,22 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo
// Apply accumulated fee split distributed to the CDP
// and update its accumulator tracker accordingly
function _applyAccumulatedFeeSplit(
bytes32 _cdpId
bytes32 _cdpId,
uint256 _newColl,
uint256 _feeSplitDistributed,
uint256 _oldPerUnitCdp,
uint256 _systemStEthFeePerUnitIndex
) internal returns (uint256, uint256, uint256, uint256, uint256) {
// TODO Ensure global states like systemStEthFeePerUnitIndex get timely updated
// whenever there is a CDP modification operation,
// such as opening, closing, adding collateral, repaying debt, or liquidating
// OR Should we utilize some bot-keeper to work the routine job at fixed interval?
_syncGlobalAccounting();

uint256 _oldPerUnitCdp = cdpStEthFeePerUnitIndex[_cdpId];
uint256 _systemStEthFeePerUnitIndex = systemStEthFeePerUnitIndex;

(
uint256 _newColl,
uint256 _newDebt,
uint256 _feeSplitDistributed,
uint _pendingDebt,
uint256 _debtIndexDelta
) = _calcSyncedAccounting(_cdpId, _oldPerUnitCdp, _systemStEthFeePerUnitIndex);

// apply split fee to given CDP
if (_feeSplitDistributed > 0) {
Cdps[_cdpId].coll = _newColl;

emit CdpFeeSplitApplied(
_cdpId,
_oldPerUnitCdp,
_systemStEthFeePerUnitIndex,
_feeSplitDistributed,
_newColl
);
}
Cdps[_cdpId].coll = _newColl;

// sync per stake index for given CDP
if (_oldPerUnitCdp != _systemStEthFeePerUnitIndex) {
cdpStEthFeePerUnitIndex[_cdpId] = _systemStEthFeePerUnitIndex;
}

return (_newColl, _newDebt, _feeSplitDistributed, _pendingDebt, _debtIndexDelta);
emit CdpFeeSplitApplied(
_cdpId,
_oldPerUnitCdp,
_systemStEthFeePerUnitIndex,
_feeSplitDistributed,
_newColl
);
}

// return the applied split fee(scaled by 1e18) and the resulting CDP collateral amount after applied
Expand Down
10 changes: 6 additions & 4 deletions packages/contracts/contracts/MultiCdpGetter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ contract MultiCdpGetter {
for (uint256 idx = 0; idx < _count; ++idx) {
_cdps[idx].id = currentCdpId;
(
_cdps[idx].debt,
_cdps[idx].coll,
,
,
_cdps[idx].stake,
/* status */
/* arrayIndex */
Expand All @@ -94,6 +94,7 @@ contract MultiCdpGetter {

) = cdpManager.Cdps(currentCdpId);

(_cdps[idx].debt, _cdps[idx].coll) = cdpManager.getSyncedDebtAndCollShares(currentCdpId);
(_cdps[idx].snapshotEBTCDebt) = cdpManager.cdpDebtRedistributionIndex(currentCdpId);

currentCdpId = sortedCdps.getNext(currentCdpId);
Expand All @@ -119,8 +120,8 @@ contract MultiCdpGetter {
for (uint256 idx = 0; idx < _count; ++idx) {
_cdps[idx].id = currentCdpId;
(
_cdps[idx].debt,
_cdps[idx].coll,
,
,
_cdps[idx].stake,
/* status */
/* arrayIndex */
Expand All @@ -129,6 +130,7 @@ contract MultiCdpGetter {

) = cdpManager.Cdps(currentCdpId);

(_cdps[idx].debt, _cdps[idx].coll) = cdpManager.getSyncedDebtAndCollShares(currentCdpId);
(_cdps[idx].snapshotEBTCDebt) = cdpManager.cdpDebtRedistributionIndex(currentCdpId);

currentCdpId = sortedCdps.getPrev(currentCdpId);
Expand Down
10 changes: 5 additions & 5 deletions packages/contracts/test/BorrowerOperationsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,9 @@ contract('BorrowerOperations', async accounts => {
assert.equal(alice_EBTCDebtRewardSnapshot_Before, 0)
assert.equal(bob_EBTCDebtRewardSnapshot_Before, 0)

const alicePendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(aliceIndex))
const bobPendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(bobIndex))
for (reward of [alicePendingEBTCDebtReward, bobPendingEBTCDebtReward]) {
const alicePendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(aliceIndex))
const bobPendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(bobIndex))
for (reward of [alicePendingRedistributedDebt, bobPendingRedistributedDebt]) {
assert.isTrue(reward.gt(toBN('0')))
}

Expand All @@ -414,9 +414,9 @@ contract('BorrowerOperations', async accounts => {
const bobNewDebt = await getCdpEntireDebt(bobIndex)

assert.isTrue(aliceNewColl.eq(aliceCollBefore.add(aliceTopUp)))
assert.isTrue(aliceNewDebt.eq(aliceDebtBefore.add(alicePendingEBTCDebtReward)))
assert.isTrue(aliceNewDebt.eq(aliceDebtBefore.add(alicePendingRedistributedDebt)))
assert.isTrue(bobNewColl.eq(bobCollBefore.add(bobTopUp)))
assert.isTrue(bobNewDebt.eq(bobDebtBefore.add(bobPendingEBTCDebtReward)))
assert.isTrue(bobNewDebt.eq(bobDebtBefore.add(bobPendingRedistributedDebt)))

/* Check that both Alice and Bob's snapshots of the rewards-per-unit-staked metrics should be updated
to the latest values of L_STETHColl and systemDebtRedistributionIndex */
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts/test/CdpManagerTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4482,8 +4482,8 @@ contract('CdpManager', async accounts => {
const carolSnapshot_L_EBTCDebt = (await cdpManager.cdpDebtRedistributionIndex(_carolCdpId))
assert.equal(carolSnapshot_L_EBTCDebt, 0)

const carol_PendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(_carolCdpId))
assert.isTrue(carol_PendingEBTCDebtReward.gt(toBN('0')))
const carol_PendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(_carolCdpId))
assert.isTrue(carol_PendingRedistributedDebt.gt(toBN('0')))
})

it("getPendingETHReward(): Returns 0 if there is no pending ETH reward", async () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts/utils/testHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,8 +556,8 @@ class TestHelper {
// console.log(`account: ${account}`)
const rawColl = (await contracts.cdpManager.Cdps(account))[1]
const rawDebt = (await contracts.cdpManager.Cdps(account))[0]
const pendingEBTCDebtReward = (await contracts.cdpManager.getPendingRedistributedDebt(account))
const entireDebt = rawDebt.add(pendingEBTCDebtReward)
const pendingRedistributedDebt = (await contracts.cdpManager.getPendingRedistributedDebt(account))
const entireDebt = rawDebt.add(pendingRedistributedDebt)
let entireColl = rawColl;
return { entireColl, entireDebt }
}
Expand Down