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

Token-manager: use bn.js #432

Merged
merged 22 commits into from Oct 8, 2018

Conversation

@2color
Copy link
Contributor

2color commented Sep 8, 2018

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

This comment has been minimized.

Copy link

CLAassistant commented Sep 8, 2018

CLA assistant check
All committers have signed the CLA.

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

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

@sohkai sohkai self-assigned this Sep 12, 2018

2color added some commits Sep 17, 2018

Sort holders table in descending order
Match the order of the stake table
@2color

This comment has been minimized.

Copy link
Contributor

2color commented Sep 17, 2018

b8a7ff2 was added to fix the following UI inconsistency:

screen shot 2018-09-17 at 20 25 18

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

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

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

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

@sohkai sohkai requested a review from bpierre Sep 17, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 21, 2018

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

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Sep 24, 2018

@2color Looking good! 💯

I added support for decimals.

@bpierre bpierre requested a review from izqui Sep 24, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 24, 2018

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 24, 2018

Coverage Status

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

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Sep 24, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 24, 2018

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

2018-09-24 at 5 41 18 pm

2018-09-24 at 5 56 12 pm

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Sep 26, 2018

screen shot 2018-09-26 at 16 08 40

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

This comment has been minimized.

Copy link
Contributor

2color commented Sep 26, 2018

The stake distribution bug has been resolved.
screen shot 2018-09-26 at 17 16 03

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 26, 2018

This is when minting 0.2 and the amountConverted is 200000000000000000

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

@chris-remus

This comment has been minimized.

Copy link

chris-remus commented Oct 1, 2018

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

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Oct 1, 2018

@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

This comment has been minimized.

Copy link
Member

bpierre commented Oct 4, 2018

@izqui The percentages should be fixed now

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

This comment has been minimized.

@2color

2color Oct 4, 2018

Contributor

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/

Remove 0% values and group them in the “Rest” item
Also fixes the detection of the rest item in the panel.

@bpierre bpierre force-pushed the 2color:use-bn branch from f1fd891 to 804760e Oct 5, 2018

@izqui

izqui approved these changes Oct 5, 2018

Copy link
Member

izqui left a comment

💫🔢 Amazing!

Just two minor things :)

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

This comment has been minimized.

@izqui

izqui Oct 5, 2018

Member

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

fraction = `${zeros}${fraction}`
const whole = amount.div(base).toString()

return `${whole}${fraction === '0' ? '' : `.${fraction.slice(0, 2)}`}`

This comment has been minimized.

@izqui

izqui Oct 5, 2018

Member

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.

screen shot 2018-10-05 at 17 27 10

This comment has been minimized.

@2color

2color Oct 8, 2018

Contributor

Nice UI touch

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

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.8%) to 96.212%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Oct 8, 2018

Merged, thank you @2color! 🎉 💯

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Oct 8, 2018

Thanks for following through! Nicely polished 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment