From fff5a6765ecc586bd855aa263c0fdfc3f957e8d7 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 7 Nov 2016 11:25:23 -0800 Subject: [PATCH 1/2] Added failing test for #787 --- test/unit/idStore-test.js | 52 ++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/test/unit/idStore-test.js b/test/unit/idStore-test.js index da465f511..34b0c992e 100644 --- a/test/unit/idStore-test.js +++ b/test/unit/idStore-test.js @@ -142,20 +142,49 @@ describe('IdentityStore', function() { }) describe('#addGasBuffer', function() { - const idStore = new IdentityStore({ - configManager: configManagerGen(), - ethStore: { - addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, - }, + it('formats the result correctly', function() { + const idStore = new IdentityStore({ + configManager: configManagerGen(), + ethStore: { + addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, + }, + }) + + const gas = '0x01' + const bnGas = new BN(gas, 16) + const result = idStore.addGasBuffer(gas) + const bnResult = new BN(result, 16) + + assert.ok(bnResult.gt(gas), 'added more gas as buffer.') + assert.equal(result.indexOf('0x'), 0, 'include hex prefix') }) - const gas = '0x01' - const bnGas = new BN(gas, 16) - const result = idStore.addGasBuffer(gas) - const bnResult = new BN(result, 16) + it('buffers reasonably', function() { + const idStore = new IdentityStore({ + configManager: configManagerGen(), + ethStore: { + addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, + }, + }) - assert.ok(bnResult.gt(gas), 'added more gas as buffer.') - assert.equal(result.indexOf('0x'), 0, 'include hex prefix') + const gas = '0x04ee59' // Actual estimated gas example + const tooBigOutput = '0x80674f9' // Actual bad output + const bnGas = new BN(gas, 16) + const correctBuffer = new BN('100000', 10) + const correct = bnGas.add(correctBuffer) + + const tooBig = new BN(tooBigOutput, 16) + console.log(`Pure estimate is ${bnGas.toString(10)}`) + console.log(`Too big is ${tooBig.toString(10)}`) + console.log(`Buffer should be ${correctBuffer.toString(10)}`) + console.log(`correct should be ${correct.toString(10)}`) + const result = idStore.addGasBuffer(gas) + const bnResult = new BN(result, 16) + + console.log(`Result was ${bnResult.toString(10)}`) + assert.equal(result, correct.toString(16), 'add the right amount') + assert.notEqual(result, tooBigOutput, 'not that bad estimate') + }) }) describe('#checkForDelegateCall', function() { @@ -169,4 +198,5 @@ describe('IdentityStore', function() { var result = idStore.checkForDelegateCall(delegateCallCode) assert.equal(result, true, 'no delegate call in provided code') }) + }) From 1896928562c728612caa7498ed82559a82a09aeb Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 7 Nov 2016 11:55:17 -0800 Subject: [PATCH 2/2] Fix gas price buffering Our gas price buffering logic had a bug, because bn.js has inconsistent behavior when using hex-prefixed output. The issue has been opened with them here: https://github.com/indutny/bn.js/issues/151 We've corrected our usage in the mean time. --- CHANGELOG.md | 1 + app/scripts/lib/idStore.js | 10 +++++----- test/unit/idStore-test.js | 14 ++++++-------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73fde7504..e75502159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Current Master +- Fix gas estimation bug. - Fix github link on info page to point at current repository. ## 2.13.6 2016-10-26 diff --git a/app/scripts/lib/idStore.js b/app/scripts/lib/idStore.js index 46d53c4e1..1010a5789 100644 --- a/app/scripts/lib/idStore.js +++ b/app/scripts/lib/idStore.js @@ -287,11 +287,11 @@ IdentityStore.prototype.checkForDelegateCall = function (codeHex) { } } -IdentityStore.prototype.addGasBuffer = function (gasHex) { - var gas = new BN(gasHex, 16) - var buffer = new BN('100000', 10) - var result = gas.add(buffer) - return ethUtil.addHexPrefix(result.toString(16)) +const gasBuffer = new BN('100000', 10) +IdentityStore.prototype.addGasBuffer = function (gas) { + const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) + const correct = bnGas.add(gasBuffer) + return ethUtil.addHexPrefix(correct.toString(16)) } // comes from metamask ui diff --git a/test/unit/idStore-test.js b/test/unit/idStore-test.js index 34b0c992e..46b3d4809 100644 --- a/test/unit/idStore-test.js +++ b/test/unit/idStore-test.js @@ -169,20 +169,18 @@ describe('IdentityStore', function() { const gas = '0x04ee59' // Actual estimated gas example const tooBigOutput = '0x80674f9' // Actual bad output - const bnGas = new BN(gas, 16) + const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) const correctBuffer = new BN('100000', 10) const correct = bnGas.add(correctBuffer) const tooBig = new BN(tooBigOutput, 16) - console.log(`Pure estimate is ${bnGas.toString(10)}`) - console.log(`Too big is ${tooBig.toString(10)}`) - console.log(`Buffer should be ${correctBuffer.toString(10)}`) - console.log(`correct should be ${correct.toString(10)}`) const result = idStore.addGasBuffer(gas) - const bnResult = new BN(result, 16) + const bnResult = new BN(ethUtil.stripHexPrefix(result), 16) - console.log(`Result was ${bnResult.toString(10)}`) - assert.equal(result, correct.toString(16), 'add the right amount') + assert.equal(result.indexOf('0x'), 0, 'included hex prefix') + assert(bnResult.gt(bnGas), 'Estimate increased in value.') + assert.equal(bnResult.sub(bnGas).toString(10), '100000', 'added 100k gas') + assert.equal(result, '0x' + correct.toString(16), 'Added the right amount') assert.notEqual(result, tooBigOutput, 'not that bad estimate') }) })