refactor TokenSymbol to Symbol and add new TokenSymbol and RoleSymbol types#2690
refactor TokenSymbol to Symbol and add new TokenSymbol and RoleSymbol types#2690
TokenSymbol to Symbol and add new TokenSymbol and RoleSymbol types#2690Conversation
onurinanc
changed the title
TokenSymbol to Symbol and add new TokenSymbol and RoleSymbol types.TokenSymbol to Symbol and add new TokenSymbol and RoleSymbol types
bobbinth
requested review from
PhilippGackstatter and
bobbinth
and removed request for
bobbinth
| /// [`as_element()`](Self::as_element), and decoded back via | ||
| /// [`try_from_encoded_felt()`](Self::try_from_encoded_felt). | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct Symbol(String); |
There was a problem hiding this comment.
I think Symbol may be way too generic of a name. Maybe ShortCapitalString or CapitalString or something similar would be better?
There was a problem hiding this comment.
@bobbinth but the issue with this naming ShortCapitalString is that we also have underscore: "_" in the Symbol type. I think we should either keep it as Symbol or find another name since ShortCapitalString doesn't mention the underscore.
Another option is to apply Option 1 here: #2683 (comment)
There was a problem hiding this comment.
Agreed that it's not ideal, but I think something like ShortCapitalString even with underscores is better than Symbol :)
Another option is to apply Option 1 here: #2683 (comment)
I'd still go with the current option (option 2) - just change the name of the Symbol to something less general.
2 participants
This PR introduces a new
Symboltype, and addsTokenSymbolandRoleSymboltypes.Related Issue: #2683