From ac49bf71fde28ef559beb77d5224aafc45f4c04e Mon Sep 17 00:00:00 2001 From: Matt de Haast Date: Thu, 22 Aug 2019 16:09:06 +0200 Subject: [PATCH 1/2] fix(balance) * Fix a bug in balance with minimum balance and outgoing packets --- src/middlewares/balance.ts | 12 ++++++++++++ test/middleware.test.js | 7 +------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/middlewares/balance.ts b/src/middlewares/balance.ts index f5516df1..718a5b53 100644 --- a/src/middlewares/balance.ts +++ b/src/middlewares/balance.ts @@ -54,6 +54,10 @@ class Balance { return this.balance } + getMinimum () { + return this.minimum + } + toJSON () { return { balance: this.balance.toString(), @@ -178,6 +182,14 @@ export default class BalanceMiddleware implements Middleware { // This means we always take the most conservative view of our balance with the upstream peer let result try { + + // Do a check before forwarding packet to see if it exceed minimum + const newBalance = balance.getValue().minus(parsedPacket.amount) + if (newBalance.lt(balance.getMinimum())) { + log.error('rejected balance update. oldBalance=%s newBalance=%s amount=%s', balance.getValue(), newBalance, parsedPacket.amount) + throw new Error(`insufficient funds. oldBalance=${balance.getValue()} proposedBalance=${newBalance}`) + } + result = await next(data) } catch (err) { log.debug('outgoing packet not applied due to error. accountId=%s amount=%s newBalance=%s', accountId, parsedPacket.amount, balance.getValue()) diff --git a/test/middleware.test.js b/test/middleware.test.js index 480520e4..a1ba38ee 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -335,13 +335,8 @@ describe('Middleware Manager', function () { destination: 'mock.test3.bob', data: Buffer.alloc(0) }) - const fulfillPacket = IlpPacket.serializeIlpFulfill({ - fulfillment: Buffer.from('HS8e5Ew02XKAglyus2dh2Ohabuqmy3HDM8EXMLz22ok', 'base64'), - data: Buffer.alloc(0) - }) - sinon.stub(this.mockPlugin3Wrapped, 'sendData') - .resolves(fulfillPacket) + sinon.stub(this.mockPlugin3Wrapped, 'sendData').throws() await this.middlewareManager.setup() const result = await this.mockPlugin1Wrapped._dataHandler(preparePacket) From f58aa2d9cee4248e8f27c5fd4f2246f1a7becbfd Mon Sep 17 00:00:00 2001 From: Matt de Haast Date: Thu, 22 Aug 2019 16:23:30 +0200 Subject: [PATCH 2/2] fix(balance) * change outgoing balance update logic --- package-lock.json | 69 ++++++++++++++++++++++---------------- src/middlewares/balance.ts | 12 ++----- 2 files changed, 43 insertions(+), 38 deletions(-) diff --git a/package-lock.json b/package-lock.json index 10b91627..b802dc78 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2426,7 +2426,8 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "aproba": { "version": "1.2.0", @@ -2456,7 +2457,7 @@ "dev": true, "optional": true, "requires": { - "balanced-match": "1.0.0", + "balanced-match": "^1.0.0", "concat-map": "0.0.1" } }, @@ -2574,7 +2575,7 @@ "dev": true, "optional": true, "requires": { - "safer-buffer": "2.1.2" + "safer-buffer": "^2.1.0" } }, "ignore-walk": { @@ -2583,7 +2584,7 @@ "dev": true, "optional": true, "requires": { - "minimatch": "3.0.4" + "minimatch": "^3.0.4" } }, "inflight": { @@ -2614,7 +2615,7 @@ "dev": true, "optional": true, "requires": { - "number-is-nan": "1.0.1" + "number-is-nan": "^1.0.0" } }, "isarray": { @@ -2629,18 +2630,20 @@ "dev": true, "optional": true, "requires": { - "brace-expansion": "1.1.11" + "brace-expansion": "^1.1.7" } }, "minimist": { "version": "0.0.8", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "minipass": { "version": "2.2.4", "bundled": true, "dev": true, + "optional": true, "requires": { "safe-buffer": "^5.1.1", "yallist": "^3.0.0" @@ -2659,6 +2662,7 @@ "version": "0.5.1", "bundled": true, "dev": true, + "optional": true, "requires": { "minimist": "0.0.8" } @@ -2682,9 +2686,9 @@ "dev": true, "optional": true, "requires": { - "debug": "2.6.9", - "iconv-lite": "0.4.21", - "sax": "1.2.4" + "debug": "^2.1.2", + "iconv-lite": "^0.4.4", + "sax": "^1.2.4" } }, "node-pre-gyp": { @@ -2711,8 +2715,8 @@ "dev": true, "optional": true, "requires": { - "abbrev": "1.1.1", - "osenv": "0.1.5" + "abbrev": "1", + "osenv": "^0.1.4" } }, "npm-bundled": { @@ -2727,8 +2731,8 @@ "dev": true, "optional": true, "requires": { - "ignore-walk": "3.0.1", - "npm-bundled": "1.0.3" + "ignore-walk": "^3.0.1", + "npm-bundled": "^1.0.1" } }, "npmlog": { @@ -2759,6 +2763,7 @@ "version": "1.4.0", "bundled": true, "dev": true, + "optional": true, "requires": { "wrappy": "1" } @@ -2781,8 +2786,8 @@ "dev": true, "optional": true, "requires": { - "os-homedir": "1.0.2", - "os-tmpdir": "1.0.2" + "os-homedir": "^1.0.0", + "os-tmpdir": "^1.0.0" } }, "path-is-absolute": { @@ -2803,10 +2808,10 @@ "dev": true, "optional": true, "requires": { - "deep-extend": "0.4.2", - "ini": "1.3.5", - "minimist": "1.2.0", - "strip-json-comments": "2.0.1" + "deep-extend": "~0.4.0", + "ini": "~1.3.0", + "minimist": "^1.2.0", + "strip-json-comments": "~2.0.1" }, "dependencies": { "minimist": { @@ -2844,7 +2849,8 @@ "safe-buffer": { "version": "5.1.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "safer-buffer": { "version": "2.1.2", @@ -2882,9 +2888,9 @@ "dev": true, "optional": true, "requires": { - "code-point-at": "1.1.0", - "is-fullwidth-code-point": "1.0.0", - "strip-ansi": "3.0.1" + "code-point-at": "^1.0.0", + "is-fullwidth-code-point": "^1.0.0", + "strip-ansi": "^3.0.0" } }, "string_decoder": { @@ -2893,15 +2899,16 @@ "dev": true, "optional": true, "requires": { - "safe-buffer": "5.1.1" + "safe-buffer": "~5.1.0" } }, "strip-ansi": { "version": "3.0.1", "bundled": true, "dev": true, + "optional": true, "requires": { - "ansi-regex": "2.1.1" + "ansi-regex": "^2.0.0" } }, "strip-json-comments": { @@ -2943,12 +2950,14 @@ "wrappy": { "version": "1.0.2", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "yallist": { "version": "3.0.2", "bundled": true, - "dev": true + "dev": true, + "optional": true } } }, @@ -4929,6 +4938,7 @@ "version": "0.1.4", "bundled": true, "dev": true, + "optional": true, "requires": { "kind-of": "^3.0.2", "longest": "^1.0.1", @@ -5739,7 +5749,8 @@ "longest": { "version": "1.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "loose-envify": { "version": "1.3.1", diff --git a/src/middlewares/balance.ts b/src/middlewares/balance.ts index 718a5b53..c2af410f 100644 --- a/src/middlewares/balance.ts +++ b/src/middlewares/balance.ts @@ -178,31 +178,25 @@ export default class BalanceMiddleware implements Middleware { return next(data) } - // We do nothing here (i.e. unlike for incoming packets) and wait until the packet is fulfilled - // This means we always take the most conservative view of our balance with the upstream peer let result try { - // Do a check before forwarding packet to see if it exceed minimum - const newBalance = balance.getValue().minus(parsedPacket.amount) - if (newBalance.lt(balance.getMinimum())) { - log.error('rejected balance update. oldBalance=%s newBalance=%s amount=%s', balance.getValue(), newBalance, parsedPacket.amount) - throw new Error(`insufficient funds. oldBalance=${balance.getValue()} proposedBalance=${newBalance}`) - } + balance.subtract(parsedPacket.amount) result = await next(data) } catch (err) { log.debug('outgoing packet not applied due to error. accountId=%s amount=%s newBalance=%s', accountId, parsedPacket.amount, balance.getValue()) this.stats.outgoingDataPacketValue.increment(account, { result : 'failed' }, +parsedPacket.amount) + balance.add(parsedPacket.amount) throw err } if (result[0] === IlpPacket.Type.TYPE_ILP_REJECT) { + balance.add(parsedPacket.amount) log.debug('outgoing packet not applied due to ilp reject. accountId=%s amount=%s newBalance=%s', accountId, parsedPacket.amount, balance.getValue()) this.stats.outgoingDataPacketValue.increment(account, { result : 'rejected' }, +parsedPacket.amount) } else if (result[0] === IlpPacket.Type.TYPE_ILP_FULFILL) { // Decrease balance on prepare - balance.subtract(parsedPacket.amount) this.maybeSettle(accountId).catch(log.error) log.trace('balance decreased due to outgoing ilp fulfill. accountId=%s amount=%s newBalance=%s', accountId, parsedPacket.amount, balance.getValue()) this.stats.balance.setValue(account, {}, balance.getValue().toNumber())