Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

In-person code review #21

Closed
fulldecent opened this issue Apr 30, 2018 · 2 comments
Closed

In-person code review #21

fulldecent opened this issue Apr 30, 2018 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@fulldecent
Copy link
Member

This issue tracks Will's notes from our code review.

@MoMannn please add the notes here you have so I can track them.

If any of that stuff is a security concern for you and you don't want to publish it right now then still post it but just encrypt it (https://superuser.com/a/370412/38689) and send me the key separately.

@xpepermint xpepermint assigned xpepermint and MoMannn and unassigned xpepermint Apr 30, 2018
@xpepermint xpepermint added the enhancement New feature or request label Apr 30, 2018
@MoMannn
Copy link
Member

MoMannn commented May 3, 2018

Notes (with my comments):

  • check other valid token implementation
    (I have not found a different implementation - the only difference I found was either in the name or that it was not a modifier but an function)
  • owner override problem (check remix)
    fixed
  • check if validNFToken is necessary in functions (to lower gas consumption)
    I determined that it was necessary
  • assembly check in a seperate function/library
    fixed
  • require in clear approval (is it necessary)
    fixed
  • mint return true - not necessary -> remove
    fixed
  • document proof -> make it private/check other implementations
    it needs to be internal for burnableXcert, added notes that is has to exist for every token.
  • remove string check
    fixed
  • TokenURi check
    fixed
  • eip165 into seperate contract?
    fixed
  • function order
    it is order by functionality
  • make style guide public
    @xpepermint can you do this?

/pull/24

@fulldecent
Copy link
Member Author

All issues addressed, closing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants