Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TokenVault: Implementing full EIP-20 support #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

villesundell
Copy link
Contributor

Previously we had a check for isToken() interface function.
The check was important to test if the address is actually a token, wrong token
would make claiming upon unsuccesful locking impossible.

However, isToken() is not part of the official EIP-20 token specification,
making this particular implementation of the TokenVault TokenMarket specific.

TokenVault should support all EIP-20 tokens, including our SecurityToken.

The check is not needed anymore, since we inherit Recoverable, which enables
us to claim any tokens which are not locked. Hence, removing the check.

Previously we had a check for isToken() interface function.
The check was important to test if the address is actually a token, wrong token
would make claiming upon unsuccesful locking impossible.

However, isToken() is not part of the official EIP-20 token specification,
making this particular implementation of the TokenVault TokenMarket specific.

TokenVault should support all EIP-20 tokens, including our SecurityToken.

The check is not needed anymore, since we inherit Recoverable, which enables
us to claim any tokens which are not locked. Hence, removing the check.
@voith
Copy link
Collaborator

voith commented Mar 22, 2019

isToken() is not part of the official EIP-20 token specification

@villesundell since isToken() isn't part of EIP-20, would it make sense to get rid of StandardTokenExt? StandardTokenExt has only isToken method and that always returns True.
Also, since isToken() was always returning True, there is not a significant benefit from this change, but sure its good to clean up if we have the bandwidth.

@villesundell
Copy link
Contributor Author

@voith I don't see a problem with isToken() per se (we have used it with practically all of our tokens, so maybe we should keep it, so the ABI would not change). The problem is that the TokenVault was depending on that, which is not EIP-20 compatible practice. Previously it was needed: we wanted to make sure the Vault is configured correctly, since claiming the tokens back was depending on the token address. Now we have a different mechanism for claiming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants