Feat/g dollar of latest develop #522
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Forked OZ's 4.8.3 version and removed unused functions: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol
for better differentiation against the OZ version
### Description Fixes mento-protocol/mento-general#562 Fixes mento-protocol/mento-general#560 - [x] Changed access controls for GoodDollarExchangeProvider according to mento-protocol/mento-general#562 - [x] Changed access controls for GoodDollarExpansionController according to mento-protocol/mento-general#560 - [x] Updated and expanded unit tests - [x] Fixed prettier config warnings - [x] Fixed all linter warnings - [x] Fixed `Lint & Test` CI job (was missing the rpc URL env var)
### Description Fixes #525 Mostly false positives where I added comments. One change I did make was adding reentrancy guards to the `mint` functions in the `GoodDollarExpansionController`. Even though the slither warnings were only a "medium" around possibly manipulating event data via re-entrancy, I thought it'd still be a good idea to disallow reentrancy for all mint functions.
### Description Fixes mento-protocol/mento-general#552 - [x] Made error messages across GoodDollar contracts more consistent - [x] Made comments across GoodDollar comments more consistent - [x] Introduced @inheritdoc to reduce comment duplication between interfaces and implementations in 0.8 contracts - [x] Changed `Broker.configureTradingLimit()` from ~~public~~ to **external**
### Description This Pr adds tests for the pricing logic of the bancor exchange provider. During the testing, we realized that the BancorFormula.sol contract is missing one function we need. This function is getAmountIn for the tokenIn being the BancorToken. The new function is called `saleCost()` The math formula for this function is derived from the `saleTargetAmount()` function already implemented. It's important that the base of power operation is larger one otherwise the power function in that contract reverts. These are the current formulas used for the different pricing functions:  This is how we derived the formula for `saleCost()`  ### Related issues - Fixes mento-protocol/mento-general#453 ### How to review - verify formula is derived correctly - exit contribution is applied correctly - tests cover everything --------- Co-authored-by: Ryan Noble <[email protected]> Co-authored-by: baroooo <[email protected]>
### Description This PR adds additional tests to to the exchange provider and includes fuzz testing ### Related issues - Fixes mento-protocol/mento-general#454 - --------- Co-authored-by: bowd <[email protected]> Co-authored-by: philbow61 <[email protected]> Co-authored-by: baroooo <[email protected]> Co-authored-by: philbow61 <[email protected]>
### Description - [x] Wrote fork tests for Good Dollar contracts - [x] Wrote fork tests for BancorExchangeProvider (this one's debatable; it's largely the same logic, but I wasn't sure if we might use this contract in isolation in the future) - [x] Deleted Good Dollar Integration test (this was already a fork test in disguise and all of its logic has been merged into the new good dollar fork tests) - [x] Added two additional `require`s to `BancorExchangeProvider.executeSwap()` to guard against potential underflows - [x] Moved `.solhint.test.json` into `./test/.solhint.json` so the VS Code solidity extension can pick it up (rn it's using the main config also for tests which results in lots of annoying red squiggles that are non-issues) - [x] Removed some Baklava remnants ### How to review - [ ] Check out this branch: `git checkout chore/gooddollar-fork-tests` - [ ] Run `yarn fork-test:celo-mainnet`, they should be green (alfajores is broken due to L2 changes unrelated to this PR) - [ ] Review the code and see if there's anything missing that you think should be covered by the fork tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A few sentences describing the overall effects and goals of the pull request's commits.
What is the current behavior, and what is the updated/expected behavior with this PR?
Other changes
Describe any minor or "drive-by" changes here.
Tested
An explanation of how the changes were tested or an explanation as to why they don't need to be.
Related issues
Backwards compatibility
Brief explanation of why these changes are/are not backwards compatible.
Documentation
The set of community facing docs that have been added/modified because of this change