Deploy factory multi chain #2
Conversation
There was a problem hiding this comment.
You should try to maintain good code quality changes.
- Don't commit files that are not relevant
- Don't commit changes to files that aren't part of the change scope.
You should use the short hand for Create2 rather than write a decidated script. See the shorthand for create2 here: https://github.com/catalystdao/catERC20/blob/6218231756d98cbbc96e110f4b2577c4faf5164b/src/catERC20Factory.sol#L101
foundry.toml
Outdated
| @@ -3,4 +3,5 @@ src = "src" | |||
| out = "out" | |||
| libs = ["lib"] | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Don't change files when not relevant
script/Create2.sol
Outdated
| revert Create2EmptyBytecode(); | ||
| } | ||
| assembly { | ||
| addr := create2(callvalue(), add(creationCode, 0x20), mload(creationCode), salt) |
There was a problem hiding this comment.
There is no need to use assembly. Furthermore, I don't understand why this is payable or why we are forwarding the Ether on this call?
src/catERC20Factory.sol
Outdated
| import { CatLockbox } from './CatLockbox.sol'; | ||
| import { CatERC20 } from './catERC20.sol'; | ||
| import { CatLockbox } from './catLockbox.sol'; | ||
|
|
There was a problem hiding this comment.
Don't change files when not relevant.
foundry.toml
Outdated
| @@ -3,4 +3,5 @@ src = "src" | |||
| out = "out" | |||
| libs = ["lib"] | |||
|
|
|||
|
|
|||
| uint256[] memory burnerLimits = new uint256[](0); | ||
| address[] memory bridges = new address[](0); | ||
|
|
||
| address deployedToken = catERC20Factory.deployXERC20("Test", "TS", minterLimits, burnerLimits, bridges); |
There was a problem hiding this comment.
The deploy function does more than just deploy the factory now.
A deployment script should be explcit in what it does. forge script DeployFactory --sig "deployToAllChains()" should be expected to deploy the factory not also an xERC20 token.
src/catERC20.sol
Outdated
| @@ -245,6 +245,7 @@ contract CatERC20 is ERC20, Ownable, IXERC20 { | |||
| * @param deltaLimit The delta that has to be applied to the limit. | |||
| * @return newLimit The new current limit | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
This file has also been edited now. See my other comments on not changing files which are not needed.
src/catERC20Factory.sol
Outdated
| import { CatLockbox } from './CatLockbox.sol'; | ||
| import { CatERC20 } from './catERC20.sol'; | ||
| import { CatLockbox } from './catLockbox.sol'; | ||
|
|
No description provided.