use proxied simpleswaps #120
Conversation
ralph-pichler
commented
Mar 5, 2021
•
ralph-pichler
commented
- uses proxies for cheaper simpleswap deployment
- uses create2 for deterministic address derivation
| @param _issuer the issuer of cheques from this chequebook (needed as an argument for "Setting up a chequebook as a payment"). | ||
| _issuer must be an Externally Owned Account, or it must support calling the function cashCheque | ||
| @param _defaultHardDepositTimeout duration in seconds which by default will be used to reduce hardDeposit allocations | ||
| */ | ||
| constructor(address _issuer, address _token, uint _defaultHardDepositTimeout) public { | ||
| function init(address _issuer, address _token, uint _defaultHardDepositTimeout) public { | ||
| require(issuer == address(0), "already initialized"); |
There was a problem hiding this comment.
You are not protecting that _issuer is not equal to address(0). Hence, it may be that the contract is already initialized when this function is called. Why don't you use the Initializable.initializer modifier from OpenZeppelin?
There was a problem hiding this comment.
cost primarily. Initializable.initializer implies setting more storage fields. by reusing the issuer field for the check we save about 20k gas.
There was a problem hiding this comment.
Ok, but then at least we should have a require statement, guaranteeing that _issuer (init argument) is not equal to address(0). Because otherwise, init can be called multiple times.
| address contractAddress = address(new ERC20SimpleSwap(issuer, ERC20Address, defaultHardDepositTimeoutDuration)); | ||
| function deploySimpleSwap(address issuer, uint defaultHardDepositTimeoutDuration, bytes32 salt) | ||
| public returns (address) { | ||
| address contractAddress = Clones.cloneDeterministic(master, keccak256(abi.encode(msg.sender, salt))); |
There was a problem hiding this comment.
What is the advantage of using cloneDeterministic over clone?
There was a problem hiding this comment.
deterministic addresses. right now we actually have an (undocumented) issue in bee where if you deploy the chequebook but then there is a reorg with transaction reordering (after parsing the deploy receipt) you might end up with a chequebook for another overlay in your state store.
contracts/SimpleSwapFactory.sol
Outdated
|
|
||
| constructor(address _ERC20Address) public { | ||
| constructor(address _ERC20Address, address _master) public { |
There was a problem hiding this comment.
I think it is cleaner if you deploy the master contract as part of the constructor of the factory
There was a problem hiding this comment.
that is a good idea, that also solves the master initialisation problem. will do that.
| @@ -1,8 +1,11 @@ | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
There was a problem hiding this comment.
If this is merged, we can close #122
