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) Restrict token mantissa to decimals #631

Merged
merged 17 commits into from
Mar 1, 2018

Conversation

fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Feb 26, 2018

Closes #592

This PR introduces a refactor of NumericInput component, due to the restrictions in its previous implementation... it was useless to solve this particular issue.

Also, as it was refactored I had to refactor how the decimals, minCap (#571) and custom gasPrice (#573) inputs were implemented.

Things to consider:

  1. The letter e is used to represent a number in scientific notation. Thus, when a number can be a float, e will be enabled.
  2. minCap was restricted to be an integer, but it should support as many decimals as the token will support
  3. JS double floating point numbers goes up to 16 decimals, thus we should refactor how the conversion to integer is being done across the whole wizard to prevent possible issues (https://hackernoon.com/a-note-on-numbers-in-ethereum-and-javascript-3e6ac3b2fad9)
  4. One last thing... If the user loads several addresses and wants to change the decimals to a lower value, he/she will need to delete the already loaded addresses. Thus, adding a "Bulk Delete" button to the reserved tokens will be helpful.

@coveralls
Copy link

coveralls commented Feb 26, 2018

Pull Request Test Coverage Report for Build 1530

  • 36 of 70 (51.43%) changed or added relevant lines in 7 files are covered.
  • 16 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.7%) to 9.495%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stores/TokenStore.js 0 1 0.0%
src/components/stepThree/index.js 0 3 0.0%
src/stores/TierStore.js 0 4 0.0%
src/components/stepTwo/index.js 0 26 0.0%
Files with Coverage Reduction New Missed Lines %
src/utils/utils.js 1 26.62%
src/App.js 6 0.0%
src/components/stepTwo/index.js 9 0.0%
Totals Coverage Status
Change from base Build 1513: 0.7%
Covered Lines: 257
Relevant Lines: 1954

💛 - Coveralls

Copy link
Contributor

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

bug-631

@fernandomg
Copy link
Contributor Author

@fvictorio done, thanks!

wrapper.find('.reserved-tokens-item-empty').children('a').at(0).simulate('click')
expect(removeCallback).toHaveBeenCalledTimes(1)
})
// it('Should reset state after adding a new address', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this commented code be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


if (!(firstCharacterIsMinus && acceptsNegativeValues) && isNaN(parseInt(e.key, 10))) e.preventDefault()
if (!acceptFloat && !allowsNegative) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that if I create a NumericInput that allows negatives but not floats, I will be able to input - and ., or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh, good catch!

I did change how this check is done. It will allow - symbol to be entered if floats or negative values are allowed. But will prevent any other special char (e, ., +) if float is not allowed.

pristine: true,
valid: INVALID
value: props.value || '',
pristine: props.pristine || true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This means we can't create a NumericInput initialized with pristine={false}. Not that we need it, but if that's the case we should do just pristine: true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified so it will default to true only if props.pristine is undefined.

thanks!

Copy link
Contributor

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

Some comments, but nothing that blocks merging this in my opinion.

@vbaranov vbaranov merged commit f8d26b9 into master Mar 1, 2018
@ghost ghost removed the awaiting for review label Mar 1, 2018
@vbaranov vbaranov deleted the restrict-token-mantissa-to-decimals-#592 branch March 1, 2018 09:54
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.

None yet

4 participants