(Fix) floorToDecimals function updating to cover all use cases #281

vbaranov · 2017-10-18T17:57:07Z

Related to #274
Function roundToDecimals, which truncates decimals to max 18, is improved to cover all use cases.

(Fix) roundToDecimals function updating to cover all use case

igorbarinov · 2017-10-19T06:21:32Z

@15chrjef please take a look at the PR

mark-antony1

Looks like a much-needed commit :)
A few comments regarding code quality.

As a general rule, any logic that is not clear to someone who has never seen our codebase should be extracted into a variable or function (this can lead to a bunch of variables and utils, but also should encourage cleaner and conciser code)
all string or number literals should be extracted as constants to a local or global constant file
make sure to keep the same level of abstraction in a function/ make sure a function only does one thing. If a function has more than one responsibility, create a new function for this.

mark-antony1 · 2017-10-19T06:26:38Z

src/components/stepFour/index.js

@@ -267,7 +267,7 @@ export class stepFour extends stepTwo {
  //FlatPricing
  getPricingStrategyParams = (web3, pricingStrategy, i, token) => {
    console.log(pricingStrategy);
-    let oneTokenInETH = roundToDecimals(1000000000000000000, 1/pricingStrategy.rate)
+    let oneTokenInETH = roundToDecimals(-18, 1/pricingStrategy.rate)


what is -18? We should not have plain strings or numbers, these values should all be extracted to constants and imported. I would also make '1/pricingStrategy.rate' a variable.

mark-antony1 · 2017-10-19T06:27:00Z

src/utils/utils.js

@@ -283,7 +283,7 @@ export const getconstructorParams = (abiConstructor, state, vals, crowdsaleNum)
                  //params.vals.push(state.web3.toWei(1/state.pricingStrategy[crowdsaleNum].rate/10**state.token.decimals, "ether"));

                  let oneTokenInETHRaw = 1/state.pricingStrategy[crowdsaleNum].rate
-                  let oneTokenInETH = roundToDecimals(1000000000000000000, oneTokenInETHRaw)
+                  let oneTokenInETH = roundToDecimals(-18, oneTokenInETHRaw)


same as above for -18

mark-antony1 · 2017-10-19T06:30:46Z

src/utils/utils.js

+export const roundToDecimals = (n, input) => Math.floor10(input, n)
+
+const decimalAdjust = (type, value, exp) => {
+    if (typeof exp === 'undefined' || +exp === 0) {


why would exp be equal to the string 'undefined'? This should most likely be the value undefined.

what are you trying to do with +exp? if its number coersion, why not make it a number when it is passed in initially?

what does having an undefined exp or an exp equal to 0 mean in words? The logic contained within the if conditional should be extracted to a variable as well.

Let's use Math.floor10 and decimalAdjust. It is well-known solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

mark-antony1 · 2017-10-19T06:31:24Z

src/utils/utils.js

+
+const decimalAdjust = (type, value, exp) => {
+    if (typeof exp === 'undefined' || +exp === 0) {
+      return Math[type](value);


what is this trying to do here?

Let's use Math.floor10 and decimalAdjust. It is well-known solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

mark-antony1 · 2017-10-19T06:32:16Z

src/utils/utils.js

+    if (typeof exp === 'undefined' || +exp === 0) {
+      return Math[type](value);
+    }
+    value = +value;


why all the '+value's? There should be a clearer way of accomplishing your intent here.

Let's use Math.floor10 and decimalAdjust. It is well-known solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

I agree. All the '+' signs in the expressions will be replaced by Math.round? Or other methods on the Math object?

it is the numeric representation: https://stackoverflow.com/questions/6682997/javascript-plus-symbol-before-variable

mark-antony1 · 2017-10-19T06:32:49Z

src/utils/utils.js

+    }
+    value = +value;
+    exp = +exp;
+    if (isNaN(value) || !(typeof exp === 'number' && exp % 1 === 0)) {


I would extract the clause in the if conditional to a variable as well as !(typeof exp === 'number' && exp % 1 === 0)

Let's use Math.floor10 and decimalAdjust. It is well-known solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

Sounds good. Please extract line 306 to a variable

mark-antony1 · 2017-10-19T06:33:48Z

src/utils/utils.js

+    if (isNaN(value) || !(typeof exp === 'number' && exp % 1 === 0)) {
+      return NaN;
+    }
+    value = value.toString().split('e');


Instead of these three variable assignments that are self referencing, build a util function to extract this logic and return the final value for 'value'

Let's use Math.floor10 and decimalAdjust. It is well-known solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

sure. Can you rename 'value' to something more descriptive?

value -> inputNumber

Updates regarding to reviewer comments

Emergency (Fix) Change gas price const to 5GWei

mark-antony1 · 2017-10-21T19:29:12Z

src/components/stepFour/utils.js

@@ -220,7 +221,7 @@ export function transferOwnership(web3, abi, addr, finalizeAgentAddr, gasLimit,
    }
    if (!tokenContract) return noContractAlert();

-    sendTXToContract(web3, tokenContract.methods.transferOwnership(finalizeAgentAddr).send({gasLimit: gasLimit, gasPrice: 21000000000}), cb);
+    sendTXToContract(web3, tokenContract.methods.transferOwnership(finalizeAgentAddr).send({gasLimit: gasLimit, gasPrice: GAS_PRICE}), cb);


What is

tokenContract.methods.transferOwnership(finalizeAgentAddr).send({gasLimit: gasLimit, gasPrice: GAS_PRICE})

I would extract this to a variable

mark-antony1

Few comments and questions on semantics. Math methods look good.

reviewer comments

vbaranov · 2017-10-22T14:52:46Z

@15chrjef, is it time to approve?

igorbarinov · 2017-10-18T20:46:29Z

src/utils/utils.js

-  return 1.0 / n * Math.ceil(n * input)
+export const roundToDecimals = (n, input) => Math.floor10(input, n)
+
+const decimalAdjust = (type, value, exp) => {


where did we get this function from?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round

Merge pull request #280 from vbaranov/stage

89168d2

(Fix) roundToDecimals function updating to cover all use case

vbaranov requested review from igorbarinov and mark-antony1 October 18, 2017 17:57

igorbarinov mentioned this pull request Oct 18, 2017

too many decimal places error #274

Closed

mark-antony1 suggested changes Oct 19, 2017

View reviewed changes

vbaranov closed this Oct 19, 2017

vbaranov reopened this Oct 19, 2017

Merge pull request #286 from vbaranov/stage

c2c29cf

Updates regarding to reviewer comments

vbaranov requested a review from mark-antony1 October 20, 2017 13:55

vbaranov added 2 commits October 20, 2017 20:18

5gwei gas price

6186a89

Merge pull request #288 from vbaranov/stage

6530f01

Emergency (Fix) Change gas price const to 5GWei

mark-antony1 reviewed Oct 21, 2017

View reviewed changes

vbaranov added 4 commits October 22, 2017 17:33

reviewer comments

5fe6743

reviewer comments

ce84c87

Merge pull request #293 from vbaranov/stage

fa989d5

reviewer comments

Merge branch 'master' into stage

85ac253

vbaranov requested a review from mark-antony1 October 22, 2017 14:49

igorbarinov approved these changes Oct 22, 2017

View reviewed changes

vbaranov added 7 commits October 22, 2017 18:30

Update constants.js

f7d26e3

Update index.js

630e868

Update index.js

81e03da

Update utils.js

2728283

Update constants.js

d163ada

Update index.js

8c93f09

TRUNC_TO_DECIMALS

ddbd462

vbaranov changed the title ~~(Fix) roundToDecimals function updating to cover all use cases~~ (Fix) floorToDecimals function updating to cover all use cases Oct 22, 2017

vbaranov merged commit 26131b0 into master Oct 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Fix) floorToDecimals function updating to cover all use cases #281

(Fix) floorToDecimals function updating to cover all use cases #281

vbaranov commented Oct 18, 2017

igorbarinov commented Oct 19, 2017

mark-antony1 left a comment •

edited

Loading

mark-antony1 Oct 19, 2017 •

edited

Loading

mark-antony1 Oct 19, 2017

mark-antony1 Oct 19, 2017

vbaranov Oct 20, 2017

mark-antony1 Oct 19, 2017

vbaranov Oct 20, 2017

mark-antony1 Oct 19, 2017

vbaranov Oct 20, 2017

mark-antony1 Oct 21, 2017

vbaranov Oct 22, 2017

mark-antony1 Oct 19, 2017

vbaranov Oct 20, 2017

mark-antony1 Oct 21, 2017

vbaranov Oct 22, 2017

mark-antony1 Oct 19, 2017

vbaranov Oct 20, 2017

mark-antony1 Oct 21, 2017

vbaranov Oct 22, 2017

mark-antony1 Oct 21, 2017

vbaranov Oct 22, 2017

mark-antony1 left a comment

vbaranov commented Oct 22, 2017

igorbarinov Oct 18, 2017

vbaranov Oct 22, 2017

(Fix) floorToDecimals function updating to cover all use cases #281

(Fix) floorToDecimals function updating to cover all use cases #281

Conversation

vbaranov commented Oct 18, 2017

igorbarinov commented Oct 19, 2017

mark-antony1 left a comment • edited Loading

Choose a reason for hiding this comment

mark-antony1 Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-antony1 left a comment

Choose a reason for hiding this comment

vbaranov commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-antony1 left a comment •

edited

Loading

mark-antony1 Oct 19, 2017 •

edited

Loading