-
Notifications
You must be signed in to change notification settings - Fork 982
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
Add support for symbols containing unicode chars #176
Conversation
57340ef
to
6c74d26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -63,6 +64,10 @@ describe('schema', () => { | |||
checkSchema(emptyList, false); | |||
}); | |||
|
|||
it('works for empty names and symbols', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have many errors in the past I want to make amends
f1786f5
to
ff5a257
Compare
@@ -231,19 +231,33 @@ | |||
"name": { | |||
"type": "string", | |||
"description": "The name of the token", | |||
"minLength": 1, | |||
"minLength": 0, | |||
"maxLength": 40, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zzmp btw, this limit isn't correct. You can have more than 40 chars and as far as I know there isn't a limit.
This pair is one https://dexscreener.com/polygon/0x841120E51aD43EfE489244728532854A352073aD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks I lost all my devices and I was able to recover some databases now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given there are existing applications that parse/store these fields from token lists, I think we should err on the side of caution in terms of what strings are allowed in the fields of a valid list (in the context of input sanitization). imo consumers of token lists should not have to worry about potentially malicious values in the list. So I would prefer we append the desired new characters/symbols to the original regex pattern rather than allowing essentially any string. Happy to hear your thoughts on this.
@matteenm I get your point. But I'm not sure if this package should be that strict. If you try to use for example the Polygon Tokens List (from their GitHub) you will see that the validation won't pass on symbols (it is also not passing for tags, but that's another issue). And here we are talking about an 'official' list. At DEX Screener we are tracking 40k+ tokens. If I need to whitelist every single special char I have, well, you know how that regexp will be in the end. I don't mind dropping this PR btw. I'm basically removing all special chars on my end in order to get the Uniswap widget working - otherwise, I can't even use the Polygon token list I mentioned. Let me know your thoughts! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x50655c814b96dA35E42942f6510014ddEA38A152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
Add BeraLand
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "My Token List", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0
@@ -231,19 +231,33 @@ | |||
"name": { | |||
"type": "string", | |||
"description": "The name of the token", | |||
"minLength": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release coin
"examples": [ | ||
"USD Coin" | ||
] | ||
}, | ||
"symbol": { | ||
"type": "string", | ||
"description": "The symbol for the token; must be alphanumeric", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
],
"examples": [ | ||
"USD Coin" | ||
] | ||
}, | ||
"symbol": { | ||
"type": "string", | ||
"description": "The symbol for the token; must be alphanumeric", | ||
"pattern": "^[a-zA-Z0-9+\\-%/$.]+$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"examples":
"examples": [ | ||
"USD Coin" | ||
] | ||
}, | ||
"symbol": { | ||
"type": "string", | ||
"description": "The symbol for the token; must be alphanumeric", | ||
"pattern": "^[a-zA-Z0-9+\\-%/$.]+$", | ||
"minLength": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USD COIN
#175