Token-manager: use bn.js #432

2color · 2018-09-08T20:24:20Z

The PR allows the assigning/minting of sums larger than 100.

Fixes #266

Approach

Pass the integers as strings in the web worker (instead of JS numbers)
Convert the strings to BN.js objects in the react application
Perform calculations with BN.js

Notes

When discussed with @sohkai, Brett suggested converting the strings to BN.js object in the web worker. When done that way, the BN.js objects were serialised as normal objects with the BN.js properties and required reinitialisation in the react application. Therefor I left the numbers in the worker as strings.

Extras

Sorted the balances to match the stake percentage table for a more coherent UI.

CLAassistant · 2018-09-08T20:24:27Z

All committers have signed the CLA.

Match the order of the stake table

2color · 2018-09-17T17:26:15Z

b8a7ff2 was added to fix the following UI inconsistency:

izqui · 2018-09-21T10:40:05Z

We need to test how this handles amounts when token balances are decimal, given that BN.js only works with integers.

cfl0ws · 2018-09-21T13:40:38Z

@bpierre to review -
https://github.com/aragon/radspec/blob/4028cdfcdf32dee59f92a990d055214cdf4ed3bf/src/helpers/tokenAmount.js

bpierre · 2018-09-24T10:35:33Z

@2color Looking good! 💯

I added support for decimals.

coveralls · 2018-09-24T11:38:22Z

Coverage decreased (-1.9%) to 95.094% when pulling f996026 on 2color:use-bn into d62f2b6 on aragon:master.

coveralls · 2018-09-24T11:38:24Z

Coverage decreased (-0.8%) to 96.212% when pulling 3c7f85b on 2color:use-bn into d62f2b6 on aragon:master.

2color · 2018-09-24T11:50:26Z

Sweet! Would be good to add tests as a next step too.

On Mon, 24 Sep 2018 at 13:38 Coveralls ***@***.***> wrote: [image: Coverage Status] <https://coveralls.io/builds/19154017> Coverage decreased (-0.8%) to 96.212% when pulling *f996026 <f996026> on 2color:use-bn* into *d62f2b6 <d62f2b6> on aragon:master*. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#432 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5mP-s1uthCZKx0Rb9AzVA_8DA3FrBxks5ueMQzgaJpZM4WgBS3> .

-- Daniel Norman

izqui · 2018-09-24T15:56:39Z

Tested locally and working well! However it looks like the stake distribution graph isn't taking into account decimals

2color · 2018-09-26T14:14:41Z

Currently the confirmation panel shows the wrong amount. This is when minting 0.2 and the amountConverted is `200000000000000000`. We'll need to have a look into that. Though it's in the wrapper.

2color · 2018-09-26T15:18:02Z

The stake distribution bug has been resolved.

sohkai · 2018-09-26T17:46:25Z

This is when minting 0.2 and the amountConverted is 200000000000000000

Ahh... this is probably due to radspec :(. Good catch.

cfl0ws · 2018-10-01T14:34:05Z

@izqui @bpierre Should this be moved to the Review column?

2color · 2018-10-01T14:43:12Z

@chris-remus There's still a bug that @bpierre reported when the token decimal base is 100. I made the wrong assumption in the last commit.

I'm trying to better understand the different possible values for tokenDecimalBase so that all cases are correctly covered.

bpierre · 2018-10-04T09:03:08Z

@izqui The percentages should be fixed now

apps/token-manager/app/src/components/SideBar.js

2color · 2018-10-04T10:55:50Z

apps/token-manager/app/src/utils.js

+    .filter(({ amount }) => !amount.isZero())
+    .map(stake => ({
+      ...stake,
+      percentage: stake.amount.mul(pctPrecision).div(total),


If I remember correctly, the reason I did the calculations with native numbers originally was that I hit problems with this type of arithmetic with BNs. Give it a go with http://aragon.aragonpm.com/#/4color.aragonid.eth/

Also fixes the detection of the rest item in the panel.

izqui

💫🔢 Amazing!

Just two minor things :)

izqui · 2018-10-05T15:14:09Z

apps/token-manager/app/src/components/Panels/AssignVotePanelContent.js

@@ -93,7 +95,7 @@ class AssignVotePanelContent extends React.Component {
            <TextInput.Number
              value={amount}
              onChange={this.handleAmountChange}
-              min={0}
+              min={minimum}
              step="any"


Step should also be minimum, to avoid people adding decimal precision that will be lost in the contract

izqui · 2018-10-05T15:29:20Z

apps/token-manager/app/src/utils.js

+  fraction = `${zeros}${fraction}`
+  const whole = amount.div(base).toString()
+
+  return `${whole}${fraction === '0' ? '' : `.${fraction.slice(0, 2)}`}`


It is better if we parse the fraction as a number and check if it is 0, otherwise when the fraction has more than 0 zero, the decimals are displayed. Had to change it here in radspec as well. We should probably move this to an utils lib somewhere as I was talking to @bpierre offline today.

Nice UI touch

bpierre · 2018-10-08T12:54:40Z

Merged, thank you @2color! 🎉 💯

2color · 2018-10-08T13:13:27Z

Thanks for following through! Nicely polished 🥇

Some related changes: - Add tests (Jest). - Support decimals. - New algorithm to calculate stake percentages (remove 0% values). - Sort holders table in descending order.

2color added 7 commits September 8, 2018 22:22

Add bn.js and pin to same version as aragon core

276646b

Convert balances and token supply to BNs

a499721

Calculate the tokenDecimalsBase in the state mapper

9b68f93

token-manager: calculate balance based with BN.js

cfef6ca

token-manager: Use BNs to call mint with string

82643b1

Leave big integers as strings

563d3fc

Fix percentage calculations

1d265d8

2color changed the title ~~Token-manager: use bn.js~~ [wip] Token-manager: use bn.js Sep 8, 2018

sohkai added this to the sprint: 1.3 milestone Sep 12, 2018

sohkai self-assigned this Sep 12, 2018

2color added 2 commits September 17, 2018 20:07

Remove unnecessary BN import

4d67591

Sort holders table in descending order

b8a7ff2

Match the order of the stake table

2color changed the title ~~[wip] Token-manager: use bn.js~~ Token-manager: use bn.js to cac Sep 17, 2018

2color changed the title ~~Token-manager: use bn.js to cac~~ Token-manager: use bn.js Sep 17, 2018

2color changed the title ~~Token-manager: use bn.js~~ Token-manager: use BN.js Sep 17, 2018

2color changed the title ~~Token-manager: use BN.js~~ Token-manager: use bn.js Sep 17, 2018

sohkai requested a review from bpierre September 17, 2018 19:14

izqui added app: token manager component: frontend labels Sep 18, 2018

cfl0ws assigned bpierre Sep 19, 2018

bpierre added 2 commits September 21, 2018 18:32

Merge master into 2color-use-bn

b970e4c

Support decimals

f996026

bpierre approved these changes Sep 24, 2018

View reviewed changes

bpierre requested a review from izqui September 24, 2018 10:35

cfl0ws mentioned this pull request Sep 25, 2018

(Lack of BN.js) Assigning large amounts of tokens does not work #266

Closed

3 tasks

token-manager: calclulate stakes with higher degree of accuracy

0692d4d

Fix calculation of the stakes percentages

027017c

2color commented Oct 4, 2018

View reviewed changes

apps/token-manager/app/src/components/SideBar.js Outdated Show resolved Hide resolved

2color commented Oct 4, 2018

View reviewed changes

Remove 0% values and group them in the “Rest” item

804760e

Also fixes the detection of the rest item in the panel.

bpierre force-pushed the use-bn branch from f1fd891 to 804760e Compare October 5, 2018 09:16

izqui approved these changes Oct 5, 2018

View reviewed changes

bpierre added 8 commits October 6, 2018 09:45

Do not display decimals when there are multiple 0

3e95756

Assign panel: prevent to set an unsupported decimal precision

8e96529

Add tests + fix a calculation issue

ad390cb

Tests formatting

3855445

formatBalance: add tests + fix some cases

a32afc7

Add Jest to the ESLint config

f9912c2

ESLint: use plugin:jest/recommended

889f10c

Run the token manager app tests

3c7f85b

bpierre merged commit f4ab93b into aragon:master Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token-manager: use bn.js #432

Token-manager: use bn.js #432

2color commented Sep 8, 2018 •

edited

Loading

CLAassistant commented Sep 8, 2018 •

edited

Loading

2color commented Sep 17, 2018

izqui commented Sep 21, 2018

cfl0ws commented Sep 21, 2018

bpierre commented Sep 24, 2018

coveralls commented Sep 24, 2018

coveralls commented Sep 24, 2018 •

edited

Loading

2color commented Sep 24, 2018 via email

izqui commented Sep 24, 2018

2color commented Sep 26, 2018

2color commented Sep 26, 2018

sohkai commented Sep 26, 2018

cfl0ws commented Oct 1, 2018

2color commented Oct 1, 2018

bpierre commented Oct 4, 2018

2color Oct 4, 2018

izqui left a comment •

edited

Loading

izqui Oct 5, 2018

izqui Oct 5, 2018

2color Oct 8, 2018

bpierre commented Oct 8, 2018

2color commented Oct 8, 2018

Token-manager: use bn.js #432

Token-manager: use bn.js #432

Conversation

2color commented Sep 8, 2018 • edited Loading

Approach

Notes

Extras

CLAassistant commented Sep 8, 2018 • edited Loading

2color commented Sep 17, 2018

izqui commented Sep 21, 2018

cfl0ws commented Sep 21, 2018

bpierre commented Sep 24, 2018

coveralls commented Sep 24, 2018

coveralls commented Sep 24, 2018 • edited Loading

2color commented Sep 24, 2018 via email

izqui commented Sep 24, 2018

2color commented Sep 26, 2018

2color commented Sep 26, 2018

sohkai commented Sep 26, 2018

cfl0ws commented Oct 1, 2018

2color commented Oct 1, 2018

bpierre commented Oct 4, 2018

2color Oct 4, 2018

Choose a reason for hiding this comment

izqui left a comment • edited Loading

Choose a reason for hiding this comment

izqui Oct 5, 2018

Choose a reason for hiding this comment

izqui Oct 5, 2018

Choose a reason for hiding this comment

2color Oct 8, 2018

Choose a reason for hiding this comment

bpierre commented Oct 8, 2018

2color commented Oct 8, 2018

2color commented Sep 8, 2018 •

edited

Loading

CLAassistant commented Sep 8, 2018 •

edited

Loading

coveralls commented Sep 24, 2018 •

edited

Loading

izqui left a comment •

edited

Loading