Implement helper functions #42

izqui · 2018-09-20T14:08:07Z

Closes #40

Helper function structure is ready, need to implement the initial helpers we need now:

@tokenAmount(tokenAddr, amount, [showSymbol=true], [precision=2]): formats the token amount based on the token decimals.
@transformTime(time, toUnit, [fromUnit='seconds']): Transforms between time units.
@formatDate(timestamp, [dateFormat='MM-DD-YYYY']): Formats timestamp as a human readable date, supports passing the format for displaying the date.
@formatPct(number, [percentageBase=10^18], [precision=2]): Formats a number as a percentage

coveralls · 2018-09-21T12:34:09Z

Coverage increased (+0.4%) to 91.119% when pulling 035d1a2 on helpers into 80aeeb0 on master.

sohkai · 2018-09-27T16:28:41Z

package.json

@@ -33,6 +33,7 @@
  },
  "dependencies": {
    "bn.js": "4.11.6",
+    "moment": "^2.22.2",


Let's use date-fns@2.0.0-alpha.7 for now since that's what our other frontend projects are using.

moment.js is yuge.

sohkai · 2018-09-27T16:31:37Z

src/helpers/lib/token.js

+    "payable":false,
+    "stateMutability":"view",
+    "type":"function"
+  },


Given that we only use the .decimals() and .symbol() parts of the token, we could strip this down into just those.

See the Finance app as an example; it just creates an object out of those and gives it to web3.eth.Contract() as the ABI.

For Finance and TokenManager. Using helpers in aragon/radspec#42

izqui · 2018-09-27T18:33:39Z

@sohkai Moved to date-fns in c6cd403. I pinned to 2.0.0-alpha.7 as it seems that they are making changes to the formatting tokens in the newest alphas and those seem less standard than the ones used in alpha.7

sohkai · 2018-09-27T21:10:38Z

Hmm, just did npm ls date-fns on aragon/aragon and it looks like it's picking up 2.0.0-alpha.18 :(. @bpierre do you think we should pin everything to alpha.7 for now and just wait until the official 2.0.0 lands?

izqui · 2018-09-27T18:35:13Z

src/helpers/transformTime.js

+    }
+
+    const capitalize = s => s.charAt(0).toUpperCase() + s.slice(1)
+    const add = require(`date-fns/add${capitalize(fromUnit)}`)


Unsure if this dynamic require could cause us trouble

Pretty sure bundlers aren't going to like this :(. Best to avoid it and add it to a map or similar.

izqui · 2018-09-28T07:05:20Z

@sohkai I haven't checked what the motivation behind changing the formatting tokens is, but given that they are changing those in alpha versions we definitely need to pin it.

See the diff between alpha 7 and alpha 18. Updating how the formatting works will be quite disruptive as it will potentially break radspec strings.

Adapt to last changes in aragon/radspec#42

sohkai · 2018-10-01T08:49:18Z

Yeah I'm worried that aragon-ui or aragon/aragon's time based helpers are also broken due to the change :(.

It looks like it was mostly motivated by the unicode standard (TR35), but this looks like it's still getting updated.

Edit: Can confirm aragon-ui's Countdown element's formatting is broken (not really in a user-visible way though) due to this update, but nothing else should be (they don't use format or parse a lot).

I'm siding with moving to the TR35 notation, since it seems more standard across different languages, is a real spec, and moment's API is actually non-standard.

cfl0ws · 2018-10-01T14:22:32Z

@izqui What Epic(s) should this be added to?

izqui · 2018-10-02T07:48:50Z

@chris-remus all of the apps will benefit from this changes, specially Finance and the Token Manager, but i wouldn't say it is specific to any of the sprints we have

cfl0ws · 2018-10-05T13:38:04Z

@sohkai and @bpierre To make decision on "Yeah I'm worried that aragon-ui or aragon/aragon's time based helpers are also broken due to the change :( "

bpierre · 2018-10-06T07:12:21Z

src/helpers/lib/formatBN.js

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


It shouldn’t be a problem in modern JS engines, but it can still be a good idea to specify the radix as a second parameter of parseInt() for a while, just in case scripts are run on unsupported engines (e.g. old mobile browser or Node.js version) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Octal_interpretations_with_no_radix

sohkai · 2018-10-21T09:49:42Z

src/parser/index.js

+        node.inputs = this.functionInputs(astBody)
+      } else {
+        // There is actually no good reason not to allow calling a helper without ()
+        // this.report(`Expected '(' for executing helper function`)


@izqui should this be uncommented?

sohkai · 2018-10-21T12:27:53Z

@izqui I've updated this PR:

8a2e04c: Updates to use date-fns@2.0.0-beta.22, and use a more "natural" and unambiguous default time format (1/2/2018 could be read as "Jan. 2, 2018" or "Feb. 1, 2018" depending on your locale)
06c6811: I noticed a lot of different number and input assumptions in the helpers; while not a problem I hope this provides some sanity. Values required to be numbers are explicitly coerced using Number() and values required to be BN explicitly create new instances of BN. Note that bn.js applies a base of 10 by default as well.
6bc383f: Removes dynamic requires from transformTime(), and simplifies its implementation. (API change) also changes the unit tokens to just use the date-fns units.
b390a29: (API change) exports the default helpers directly onto the module, rather than under a defaultHelpers namespace; seems to better reflect how the rest of the modules are exported.

izqui · 2018-10-22T10:18:40Z

src/helpers/transformTime.js

      throw new Error(`@transformTime: Time unit ${fromUnit} is not supported as a fromUnit`)
    }

-    if (!SUPPORTED_TO_UNITS.has(toUnit)) {
+    if (toUnit && !ADD_UNIT_FN.has(toUnit)) {


I think we should have a special identifier for the 'best' unit, otherwise it won't be possible to format using that unit if a fromUnit is provided as well (as we cannot pass null from radspec)

izqui added 2 commits September 20, 2018 16:05

Scanner, parser, evaluator: Implement helper functions

8116071

Add example helpers

8ff3081

izqui self-assigned this Sep 20, 2018

izqui added this to the sprint: 1.3 milestone Sep 20, 2018

izqui added the enhancement label Sep 20, 2018

Make helpers a class and provide eth object to helpers

0d7c770

izqui force-pushed the helpers branch from 092067f to e981fe1 Compare September 21, 2018 12:12

Implement tokenAmount helper

b920e9f

izqui force-pushed the helpers branch from e981fe1 to b920e9f Compare September 21, 2018 12:14

izqui added 2 commits September 21, 2018 14:29

Lint

2204535

Fix only testing for helper cases

274449c

izqui added 6 commits September 21, 2018 14:42

Implement formatDate helper

8e22222

Implement transformTime helper

4028cdf

Move formatBN logic to its own file

dc0de7f

Implement formatPct helper

f011f3d

Lint

625fa3e

Document helper functions

b140217

izqui changed the title ~~[WIP] Implement helper functions~~ Implement helper functions Sep 24, 2018

izqui requested a review from sohkai September 24, 2018 17:21

Style fix

f670e04

sohkai reviewed Sep 27, 2018

View reviewed changes

bingen pushed a commit to aragon/aragon-apps that referenced this pull request Sep 27, 2018

Use new radspec helpers

3195745

For Finance and TokenManager. Using helpers in aragon/radspec#42

izqui added 2 commits September 27, 2018 19:31

Strip down ERC20 ABI to only have the functions that we use

03c8478

Migrate from moment.js to date-fns

c6cd403

izqui commented Sep 28, 2018

View reviewed changes

bingen pushed a commit to aragon/aragon-apps that referenced this pull request Sep 28, 2018

Use new radspec helpers

26c750c

Adapt to last changes in aragon/radspec#42

bpierre reviewed Oct 6, 2018

View reviewed changes

izqui and others added 4 commits October 8, 2018 12:01

Add explicit number base

973616d

Improve formatBN

4352ea7

Merge branch 'master' into helpers

fde131c

Fix linting

8c92fa8

sohkai reviewed Oct 21, 2018

View reviewed changes

sohkai added 5 commits October 21, 2018 14:10

Update and pin date-fns to 2.0.0-beta.22

8a2e04c

Refactor number assumptions and conversion in helper functions

06c6811

Refactor transformTime helper to not use dynamic requires

6bc383f

Export default helpers directly in helpers module

b390a29

Code style

2b02323

sohkai force-pushed the helpers branch from eca2008 to 2b02323 Compare October 21, 2018 12:24

izqui commented Oct 23, 2018

View reviewed changes

izqui and others added 2 commits October 23, 2018 09:12

Make best toUnit an explicit parameter

fa9943c

test: add example for using 'best' in transformTime

035d1a2

sohkai merged commit e1decc6 into master Oct 23, 2018

sohkai deleted the helpers branch October 23, 2018 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement helper functions #42

Implement helper functions #42

izqui commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 21, 2018 •

edited

Loading

sohkai Sep 27, 2018

sohkai Sep 27, 2018

izqui commented Sep 27, 2018

sohkai commented Sep 27, 2018

izqui Sep 27, 2018

sohkai Oct 1, 2018

izqui commented Sep 28, 2018 •

edited

Loading

sohkai commented Oct 1, 2018 •

edited

Loading

cfl0ws commented Oct 1, 2018

izqui commented Oct 2, 2018

cfl0ws commented Oct 5, 2018

bpierre Oct 6, 2018

sohkai Oct 21, 2018

sohkai commented Oct 21, 2018

izqui Oct 22, 2018

Implement helper functions #42

Implement helper functions #42

Conversation

izqui commented Sep 20, 2018 • edited Loading

coveralls commented Sep 21, 2018 • edited Loading

sohkai Sep 27, 2018

Choose a reason for hiding this comment

sohkai Sep 27, 2018

Choose a reason for hiding this comment

izqui commented Sep 27, 2018

sohkai commented Sep 27, 2018

izqui Sep 27, 2018

Choose a reason for hiding this comment

sohkai Oct 1, 2018

Choose a reason for hiding this comment

izqui commented Sep 28, 2018 • edited Loading

sohkai commented Oct 1, 2018 • edited Loading

cfl0ws commented Oct 1, 2018

izqui commented Oct 2, 2018

cfl0ws commented Oct 5, 2018

bpierre Oct 6, 2018

Choose a reason for hiding this comment

sohkai Oct 21, 2018

Choose a reason for hiding this comment

sohkai commented Oct 21, 2018

izqui Oct 22, 2018

Choose a reason for hiding this comment

izqui commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 21, 2018 •

edited

Loading

izqui commented Sep 28, 2018 •

edited

Loading

sohkai commented Oct 1, 2018 •

edited

Loading