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

GoodDollar Fork Tests #530

Merged
merged 21 commits into from
Oct 23, 2024

Conversation

chapati23
Copy link
Contributor

@chapati23 chapati23 commented Oct 16, 2024

Description

  • Wrote fork tests for Good Dollar contracts
  • 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)
  • 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)
  • Added two additional requires to BancorExchangeProvider.executeSwap() to guard against potential underflows
  • 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)
  • 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

@chapati23 chapati23 changed the title Chore/gooddollar fork tests GoodDollar Fork Tests Oct 17, 2024
@chapati23 chapati23 changed the title GoodDollar Fork Tests Draft: GoodDollar Fork Tests Oct 17, 2024
@chapati23 chapati23 force-pushed the chore/gooddollar-fork-tests branch from 7931c72 to 04f0942 Compare October 18, 2024 13:23
@chapati23 chapati23 changed the title Draft: GoodDollar Fork Tests GoodDollar Fork Tests Oct 22, 2024
Copy link
Contributor

@baroooo baroooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contract changes look good, tests will be reviewed together with other changes in the final PR

Copy link
Contributor

@RyRy79261 RyRy79261 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I did have an intermittent test fail on the rateFeedDependencies_haltDependantTrading test however I'd assume that's related to state of the chain when forking.

@chapati23 To confirm if this potential erroring is an issue however I can't deliberately cause the error to happen so I would assume it's a natural behaviour of the on chain logic

@baroooo baroooo merged commit 040b7fe into feat/GDollar-of-latest-develop Oct 23, 2024
5 checks passed
@baroooo baroooo deleted the chore/gooddollar-fork-tests branch October 23, 2024 14:31
bowd pushed a commit that referenced this pull request Oct 24, 2024
7704a9b chore: reserve storage gap in new contracts
040b7fe GoodDollar Fork Tests (#530)
2ee14b4 Feat(454): Exchange provider tests  (#529)
578747c test: add BancorExchangeProvider pricing tests (#532)
fcd3d02 feat: improve tests around the expansion controller (#533)
d3a31fa Streamline comments and errors (#528)
ec64f0a fix: ☦ pray to the lords of echidna
13d3a98 fix: add back forge install to the CI step
c236009 fix: echidna tests
c50a198 feat: add Broker liquidity check (#523)
914ba7d Address Slither Issues in GoodDollar/Bancor Contracts (#526)
8bcf3fd Feat/change gdollar modifiers (#524)
379a511 feat: make validate() internal
bcbd9aa chore: added "yarn todo" task to log out open todos in the code
0631c60 refactor: renamed SafeERC20 to SafeERC20MintableBurnable
2800297 feat: simplified SafeERC20
e858391 feat: extend open zeppelin's IERC20 instead of duplicating it
196f6be chore: fixed linter and compiler warnings
096efe2 test: fix fork integration test
e5308d9 feat: add GoodDollar contracts + tests
3135626 feat: update Broker + TradingLimits to 0.8 and make G$ changes
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.

3 participants