adding ERC721 detection to TokensController #524
Conversation
adonesky1
force-pushed
the
add-erc721detection-tokensController
branch
from
4685157 to
e69de03
Compare
adonesky1
force-pushed
the
add-erc721detection-tokensController
branch
from
2e7a1cf to
74e62f1
Compare
|
@Gudahtt sorry this PR has gotten quite large... Let me know if you want to look through it together? |
|
It doesn't look too large, though I haven't yet looked at the tests. Thanks for the offer - I'll let message you if anything seems unclear. |
adonesky1
force-pushed
the
add-erc721detection-tokensController
branch
from
5882088 to
53a30cb
Compare
src/assets/TokensController.test.ts
Outdated
| ); | ||
| const tokenAddress = '0x06012c8cf97BEaD5deAe237070F9587f8E7A266d'; | ||
|
|
||
| const result = await tokensController._detectIsERC721(tokenAddress); |
There was a problem hiding this comment.
We don't typically test private functions if we can avoid it. It makes refactoring more difficult, and it can even be actively misleading if the tests use the internal function differently than it's used in practice. We should be able to test all paths via the public function, otherwise it might be a sign that the private function is more complex than it should be.
Also this case is tested already by the "should add isERC721 = true to token object in state when token is collectible and in our contract-metadata repo" test. The main difference being that here the cryptokitties address is hard-coded, whereas in that other test it uses the first entry in the map.
There was a problem hiding this comment.
I guess the same applies for the next two tests as well. Except we don't have a test case for updateTokenType that returns false 🤔
…heck of contract-maps in _detectERC721
adonesky1
force-pushed
the
add-erc721detection-tokensController
branch
from
c7ee5eb to
7b51b9b
Compare
|
@Gudahtt thanks again for your always thorough reviews! I've cleaned up the tests, made them more thorough for all possible scenarios around the isERC721 flag functionality and made them only point to public functions. Also added a conditional block in the _detectIsERC721 to do the same check against our contract-maps for ERC20 tokens as we do for ERC721 to avoid unnecessary network requests for data we already have. Let me know if you have any questions and thanks again! |
* adding ERC721 detection to TokensController * modifying provider and config/state initialization * add watchAsset tests * lint fixes * add more tests related to new functionality to TokensController * addressing feedback * lint * fixing tests and try/catch * small addition to allow pre-ignoring tokens * remove await from stubs and lint * removing redundant tests * mistaken change to package.json * clean up isERC721 flag tests + add conditional block to enhance our check of contract-maps in _detectERC721
* adding ERC721 detection to TokensController * modifying provider and config/state initialization * add watchAsset tests * lint fixes * add more tests related to new functionality to TokensController * addressing feedback * lint * fixing tests and try/catch * small addition to allow pre-ignoring tokens * remove await from stubs and lint * removing redundant tests * mistaken change to package.json * clean up isERC721 flag tests + add conditional block to enhance our check of contract-maps in _detectERC721
Until we support collectibles in the extension, we need the controller that tracks token state to be aware of whether tokens are ERC721, in order to support behavior in the extension that prevents users from attempting to send ERC721 tokens and losing gas in a transaction we cannot execute.
This PR adds the isERC721 flag to tokens in the TokensController with all of the required methods and wiring to land it in the extension (an upcoming PR in the extension repo).