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.

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

Change vow to use a proxy #241

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

Change vow to use a proxy #241

wants to merge 2 commits into from

Conversation

hexonaut
Copy link
Contributor

@hexonaut hexonaut commented Dec 10, 2021

Vow will need to be replaced when we update the flap/flop mechanism to dutch auctions. During that replacement I recommend we switch to using an upgradable proxy design so that vow address will remain constant moving forward. Upgradable contracts is in general an anti-pattern, but for this specific instance it makes a lot of sense.vow is one of the most commonly referenced contracts only behind vat, dai and daiJoin. In almost all instances of referencing vow the contract is either pushing revenue to the surplus buffer or pulling debt from it. Both of these operations require no knowledge of the vow interface. It is simply an address.

By making the vow address constant we can make the variable immutable in all referencing contracts which will not only save gas, it will also make dealing with vow upgrades no longer a concern. Current solutions involve either redeploying the contract (https://github.com/makerdao/dss-flash/blob/master/src/flash.sol#L59), filing a new vow (https://github.com/makerdao/dss-wormhole/blob/master/src/WormholeJoin.sol#L109) or doing a dynamic lookup from the chainlog (https://github.com/makerdao/dss-direct-deposit/blob/master/src/DssDirectDepositAaveDai.sol#L348). All of these solutions are sub-par imo. By using an upgradable proxy design we can change the vow without having to iterate over the many many locations it is referenced.

Furthermore with the file(...) solution it may be the case that eventually there isn't enough room in the block to synchronously update all the locations vow is referenced. I would like to get ahead of this problem before it bites us in the ass in a few years.

The proposed changes are designed to be minimal, so the first step is to just move over to the proxy design with no other changes. It probably makes sense to add events as well, so if people want to proceed I'll add those. Next step can be to upgrade the flop and flap auctions.

@transmissions11
Copy link

transmissions11 commented Dec 10, 2021

why not make a separate contract for storing the surplus dai and have the vow be a ward to it? that way people can rely on that address as a bucket to send funds to while the accounting logic can be swapped out freely without the need to involve messy proxies?

@hexonaut
Copy link
Contributor Author

Open to either way as long as we can get the vow accounting address to be constant. I'll see about mocking up the alternative.

@godsflaw
Copy link
Contributor

Yo, what super-human notification system are you running @transmissions11? Does it only trigger on controversial upgradability PRs?

if (PR == s/implementation/g) {
  // hard wake-up
  electrodes.fire();
}

@luke7211
Copy link

If proxy is necessary, shouldn't EIP-1967 Transparent Proxy pattern be used ?

@hexonaut
Copy link
Contributor Author

If proxy is necessary, shouldn't EIP-1967 Transparent Proxy pattern be used ?

I am following the long MakerDAO history of rolling everything ourselves.

Kidding aside, this is similar to the proxy design we are using here: https://github.com/makerdao/dss-charter/blob/master/src/CharterManager.sol#L62 (shield your eyes @transmissions11)

I'm not the biggest fan of fully transparent proxies personally, but if you have a good reason to use the exact transparent proxy I don't care either way that much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants