Cropper tack shutdown

[talbaneth]

No description provided.

[julienmartinlevrai]

LGTM

[kmbarry1]

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

[kmbarry1]

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.

[julienmartinlevrai]

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

[kmbarry1]

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.

[talbaneth]

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).

2. 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.

[julienmartinlevrai]

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.

[gbalabasquer]

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.

[julienmartinlevrai]

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.

[gbalabasquer]

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

[kmbarry1]

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.

[gbalabasquer]

Left a comment in the discussion. IMO is fine as it is but will re-review if further changes are added.

[julienmartinlevrai]

I still think tacking should be added when the vault holder frees their vault, but I’m not vetoing on that.

[kmbarry1]

LGTM

I don't view the ongoing discussion re: tacking as a blocker.

[julienmartinlevrai]

The contract deployed at mainnet address 0x38f7C166B5B22906f04D8471E241151BA45d97Af is verified on Etherscan and its code matches the code from commit c755fd62f876f0b25a93028392dc850c0dc49f6e for the DssProxyActionsCropper.sol file on this branch. Its three constructor arguments match current contracts of the system and it has no wards or any other form of auth mechanism.

LGTM for inclusion of the above contract address in the mainnet chainlog.

[kmbarry1]

Confirmed the same as Julien--code matches the DssProxyActionsEndCropper contract from commit c755fd6 modulo minor interface name changes from flattening. Constructor args match appropriate contracts. LGTM for chainlog inclusion.