-
Notifications
You must be signed in to change notification settings - Fork 80
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
Flipper for zero slippage trading of OUSD for stablecoins #558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun! Took a first pass at reviewing. Looks very good. Just a few minor comments inline.
contracts/deploy/001_core.js
Outdated
@@ -484,6 +484,24 @@ const deployCore = async () => { | |||
log("Initialized OUSD"); | |||
}; | |||
|
|||
// Deploy the Flipper tradeing contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally fine to add to this migration file for dev. But for mainnet we'll need a separate migration file depoy_015.js. I can help you with that if you need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love if you would set up the deploy file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
onlyGovernor | ||
nonReentrant | ||
{ | ||
IERC20(token).safeTransfer(_governor(), amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if as opposed to transfering to the governor we should pass in a destination address for more flexibility. So that for example we can if needed easily move the funds back to a hot wallet of our choice. Same comment for withdrawAll below.
Also note the failing slither test:
|
Set slither hide the errors about Tether not being ERC20 compliant. |
This'll get arbed if a stablecoin goes off peg by a bit by chaining calls i.e. buy for usdt, sell for dai. Probably not an issue if we don't keep too much in it. |
Yeah. It's practically guaranteed to have the less valuable of the set of stablecoins. I figure we launch it with a small amount of capital, and see what happens and how useful it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Deployed to Mainnet. @DanielVF I let you merge this PR in master after checking that @tomlinton does not have any objection (there is still one of his comment that is unaddressed). |
@tomlinton Replying to both of your comments here, since they are related.
Yeah, doing multiple coins ensures that this will tend towards having the lowest valued coins in pile. Time will tell how it works out. A limit does two things:
All that said, we may later want to raise the limit. |
Merged to master and created tag deploy-015 |
Concept
The flipper contract allows 1-1 trading between stablecoins and OUSD. This allows easy in and and out during normal as users "flip" the available capital back and forth between OUSD and stablecoins.
Ownership
Funds in the flipper are company funds and not user funds. The owner should not be set to the usual timelock governance nor to the strategist role. Assuming OUSD growth, the owner will periodically need to withdraw stablecoins and mint more OUSD.
Low Gas Trading
The six trading functions optimize for low gas usage. Contributing to this, they make no internal calls, nor do they read from internal storage or write to internal storage. They also have one function per trading vector, which moves complexity and gas cost out of the contract.
This low gas focus means that the trading functions are not written in our usual style and do not use SafeERC20. We'll need to test them on fork, and look carefully at each.
When we deploy the contract for real, we should change to code to having the contract addresses as constants, rather than set by the constructor.
Availability
Unlike uniswap, there is very much not guarantee that the tokens you want to trade for are available. This is an opportunistic money saving method that may be there, or may not. UI's and user experience need to communicate this.
Rounding
I went for pragmatic simplicity here. It is possible for request more OUSD and pay less USDC, USDT than owed. By extension is also possible to pay no USDT and get a sliver of OUSD. If ETH were $1, and gas 1 gwie, then someone would spend 8000% in gas over the OUSD received.
Similarly, if we did every transaction that has ever gone through the Uniswap 2 router (24 million), and each one went for maximum rounding error, then our rounding losses would be $24.
Still makes me make faces. I welcome second opinions.
Contract change checklist: