Skip to content
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

Precision from contract #78

Merged
merged 2 commits into from Sep 18, 2019

Conversation

@schmidsi
Copy link
Contributor

commented Sep 11, 2019

In contrast to the comment on #77:

I really like this! I would only push for having precision be set with a default (2 or 3), so it's fairly similar to the current behaviour, and using -1 or null to signify the precision as the token's decimal range.

I implemented the solution with the default precision set to undefined. It will break more tests if I go to the mentioned mode; precision set to 2 would break this for example:

[{
    source: 'Balance: `@tokenAmount(token, balance)`',
    bindings: { token: address('0x89d24a6b4ccb1b6faa2625fe562bdd9a23260359'), balance: int('10000000000000000000') }
  }, 'Balance: 10 DAI'],

Anyways, I'm happy to change the behaviour to something else.

schmidsi added 2 commits Sep 11, 2019
@coveralls

This comment has been minimized.

Copy link

commented Sep 11, 2019

Coverage Status

Coverage increased (+0.1%) to 91.219% when pulling ef71bcb on schmidsi:precision-from-contract into d49ddba on aragon:master.

@luisivan luisivan requested a review from izqui Sep 16, 2019
@izqui
izqui approved these changes Sep 16, 2019
@@ -4,18 +4,25 @@ export const tenPow = x => (
(new BN(10)).pow(new BN(x))
)

export const formatBN = (amount, base, precision) => {
export const formatBN = (amount, base, precision, fixed = false) => {

This comment has been minimized.

Copy link
@izqui

izqui Sep 16, 2019

Member
Suggested change
export const formatBN = (amount, base, precision, fixed = false) => {
export const formatBN = (amount, base, precision, fixed = true) => {

I think that keeping the default for fixed to true should make everything else that relies on formatBN(e.g. formatPct) working in a similar way as it was before.

This comment has been minimized.

Copy link
@schmidsi

schmidsi Sep 18, 2019

Author Contributor

When setting fixed to true by default, the following tests break:

 2 tests failed

  examples › examples › 58 - Change required support to `@formatPct(support)`%

  /Users/schmidsi/Development/@aragon/radspec/test/examples/examples.js:365

   364:     )        
   365:     t.is(    
   366:       actual,

  Expected "Change required support to `@formatPct(support)`%" to evaluate to "Change required support to 50%", but evaluated to "Change required support to 50.00%"

  Difference:

  - 'Change required support to 50.00%'
  + 'Change required support to 50%'



  examples › examples › 59 - Change required support to `@formatPct(support, 10 ^ 18, 1)`%

  /Users/schmidsi/Development/@aragon/radspec/test/examples/examples.js:365

   364:     )        
   365:     t.is(    
   366:       actual,

  Expected "Change required support to `@formatPct(support, 10 ^ 18, 1)`%" to evaluate to "Change required support to 40.4%", but evaluated to "Change required support to ~40.4%"

  Difference:

  - 'Change required support to ~40.4%'
  + 'Change required support to 40.4%'

I had backwards compatibility in mind and tried to change as little of the tests as possible. But I don't know about the projects where this package is used. Does defaulting fixed = true break less tests in your other repos?

This comment has been minimized.

Copy link
@izqui

izqui Sep 18, 2019

Member

It shouldn't break anything, no. I think it's fine going with false by default then

@izqui izqui merged commit 35a446d into aragon:master Sep 18, 2019
3 checks passed
3 checks passed
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@schmidsi schmidsi deleted the schmidsi:precision-from-contract branch Sep 19, 2019
return whole
}

return `${whole}.${fraction}`
const prefix = (new BN(slicedFraction).eq(new BN(fraction)) || !fixed) ? '' : '~'

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

Just a note here: !fixed || slicedFraction === fraction may have been easier to read. However, do we need the !fixed condition? Should we show the ~ whenever we truncate?

And slicedFraction could probably be renamed fractionAfterPrecision or something :).

@sohkai

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

I implemented the solution with the default precision set to undefined. It will break more tests if I go to the mentioned mode; precision set to 2 would break this for example:

Ah yes, I had originally thought that we would always truncate the trailing 0s ,and hence those mentioned tests wouldn't break, but I agree with how you've done it; the behaviour is more consistent :). If a user really just wants to send 1 wei, it's probably better to show 0.00....01 rather than ~0.000 in a UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.