search

makerdao / dss-exec-lib

DSS Executive Spellcrafting Library Contracts
GNU Affero General Public License v3.0
36 stars 21 forks source link

Option to adjust global DC with setIlkDebtCeiling #69

Open hexonaut opened 3 years ago

hexonaut commented 3 years ago

Sometimes you want to set a DC to a particular value, but also want to update the global Line as well (such as the case of setting a DC to 0). It would help to have a bool _global at the end of setIlkDebtCeiling to keep this in sync.

gbalabasquer commented 3 years ago

Will we use it often enough to deserve the function? In general we have more than 1 debt ceiling adjustment per spell, being this approach less efficient than adjusting the global debt ceiling at once. I agree it is less error prone but if we are gonna use it only in spells that adjust one debt ceiling, then I don't think it deserves the implementation.

hexonaut commented 3 years ago

I think you are misunderstanding the use case. Before the DC IAM we would be able to set the DC to 0 on any collateral by subtracting out the current line because it was constant. Now with the DC IAM we may want to set the DC to 0 on some of these, but we can no longer use the decreaseIlkDebtCeiling because the line is no longer deterministic. Furthermore it may even not be a full DAI unit. I suggest adding this flag to setIlkDebtCeiling for the probably common case where we want the DC to be at 0 and we don't care what it currently is (and we want the global DC to follow).

brianmcmichael commented 3 years ago

This would add a lot of overhead to the function to determine whether the new amount is higher or lower, calculate the difference, and then do the set, which is why we went with the simpler increase/decrease functions.

I can see what you're aiming for here, but I wonder if it would instead be better to add a unsetIlkLine(bytes32 ilk) function that just sets the ilk value to zero and adjusts the global value accordingly.

gbalabasquer commented 3 years ago

I think you are misunderstanding the use case. Before the DC IAM we would be able to set the DC to 0 on any collateral by subtracting out the current line because it was constant. Now with the DC IAM we may want to set the DC to 0 on some of these, but we can no longer use the decreaseIlkDebtCeiling because the line is no longer deterministic. Furthermore it may even not be a full DAI unit. I suggest adding this flag to setIlkDebtCeiling for the probably common case where we want the DC to be at 0 and we don't care what it currently is (and we want the global DC to follow).

Got it. However my premise is the same. How many times are we gonna use this local + global debt ceiling update together? I mean also for increaseIlkDebtCeiling, decreaseIlkDebtCeiling. Just wondering if we want to update multiple times the same storage in a spell just because is easier to reason about it. The answer might be yes, in that case, I think I agree to have a function for unsetting the value and also touching the global line. Otherwise I'd prefer to remove it everywhere and just adjust the global debt ceiling once in a different and unique call (more error prone for sure but more gas efficient as well).

hexonaut commented 3 years ago

TBH I prefer to have the gas inefficiency. I think it's worth the cognitive trade off of managing the global DC separately. Simplicity should outweigh gas in most of these decisions imo.

This would add a lot of overhead to the function to determine whether the new amount is higher or lower, calculate the difference, and then do the set, which is why we went with the simpler increase/decrease functions.

I can see what you're aiming for here, but I wonder if it would instead be better to add a unsetIlkLine(bytes32 ilk) function that just sets the ilk value to zero and adjusts the global value accordingly.

I'd be okay with a set DC to 0 unsetIlkLine as a compromise, but my general view is to provide simple high-level constructs to the spell crafter and hide the complexity in exec-lib. unsetIlkLine should cover most of the use cases, but we could also have a generalized solution by adding the setIlkDebtCeiling overhead. I could see the setIlkDebtCeiling being set to non-zero in the case where we want to switch from the autoline back to a fixed DC. There is currently no way to do that in the exec-lib. If you are worried about gas we can maintain backwards compat by just keeping the same logic with _global = false.

gbalabasquer commented 3 years ago

I can see the reasons to use it and to avoid using it. So it might be the best to have the option with the global value and be more flexible. Just as a note, we are assuming the Line has to be the sum of all the lines. That seems to be the case on what governance wants to follow, but that pattern might change in the future. It is not necessary true that the risk we want to take as a total has to be the sum of the max risk on each collateral.