search

starknet-io / SNIPs

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

SNIP-17: Safe Transfer for Fungible Tokens #89

Closed boyuanx closed 2 months ago

boyuanx commented 4 months ago

DIscussion: https://community.starknet.io/t/snip-89-safe-transfer-for-fungible-tokens/

dor-starkware commented 4 months ago

Hi @boyuanx,

As per SNIP-1 guidelines, there are a few elements that need to be addressed in SNIP-89 before it can move forward:

Preamble Issues:

Rationale: This section is mandatory and should provide a detailed explanation of the design decisions made. It should also discuss any alternate designs considered and why they were not chosen, as well as related work in other languages or standards. For example, you can explain why the interface checks were implemented this way and what alternatives were considered.

Backwards Compatibility: This section is also mandatory if the SNIP introduces backwards incompatibilities. If there are no such incompatibilities, this should be explicitly stated. If there are any, please describe them and explain how they will be mitigated.

Please make these updates to comply with the SNIP-1 guidelines. Once these sections are added, we can proceed with the review process.

Thank you!

boyuanx commented 4 months ago

@dor-starkware Please check again, thanks!

boyuanx commented 4 months ago

Also, we can have a discussion on whether to use Array or Span for the input argument here.

github-actions[bot] commented 3 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!

boyuanx commented 3 months ago

Bump

dor-starkware commented 3 months ago

Hi @boyuanx,

Thank you for your patience. I’ve reviewed your recent commits and have a few minor requests before proceeding with the merge:

  1. Could you please update the "Implementation" section to "Reference Implementation" to ensure consistency in section titles?
  2. Similarly, please revise the "Security" section to "Security Considerations" for uniformity.
  3. Additionally, in the preamble section, please update the SNIP number to 17.

Thank you!

boyuanx commented 3 months ago

@dor-starkware please check again, thanks!

dor-starkware commented 2 months ago

Hi @boyuanx,

Thank you for making the necessary adjustments to the SNIP. I noticed that there are links in the document that are supposed to lead to the SNIP-17 page (which corresponds to this SNIP). However, these links currently lead to a non-existent page, resulting in a 404 error. Could you please remove these links?

Additionally, could you please change the status in the preamble from 'Draft' to 'Review'?

Once these changes are made, I’ll proceed with merging your SNIP into the main branch.

Thank you!

boyuanx commented 2 months ago

@dor-starkware I have made those changes, please check :)