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

chore: BalCoW v1.3.0 release #135

Merged
merged 49 commits into from
Jul 26, 2024
Merged

chore: BalCoW v1.3.0 release #135

merged 49 commits into from
Jul 26, 2024

Conversation

wei3erHase
Copy link
Member

@wei3erHase wei3erHase commented Jul 4, 2024

PR addresses changes proposed by Wonderland's Internal Review [W] and Certora's Security Assessment [C].

extras

0xteddybear and others added 12 commits June 26, 2024 11:05
* chore: smallest of fixes

* feat: dont publicly expose the current commitment

* feat: use _viewlock_ check for commitment

* docs: improve readme

* feat: use SafeERC20 to support tokens without transfer return value

* test: assert the expected pool bytecode is deployed

* feat: use SafeERC20 to support underlying tokens without transfer return value
* fix: renaming TokenAmountInAboveMaxIn for AboveMaxRatio

* fix: swapExactAmountOut natspec
* ci: ensure gas snapshots are updated

* chore: break snapshots to see if it is caught

* fix: fetch some git history so diff has something to compare against

* fix: run diff after running tests

* fix: rename gas snapshot test so its run in CI

* fix: run diff after running _integration_ tests

* chore: update forge snapshots to their current values
* chore: checkout btt bfactory tests from feat/new-unit-tests

* chore: update BFactory tests for custom errors

* chore: get BCoWFactory tests to run for the time being

* test: mock _newBPool and test it separately

* test: unit test methods defined in BCoWFactory

* fix: feedback from review

* chore: setBLabs -> BLabs

* fix: feedback from review
* chore: adding BNum test files from #122

* fix: inheriting BConst

* fix: reusing constants

* feat: improving btt test with fuzzing

* chore: cleanup tree and tests

* feat: minor improvements to tree + fuzz

* fix: minor tweaks to tree and comments

* fix: clean up bound

* fix: test:scaffold script

* fix: applying tree updated names

* fix: bdiv tests

* fix: improving tree branching and expectations

* fix: rm unnecessary conditional

* fix: revert dumb bun break

* fix: example parenthesis
* test: adding BTT tests for BMath (#123)

* fix: improving tree and filling empty blocks

* fix: solhint json

* fix: branch phrasing

* fix: parenthesis

* fix: rm revert reason branches

* fix: improving calc spot price branches

* fix: improving calcOutGivenIn branches and tests

* feat: improving calc In and Out branches and tests

* feat: improving calcPoolOut tree and tests

* feat: adding calcSingleIn calcs

* feat: adding calcSingleOut calcs

* feat: adding caclPoolIn calcs expectations

* feat: improving calcPoolIn tests

* feat: populating missing tests

* refactor: mv setup to file start

* feat: cleanup internal branch assumptions

* fix: rm fn mutability warnings

* feat: adding some missing branches

* fix: revert expectation comment

---------

Co-authored-by: teddy <[email protected]>
* feat: adding address zero checks in setters

* fix: updating gas snapshots

* fix: comment in test
* ci: setup bulloak in CI

* fix: use explicit search instead of depending on bulloaks globbing
* feat: adding join and exit snaps

* feat: caching array.length into memory

* feat: changing /2 for >>1 when possible

* feat: making join/exit integration test 3-token

* feat: making factory immutable in BPool

* fix: comments

* fix: failing bytecode match tests

* fix: optimizing _afterFinalize loop

* fix: wbtc units in integration test
* fix: failing tests on setController (address zero)

* fix: update gas snapshot
wei3erHase and others added 13 commits July 4, 2024 21:20
* fix: typos in IBCoWPool

* fix: replace erc20 for token

* fix: increase/decrease approval arg names

* chore: update gas snapshots

* fix: consistent argument naming

* fix: rm global ffi and fix yarn:test

* fix: consistent _newBPool return

* fix: rm obvious return arg names

* fix: consistent returns in BPool

* fix: using trailing underscore for orderHash

* fix: adding strict linter rule and fixing issues

* feat: adding deny warnings to foundry toml

* fix: update gas snapshot

* fix: failing tests

* fix: rm deny warnings

* feat: raising optimizer to 500 runs

* fix: failing test
* test: migrate some easy methods to BTT

* test: btt tests for bind

* test: btt tests for unbind

* refactor: move BPoolBase onto its own file to please bulloak

* fix: fix bind/unbind tree

* refactor: manually mock every bind side effect for unbind

* chore: renames in bpool tree

* fix: various renames in .tree and .t.sol from code review

* refactor: every non-trivial method has its own tree now

* refactor: better mocking

* refactor: flatten trees

* fix: small fixes from code review

* test: fuzz start total weight and existing tokens

* test: test the entire behaviour of reentrancy locks

* fix: consistent bind and unbind trees (#140)

* fix: consistent bind and unbind trees

* fix: reentrancy lock expectations

* fix: unfuzzing initial weight in bind

---------

Co-authored-by: Weißer Hase <[email protected]>
* feat: adding finalized and notFinalized modifiers to BPool

* fix: rm global ffi and fix yarn:test

* fix: setSwapFee is notFinalized

* fix: rm remanent changes
* ci: ensure smock files are up to date

* chore: update existing smocked files
* feat: make a pools factory public

* chore: use autogenerated mockbpool

* fix: nits

* fix: gas snapshot

---------

Co-authored-by: Weißer Hase <[email protected]>
Co-authored-by: Weißer Hase <[email protected]>
* fix: max order duration check

* fix: rm SpotPriceAfterBelowMaxPrice error for SpotPriceAboveMaxPrice

* fix: test names

* fix: gas snapshot
* fix: isBPool argument name

* fix: updating smock contracts to latest changes

* fix: gas snapshot

* fix: fmt
* chore: mock internal bpool methods

* chore: move common secondToken up to base contract

* test: btt joinPool tests

* refactor: move bound token balance out of the base contract

* fix: feedback from review

* fix: update tree

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: consistent tree indent

* fix: join btt test file (#145)

* fix: improving join BTT test file

* fix: minor final touches

* fix: using _mintPoolShare instead of deal

---------

Co-authored-by: Weißer Hase <[email protected]>
* test: btt tests for exitPool

* chore: delete preexisting tests

* refactor: consistently use tokens array for tree tests

* test: assume behaviour of _pullPoolShare correct

* test: exitFee behaviour

* test: small fixes from feedback

* fix: only mock transfers where required
src/contracts/BPool.sol Outdated Show resolved Hide resolved
src/contracts/BPool.sol Outdated Show resolved Hide resolved
turtlemoji
turtlemoji previously approved these changes Jul 12, 2024
Copy link

@turtlemoji turtlemoji left a comment

Choose a reason for hiding this comment

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

approved!

0xteddybear and others added 15 commits July 22, 2024 11:35
* refactor: explicitly set weights in every scenario, remove it from base contract

* test: btt tests for joinswapExternAmountIn

* chore: remove preexisting unit tests replaced by ones in this pr

* test: query tokenIn balance
* refactor: explicitly set weights in every scenario, remove it from base contract

* test: btt tests for joinswapExternAmountIn

* chore: remove preexisting unit tests replaced by ones in this pr

* test: query tokenIn balance

* test: btt tests for exitswapPoolAmountIn

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: feedback from review
* test: btt tests for bpool.swapExactAmountIn

* chore: delete preexisting unit tests

* test: small renames from feedback

* test: be explicit about untestable code

* test: adding skipped test for unreachable condition

* test: code wasnt so unreachable after all

* refactor: get rid of _setRecord

* test: btt tests for bcowpool.verify

* chore: delete preexisting unit tests

* chore: testcase renaming from review

* chore: get rid of _setTokens altogether

* test: fuzz all possible valid order.sellAmount values

* chore: rename correctOrder -> validOrder

* fix: rename base file so it is skipped by coverage

* test: ensure verify asks for ERC20 balances

* chore: make bun happy
* test: btt tests for bpool.swapExactAmountIn

* chore: delete preexisting unit tests

* test: small renames from feedback

* test: be explicit about untestable code

* test: adding skipped test for unreachable condition

* test: code wasnt so unreachable after all

* refactor: get rid of _setRecord

* test: btt tests for bcowpool.verify

* chore: delete preexisting unit tests

* chore: testcase renaming from review

* chore: get rid of _setTokens altogether

* test: fuzz all possible valid order.sellAmount values

* chore: rename correctOrder -> validOrder

* test: btt tests for bpool.finalize

* test: btt tests for bcowpool.finalize

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: feedback from review

* refactor: make caller==controller default scenario

* fix: reorganize .tree

* fix: feedback from review

* fix: feedback from review, calling internal method directly
* test: btt tests for joinswapPoolAmountOut

* fix: typos in tree

* fix: missing stuff from dev mergeback
* test: btt tests for bcowpool.commit

* chore: remove preexisting unit tests replaced by ones in this pr

* test: reorganize commit tree

* fix: make bulloak happy
* feat: adding BTT tests for BPool getters

* feat: adding tests for spotPrice getters

* feat: adding btt tests for bPool setters (#166)

* fix: rm legacy bPool tests

* fix: manually calculating spot price

* fix: test issues on merge conflicts

* fix: using funny spot prices

---------

Co-authored-by: teddy <[email protected]>
* feat: non-weighted tradeable order test

* fix: adding GetTradeableOrder library

* feat: adding weights to the math

* feat: variable weights

* feat: adding test for helper

* feat: adding support for weights

* fix: fmt

* fix: sending correct price vector

* fix: making helper return valid signature

* feat: deprecated weights in helper

* feat: updated buyAmount as per aaf44a3

* refactor: mv GetTradeableOrder to libraries dir

* fix: update comment on BCoWHelper

* fix: rm unused variable warning

* feat: adding BTT test for BCoWHelper

* fix: linter warnings

* fix: rm remanent line change

* fix: rm unused lines

* fix: missing natspec

* fix: uncommenting assertion in test

* fix: missing natspec

* fix: safety checks comments

* feat: vendoring interface from cow-amm

* fix: lint run

* chore: merge dev

* fix: gas snapshot

* refactor: reordered helper flow and cleanup

* feat: vendoring GetTradeableOrder from cow-amm

* feat: improving commitment expectation

* fix: typos in comments

* fix: overwriting sellAmount in order to avoid rounding issues (#154)

* fix: typos in comments

* fix: overwriting sellAmount in order to avoid rounding issues

* feat: improving the test case to cover both cases

* chore: addressing contract changes from PR comments

* chore: addressing comments in tests

* fix: adding helper mock

* fix: addressing comments from PR

* refactor: deprecate fuzzed integration valid order test in favour of unit test

* feat: simplifying helper integration test

* feat: improving unit test for valid order

* feat: adding call tokens expectation

* fix: branching different behaviours given skewness

* fix: adding comments on the skewness sign
* feat: adding btt tests for bToken

* feat: testing address zero checks on increase/decrease approvals

* feat: adding more checks to bToken tests
* test: btt tests for bpool.swapExactAmountIn

* chore: delete preexisting unit tests

* test: small renames from feedback

* test: be explicit about untestable code

* test: adding skipped test for unreachable condition

* test: code wasnt so unreachable after all

* refactor: get rid of _setRecord

* test: btt tests for bcowpool.verify

* chore: delete preexisting unit tests

* chore: testcase renaming from review

* chore: get rid of _setTokens altogether

* test: fuzz all possible valid order.sellAmount values

* chore: rename correctOrder -> validOrder

* test: btt tests for bcowpool.isValidSignature

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: more descriptive tree
* test: btt tests for bpool.swapExactAmountIn

* chore: delete preexisting unit tests

* test: small renames from feedback

* test: be explicit about untestable code

* test: adding skipped test for unreachable condition

* test: code wasnt so unreachable after all

* refactor: get rid of _setRecord

* test: btt tests for bcowpool.verify

* chore: delete preexisting unit tests

* chore: testcase renaming from review

* chore: get rid of _setTokens altogether

* test: fuzz all possible valid order.sellAmount values

* chore: rename correctOrder -> validOrder

* test: btt tests for bpool.finalize

* test: btt tests for bcowpool.finalize

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: feedback from review

* refactor: make caller==controller default scenario

* test: btt tests for bcowpool constructor

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: feedback from review

* fix: make bulloak happy

* fix: mergeback mistake

---------

Co-authored-by: Weißer Hase <[email protected]>
* test: btt tests for push/pull underlying

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: safeERC20 returns different errors with vs without return data

* fix: finalize test that was broken on main

* fix: feedback from review
* test: btt tests for exitswapExternAmountOut

* chore: remove preexisting unit tests replaced by ones in this pr

* fix: dont call setup twice

* chore: finally remove bpool.t.sol
* chore: preparing package for release

* fix: broken links
@wei3erHase wei3erHase marked this pull request as ready for review July 24, 2024 18:23
* feat: adding deployment script with faucet erc20s

* fix: missing files

* feat: update appData to a valid one

* fix: natspec in IFaucet

* fix: final tweaks

* refactor: deployment script

* chore: testnet deployment (updated registry)

* fix: rm solhint 1-contract-per-file rule in script dir

* fix: update bDao address

* fix: gas snapshots

* fix: refresh snapshots

* fix: extra line

* chore: testnet redeployment

* chore: add grafiti to factory

* chore: deployment addresses

* chore: rm grafiti from factory

* refactor: reorganize deployment and scripting architecture

* fix: linter warnings

* fix: deprecate mainnet and testnet id constants

* fix: contract comments

* feat: adding explicit revert on unknown chains

* feat: adding BCoWHelper to deployment script and registry
@wei3erHase wei3erHase changed the title chore: BalCoW beta release chore: BalCoW v1.0.0 release Jul 24, 2024
Copy link

@0xteddybear 0xteddybear left a comment

Choose a reason for hiding this comment

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

most comments are questions, a few can be easily fixed, can take care of those tomorrow

pool.bind(address(dai), DAI_LP_AMOUNT, 8e18); // 80% weight
pool.bind(address(weth), WETH_LP_AMOUNT, 2e18); // 20% weight
pool.bind(address(wbtc), WBTC_LP_AMOUNT, 10e18); // +100% weight (unused in swaps)

Choose a reason for hiding this comment

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

now it's 40% 10% 10%

Copy link
Member Author

Choose a reason for hiding this comment

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

nee, is indistinct, because we're never swapping WBTC, from DAI-WETH perspective pool is 80-20

Choose a reason for hiding this comment

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

we didn't remove this 😭

address public cowSolutionSettler = makeAddr('cowSolutionSettler');
bytes32 public domainSeparator = bytes32(bytes2(0xf00b));
address public vaultRelayer = makeAddr('vaultRelayer');
address public tokenIn;

Choose a reason for hiding this comment

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

we could get rid of these to be consistent with BPoolBase

function setUp() public virtual override {
super.setUp();

vm.mockCall(tokens[0], abi.encodePacked(IERC20.transferFrom.selector), abi.encode());

Choose a reason for hiding this comment

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

opinion on giving tokens[0] its own variable?

│ └── it should revert // division by zero
├── when token balance out is zero
│ └── it should return zero
├── when swap fee and exit fee are zero

Choose a reason for hiding this comment

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

in retrospect, do you consider all of these testcases would have been easier/cleaner with an array of valid scenarios, like I just did for bPow?

Copy link
Member Author

Choose a reason for hiding this comment

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

wdym? we require both to be zero to revert 🤔

Choose a reason for hiding this comment

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

I commented on the wrong line, sorry. I meant all of the non-reverting cases that should return correct value

@wei3erHase wei3erHase requested a review from 0xteddybear July 25, 2024 13:03
0xteddybear and others added 3 commits July 25, 2024 11:32
* chore: remove unused utils directory

* test: missing case for joinswapPoolAmountOut

* test: cover different bmul branches explicitly

* test: cover {push,pull}PoolShares

* test: token ratio > spot price before on swapExactAmountOut

* test: cover bpowApprox with a few valid scenarios
* chore: rename BPoolBase.sol BPoolBase.t.sol

* chore: remove BCoWPoolBase.sol

* chore: remove token{In,Out} from base setup

* chore: give token to bind its own variable
* chore: adding deployment addresses

* fix: addressing comments in PR

* fix: ouch
@wei3erHase wei3erHase merged commit 4289bb8 into main Jul 26, 2024
4 checks passed
@defi-wonderland defi-wonderland deleted a comment from wei3erHase Jul 26, 2024
@wei3erHase wei3erHase changed the title chore: BalCoW v1.0.0 release chore: BalCoW v1.3.0 release Jul 31, 2024
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