Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Fixes known issues in the crowdsale demo #100

Merged
merged 9 commits into from Oct 30, 2018
Merged

Fixes known issues in the crowdsale demo #100

merged 9 commits into from Oct 30, 2018

Conversation

WyattMufson
Copy link
Contributor

@WyattMufson WyattMufson commented Oct 5, 2018

What current issue(s) from Github does this address?
#98 #99

What problem does this PR solve?
Adds OnTransfer to the minting in the deploy function and fixes total supply. Also makes sure that the nep5 functions are checking that the arguments are in the right format

How did you solve this problem?
#98 and #99 are pretty self explanatory. Length check for addresses for the argument formatting issue

How did you make sure your solution works?
Standardized it with the standalone NEP5 demo and another contract that I've tested on my private net

Are there any special changes in the code that we should be aware of?
No

Is there anything else we should know?
The NEP5 demo has slightly different code than the one in the crowdsale demo. They should probably be fully standardized in the future

@WyattMufson WyattMufson changed the base branch from development to master October 5, 2018 06:57
@WyattMufson
Copy link
Contributor Author

Looks like some of the tests are failing (like one due to the use of totalSupply over circulation). Fixing these now and will commit the updates tests.

@coveralls
Copy link

coveralls commented Oct 5, 2018

Coverage Status

Coverage remained the same at 92.508% when pulling ddc9873 on WyattMufson:master into acd9793 on CityOfZion:master.

@WyattMufson
Copy link
Contributor Author

WyattMufson commented Oct 6, 2018

Now also meant to address #101 and #102

@WyattMufson
Copy link
Contributor Author

Interested in getting feedback on this^

@localhuman
Copy link
Collaborator

Looks good, thanks for the changes!

I would only add that the demo is only for compiler testing purposes and shouldn't be confused for something that is a production ready NEP5 token.

@localhuman localhuman merged commit a00eefd into CityOfZion:master Oct 30, 2018
@WyattMufson
Copy link
Contributor Author

@localhuman is there an example of a place with a production ready NEP5 contract? The awesome-neo repo lists these contracts and the ICO-Templates repo is empty so far. Would it make sense to put some of this there?

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

Successfully merging this pull request may close these issues.

None yet

3 participants