-
Notifications
You must be signed in to change notification settings - Fork 91
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
Feature/pool v2 #832
Feature/pool v2 #832
Conversation
Hi @rndquu I see still tests failing in the set up stage, please ping once the pull request will be ready for review. |
The PR is draft so it is not ready yet |
@gitcoindev I can't request a review from you because you're not a member of the ubiquity organization. Anyway pls take a look at the current PR. @pavlovcik Should we add @gitcoindev to the Software Development team? |
Yes |
@rndquu can we have some testing to this, or is it planned on a future PR? |
packages/contracts/scripts/deploy/dollar/solidityScripting/01_Diamond.s.sol
Outdated
Show resolved
Hide resolved
Tests are implemented: https://github.com/ubiquity/ubiquity-dollar/blob/286c0cdf0841655e46b91b55d9ee56899a281508/packages/contracts/test/diamond/facets/UbiquityPoolFacet.t.sol |
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.
The "check storage layout" is safe since this is a new storage induction, looks good to me, we need testing on the subsequently issue about the either the dollar peg or new oracle manipulation, probably the decision was internal business, we could or we could not the same according to our purpose
@gitcoindev let us hear your thoughts on this one |
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 tried to carefully go through the changes as it is not a small pr. I have a few questions/remarks, but overall good job! I also verified the LUSD token address is correct. I went through https://docs.frax.finance/ to check internals. With the tests passing and "check storage layout" workflow failing I would approve since the Diamond deployment was not done yet.
packages/contracts/scripts/deploy/dollar/solidityScripting/01_Diamond.s.sol
Show resolved
Hide resolved
I really think it's prudent to use an oracle instead of assuming $1.00. LUSD historically has been between ~0.99-1.05. ~6% is a pretty significant potential deficit. It comes down to how long that would take to implement (I would imagine not longer than a couple weeks) |
Ok, #830 (where collateral oracle should be implemented) will be part of the production deploy |
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.
Thank you for addressing the review comments. I did another review round and approving from my side.
@rndquu we are having styling issues, we should decide whether to allow function setRndnuu() private {
// code
} or function setRndnuu() private
{
} this can be seen here as well as many other parts of the code |
I looked into this and we would have to make a custom tool to enforce this with lint-staged I believe. |
We have the old UbiquityPool contract which is a stripped down version of FraxPool (v1).
The old UbiquityPool has some drawbacks:
This PR introduces a new UbiquityPool contract which is an (almost) exact copy of FraxPoolV3 where most of the above issues are solved.
With the new UbiquityPool contract users are able to mint/redeem
Dollar
tokens for collateral tokens (we start with theLUSD
token).The only catch is the collateral price in USD. The original FraxPoolV3 by default uses only USD-pegged stablecoins as collateral. Hence the collateral price is set to
$1.00
on adding a new collateral token. While the FraxPoolV3 contract has the method for setting the collateral price in USD the original contract has never called this method because underlying USD-collateral tokens have never depegged + Frax protocol simply paused the pool when they got enough of collateral liquidity.While it seems to be ok to use hardcoded
$1.00
price for collateral tokens it is better to rely on oracles (can't say why Frax didn't implement them for getting collateral token prices) hence this issue was created. I think we can deploy with hardcoded$1.00
collateral prices (unless somebody has reasons of why it might be bad) and when #830 is ready update the UbiquityPool to use oracles for collateral prices.P.S. The "check storage layout" workflow fails because the new pool modifies the order of state variables (compared to the old pool) but since the Diamond has never been deployed we can neglect this failing CI run.