starknet-io / SNIPs

Starknet Improvement Proposal repository
MIT License
158 stars 99 forks source link

SNIP-12 Defining types #77

Closed gaetbout closed 4 months ago

gaetbout commented 9 months ago

Definition of a type

penovicp commented 9 months ago

I would suggest adding two more rules to reduce potential ambiguity some more:

sgc-code commented 9 months ago

I would suggest adding two more rules to reduce potential ambiguity some more:

  • Name can't be enclosed in whitespace characters
  • Name can't contain the comma (,) character (this is since enum types use it as a delimiter)

hi @penovicp, thanks for your feedback

penovicp commented 9 months ago

For the whitespace rule I would assume just using the word "whitespace" and relying on the \s RegEx could be sufficient. I would hope that it's mostly standardized between different programming languages. Admittedly, I haven't done a deep dive on the topic to know if there are any differences/edge cases for more obscure character sets.

For the enum rule, if the incoming type definition for the enum contains { "name": "Variant N", "type": "(u128,u128)" }, there is ambiguity if the variant is a u128 tuple or a u128,u128 type.

sgc-code commented 8 months ago

For the whitespace rule I would assume just using the word "whitespace" and relying on the \s RegEx could be sufficient. I would hope that it's mostly standardized between different programming languages. Admittedly, I haven't done a deep dive on the topic to know if there are any differences/edge cases for more obscure character sets.

For the enum rule, if the incoming type definition for the enum contains { "name": "Variant N", "type": "(u128,u128)" }, there is ambiguity if the variant is a u128 tuple or a u128,u128 type.

Regarding white space, we prefer to keep it as is, because we are not sure about that all regex work the same, and because that change won't actually make SNIP-12 safer

Regarding the enums, you are right, and we have updated this PR with that change.

I believe this PR is now ready to be merged so different SDKs can implement it

github-actions[bot] commented 7 months ago

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. This PR will be closed and locked in 7 days if no further activity occurs. Thank you for your contributions!

sgc-code commented 7 months ago

@ArielElp can we merge this?

gaetbout commented 7 months ago

@ArielElp up 😅

github-actions[bot] commented 6 months ago

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. This PR will be closed and locked in 7 days if no further activity occurs. Thank you for your contributions!

gaetbout commented 6 months ago

@ArielElp 🙏 ?

github-actions[bot] commented 5 months ago

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. This PR will be closed and locked in 7 days if no further activity occurs. Thank you for your contributions!

gaetbout commented 5 months ago

le dot of activity

github-actions[bot] commented 4 months ago

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. This PR will be closed and locked in 7 days if no further activity occurs. Thank you for your contributions!

gaetbout commented 4 months ago

Le ping to stay alive