Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Fix) Don't continue to step 4 if min cap is not valid #650

Merged
merged 3 commits into from
Mar 6, 2018

Conversation

fvictorio
Copy link
Contributor

Closes #621.

This checks that the min cap isn't greater than any supply, not the sum of all the supplies. Otherwise, it may be the case that you can't buy in any tier.

For example, if you have a tier with supply 10 and another with supply 5, and your min cap is 11, it's less than the sum of all supplies, but you can't buy in none of the tiers.

I don't like at all the error message in this fix: it shows a message that basically says "this field is wrong, and one of these three things is the one causing the error".

It would be much better UI to show only the messages that are relevant. For example, if you input "-1" it just says "Value must be positive". If you input "1.333" and the token only has two decimals, it says "Decimals should not exceed...". If you input "-1.333", it should show both messages.

I didn't do it that way in this PR because it's a much bigger change, but if you agree with this, I will create an issue to improve it.

@coveralls
Copy link

coveralls commented Mar 2, 2018

Pull Request Test Coverage Report for Build 1567

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 11 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.07%) to 9.544%

Files with Coverage Reduction New Missed Lines %
src/index.js 4 0.0%
src/components/Common/WhitelistInputBlock.js 7 0.0%
Totals Coverage Status
Change from base Build 1551: 0.07%
Covered Lines: 258
Relevant Lines: 1948

💛 - Coveralls

Copy link
Collaborator

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if you have a tier with supply 10 and another with supply 5, and your min cap is 11, it's less than the sum of all supplies, but you can't buy in none of the tiers.

But if I have supply 10 at the 1st tier and supply 5 for the 2nd, and my min cap is 9. I guess it should work. But it doesn't.

@fvictorio
Copy link
Contributor Author

fvictorio commented Mar 5, 2018

Yes, I did that intentionally. Otherwise, you won't be able to buy anything at all in the second tier. But if you think we should use the sum of all the supplies instead of the minimum, I can change it.

@vbaranov
Copy link
Collaborator

vbaranov commented Mar 5, 2018

Otherwise, you won't be able to buy anything at all in the second tier

Right, but I think this case shouldn't prevent crowdsale creation. Logic could be: If a user can't buy on any tier, we should prevent creation. But if any tier has supply more or equals a user's min cap, we shouldn't prevent.

@fvictorio
Copy link
Contributor Author

@vbaranov Updated. Now I check the global min cap against the max supply instead (if the max supply is less than the global min cap, then every tier has a supply that is less than the global min cap).

@vbaranov
Copy link
Collaborator

vbaranov commented Mar 6, 2018

@fvictorio This configuration doesn't work for me:

screencapture-deploy-preview-650-ico-wizard-netlify-3-1520352662677

@fvictorio
Copy link
Contributor Author

That's strange, I typed the same values and it worked for me. I tried both typing the tiers first and the min cap second, and the other way around.

@vbaranov
Copy link
Collaborator

vbaranov commented Mar 6, 2018

Yes, it is not repeated to me anymore too.

@vbaranov vbaranov merged commit 5a228c4 into master Mar 6, 2018
@ghost ghost removed the awaiting for review label Mar 6, 2018
@vbaranov vbaranov deleted the global-min-cap-greater-supply-#621 branch March 6, 2018 16:50
@fvictorio
Copy link
Contributor Author

Maybe it's an issue with a very specific sequence of steps (although I hope not). Let's keep our eyes open in case it happens again.

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

Successfully merging this pull request may close these issues.

(Fix) investor global min cap can be greater than total supply of the tiers
4 participants