-
Notifications
You must be signed in to change notification settings - Fork 28
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: symbol string size #95
base: v3
Are you sure you want to change the base?
Conversation
Currently on this branch, a v2 pool containing a token with too large of a symbol returns "-NA-" for the pool symbol which is probably not ideal. Need to understand how to slice the response of the |
contracts/LpSugar.vy
Outdated
return abi_decode(response, String[100]) | ||
# String size cannot be larger than max_outsize - 64 bytes | ||
if len(response) > 0 and len(response) <= 128: | ||
return abi_decode(response, String[128]) |
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.
My understanding is that this should also be a String[192]
if we're doing the max_outsize=192
, no?
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.
The compiler only allows the string size to be a maximum of max_outsize - 64
. But unfortunately there are cases where the raw_call spits out a response that is too large to fit into a string the size of max_outsize - 64
@ethzoomer do you have a contract we can use to test this? I've simplified the conversion to be based on bytes btw, but would love to add a test before this is merged. |
@stas Ty good idea, this token was the one with the large symbol: |
This should be up for review now. Vyper doesn't let us have dynamic string size initialization. Because of this, we can't really use To avoid that, we basically ignore any bytes array larger than 96, meaning 10 chars. Larger token symbols will display as |
Reported the limitations upstream |
good call o adding a test for the pool... I forgot the symbol 💩 will carry over there too!
I'll update the symbols to 30 chars Ty ty for reviewing this! |
c4ba768
to
9de4871
Compare
Closes #94