enable erc20 tokens for english auction#8
enable erc20 tokens for english auction#80xmapachex wants to merge 1 commit intoggonzalez94:mainfrom
Conversation
ggonzalez94
left a comment
There was a problem hiding this comment.
Hey @MrRaccxxn sorry for being such a lousy reviewer, but I finally had the time to think more about this.
At first the idea of using the same contract and two functions made sense(or we could even consider using the same). But after thinking more about it I now believe an inheritance model makes more sense. Let me ellaborate:
We have a BaseEnglishAuction contract with the core logic for bidding, outbidding, refunds, etc. and we have two other abstract contracts that inherit from it EnglishAuctionETH and EnglishAuctionERC20. The
base contract will still handle most of the logic and in the placeBid function can call an internal(virtual) _transferIn function and the withdrawRefund can call a _transferOut function that each of the child contracts can override to deal with Eth and ERC20.
While more complicated I believe this makes sense because of:
- More gas efficient(no need to hold extra variables or do extra checks that you don't need)
- Cheaper to deploy(since the bytecode should be smaller)
- More secure, since there are less paths to test(less if's)
- This can be extended for the other auction types more easily
Let me know what you think and if you have some pushback on the idea, and happy to discuss. I have some rough idea how the code should look like, so if you agree with the new approach I can post an initial draft of the structure(or feel free to go ahead and take a stab at it)
| /// @dev Indicates if the auction uses native currency or an ERC20 token (Default as native currency) | ||
| bool private immutable isNativeCurrency = true; | ||
|
|
||
| /// @dev The ERC20 token used for the auction if `isNativeCurrency` is false | ||
| IERC20 public erc20Token; |
There was a problem hiding this comment.
I don't think we need two variables. I know the cost of the immutable is fairly negligible, but we can just have the erc20Token variable and check for address(0) which means we are using the native token
| uint256 _extensionThreshold, | ||
| uint256 _extensionPeriod | ||
| uint256 _extensionPeriod, | ||
| address _erc20Token |
There was a problem hiding this comment.
nit: we should add _erc20Token to the param list.
* @param _erc20Token The address of the erc20 token used for the auction. Zero address if the auction will be conducted with native currency.
Fixes #1