Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Cropper tack shutdown #34

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Cropper tack shutdown #34

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11b11bf
Tack from End in cash in proxy actions - wip
talbaneth Aug 18, 2022
00a4d6d
Finalize code, align test
talbaneth Aug 18, 2022
c755fd6
Fix comments
talbaneth Aug 21, 2022
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
11 changes: 9 additions & 2 deletions src/DssProxyActionsCropper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ interface GemJoinLike {
function gem() external returns (GemLike);
function ilk() external returns (bytes32);
function bonus() external returns (GemLike);
function tack(address src, address dst, uint256 wad) external;
}

interface DaiJoinLike {
Expand Down Expand Up @@ -760,13 +761,16 @@ contract DssProxyActionsEndCropper is Common {
bytes32 ilk = GemJoinLike(ethJoin).ilk();
EndLike(end).cash(ilk, wad);
uint256 wadC = _mul(wad, EndLike(end).fix(ilk)) / RAY;
address urnProxy = CropperLike(cropper).getOrCreateProxy(address(this));
// Flux to the proxy's UrnProxy in cropper, so it can be pulled out with the managed gem join
VatLike(vat).flux(
ilk,
address(this),
CropperLike(cropper).getOrCreateProxy(address(this)),
urnProxy,
wadC
);
// Tack from End to allow fleeing, assumes vaults stakes were tacked to End after skimming
GemJoinLike(ethJoin).tack(end, urnProxy, wadC);
// Exits WETH amount to proxy address as a token
CropperLike(cropper).flee(ethJoin, address(this), wadC);
// Converts WETH to ETH
Expand All @@ -783,13 +787,16 @@ contract DssProxyActionsEndCropper is Common {
bytes32 ilk = GemJoinLike(gemJoin).ilk();
EndLike(end).cash(ilk, wad);
uint256 wadC = _mul(wad, EndLike(end).fix(ilk)) / RAY;
address urnProxy = CropperLike(cropper).getOrCreateProxy(address(this));
// Flux to the proxy's UrnProxy in cropper, so it can be pulled out with the managed gem join
VatLike(vat).flux(
ilk,
address(this),
CropperLike(cropper).getOrCreateProxy(address(this)),
urnProxy,
wadC
);
// Tack from End to allow fleeing, assumes vaults stakes were tacked to End after skimming
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Tack from End to allow fleeing, assumes vaults stakes were tacked to End after skimming
// Tack from the End to allow fleeing, assumes vaults' stakes were tacked to the End after skimming

Copy link
Contributor

Choose a reason for hiding this comment

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

Also...is this a good assumption? freeEth and freeGem do not seem to tack to the End, although maybe it is in some external dependency like the Cropper.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be added to the _free function after skimming the vault

Copy link
Contributor

Choose a reason for hiding this comment

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

It can, but do you think it should be done? My intuition is "yes" since we're assuming it here, but I haven't thought about the Cropper code in a while so I may be missing something.

Copy link
Contributor Author

@talbaneth talbaneth Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's separate the 2 questions:

  1. Is it a good assumption that each vaults' stakes were tacked to the End after skimming?

The current cashETH/cashGem implementation bundles vat.flux to the redeemer's urnProxy and cropper.flee from the urnProxy. Only in between these calls does the redeemer's urnProxy has enough collateral to allow tacking to it, so the tacking has to take place there.

One option was to tack in cashETH/cashGem from a list of addresses that have excess stake without necessarily involving the end, but that is messy as there could be multiple such fragmented addresses and having the redeemer looking for them is cumbersome.

The preferred option imo is to force tacking from the end. In case there is not enough stake in the end then the action would just fail, forcing tacking from more vaults to the end.
This might be less efficient in some scenarios but is better imo since it can all be done permissionlessly at a single point in time (after skimming all vaults). Trying to concentracte as much stake at the End is also a desired property for tracking purposes and would make our life easier.
Note that it is similar to the existing situation (though not identical*) where you might not be able to cash if not enough vaults were skimmed (here you cannot cash if not enough vaults were tacked to end).

*It's not identical (and a bit more tricky) since a vault which is skimmed would not necessarily be the source of the tacking to end -
A vault owner can call cropper.quit as part of the shutdown process (can do it before or after skimming).
That vat.forks the vault but does not immediately move the stake along. The original vault's stake can then be explicitly tacked along to the forked destination (or not).
So as mentioned this means that a vault that is skimmed would not necessarily be the direct source of the tacking to end (as it might be forked beforehand without tacking the stake along).
That makes it less convenient for shutdown tacking keepers/scripts, and in some cases, the keepers/scripts will need to track the actual stake before tacking it to end (and not simply follow only Skim events).
(@julienmartinlevrai , we need to discuss how sophisticated we want the current keeper to be).

  1. Should we add tacking to freeEth and freeGem?

freeEth and freeGem are for vault owners to exit their excess collateral.
Tacking the non-excess collateral to end would not help the vault owners, so adding it here as a "volunteering" / "system good" step seems weird. Note that this is different from skimming, which does benefit the vault owners (as it is a pre-condition for End.free).
Moreover, given the discussion in (1) which claims that the skimmed vault is not necessarily the source of the needed tacking (since the position might have been forked without tacking) I'd say not to do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

the skimmed vault is not necessarily the source of the needed tacking (since the position might have been forked without tacking)

If the vault has stake < ink, freeETH and freeGem will fail because they call exit. So we can safely assume stake >= ink. Then we could just tack(stake - ink).

Let’s make our life easier. Vault owners will have no issue contributing a bit of gas to the "system good" if we suggest they should. And if they really don’t want to, they can always do their own thing.

Copy link
Contributor

@gbalabasquer gbalabasquer Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't tend to do that just because tacking to the end is not part of the interest of the free user (as Tal mentioned) but also because it would give a false assumption. If users would end up using a custom proxy action that doesn't do it then the keeper would be assuming something that hasn't been enforced.
Anyway I don't have a super strong opinion on this, so if you want to add it, I won't opposed either.

Copy link
Contributor

@julienmartinlevrai julienmartinlevrai Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

but also because it would give a false assumption

We are already making a false assumption as we tack from the End. Adding a tack to the End makes that assumption less false.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is the same than assuming the End has enough ink for cashing (enough CDPs were skimmed). That's work for the keepers.

Copy link
Contributor

Choose a reason for hiding this comment

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

If users would end up using a custom proxy action that doesn't do it then the keeper would be assuming something that hasn't been enforced.

The Keeper cannot assume it in any case, agreed. But I'm somewhat partial to trying to make things as smooth as possible, given how chaotic any real ES is bound to be.

Tacking the non-excess collateral to end would not help the vault owners, so adding it here as a "volunteering" / "system good" step seems weird.

Except that in this same contract, the assumption that tacking was done is made. So to me doing the tacking in freeEth/freeGem makes sense, and can serve as a guide for other integrators to know at least what needs to happen eventually. The argument against this that makes the most sense to me is that Vault holders don't have any obligation to pay for this operation (strictly speaking, it is DAI holders who have an incentive to care). But as pointed out, no one has to use this contract, and I don't think the cost will be too extreme.

So ultimately it doesn't seem like a big deal. Either more work for Keepers/DAI redeemers, or a little more cost for Vault holders to exit. I guess it's probably most consistent with the design philosophy of other Maker contracts to omit the tack calls here.

GemJoinLike(gemJoin).tack(end, urnProxy, wadC);
// Exits token amount to the user's wallet as a token
uint256 amt = wadC / 10 ** (18 - GemJoinLike(gemJoin).dec());
CropperLike(cropper).flee(gemJoin, msg.sender, amt);
Expand Down
57 changes: 49 additions & 8 deletions src/test/DssProxyActionsCropper.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,34 @@ contract MockCdpManager {
}
}

contract User {
DSProxy public proxy;
address public dssProxyActionsEnd;

receive() external payable {}

constructor(ProxyRegistry registry, address _dssProxyActionsEnd) public {
proxy = DSProxy(registry.build());
dssProxyActionsEnd = _dssProxyActionsEnd;
}

function approve(address token, address usr, uint256 amount) public {
Token(token).approve(usr, amount);
}

function end_pack(address a, address b, uint256 c) public {
proxy.execute(dssProxyActionsEnd, abi.encodeWithSignature("pack(address,address,uint256)", a, b, c));
}

function end_cashETH(address a, address b, uint256 c) public {
proxy.execute(dssProxyActionsEnd, abi.encodeWithSignature("cashETH(address,address,uint256)", a, b, c));
}

function end_cashGem(address a, address b, uint256 c) public {
proxy.execute(dssProxyActionsEnd, abi.encodeWithSignature("cashGem(address,address,uint256)", a, b, c));
}
}

contract ProxyCalls {
DSProxy proxy;
address dssProxyActions;
Expand Down Expand Up @@ -669,7 +697,9 @@ contract DssProxyActionsTest is DssDeployTestBase, ProxyCalls {
(inkV, artV) = vat.urns("ETH", charterProxy);
assertEq(inkV, 0);
assertEq(artV, 0);
uint256 remainInkVal = 2 ether - 300 * end.tag("ETH") / 10 ** 9; // 2 ETH (deposited) - 300 DAI debt * ETH cage price
uint256 skimmedEth = vat.gem("ETH", address(end));
assertEq(skimmedEth, 300 * end.tag("ETH") / 10 ** 9);
uint256 remainInkVal = 2 ether - skimmedEth; // 2 ETH (deposited) - 300 DAI debt * ETH cage price
assertEq(address(this).balance, prevBalanceETH + remainInkVal);
assertProxyRewarded(address(ethManagedJoin), 100 * 10 ** 12);

Expand All @@ -678,7 +708,9 @@ contract DssProxyActionsTest is DssDeployTestBase, ProxyCalls {
(inkV, artV) = vat.urns("WBTC", charterProxy);
assertEq(inkV, 0);
assertEq(artV, 0);
remainInkVal = (1 ether - 5 * end.tag("WBTC") / 10 ** 9) / 10 ** 10; // 1 WBTC (deposited) - 5 DAI debt * WBTC cage price
uint256 skimmedWbtc = vat.gem("WBTC", address(end));
assertEq(skimmedWbtc, 5 * end.tag("WBTC") / 10 ** 9);
remainInkVal = (1 ether - skimmedWbtc) / 10 ** 10; // 1 WBTC (deposited) - 5 DAI debt * WBTC cage price
assertEq(wbtc.balanceOf(address(this)), prevBalanceWBTC + remainInkVal);
assertProxyRewarded(address(ethManagedJoin), 200 * 10 ** 12);

Expand All @@ -687,14 +719,23 @@ contract DssProxyActionsTest is DssDeployTestBase, ProxyCalls {
end.flow("ETH");
end.flow("WBTC");

dai.approve(address(proxy), 305 ether);
this.end_pack(address(daiJoin), address(end), 305 ether);
User user = new User(registry, dssProxyActionsEnd);

// Move dai to user so he can redeem it for collateral
dai.transfer(address(user), 305 ether);

user.approve(address(dai), address(user.proxy()), 305 ether);
user.end_pack(address(daiJoin), address(end), 305 ether);

// Tack from the skimmed vaults to End, required for cashing
ethManagedJoin.tack(charterProxy, address(end), ethManagedJoin.stake(charterProxy));
wbtcJoin.tack(charterProxy, address(end), wbtcJoin.stake(charterProxy));

this.end_cashETH(address(ethManagedJoin), address(end), 305 ether);
this.end_cashGem(address(wbtcJoin), address(end), 305 ether);
user.end_cashETH(address(ethManagedJoin), address(end), 305 ether);
user.end_cashGem(address(wbtcJoin), address(end), 305 ether);

assertEq(address(this).balance, prevBalanceETH + 2 ether - 1); // (-1 rounding)
assertEq(wbtc.balanceOf(address(this)), prevBalanceWBTC + 1 * 10 ** 8 - 1); // (-1 rounding)
assertEq(address(user).balance, skimmedEth - 1); // (-1 rounding)
assertEq(wbtc.balanceOf(address(user)), skimmedWbtc / 10 ** 10 - 1); // (-1 rounding)
}

receive() external payable {}
Expand Down