erc20 proxy deploy #115
erc20 proxy deploy #115
Conversation
| _symbol = symbol_; | ||
|
|
||
| // Transfer ownership to the initializer. | ||
| _transferOwnership(owner); |
There was a problem hiding this comment.
What is the purpose of transferring to an external owner besides JBTokens?
There was a problem hiding this comment.
none ever. should always be JBTokens, but JBTokens will always send itself.
test/TestTokenFlow.sol
Outdated
| @@ -74,7 +74,8 @@ contract TestTokenFlow_Local is TestBaseWorkflow { | |||
| _controller.deployERC20For({projectId: _projectId, name: "TestName", symbol: "TestSymbol", salt: bytes32(0)}); | |||
| } else { | |||
| // Create a new `IJBToken` and change it's owner to the `JBTokens` contract. | |||
| IJBToken _newToken = new JBERC20({name: "NewTestName", symbol: "NewTestSymbol", owner: _projectOwner}); | |||
| IJBToken _newToken = new JBERC20(); | |||
| _newToken.initialize({id: _projectId, name: "NewTestName", symbol: "NewTestSymbol", owner: _projectOwner}); | |||
There was a problem hiding this comment.
When we make changes to the impl, should we explore/test unintended uses/circumventions and ensure they fail? i.e. I don't initialize my token but transfer ownership to JBTokens, and try to configure my project with that token.
There was a problem hiding this comment.
if you've got a fun edge case in mind, always worth documenting with a test imo. please and thanks!
src/JBTokens.sol
Outdated
| ? new JBERC20(name, symbol, address(this)) | ||
| : new JBERC20{salt: salt}(name, symbol, address(this)); | ||
| ? IJBToken(Clones.clone(address(TOKEN))) | ||
| : IJBToken(Clones.cloneDeterministic(address(TOKEN), salt)); |
There was a problem hiding this comment.
Shouldn't we add an
salt = keccak256(abi.encode(msg.sender, salt));
Otherwise you can steal another projects salt by configuring a controller that lets you pass any salt.
src/JBERC20.sol
Outdated
| ERC20Permit(name) | ||
| Ownable(owner) | ||
| {} | ||
| constructor() Ownable(address(this)) ERC20("", "") ERC20Permit("JBToken") {} |
There was a problem hiding this comment.
Shouldn't we do something like
| constructor() Ownable(address(this)) ERC20("", "") ERC20Permit("JBToken") {} | |
| constructor() Ownable(address(this)) ERC20("", "") ERC20Permit("JBToken") { | |
| // Disable initialization on singleton instance. | |
| projectId = type(uint256).max; | |
| } |
There was a problem hiding this comment.
srry, projectId is removed!!
There was a problem hiding this comment.
Perhaps if we use DrG suggestion about using the name and/or symbol as a stop for re-initialization we can do
| constructor() Ownable(address(this)) ERC20("", "") ERC20Permit("JBToken") {} | |
| constructor() Ownable(address(this)) ERC20("invalid", "invalid") ERC20Permit("JBToken") {} |
There was a problem hiding this comment.
i hadn't pushed up my local commits... @xBA5ED review the latest and lets see if this still makes sense. we use name, but just check for empty string.
src/JBERC20.sol
Outdated
| /// @param owner The owner of the token. | ||
| function initialize(string memory name_, string memory symbol_, address owner) public override { | ||
| // Stop re-initialization. | ||
| if (this.owner() != address(0) || owner == address(0)) revert(); |
There was a problem hiding this comment.
if the owner calls Ownable::renounceOwnership(), then anyone can re-initialise it - rn it's ok as it should always be JBTokens the owner, but isn't a bit smelly for the long run? Why not just checking if the name/symbol are already set instead (there is no "clearNameAndSymbol()")
| /// @param owner The owner of the token. | ||
| function initialize(string memory name_, string memory symbol_, address owner) public override { | ||
| // Stop re-initialization. | ||
| if (bytes(_name).length != 0 || bytes(name_).length == 0) revert(); |
There was a problem hiding this comment.
fine but had to read it twice before understanding it :D
There was a problem hiding this comment.
hmm good feedback. how might we improve
Description
Deploy ERC-20's using proxy pattern to reduce gas.
Limitations & risks
New token deploy pattern, needs to be double checked but its similar to the 721 hook implementation
Check-list
Interactions
These changes will impact the following contracts:
JBTokens
JBERC20