From f1fe830c359a6f487d6675438a10a28fae14e6f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wei=C3=9Fer=20Hase?= Date: Wed, 31 Jul 2024 17:52:33 +0200 Subject: [PATCH 1/3] feat: adding erc20 name and symbol to constructor (#182) * feat: adding erc20 name and symbol to constructor * fix: added natspec params --- .forge-snapshots/newBCoWFactory.snap | 2 +- .forge-snapshots/newBCoWPool.snap | 2 +- .forge-snapshots/newBFactory.snap | 2 +- .forge-snapshots/newBPool.snap | 2 +- package.json | 2 +- script/Script.s.sol | 5 ++++- src/contracts/BCoWFactory.sol | 6 ++++-- src/contracts/BCoWPool.sol | 9 ++++++++- src/contracts/BFactory.sol | 10 ++++++---- src/contracts/BPool.sol | 3 ++- src/contracts/BToken.sol | 2 +- src/interfaces/IBFactory.sol | 4 +++- test/integration/BCoWHelper.t.sol | 7 +++++-- test/integration/BPool.t.sol | 5 ++++- test/integration/DeploymentGas.t.sol | 7 +++++-- test/manual-smock/MockBCoWFactory.sol | 23 ++++++++++++----------- test/manual-smock/MockBCoWPool.sol | 7 ++++++- test/smock/MockBFactory.sol | 23 ++++++++++++----------- test/smock/MockBPool.sol | 2 +- test/smock/MockBToken.sol | 2 +- test/unit/BCoWFactory.t.sol | 6 ++++-- test/unit/BCoWHelper.t.sol | 4 +++- test/unit/BCoWPool/BCoWPool.t.sol | 2 +- test/unit/BCoWPool/BCoWPool.tree | 4 ++-- test/unit/BCoWPool/BCoWPoolBase.t.sol | 2 +- test/unit/BFactory.t.sol | 13 ++++++++----- test/unit/BPool/BPool.t.sol | 8 ++++++-- test/unit/BPool/BPool.tree | 2 ++ test/unit/BPool/BPoolBase.t.sol | 4 +++- test/unit/BToken.t.sol | 13 ++++++++----- 30 files changed, 117 insertions(+), 66 deletions(-) diff --git a/.forge-snapshots/newBCoWFactory.snap b/.forge-snapshots/newBCoWFactory.snap index ae0b3b91..2aa3683c 100644 --- a/.forge-snapshots/newBCoWFactory.snap +++ b/.forge-snapshots/newBCoWFactory.snap @@ -1 +1 @@ -4200675 \ No newline at end of file +4314884 \ No newline at end of file diff --git a/.forge-snapshots/newBCoWPool.snap b/.forge-snapshots/newBCoWPool.snap index 319c0adc..538dee2c 100644 --- a/.forge-snapshots/newBCoWPool.snap +++ b/.forge-snapshots/newBCoWPool.snap @@ -1 +1 @@ -3397437 \ No newline at end of file +3401020 \ No newline at end of file diff --git a/.forge-snapshots/newBFactory.snap b/.forge-snapshots/newBFactory.snap index 56f18daf..6744e1e9 100644 --- a/.forge-snapshots/newBFactory.snap +++ b/.forge-snapshots/newBFactory.snap @@ -1 +1 @@ -3442127 \ No newline at end of file +3564561 \ No newline at end of file diff --git a/.forge-snapshots/newBPool.snap b/.forge-snapshots/newBPool.snap index 88d3594d..7ebbb489 100644 --- a/.forge-snapshots/newBPool.snap +++ b/.forge-snapshots/newBPool.snap @@ -1 +1 @@ -2841995 \ No newline at end of file +2845617 \ No newline at end of file diff --git a/package.json b/package.json index 51d48ad1..b72fc12a 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "test": "yarn test:integration && yarn test:unit", "test:integration": "forge test --ffi --match-path 'test/integration/**' -vvv --isolate", "test:local": "FOUNDRY_FUZZ_RUNS=100 forge test -vvv", - "test:scaffold": "bulloak check --fix test/unit/*.tree && forge fmt", + "test:scaffold": "bulloak check --fix test/unit/**/*.tree && forge fmt", "test:unit": "forge test --match-path 'test/unit/**' -vvv", "test:unit:deep": "FOUNDRY_FUZZ_RUNS=5000 yarn test:unit" }, diff --git a/script/Script.s.sol b/script/Script.s.sol index 6d86aa49..8ca344ee 100644 --- a/script/Script.s.sol +++ b/script/Script.s.sol @@ -43,7 +43,10 @@ contract TestnetScript is BaseScript { IFaucet(_SEPOLIA_FAUCET).drip(_SEPOLIA_DAI_TOKEN); IFaucet(_SEPOLIA_FAUCET).drip(_SEPOLIA_USDC_TOKEN); - IBPool bPool = bCoWFactory.newBPool(); + string memory name = 'Balancer CoWAMM Pool'; + string memory symbol = 'BPT'; + + IBPool bPool = bCoWFactory.newBPool(name, symbol); IERC20(_SEPOLIA_BAL_TOKEN).approve(address(bPool), type(uint256).max); IERC20(_SEPOLIA_DAI_TOKEN).approve(address(bPool), type(uint256).max); diff --git a/src/contracts/BCoWFactory.sol b/src/contracts/BCoWFactory.sol index 1eadeefe..81652692 100644 --- a/src/contracts/BCoWFactory.sol +++ b/src/contracts/BCoWFactory.sol @@ -31,9 +31,11 @@ contract BCoWFactory is BFactory, IBCoWFactory { /** * @dev Deploys a BCoWPool instead of a regular BPool. + * @param name The name of the Pool ERC20 token + * @param symbol The symbol of the Pool ERC20 token * @return bCoWPool The deployed BCoWPool */ - function _newBPool() internal virtual override returns (IBPool bCoWPool) { - bCoWPool = new BCoWPool(SOLUTION_SETTLER, APP_DATA); + function _newBPool(string memory name, string memory symbol) internal virtual override returns (IBPool bCoWPool) { + bCoWPool = new BCoWPool(SOLUTION_SETTLER, APP_DATA, name, symbol); } } diff --git a/src/contracts/BCoWPool.sol b/src/contracts/BCoWPool.sol index 6ac87bec..434929bc 100644 --- a/src/contracts/BCoWPool.sol +++ b/src/contracts/BCoWPool.sol @@ -46,7 +46,14 @@ contract BCoWPool is IERC1271, IBCoWPool, BPool, BCoWConst { /// @inheritdoc IBCoWPool bytes32 public immutable APP_DATA; - constructor(address cowSolutionSettler, bytes32 appData) BPool() { + constructor( + address cowSolutionSettler, + bytes32 appData, + // solhint-disable-next-line no-unused-vars + string memory name, + // solhint-disable-next-line no-unused-vars + string memory symbol + ) BPool(name, symbol) { SOLUTION_SETTLER = ISettlement(cowSolutionSettler); SOLUTION_SETTLER_DOMAIN_SEPARATOR = ISettlement(cowSolutionSettler).domainSeparator(); VAULT_RELAYER = ISettlement(cowSolutionSettler).vaultRelayer(); diff --git a/src/contracts/BFactory.sol b/src/contracts/BFactory.sol index 19450439..22a768db 100644 --- a/src/contracts/BFactory.sol +++ b/src/contracts/BFactory.sol @@ -21,8 +21,8 @@ contract BFactory is IBFactory { } /// @inheritdoc IBFactory - function newBPool() external returns (IBPool bPool) { - bPool = _newBPool(); + function newBPool(string memory name, string memory symbol) external returns (IBPool bPool) { + bPool = _newBPool(name, symbol); _isBPool[address(bPool)] = true; emit LOG_NEW_POOL(msg.sender, address(bPool)); bPool.setController(msg.sender); @@ -62,10 +62,12 @@ contract BFactory is IBFactory { /** * @notice Deploys a new BPool. + * @param name The name of the Pool ERC20 token + * @param symbol The symbol of the Pool ERC20 token * @dev Internal function to allow overriding in derived contracts. * @return bPool The deployed BPool */ - function _newBPool() internal virtual returns (IBPool bPool) { - bPool = new BPool(); + function _newBPool(string memory name, string memory symbol) internal virtual returns (IBPool bPool) { + bPool = new BPool(name, symbol); } } diff --git a/src/contracts/BPool.sol b/src/contracts/BPool.sol index 3c3b8bc4..d25f153c 100644 --- a/src/contracts/BPool.sol +++ b/src/contracts/BPool.sol @@ -80,7 +80,8 @@ contract BPool is BToken, BMath, IBPool { _; } - constructor() { + // solhint-disable-next-line no-unused-vars + constructor(string memory name, string memory symbol) BToken(name, symbol) { _controller = msg.sender; FACTORY = msg.sender; _swapFee = MIN_FEE; diff --git a/src/contracts/BToken.sol b/src/contracts/BToken.sol index 157a4440..4c277871 100644 --- a/src/contracts/BToken.sol +++ b/src/contracts/BToken.sol @@ -8,7 +8,7 @@ import {ERC20} from '@openzeppelin/contracts/token/ERC20/ERC20.sol'; * @notice Balancer Pool Token base contract, providing ERC20 functionality. */ contract BToken is ERC20 { - constructor() ERC20('Balancer Pool Token', 'BPT') {} + constructor(string memory name, string memory symbol) ERC20(name, symbol) {} /** * @notice Increase the allowance of the spender. diff --git a/src/interfaces/IBFactory.sol b/src/interfaces/IBFactory.sol index 672ba7ca..30638e05 100644 --- a/src/interfaces/IBFactory.sol +++ b/src/interfaces/IBFactory.sol @@ -30,9 +30,11 @@ interface IBFactory { /** * @notice Creates a new BPool, assigning the caller as the pool controller + * @param name The name of the Pool ERC20 token + * @param symbol The symbol of the Pool ERC20 token * @return bPool The new BPool */ - function newBPool() external returns (IBPool bPool); + function newBPool(string memory name, string memory symbol) external returns (IBPool bPool); /** * @notice Sets the BDao address in the factory diff --git a/test/integration/BCoWHelper.t.sol b/test/integration/BCoWHelper.t.sol index 2bd63ab3..f7aa0aeb 100644 --- a/test/integration/BCoWHelper.t.sol +++ b/test/integration/BCoWHelper.t.sol @@ -49,6 +49,9 @@ contract BCoWHelperIntegrationTest is Test { uint256 constant SKEWENESS_RATIO = 95; // -5% skewness uint256 constant EXPECTED_FINAL_SPOT_PRICE = INITIAL_SPOT_PRICE * 100 / SKEWENESS_RATIO; + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; + function setUp() public { vm.createSelectFork('mainnet', 20_012_063); @@ -61,8 +64,8 @@ contract BCoWHelperIntegrationTest is Test { deal(address(WETH), lp, type(uint256).max, false); vm.startPrank(lp); - basicPool = ammFactory.newBPool(); - weightedPool = ammFactory.newBPool(); + basicPool = ammFactory.newBPool(ERC20_NAME, ERC20_SYMBOL); + weightedPool = ammFactory.newBPool(ERC20_NAME, ERC20_SYMBOL); DAI.approve(address(basicPool), type(uint256).max); WETH.approve(address(basicPool), type(uint256).max); diff --git a/test/integration/BPool.t.sol b/test/integration/BPool.t.sol index 36deb047..e6819882 100644 --- a/test/integration/BPool.t.sol +++ b/test/integration/BPool.t.sol @@ -53,13 +53,16 @@ abstract contract BPoolIntegrationTest is Test, GasSnapshot { uint256 public constant WETH_OUT_AMOUNT = 94_049_266_814_811_022; // 0.094 ETH uint256 public constant DAI_OUT_AMOUNT_INVERSE = 94_183_552_501_642_552_000; // 94.1 DAI + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; + function setUp() public virtual { vm.createSelectFork('mainnet', 20_012_063); factory = _deployFactory(); vm.startPrank(lp); - pool = factory.newBPool(); + pool = factory.newBPool(ERC20_NAME, ERC20_SYMBOL); deal(address(dai), lp, DAI_LP_AMOUNT); deal(address(weth), lp, WETH_LP_AMOUNT); diff --git a/test/integration/DeploymentGas.t.sol b/test/integration/DeploymentGas.t.sol index d4604024..cca65c1f 100644 --- a/test/integration/DeploymentGas.t.sol +++ b/test/integration/DeploymentGas.t.sol @@ -18,6 +18,9 @@ contract DeploymentIntegrationGasTest is Test, GasSnapshot { address solutionSettler; address deployer = makeAddr('deployer'); + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; + function setUp() public { vm.startPrank(deployer); bFactory = new BFactory(); @@ -38,11 +41,11 @@ contract DeploymentIntegrationGasTest is Test, GasSnapshot { function testPoolDeployment() public { snapStart('newBPool'); - bFactory.newBPool(); + bFactory.newBPool(ERC20_NAME, ERC20_SYMBOL); snapEnd(); snapStart('newBCoWPool'); - bCowFactory.newBPool(); + bCowFactory.newBPool(ERC20_NAME, ERC20_SYMBOL); snapEnd(); } } diff --git a/test/manual-smock/MockBCoWFactory.sol b/test/manual-smock/MockBCoWFactory.sol index c4b2e427..2a74c9c8 100644 --- a/test/manual-smock/MockBCoWFactory.sol +++ b/test/manual-smock/MockBCoWFactory.sol @@ -21,23 +21,24 @@ contract MockBCoWFactory is BCoWFactory, Test { vm.mockCall(address(this), abi.encodeWithSignature('logBCoWPool()'), abi.encode()); } - function mock_call__newBPool(IBPool bCoWPool) public { - vm.mockCall(address(this), abi.encodeWithSignature('_newBPool()'), abi.encode(bCoWPool)); + function mock_call__newBPool(string memory name, string memory symbol, IBPool bCoWPool) public { + vm.mockCall(address(this), abi.encodeWithSignature('_newBPool(string,string)', name, symbol), abi.encode(bCoWPool)); } - function _newBPool() internal override returns (IBPool bCoWPool) { - (bool _success, bytes memory _data) = address(this).call(abi.encodeWithSignature('_newBPool()')); + function _newBPool(string memory name, string memory symbol) internal override returns (IBPool bCoWPool) { + (bool _success, bytes memory _data) = + address(this).call(abi.encodeWithSignature('_newBPool(string,string)', name, symbol)); if (_success) return abi.decode(_data, (IBPool)); - else return super._newBPool(); + else return super._newBPool(name, symbol); } - function call__newBPool() public returns (IBPool bCoWPool) { - return _newBPool(); + function call__newBPool(string memory name, string memory symbol) public returns (IBPool bCoWPool) { + return _newBPool(name, symbol); } - function expectCall__newBPool() public { - vm.expectCall(address(this), abi.encodeWithSignature('_newBPool()')); + function expectCall__newBPool(string memory name, string memory symbol) public { + vm.expectCall(address(this), abi.encodeWithSignature('_newBPool(string,string)', name, symbol)); } // MockBFactory methods @@ -57,8 +58,8 @@ contract MockBCoWFactory is BCoWFactory, Test { return _bDao; } - function mock_call_newBPool(IBPool bPool) public { - vm.mockCall(address(this), abi.encodeWithSignature('newBPool()'), abi.encode(bPool)); + function mock_call_newBPool(string memory name, string memory symbol, IBPool bPool) public { + vm.mockCall(address(this), abi.encodeWithSignature('newBPool(string,string)', name, symbol), abi.encode(bPool)); } function mock_call_setBDao(address bDao) public { diff --git a/test/manual-smock/MockBCoWPool.sol b/test/manual-smock/MockBCoWPool.sol index f070bb0c..72fb978d 100644 --- a/test/manual-smock/MockBCoWPool.sol +++ b/test/manual-smock/MockBCoWPool.sol @@ -33,7 +33,12 @@ contract MockBCoWPool is BCoWPool, Test { } /// MockBCoWPool mock methods - constructor(address cowSolutionSettler, bytes32 appData) BCoWPool(cowSolutionSettler, appData) {} + constructor( + address cowSolutionSettler, + bytes32 appData, + string memory name, + string memory symbol + ) BCoWPool(cowSolutionSettler, appData, name, symbol) {} function mock_call_commit(bytes32 orderHash) public { vm.mockCall(address(this), abi.encodeWithSignature('commit(bytes32)', orderHash), abi.encode()); diff --git a/test/smock/MockBFactory.sol b/test/smock/MockBFactory.sol index ab754fa5..4799b401 100644 --- a/test/smock/MockBFactory.sol +++ b/test/smock/MockBFactory.sol @@ -23,8 +23,8 @@ contract MockBFactory is BFactory, Test { constructor() BFactory() {} - function mock_call_newBPool(IBPool bPool) public { - vm.mockCall(address(this), abi.encodeWithSignature('newBPool()'), abi.encode(bPool)); + function mock_call_newBPool(string memory name, string memory symbol, IBPool bPool) public { + vm.mockCall(address(this), abi.encodeWithSignature('newBPool(string,string)', name, symbol), abi.encode(bPool)); } function mock_call_setBDao(address bDao) public { @@ -43,22 +43,23 @@ contract MockBFactory is BFactory, Test { vm.mockCall(address(this), abi.encodeWithSignature('getBDao()'), abi.encode(_returnParam0)); } - function mock_call__newBPool(IBPool bPool) public { - vm.mockCall(address(this), abi.encodeWithSignature('_newBPool()'), abi.encode(bPool)); + function mock_call__newBPool(string memory name, string memory symbol, IBPool bPool) public { + vm.mockCall(address(this), abi.encodeWithSignature('_newBPool(string,string)', name, symbol), abi.encode(bPool)); } - function _newBPool() internal override returns (IBPool bPool) { - (bool _success, bytes memory _data) = address(this).call(abi.encodeWithSignature('_newBPool()')); + function _newBPool(string memory name, string memory symbol) internal override returns (IBPool bPool) { + (bool _success, bytes memory _data) = + address(this).call(abi.encodeWithSignature('_newBPool(string,string)', name, symbol)); if (_success) return abi.decode(_data, (IBPool)); - else return super._newBPool(); + else return super._newBPool(name, symbol); } - function call__newBPool() public returns (IBPool bPool) { - return _newBPool(); + function call__newBPool(string memory name, string memory symbol) public returns (IBPool bPool) { + return _newBPool(name, symbol); } - function expectCall__newBPool() public { - vm.expectCall(address(this), abi.encodeWithSignature('_newBPool()')); + function expectCall__newBPool(string memory name, string memory symbol) public { + vm.expectCall(address(this), abi.encodeWithSignature('_newBPool(string,string)', name, symbol)); } } diff --git a/test/smock/MockBPool.sol b/test/smock/MockBPool.sol index bae353f3..31934af0 100644 --- a/test/smock/MockBPool.sol +++ b/test/smock/MockBPool.sol @@ -53,7 +53,7 @@ contract MockBPool is BPool, Test { return _totalWeight; } - constructor() BPool() {} + constructor(string memory name, string memory symbol) BPool(name, symbol) {} function mock_call_setSwapFee(uint256 swapFee) public { vm.mockCall(address(this), abi.encodeWithSignature('setSwapFee(uint256)', swapFee), abi.encode()); diff --git a/test/smock/MockBToken.sol b/test/smock/MockBToken.sol index d1a2b3a7..a6fae756 100644 --- a/test/smock/MockBToken.sol +++ b/test/smock/MockBToken.sol @@ -5,7 +5,7 @@ import {BToken, ERC20} from '../../src/contracts/BToken.sol'; import {Test} from 'forge-std/Test.sol'; contract MockBToken is BToken, Test { - constructor() BToken() {} + constructor(string memory name, string memory symbol) BToken(name, symbol) {} function mock_call_increaseApproval(address spender, uint256 amount, bool success) public { vm.mockCall( diff --git a/test/unit/BCoWFactory.t.sol b/test/unit/BCoWFactory.t.sol index 9c2bfca2..d1948bf5 100644 --- a/test/unit/BCoWFactory.t.sol +++ b/test/unit/BCoWFactory.t.sol @@ -12,6 +12,8 @@ contract BCoWFactoryTest is Test { address public factoryDeployer = makeAddr('factoryDeployer'); address public solutionSettler = makeAddr('solutionSettler'); bytes32 public appData = bytes32('appData'); + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; MockBCoWFactory factory; @@ -37,10 +39,10 @@ contract BCoWFactoryTest is Test { function test__newBPoolWhenCalled() external { vm.prank(address(factory)); - bytes memory _expectedCode = address(new BCoWPool(solutionSettler, appData)).code; // NOTE: uses nonce 1 + bytes memory _expectedCode = address(new BCoWPool(solutionSettler, appData, ERC20_NAME, ERC20_SYMBOL)).code; // NOTE: uses nonce 1 address _futurePool = vm.computeCreateAddress(address(factory), 2); - IBCoWPool _newPool = IBCoWPool(address(factory.call__newBPool())); + IBCoWPool _newPool = IBCoWPool(address(factory.call__newBPool(ERC20_NAME, ERC20_SYMBOL))); assertEq(address(_newPool), _futurePool); // it should set the new BCoWPool solution settler assertEq(address(_newPool.SOLUTION_SETTLER()), solutionSettler); diff --git a/test/unit/BCoWHelper.t.sol b/test/unit/BCoWHelper.t.sol index 23964575..a6c42748 100644 --- a/test/unit/BCoWHelper.t.sol +++ b/test/unit/BCoWHelper.t.sol @@ -28,6 +28,8 @@ contract BCoWHelperTest is Test { uint256 constant VALID_WEIGHT = 1e18; uint256 constant BASE = 1e18; + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; function setUp() external { factory = new MockBCoWFactory(address(0), bytes32(0)); @@ -39,7 +41,7 @@ contract BCoWHelperTest is Test { vm.mockCall( solutionSettler, abi.encodePacked(ISettlement.vaultRelayer.selector), abi.encode(makeAddr('vaultRelayer')) ); - pool = new MockBCoWPool(makeAddr('solutionSettler'), bytes32(0)); + pool = new MockBCoWPool(makeAddr('solutionSettler'), bytes32(0), ERC20_NAME, ERC20_SYMBOL); // creating a valid pool setup factory.mock_call_isBPool(address(pool), true); diff --git a/test/unit/BCoWPool/BCoWPool.t.sol b/test/unit/BCoWPool/BCoWPool.t.sol index 5c8c79f3..c89b3606 100644 --- a/test/unit/BCoWPool/BCoWPool.t.sol +++ b/test/unit/BCoWPool/BCoWPool.t.sol @@ -42,7 +42,7 @@ contract BCoWPool is BCoWPoolBase { vm.expectCall(_settler, abi.encodePacked(ISettlement.domainSeparator.selector)); // it should query the solution settler for the vault relayer vm.expectCall(_settler, abi.encodePacked(ISettlement.vaultRelayer.selector)); - MockBCoWPool pool = new MockBCoWPool(_settler, _appData); + MockBCoWPool pool = new MockBCoWPool(_settler, _appData, ERC20_NAME, ERC20_SYMBOL); // it should set the solution settler assertEq(address(pool.SOLUTION_SETTLER()), _settler); // it should set the domain separator diff --git a/test/unit/BCoWPool/BCoWPool.tree b/test/unit/BCoWPool/BCoWPool.tree index 9331eb1f..54d5f962 100644 --- a/test/unit/BCoWPool/BCoWPool.tree +++ b/test/unit/BCoWPool/BCoWPool.tree @@ -1,4 +1,4 @@ -BCoWPool::Constructor +BCoWPool::constructor └── when called ├── it should set the solution settler ├── it should query the solution settler for the domain separator @@ -16,7 +16,7 @@ BCoWPool::_afterFinalize └── when factorys logBCoWPool reverts └── it emits a COWAMMPoolCreated event -BCoWPool::Commit +BCoWPool::commit ├── when reentrancy lock is set │ └──it should revert ├── when sender is not solution settler diff --git a/test/unit/BCoWPool/BCoWPoolBase.t.sol b/test/unit/BCoWPool/BCoWPoolBase.t.sol index db328d1d..b7e564cf 100644 --- a/test/unit/BCoWPool/BCoWPoolBase.t.sol +++ b/test/unit/BCoWPool/BCoWPoolBase.t.sol @@ -19,6 +19,6 @@ contract BCoWPoolBase is BPoolBase, BCoWConst, BNum { super.setUp(); vm.mockCall(cowSolutionSettler, abi.encodePacked(ISettlement.domainSeparator.selector), abi.encode(domainSeparator)); vm.mockCall(cowSolutionSettler, abi.encodePacked(ISettlement.vaultRelayer.selector), abi.encode(vaultRelayer)); - bCoWPool = new MockBCoWPool(cowSolutionSettler, appData); + bCoWPool = new MockBCoWPool(cowSolutionSettler, appData, ERC20_NAME, ERC20_SYMBOL); } } diff --git a/test/unit/BFactory.t.sol b/test/unit/BFactory.t.sol index 698f8823..6fc7e74d 100644 --- a/test/unit/BFactory.t.sol +++ b/test/unit/BFactory.t.sol @@ -14,6 +14,9 @@ import {MockBFactory} from 'test/smock/MockBFactory.sol'; contract BFactoryTest is Test { address factoryDeployer = makeAddr('factoryDeployer'); + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; + MockBFactory factory; function setUp() external { @@ -31,9 +34,9 @@ contract BFactoryTest is Test { function test_NewBPoolWhenCalled(address _deployer, address _newBPool) external { assumeNotForgeAddress(_newBPool); vm.mockCall(_newBPool, abi.encodePacked(IBPool.setController.selector), abi.encode()); - factory.mock_call__newBPool(IBPool(_newBPool)); + factory.mock_call__newBPool(ERC20_NAME, ERC20_SYMBOL, IBPool(_newBPool)); // it should call _newBPool - factory.expectCall__newBPool(); + factory.expectCall__newBPool(ERC20_NAME, ERC20_SYMBOL); // it should set the controller of the newBPool to the caller vm.expectCall(_newBPool, abi.encodeCall(IBPool.setController, (_deployer))); // it should emit a PoolCreated event @@ -41,7 +44,7 @@ contract BFactoryTest is Test { emit IBFactory.LOG_NEW_POOL(_deployer, _newBPool); vm.prank(_deployer); - IBPool pool = factory.newBPool(); + IBPool pool = factory.newBPool(ERC20_NAME, ERC20_SYMBOL); // it should add the newBPool to the list of pools assertTrue(factory.isBPool(address(_newBPool))); @@ -51,10 +54,10 @@ contract BFactoryTest is Test { function test__newBPoolWhenCalled() external { vm.prank(address(factory)); - bytes memory _expectedCode = address(new BPool()).code; // NOTE: uses nonce 1 + bytes memory _expectedCode = address(new BPool(ERC20_NAME, ERC20_SYMBOL)).code; // NOTE: uses nonce 1 address _futurePool = vm.computeCreateAddress(address(factory), 2); - address _newBPool = address(factory.call__newBPool()); + address _newBPool = address(factory.call__newBPool(ERC20_NAME, ERC20_SYMBOL)); assertEq(_newBPool, _futurePool); // it should deploy a new BPool assertEq(_newBPool.code, _expectedCode); diff --git a/test/unit/BPool/BPool.t.sol b/test/unit/BPool/BPool.t.sol index a14f3d9f..29b28b6a 100644 --- a/test/unit/BPool/BPool.t.sol +++ b/test/unit/BPool/BPool.t.sol @@ -46,10 +46,14 @@ contract BPool is BPoolBase, BMath { bPool.set__controller(controller); } - function test_ConstructorWhenCalled(address _deployer) external { + function test_ConstructorWhenCalled(address _deployer, string memory _name, string memory _symbol) external { vm.startPrank(_deployer); - MockBPool _newBPool = new MockBPool(); + MockBPool _newBPool = new MockBPool(_name, _symbol); + // it should set the ERC20 name + assertEq(_newBPool.name(), _name); + // it should set the ERC20 symbol + assertEq(_newBPool.symbol(), _symbol); // it sets caller as controller assertEq(_newBPool.call__controller(), _deployer); // it sets caller as factory diff --git a/test/unit/BPool/BPool.tree b/test/unit/BPool/BPool.tree index 0405f003..fe9773d9 100644 --- a/test/unit/BPool/BPool.tree +++ b/test/unit/BPool/BPool.tree @@ -1,5 +1,7 @@ BPool::constructor └── when called + ├── it should set the ERC20 name + ├── it should set the ERC20 symbol ├── it sets caller as controller ├── it sets caller as factory ├── it sets swap fee to MIN_FEE diff --git a/test/unit/BPool/BPoolBase.t.sol b/test/unit/BPool/BPoolBase.t.sol index 1495f9d1..f79e0209 100644 --- a/test/unit/BPool/BPoolBase.t.sol +++ b/test/unit/BPool/BPoolBase.t.sol @@ -14,9 +14,11 @@ contract BPoolBase is Test, BConst { address[] public tokens; MockBPool public bPool; + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; function setUp() public virtual { - bPool = new MockBPool(); + bPool = new MockBPool(ERC20_NAME, ERC20_SYMBOL); tokens.push(makeAddr('token0')); tokens.push(makeAddr('token1')); } diff --git a/test/unit/BToken.t.sol b/test/unit/BToken.t.sol index ce2f54f2..1673ddc6 100644 --- a/test/unit/BToken.t.sol +++ b/test/unit/BToken.t.sol @@ -14,20 +14,23 @@ contract BToken is Test { address public spender = makeAddr('spender'); address public target = makeAddr('target'); + string constant ERC20_NAME = 'Balancer Pool Token'; + string constant ERC20_SYMBOL = 'BPT'; + function setUp() external { - bToken = new MockBToken(); + bToken = new MockBToken(ERC20_NAME, ERC20_SYMBOL); vm.startPrank(caller); // sets initial approval (cannot be mocked) bToken.approve(spender, initialApproval); } - function test_ConstructorWhenCalled() external { - MockBToken _bToken = new MockBToken(); + function test_ConstructorWhenCalled(string memory _name, string memory _symbol) external { + MockBToken _bToken = new MockBToken(_name, _symbol); // it sets token name - assertEq(_bToken.name(), 'Balancer Pool Token'); + assertEq(_bToken.name(), _name); // it sets token symbol - assertEq(_bToken.symbol(), 'BPT'); + assertEq(_bToken.symbol(), _symbol); } function test_IncreaseApprovalRevertWhen_SenderIsAddressZero() external { From d22ed864d2d1a0cf97e1171fc99467a32a6a75f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wei=C3=9Fer=20Hase?= Date: Wed, 31 Jul 2024 18:45:03 +0200 Subject: [PATCH 2/3] feat: implementing forceApprove in _afterFinalize hook (#183) * test: adding expectations on _afterFinalize test * feat: adding forceApprove to BCoWPool::_afterFinalize * chore: updating gas snapshots --- .forge-snapshots/newBCoWFactory.snap | 2 +- .forge-snapshots/newBCoWPool.snap | 2 +- src/contracts/BCoWPool.sol | 4 ++- test/unit/BCoWPool/BCoWPool.t.sol | 39 ++++++++++++++++++++++++++++ test/unit/BCoWPool/BCoWPool.tree | 8 ++++++ 5 files changed, 52 insertions(+), 3 deletions(-) diff --git a/.forge-snapshots/newBCoWFactory.snap b/.forge-snapshots/newBCoWFactory.snap index 2aa3683c..efa7ca76 100644 --- a/.forge-snapshots/newBCoWFactory.snap +++ b/.forge-snapshots/newBCoWFactory.snap @@ -1 +1 @@ -4314884 \ No newline at end of file +4360005 \ No newline at end of file diff --git a/.forge-snapshots/newBCoWPool.snap b/.forge-snapshots/newBCoWPool.snap index 538dee2c..e1899b7e 100644 --- a/.forge-snapshots/newBCoWPool.snap +++ b/.forge-snapshots/newBCoWPool.snap @@ -1 +1 @@ -3401020 \ No newline at end of file +3442739 \ No newline at end of file diff --git a/src/contracts/BCoWPool.sol b/src/contracts/BCoWPool.sol index 434929bc..ef2a7811 100644 --- a/src/contracts/BCoWPool.sol +++ b/src/contracts/BCoWPool.sol @@ -21,6 +21,7 @@ import {BPool} from './BPool.sol'; import {GPv2Order} from '@cowprotocol/libraries/GPv2Order.sol'; import {IERC1271} from '@openzeppelin/contracts/interfaces/IERC1271.sol'; import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; +import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; import {IBCoWFactory} from 'interfaces/IBCoWFactory.sol'; import {IBCoWPool} from 'interfaces/IBCoWPool.sol'; @@ -33,6 +34,7 @@ import {ISettlement} from 'interfaces/ISettlement.sol'; */ contract BCoWPool is IERC1271, IBCoWPool, BPool, BCoWConst { using GPv2Order for GPv2Order.Data; + using SafeERC20 for IERC20; /// @inheritdoc IBCoWPool address public immutable VAULT_RELAYER; @@ -146,7 +148,7 @@ contract BCoWPool is IERC1271, IBCoWPool, BPool, BCoWConst { function _afterFinalize() internal override { uint256 tokensLength = _tokens.length; for (uint256 i; i < tokensLength; i++) { - IERC20(_tokens[i]).approve(VAULT_RELAYER, type(uint256).max); + IERC20(_tokens[i]).forceApprove(VAULT_RELAYER, type(uint256).max); } // Make the factory emit the event, to be easily indexed by off-chain agents diff --git a/test/unit/BCoWPool/BCoWPool.t.sol b/test/unit/BCoWPool/BCoWPool.t.sol index c89b3606..4355ee30 100644 --- a/test/unit/BCoWPool/BCoWPool.t.sol +++ b/test/unit/BCoWPool/BCoWPool.t.sol @@ -3,6 +3,9 @@ pragma solidity 0.8.25; import {IERC20} from '@cowprotocol/interfaces/IERC20.sol'; +import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; +import {Address} from '@openzeppelin/contracts/utils/Address.sol'; + import {BCoWPoolBase} from './BCoWPoolBase.t.sol'; import {IBCoWFactory} from 'interfaces/IBCoWFactory.sol'; @@ -53,6 +56,42 @@ contract BCoWPool is BCoWPoolBase { assertEq(pool.APP_DATA(), _appData); } + function test__afterFinalizeRevertWhen_OneOfTheTokensReturnsFalse() external { + vm.mockCall( + tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode(false) + ); + // it should revert + vm.expectRevert(abi.encodeWithSelector(SafeERC20.SafeERC20FailedOperation.selector, tokens[1])); + bCoWPool.call__afterFinalize(); + } + + function test__afterFinalizeWhenOneOfTheTokensDoesntReturnAValue() external { + vm.mockCall( + tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode() + ); + // it assumes approval success + bCoWPool.call__afterFinalize(); + } + + function test__afterFinalizeRevertWhen_OneOfTheTokensRevertsWithoutData() external { + vm.mockCallRevert( + tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode() + ); + // it should revert with FailedInnerCall + vm.expectRevert(Address.FailedInnerCall.selector); + bCoWPool.call__afterFinalize(); + } + + function test__afterFinalizeRevertWhen_OneOfTheTokensRevertsWithData(bytes memory errorData) external { + vm.assume(keccak256(errorData) != keccak256(bytes(''))); + vm.mockCallRevert( + tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), errorData + ); + // it should revert with same error data + vm.expectRevert(errorData); + bCoWPool.call__afterFinalize(); + } + function test__afterFinalizeWhenCalled() external { // it calls approve on every bound token vm.expectCall(tokens[0], abi.encodeCall(IERC20.approve, (vaultRelayer, type(uint256).max))); diff --git a/test/unit/BCoWPool/BCoWPool.tree b/test/unit/BCoWPool/BCoWPool.tree index 54d5f962..ceb24d4f 100644 --- a/test/unit/BCoWPool/BCoWPool.tree +++ b/test/unit/BCoWPool/BCoWPool.tree @@ -8,6 +8,14 @@ BCoWPool::constructor └── it should set the app data BCoWPool::_afterFinalize +├── when one of the tokens returns false +│ └── it should revert +├── when one of the tokens doesnt return a value +│ └── it assumes approval success +├── when one of the tokens reverts without data +│ └── it should revert // FailedInnerCall +├── when one of the tokens reverts with data +│ └── it should revert ├── when called │ ├── it calls approve on every bound token │ └── it calls logBCoWPool on the factory From cee805037dc33453ce5b8b2fca4f7fb6520974c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wei=C3=9Fer=20Hase?= Date: Thu, 1 Aug 2024 18:04:48 +0200 Subject: [PATCH 3/3] chore: redeployment addresses (#186) --- README.md | 14 +++++++------- script/Registry.s.sol | 12 ++++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 6d3f80ac..7220b0ba 100644 --- a/README.md +++ b/README.md @@ -61,14 +61,14 @@ yarn test # run the tests # Deployments Ethereum Mainnet: - - BCoWFactory: [0x23fcC2166F991B8946D195de53745E1b804C91B7](https://etherscan.io/address/0x23fcC2166F991B8946D195de53745E1b804C91B7) - - BCoWHelper: [0x5F6e7D3ef6e9aedD21C107BF8faA610f1215C730 ](https://etherscan.io/address/0x5F6e7D3ef6e9aedD21C107BF8faA610f1215C730 ) + - BCoWFactory: [0xf76c421bAb7df8548604E60deCCcE50477C10462](https://etherscan.io/address/0xf76c421bAb7df8548604E60deCCcE50477C10462) + - BCoWHelper: [0x3FF0041A614A9E6Bf392cbB961C97DA214E9CB31](https://etherscan.io/address/0x3FF0041A614A9E6Bf392cbB961C97DA214E9CB31) Ethereum Sepolia: - - BCoWFactory: [0xF3F089AF5FaAF8784B445593B3bd8A514EaA3433 ](https://sepolia.etherscan.io/address/0xF3F089AF5FaAF8784B445593B3bd8A514EaA3433 ) - - BCoWHelper: [0x07E7E9e3f4E715Ef1434b7f865fedBCE82Dd41Ba](https://sepolia.etherscan.io/address/0x07E7E9e3f4E715Ef1434b7f865fedBCE82Dd41Ba) - - BCoWPool: [0x4Cc911897fFCC5553627d454533D944F1D78CBdE](https://sepolia.etherscan.io/address/0x4Cc911897fFCC5553627d454533D944F1D78CBdE) + - BCoWFactory: [0x1E3D76AC2BB67a2D7e8395d3A624b30AA9056DF9](https://sepolia.etherscan.io/address/0x1E3D76AC2BB67a2D7e8395d3A624b30AA9056DF9) + - BCoWHelper: [0xf5CEd4769ce2c90dfE0084320a0abfB9d99FB91D](https://sepolia.etherscan.io/address/0xf5CEd4769ce2c90dfE0084320a0abfB9d99FB91D) + - BCoWPool: [0xE4aBfDa4E8c02fcAfC34981daFAeb426AA4186e6](https://sepolia.etherscan.io/address/0xE4aBfDa4E8c02fcAfC34981daFAeb426AA4186e6) Gnosis Mainnet: - - BCoWFactory: [0x7573B99BC09c11Dc0427fb9c6662bc603E008304](https://gnosisscan.io/address/0x7573B99BC09c11Dc0427fb9c6662bc603E008304) - - BCoWHelper: [0x85315994492E88D6faCd3B0E3585c68A4720627e](https://gnosisscan.io/address/0x85315994492E88D6faCd3B0E3585c68A4720627e) \ No newline at end of file + - BCoWFactory: [0x703Bd8115E6F21a37BB5Df97f78614ca72Ad7624](https://gnosisscan.io/address/0x703Bd8115E6F21a37BB5Df97f78614ca72Ad7624) + - BCoWHelper: [0x198B6F66dE03540a164ADCA4eC5db2789Fbd4751](https://gnosisscan.io/address/0x198B6F66dE03540a164ADCA4eC5db2789Fbd4751) \ No newline at end of file diff --git a/script/Registry.s.sol b/script/Registry.s.sol index 8934e155..5c413798 100644 --- a/script/Registry.s.sol +++ b/script/Registry.s.sol @@ -16,16 +16,16 @@ abstract contract Registry is Params { constructor(uint256 chainId) Params(chainId) { if (chainId == 1) { // Ethereum Mainnet - bCoWFactory = BCoWFactory(0x23fcC2166F991B8946D195de53745E1b804C91B7); - bCoWHelper = BCoWHelper(0x5F6e7D3ef6e9aedD21C107BF8faA610f1215C730); + bCoWFactory = BCoWFactory(0xf76c421bAb7df8548604E60deCCcE50477C10462); + bCoWHelper = BCoWHelper(0x3FF0041A614A9E6Bf392cbB961C97DA214E9CB31); } else if (chainId == 100) { // Gnosis Mainnet - bCoWFactory = BCoWFactory(0x7573B99BC09c11Dc0427fb9c6662bc603E008304); - bCoWHelper = BCoWHelper(0x85315994492E88D6faCd3B0E3585c68A4720627e); + bCoWFactory = BCoWFactory(0x703Bd8115E6F21a37BB5Df97f78614ca72Ad7624); + bCoWHelper = BCoWHelper(0x198B6F66dE03540a164ADCA4eC5db2789Fbd4751); } else if (chainId == 11_155_111) { // Ethereum Sepolia [Testnet] - bCoWFactory = BCoWFactory(0xF3F089AF5FaAF8784B445593B3bd8A514EaA3433); - bCoWHelper = BCoWHelper(0x07E7E9e3f4E715Ef1434b7f865fedBCE82Dd41Ba); + bCoWFactory = BCoWFactory(0x1E3D76AC2BB67a2D7e8395d3A624b30AA9056DF9); + bCoWHelper = BCoWHelper(0xf5CEd4769ce2c90dfE0084320a0abfB9d99FB91D); } else { revert('Registry: unknown chain ID'); }