Pool for native tokens #62
Conversation
src/zkbob/ZkBobPoolETH.sol
Outdated
| function _withdrawNative(address user, uint256 tokenAmount) internal override returns (uint256 spentAmount) { | ||
| IWETH9(token).withdraw(tokenAmount); | ||
| if (!payable(user).send(tokenAmount)) { | ||
| new Sacrifice{value: tokenAmount}(user); |
There was a problem hiding this comment.
We might want to start thinking about another approach, since self-destruct is gonna be deprecated this or next year - https://eips.ethereum.org/EIPS/eip-4760
So far, the only reasonable approach I can suggest is to allow native withdrawals only for EOA addresses.
@akolotov wdyt?
There was a problem hiding this comment.
BTW, did you see if any of popular protocols use an approach like we use with the selfdestruct?
There was a problem hiding this comment.
I haven't seen such.
Just took a quick look at Aztec codebase, ETH from such failed withdrawals is kept on the contract balance, seems like with no way to recover it - https://github.com/AztecProtocol/aztec-connect/blob/a601e38f2866babae785fd9e0313ee02f208a754/contracts/src/core/processors/RollupProcessor.sol#L1262-L1269
There was a problem hiding this comment.
In case of WETH pool, acceptable solution might be to transfer WETH as a fallback in case of ETH transfer failure. I think we should go with that option for now.
As for the TokenSeller, I think we should wait until the concrete self-destruct deprecation EIP will be chosen and finalized.
src/zkbob/ZkBobPoolETH.sol
Outdated
| function _withdrawNative(address user, uint256 tokenAmount) internal override returns (uint256 spentAmount) { | ||
| IWETH9(token).withdraw(tokenAmount); | ||
| if (!payable(user).send(tokenAmount)) { | ||
| new Sacrifice{value: tokenAmount}(user); |
There was a problem hiding this comment.
BTW, did you see if any of popular protocols use an approach like we use with the selfdestruct?
test/zkbob/ZkBobPoolETH.t.sol
Outdated
| } | ||
|
|
||
| function testSimpleTransaction() public { | ||
| bytes memory data1 = _encodePermitDeposit(0.5 ether, 0.01 ether); |
There was a problem hiding this comment.
Since the order of permit deposits does not mater with permit2 consider to make an explicit test for this. For example, there could be like this:
bytes memory data2 = _encodePermitDeposit(0.75 ether, 0.01 ether);
bytes memory data1 = _encodePermitDeposit(0.5 ether, 0.01 ether);
_transact(data1);
_transact(data2);
src/zkbob/ZkBobPoolETH.sol
Outdated
| function _withdrawNative(address user, uint256 tokenAmount) internal override returns (uint256 spentAmount) { | ||
| IWETH9(token).withdraw(tokenAmount); | ||
| if (!payable(user).send(tokenAmount)) { | ||
| new Sacrifice{value: tokenAmount}(user); |
There was a problem hiding this comment.
In case of WETH pool, acceptable solution might be to transfer WETH as a fallback in case of ETH transfer failure. I think we should go with that option for now.
As for the TokenSeller, I think we should wait until the concrete self-destruct deprecation EIP will be chosen and finalized.
Co-authored-by: Kirill Fedoseev <[email protected]>
This merge contains the following set of changes: - Pool for native tokens (#62)
No description provided.