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

Use string for name and symbol #28

Merged
merged 1 commit into from
May 20, 2021
Merged

Conversation

brianmcmichael
Copy link
Contributor

Recent versions of solc do not treat bytes32 and string in the same way that it used to, they are no longer comparable or directly castable. Therefore, creating token integrations with more recent versions of solc have difficulty processing the DS-Token's bytes32 return from name and string. The ERC20 standard does call for string types to be used for these optional returns so I'm just putting this forth as a suggestion.

@d-xo
Copy link
Contributor

d-xo commented Jul 9, 2020

Is there some nice way to have these strings be constant? dynamic types and memory parameters are quite annoying for FV.

@brianmcmichael
Copy link
Contributor Author

@xwvvvvwx that gets into a philosophical discussion. In ds-token now, they symbol is set via the constructor and the name is set via a setter function. To be constant you'd have to hard-code them into the contract prior to deployment and you wouldn't be able to pass them as params. I don't want to change those patterns without the blessing of the maintainers. solc 0.6.5+ allows for immutable variables to be set via a constructor, but I'm not sure if that would achieve any benefit by way of formal verification, and the existing ds-token isn't 0.6.x compatible anyways.

@gbalabasquer
Copy link
Contributor

Unfortunately immutable doesn't work with string type.

@d-xo
Copy link
Contributor

d-xo commented May 18, 2021

@brianmcmichael If you rebase this onto latest master I will merge

@brianmcmichael brianmcmichael force-pushed the tokenstring branch 2 times, most recently from 99db334 to 370c3f1 Compare May 19, 2021 02:21
@brianmcmichael
Copy link
Contributor Author

Rebased.

@d-xo
Copy link
Contributor

d-xo commented May 19, 2021

oops, looks like merging the decimals PR created a conflict here, can you rebase again.

@brianmcmichael
Copy link
Contributor Author

Rebased again.

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.

None yet

4 participants