Voting: use BN.js everywhere #1113

rperez89 · 2020-04-08T02:35:34Z

… contract itself

Co-authored-by: Pierre Bertet <[email protected]>

* Payroll: Fix accrued salary calculation * Payroll: Improve casting when calculating last payroll date

* Copy improvements * More specific copy * Reverting controversial change

* Change confusing token manager copy

* voting: fetch active account votes in script * Move connectedAccount out of the store and cache * Add comment about using strings over symbols * Move functions to helpers * Update comments Co-Authored-By: 2color <[email protected]> * Add newline

Improve the filters layout: - Move TransfersFilters to its own component. - Make the layout work at all sizes. - Update the download button so it looks like the other icon buttons. - Update some labels: - “Date range” => “Period” - “Transfer type” => “Type” (since it is already below the “Transfers” title). - Select the last 90 days (rolling) by default.

* Voting: fix bug with votes not loading initially * Add comment

Fixes aragon/client#734. With aragon/aragon.js#277 we now get the actual errors back from the RPC when a call goes wrong (e.g. naive `token.symbol()` on DAI) and we need to handle these explicitly. This PR converts a lot of the token fallback-related bits into simpler Promise-based code and adds explicit handlers for their error cases.

…alances tooltip (aragon#1121)

- Add a new TokenAmount class. - Add a new Percentage class. - Add bnPercentageToNumber(). - Add formatBnPercentage(). - Format amounts / percentages from the TokenAmount / Percentage classes. - Convert the calculations to BN.js. - Remove `numData` from the vote data structure. - Use formatTokenAmount().

bpierre · 2020-06-11T20:01:30Z

I added some changes to remove the Number-based calculations from the app, and rely on formatTokenAmount() to format amounts.

Changes:

Add a new TokenAmount class.
Add a new Percentage class.
Add bnPercentageToNumber().
Add formatBnPercentage().
Format amounts / percentages from the TokenAmount / Percentage classes.
Convert the calculations to BN.js.
Remove numData from the vote data structure.

Evalir

Very very minor nitpicks! This is looking awesome!

Evalir · 2020-06-23T00:35:54Z

apps/voting/app/src/components/SummaryRows.js

@@ -1,17 +1,16 @@
 import React from 'react'
 import styled from 'styled-components'
-import { GU, textStyle, useTheme } from '@aragon/ui'
+import { GU, textStyle, formatTokenAmount, useTheme } from '@aragon/ui'


We can move this behind textStyle so it's alphabetized 🙏

Evalir · 2020-06-23T00:37:28Z

apps/voting/app/src/app-state-reducer.js

@@ -1,5 +1,6 @@
 import BN from 'bn.js'
 import { hasLoadedVoteSettings } from './vote-settings'
+import { Percentage, TokenAmount } from './math-utils'


Hmm, we don't have to do it now, but I think we could use our newly released TokenAmount lib!

Evalir · 2020-06-23T00:38:43Z

apps/voting/app/src/hooks/useExtendedVoteData.js

+import { formatTokenAmount } from '@aragon/ui'
 import { useAragonApi } from '@aragon/api-react'


Now that I'm seeing the order, shouldn't we move @aragon/ui below @aragon/api-react?

Evalir · 2020-06-23T00:50:03Z

apps/voting/app/src/vote-utils.js


-  const totalVotes = yea.add(nay)
+  const totalVotes = yea.value().add(nay.value())


Just realized this probably could equal -2 if both yea and nay get passed in as -1, but, AFAIK, this can't really happen as they're not being set at -1 at any moment.

sohkai · 2020-06-25T12:43:52Z

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

-) {
-  if (!tokenContract || !connectedAccount) {
-    return -1
+export async function getUserBalanceAt(account, snapshotBlock, tokenContract) {


I wonder if we should also make these return BN objects (or null), so all the number-related APIs use BN.js?

sohkai · 2020-06-25T12:52:07Z

apps/voting/app/src/math-utils.js

+}
+
+// Converts a percentage expressed as a value + base into a number between 0 and 1.
+export function bnPercentageToNumber(value, base, precision = 10 ** 9) {


Just a bit worried with the precision being too big—wonder if we should use something like maxDecimalPrecision instead (just the number of decimals)?

sohkai · 2020-06-25T12:57:59Z

apps/voting/app/src/app-state-reducer.js

@@ -16,6 +17,7 @@ function appStateReducer(state) {
    connectedAccountVotes,
  } = state

+  const pctBaseBn = new BN(pctBase)
  const pctBaseNum = parseInt(pctBase, 10)


We can probably remove this now?

sohkai · 2020-06-25T13:01:46Z

apps/voting/app/src/app-state-reducer.js

+              supportRequired: new Percentage(data.supportRequired, pctBaseBn),
+              votingPower: new TokenAmount(data.votingPower, tokenDecimals),
+              yea: new TokenAmount(data.yea, tokenDecimals),
+              nay: new TokenAmount(data.nay, tokenDecimals),


What do you think about also including the token symbol in the TokenAmount?

sohkai · 2020-06-25T13:03:13Z

apps/voting/app/src/components/SummaryRows.js

 import You from './You'
-import { formatNumber } from '../math-utils'
 import { VOTE_NAY, VOTE_YEA } from '../vote-types'

 function SummaryRows({ yea, nay, symbol, connectedAccountVote }) {


Wanted to double check—what type is yea and nay? We're accessing yea.pct but I'm not sure this exists on the object anymore.

sohkai · 2020-06-25T13:04:52Z

apps/voting/app/src/components/VoteCard/VoteOptions.js

+
+  return (
+    parseInt(
+      divideRoundBigInt(


It's starting to look like we may want another utility :).

Maybe divideBN() and we can give it a decimalsPrecision option?

sohkai · 2020-06-25T13:25:24Z

apps/voting/app/src/math-utils.js

+
+  // Tolerate having too many digits by correcting the value.
+  if (basePrecision > Number.MAX_SAFE_INTEGER) {
+    digits = Math.floor(Math.log(Number.MAX_SAFE_INTEGER) / Math.log(10))


Not sure about this:

I think we should make this max precision value be a constant

This value is actually only 15, and and I think we should at least support 18 decimals (since that's what tokens use, as well as our contract's pctBase exponent).

It's unlikely someone will want something this large, but I'm also wondering if we shouldn't just cap the precision at the pctBase's exponent (since it'll just be 0s after that anyway).

I think we should make this max precision value be a constant

Why? This is already on the Javascript Number object, and so having this as a constant seems a bit redundant to me.

This value is actually only 15, and and I think we should at least support 18 decimals (since that's what tokens use, as well as our contract's pctBase exponent).

This is true, and relates to #1177 in terms of having an utility to safely scale up normal numbers to Bigints.

Why? This is already on the Javascript Number object, and so having this as a constant seems a bit redundant to me.

Because Math.floor(Math.log(Number.MAX_SAFE_INTEGER) / Math.log(10)) is pure and doesn't need to be calculated every time

eliobricenov · 2021-02-24T21:07:34Z

hey, I continued the last changes you requested here https://github.com/eliobricenov/aragon-apps/commits/voting-token-amount-fix-v2

Should I create a new PR with these changes?

C.C @nivida

sohkai and others added 30 commits April 16, 2019 09:47

chore: small clean up for published package (aragon#801)

5c155b1

chore: increase travis cache timeout (aragon#799)

102d82d

apps-finance: v3.0.0

a93af5a

apps-token-manager: v2.1.0

5ef7ff5

apps-vault: v4.1.0

2e31f2c

apps-voting: v2.1.0

5e2ea64

shared-test-helpers: v1.2.0

47f3926

Agent: allow tests to be shared (aragon#800)

6f89d84

apps-agent: v1.1.0

5ecd0ea

chore: update apps' dependency apps (aragon#802)

123f956

Finance: revert vault dependency upgrade since it doesn't compile the…

b6ee590

… contract itself

apps-agent: v1.1.1

826c931

feat: add more app center metadata (aragon#803)

98205ba

Co-authored-by: Pierre Bertet <[email protected]>

feat: upgrade to @aragon/[email protected] (aragon#804)

7dbce64

Copy improvements (aragon#805)

cfabec0

Payroll: Fix accrued salary calculation (aragon#798)

475b883

* Payroll: Fix accrued salary calculation * Payroll: Improve casting when calculating last payroll date

Payroll: Fix exchange rates request call (aragon#797)

57d8978

Copy improvements (aragon#795)

b2fcc4d

* Copy improvements * More specific copy * Reverting controversial change

Replace confusing Token Manager copy (aragon#807)

9b4bb77

* Change confusing token manager copy

Apps: Use shared TimeHelpersMock for tests (aragon#786)

92fd5e5

Voting: clarify polls (aragon#808)

231a43b

Payroll: Add reentracy guards (aragon#778)

eaf09e7

Finance: fix the download button (aragon#810)

0ddce97

Voting: fix bug with votes not loading initially (aragon#812)

0bf6ff4

* Voting: fix bug with votes not loading initially * Add comment

Finance: upgrade @aragon/templates-tokens (aragon#815)

0183acd

chore: remove unnecessary dependencies (aragon#817)

5eb9872

Apps: Tests enhancements (aragon#788)

65a998f

Andy Hook and others added 11 commits June 9, 2020 21:30

Survey, Token Manager: add prettier config (aragon#1155)

ced3640

Finance: update BN.js usage, use formatTokenAmount(), add rounded b…

b90ebc4

…alances tooltip (aragon#1121)

Finance: update verified token list and add Aave Tokens (aragon#1143)

3dc501b

Agent: update token list and add Aave tokens (aragon#1144)

4a47a4f

Tokens: Vesting UI (aragon#1137)

792959f

Voting: Multiple transaction steps (aragon#1152)

af250ff

Agent: use formatTokenAmount() and add rounding tooltip (aragon#1148)

9bcd8e1

Tokens: Replace formatBalance for formatTokenAmount (aragon#1158)

6249ade

Finance: fix withdrawal panel not detecting tokens (aragon#1159)

1753751

Merge branch 'master' into voting-token-amount-fix

348d1ba

bpierre force-pushed the voting-token-amount-fix branch from 9a6d907 to 7c2fdc1 Compare June 11, 2020 19:58

bpierre requested review from sohkai and removed request for bpierre June 11, 2020 20:01

bpierre added 2 commits June 11, 2020 21:08

Bump aragonUI to 1.4.2

bd5a698

Quorum progress: fix precision

996aecf

bpierre changed the title ~~Voting: Wrong token amount used to vote~~ Voting: use BN.js everywhere Jun 12, 2020

sohkai requested review from delfipolito and removed request for Evalir June 16, 2020 09:40

sohkai mentioned this pull request Jun 19, 2020

Switch out app utilities with AragonUI utilities #1166

Closed

3 tasks

Evalir approved these changes Jun 23, 2020

View reviewed changes

delfipolito mentioned this pull request Jun 25, 2020

Agreements voting app: 1 – New vote detail layout #1178

Merged

sohkai reviewed Jun 25, 2020

View reviewed changes

sohkai mentioned this pull request Jul 1, 2020

Voting: make amount of tokens always display 3 decimals #1112

Closed

eliobricenov mentioned this pull request Feb 24, 2021

Voting: amount of tokens shown that I voted with does not match real holdings aragon/client#1347

Open

eliobricenov mentioned this pull request Feb 26, 2021

Voting: use BN.js everywhere #1113 #1283

Closed

WissenIstNacht closed this Sep 29, 2021

WissenIstNacht force-pushed the master branch from 6e14239 to f7ef93b Compare September 29, 2021 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voting: use BN.js everywhere #1113

Voting: use BN.js everywhere #1113

rperez89 commented Apr 8, 2020 •

edited

Loading

bpierre commented Jun 11, 2020 •

edited

Loading

Evalir left a comment

Evalir Jun 23, 2020

Evalir Jun 23, 2020

Evalir Jun 23, 2020

Evalir Jun 23, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

sohkai Jun 25, 2020

Evalir Jun 26, 2020

sohkai Jun 26, 2020

eliobricenov commented Feb 24, 2021

		import { formatTokenAmount } from '@aragon/ui'
		import { useAragonApi } from '@aragon/api-react'


		const totalVotes = yea.add(nay)
		const totalVotes = yea.value().add(nay.value())

Voting: use BN.js everywhere #1113

Voting: use BN.js everywhere #1113

Conversation

rperez89 commented Apr 8, 2020 • edited Loading

bpierre commented Jun 11, 2020 • edited Loading

Evalir left a 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

eliobricenov commented Feb 24, 2021

rperez89 commented Apr 8, 2020 •

edited

Loading

bpierre commented Jun 11, 2020 •

edited

Loading