add erc20 and erc721 deposit function to the Vault #53
Conversation
|
@0xb337r007 this is still marked as a WIP, do you want us to wait until the ERC721 changes land here as well? |
gravityblast
force-pushed
the
vault-deposit-erc20-3
branch
from
e620ed2 to
82bceba
Compare
gravityblast
force-pushed
the
vault-deposit-erc20-3
branch
from
82bceba to
e354b7a
Compare
gravityblast
force-pushed
the
vault-deposit-erc20-3
branch
from
e354b7a to
bea1149
Compare
There was a problem hiding this comment.
Great work @0xb337r007
I have some minor comments. And I believe there's 1-2 tests missing.
| error CommunityVault_ERC721TokenNotDeposited(); | ||
|
|
||
| mapping(address => uint256) public erc20TokenBalances; | ||
| mapping(address => EnumerableSet.UintSet) private erc721TokenIds; |
There was a problem hiding this comment.
| mapping(address => EnumerableSet.UintSet) private erc721TokenIds; | |
| mapping(address owners => EnumerableSet.UintSet balances) private erc721TokenIds; |
Just cosmetics, but I think we can use named mapping annotation here
There was a problem hiding this comment.
we need to upgrade to a newer version of the compiler to do it, shall we do it in another PR?
in this case instead of owners should be tokens
There was a problem hiding this comment.
Yes that can be done in a different PR. Good point.
| bool added = erc721TokenIds[token].add(tokenIds[i]); | ||
| if (!added) { | ||
| revert CommunityVault_ERC721TokenAlreadyDeposited(); | ||
| } |
There was a problem hiding this comment.
Shouldn't we perform this check before we even try to transfer it?
There was a problem hiding this comment.
good idea, I moved it
| * to indicate the contract implements `onERC721Received` as per ERC721. | ||
| */ | ||
| function onERC721Received(address, address, uint256, bytes calldata) public pure override returns (bytes4) { | ||
| return this.onERC721Received.selector; |
There was a problem hiding this comment.
I think here we still need to check if:
a) the received ERC721 doesn't already exist in the set and
b) do a) only if we know for sure that this onReceived event wasn't triggered from a previous depositERC721() call
Thoughts @0xb337r007 ?
There was a problem hiding this comment.
I completely skipped the check here on purpose to support only the deposit function. otherwise we might have strange scenarios in case we call deposit with a token that doesn't call the onERC721Received
There was a problem hiding this comment.
Hm.. okay maybe we can revisit this in the future.
I might be missing something here, but if such a token would not call onERC721Received it's perfectly fine no? We still track via depositERC721. I'd even consider this one of the "simpler" cases as it'd just be as if onERC721Received didn't exist.
For now we can leave it like this, but I still think we should consider supporting both unless there's good reasons not to.
| vm.stopPrank(); | ||
|
|
||
| assertEq(erc721Token.balanceOf(address(vault)), initialVaultBalance + 2); | ||
| assertEq(vault.erc721TokenBalances(address(erc721Token)), initialTokenBalanceValue + 2); |
There was a problem hiding this comment.
I think we need some tests for the error cases of depositERC721 here
gravityblast
force-pushed
the
vault-deposit-erc20-3
branch
from
bea1149 to
591cb55
Compare
0xb337r007
changed the title
Description
closes #31
Added 2 functions to deposit erc20 and erc721 tokens and register erc20 balances and erc721 ids.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge snapshot?pnpm lint?forge test?pnpm verify?