Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Bug fix: Properly apply gasMultiplier to gasEstimates #2565

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

CruzMolina
Copy link
Contributor

Fixes a bad math bug (#2539)!

@CruzMolina CruzMolina requested a review from gnidan November 6, 2019 18:31
@coveralls
Copy link

Coverage Status

Coverage remained the same at 74.12% when pulling e43a5ae on fixGasMultiplier into a158745 on develop.

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QED? (and hope @haltman-at doesn't see my poor formal methods...)


return bignum.mul(secondOperand);
return numerator.div(denominator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my algebra is rusty, but I had to reason this out in vim for myself to make sense of it, so thought I'd share:

This PR changes this:            To this:

        round( d * 10^P )            n * round( d * 10^P )
v = n * -----------------        v = ---------------------
            10^P                              10^P

We're trying to do this:

v = n * d

Or, expressed as digit placeholders (with underscores as precision gaps):

VVVV.0 = NNNN.0 * D.DDD____

where:

  • v is the result we want
  • n is bignum,
  • d is decimal
  • P is significantFigures, i.e. some amount of precision

To [try to?] show how this might play out, the new code does steps:

  1. Scale for desired precision (multiply+round):

    D.DDD____  ->  DDDD.0
    
  2. Multiply by our bingum:

    NNNN.0 * DDDD.0 = VVVV____.0
    
  3. Then divide by original scale:

    VVVV____.0  ->  VVVV.____
    

No loss of precision.

To confirm why the old one was bad, the difference is that we scaled then immediately de-scaled:

  1. Scale:

    D.DDD____  ->  DDDD.0
    
  2. De-scale:

    DDDD.0  ->  D.0
    
  3. Then multiply:

    NNNN.0 * D.0 = VV__.0
    

OK, I'm convinced this is correct and that the old one was not correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Should this reasoning be a code comment? Would that spare future incredulity?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'l leave this convo open for a future Truffler. ⏲

@CruzMolina CruzMolina merged commit 60babc3 into develop Nov 7, 2019
@CruzMolina CruzMolina deleted the fixGasMultiplier branch November 7, 2019 16:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants