-
Notifications
You must be signed in to change notification settings - Fork 984
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
fix tokens validation #10923
fix tokens validation #10923
Conversation
Jenkins BuildsClick to see older builds (8)
|
90% of end-end tests have passed
Failed tests (10)Click to expand
Passed tests (86)Click to expand |
0% of end-end tests have passed
Failed tests (1)Click to expand
|
@flexsurfer can't reproduce |
@churik i just fixed validation in e2e but i didn't fix warnings, so currently there are warnings in e2e |
@flexsurfer so what we need here? to tap OK to dismiss the warning? Also not clear what can I test to check PR itself. |
we need a decision what to do with these warnings, my suggestion we just need to fix tokens data in status-react according to contracts @Serhy wdyt ? |
Yes, correct. To set token names according to their |
@flexsurfer just found:
https://etherscan.io/token/0xb932a70a57673d89f4acffbe830e8ed7f75fb9e0 We use https://etherscan.io/token/0x41a322b28d0ff354040e2cbc676f0320d8c8850d But when I made transaction in Tr hash: 0x888aa55af3e6e495af94fe6a61b6659d12237b3bc27b08561862b622815bb9ad |
Another one:
|
7258045
to
09af1f2
Compare
@flexsurfer #10923 (comment) |
i'm not sure what to do with these two cases @hesterbruikman
|
according to FXC https://etherscan.io/address/0xc92D6E3E64302C59d734f3292E2A13A13D7E1817 - last transaction was 92 days ago; not sure it is used now. |
ok so for 1 we can remove "old" token and prevent creating tokens with the same symbol , not sure what to do with 2 |
@flexsurfer is the issue that we don't have an icon for MANA (decentraland)? |
@hesterbruikman no, the issue that it has a symbol name |
I thought that we may need to have some balance between token names and symbols used in the contracts and, like, in more centralised world. Similar thing with DAI and SAI tokens: There are some other token with similar story when contract symbol is not the same (or even similar) to what in coinmarketcap. So what I suggest at this point is to use DCN for Dentacoin which should fix (2) in #10923 (comment) and so we able to push this in v1.6 @flexsurfer @hesterbruikman what do you think? |
That makes sense to me @Serhy @flexsurfer any thoughts on the best approach to update in general? As I understand, it's best to get token name from token tracker on Etherscan (is that right @Serhy?) What I recall from the most recent round was that Simon provided icons for a range of tokens. @Ferossgp then wrote a script to get info for those tokens. From what I gather it's probably more durable for us to contribute to https://github.com/ethereum-lists/tokens providing icon design there and using an open source list. Wdyt? |
@Serhy i would agree with you, but still think we should keep validation and show this error in e2e and handle it , also we need to prevent creating custom token with an existing symbol |
09af1f2
to
a285341
Compare
@Serhy so i've fixed all issues, removed this FXC |
a285341
to
02f820a
Compare
added two more validation, symbols and addresses duplication, and found 1 item with the duplicated symbol and one with duplicated address |
93% of end-end tests have passed
Failed tests (7)Click to expand
Passed tests (91)Click to expand
|
100% of end-end tests have passed
Passed tests (7)Click to expand
|
Tested on Android 8.1, Android 9 and iOS 13 |
Signed-off-by: andrey <motor4ik@gmail.com>
02f820a
to
301f1c8
Compare
Indeed looks like we use the ERC20 token address where we should be using the ERC721 token address Symbol SUPR @flexsurfer can you please take a look to see if that works. Let me know if I need to create a separate issue |
it needs a separate issue for sure |
fix tokens validation
fixes: #10922