-
Notifications
You must be signed in to change notification settings - Fork 286
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
fix(core-transaction-pool-mem): sort transactions by numerical fee value #1288
Conversation
return -1 | ||
} | ||
if (a.transaction.fee < b.transaction.fee) { | ||
if (+bignumify(a.transaction.fee).toFixed() < +bignumify(b.transaction.fee).toFixed()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -218,10 +219,10 @@ class Mem { | |||
getTransactionsOrderedByFee () { | |||
if (!this.allIsSorted) { | |||
this.all.sort(function (a, b) { | |||
if (a.transaction.fee > b.transaction.fee) { | |||
if (+bignumify(a.transaction.fee).toFixed() > +bignumify(b.transaction.fee).toFixed()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov Report
@@ Coverage Diff @@
## develop #1288 +/- ##
===========================================
- Coverage 77.58% 77.55% -0.03%
===========================================
Files 397 397
Lines 6925 6925
Branches 952 952
===========================================
- Hits 5373 5371 -2
- Misses 1364 1366 +2
Partials 188 188
Continue to review full report at Codecov.
|
I resolved the test issues again this time. Next time run the tests locally before you send a PR and make sure they pass. |
@faustbrian Sorry, I just saw the bug but it was 2:20 AM here... |
In getTransactionsOrderedByFee()
a.transaction.fee
andb.transaction.fee
are BigNumber objects not numbers and the comparison fails.Checklist