From 878942a80c2e48765adadcf0f24c8d639a569862 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Mon, 2 Aug 2021 14:56:30 +0700 Subject: [PATCH 1/2] fix: work around huge gas cost when reducing ICR of bottom Trove Fixes #659. --- .../src/PopulatableEthersLiquity.ts | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/packages/lib-ethers/src/PopulatableEthersLiquity.ts b/packages/lib-ethers/src/PopulatableEthersLiquity.ts index 65570f059..96b91b64b 100644 --- a/packages/lib-ethers/src/PopulatableEthersLiquity.ts +++ b/packages/lib-ethers/src/PopulatableEthersLiquity.ts @@ -693,7 +693,8 @@ export class PopulatableEthersLiquity } private async _findHintsForNominalCollateralRatio( - nominalCollateralRatio: Decimal + nominalCollateralRatio: Decimal, + ownAddress?: string ): Promise<[string, string]> { const { sortedTroves, hintHelpers } = _getContracts(this._readable.connection); const numberOfTroves = await this._readable.getNumberOfTroves(); @@ -733,21 +734,35 @@ export class PopulatableEthersLiquity const { hintAddress } = results.reduce((a, b) => (a.diff.lt(b.diff) ? a : b)); - const [prev, next] = await sortedTroves.findInsertPosition( + let [prev, next] = await sortedTroves.findInsertPosition( nominalCollateralRatio.hex, hintAddress, hintAddress ); - return prev === AddressZero ? [next, next] : next === AddressZero ? [prev, prev] : [prev, next]; + if (ownAddress) { + if (prev === ownAddress) { + prev = await sortedTroves.getPrev(prev); + } else if (next === ownAddress) { + next = await sortedTroves.getNext(next); + } + } + + if (prev === AddressZero) { + prev = next; + } else if (next === AddressZero) { + next = prev; + } + + return [prev, next]; } - private async _findHints(trove: Trove): Promise<[string, string]> { + private async _findHints(trove: Trove, ownAddress?: string): Promise<[string, string]> { if (trove instanceof TroveWithPendingRedistribution) { throw new Error("Rewards must be applied to this Trove"); } - return this._findHintsForNominalCollateralRatio(trove._nominalCollateralRatio); + return this._findHintsForNominalCollateralRatio(trove._nominalCollateralRatio, ownAddress); } private async _findRedemptionHints( @@ -775,7 +790,10 @@ export class PopulatableEthersLiquity partialRedemptionLowerHint ] = partialRedemptionHintNICR.isZero() ? [AddressZero, AddressZero] - : await this._findHintsForNominalCollateralRatio(decimalify(partialRedemptionHintNICR)); + : await this._findHintsForNominalCollateralRatio( + decimalify(partialRedemptionHintNICR) + // XXX: if we knew the partially redeemed Trove's address, we'd pass it here + ); return [ decimalify(truncatedLUSDamount), @@ -942,7 +960,7 @@ export class PopulatableEthersLiquity const currentBorrowingRate = decayBorrowingRate(0); const adjustedTrove = trove.adjust(normalizedParams, currentBorrowingRate); - const hints = await this._findHints(adjustedTrove); + const hints = await this._findHints(adjustedTrove, address); const { maxBorrowingRate, @@ -1139,7 +1157,7 @@ export class PopulatableEthersLiquity await stabilityPool.estimateAndPopulate.withdrawETHGainToTrove( { ...overrides }, compose(addGasForPotentialListTraversal, addGasForLQTYIssuance), - ...(await this._findHints(finalTrove)) + ...(await this._findHints(finalTrove, address)) ) ); } From 93b7b6c1097eb23555c6e85f2b5890bcb8e6d63d Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Tue, 3 Aug 2021 19:29:27 +0700 Subject: [PATCH 2/2] test: add test cases for hint-related workarounds --- .../src/PopulatableEthersLiquity.ts | 5 ++ packages/lib-ethers/test/Liquity.test.ts | 86 +++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/packages/lib-ethers/src/PopulatableEthersLiquity.ts b/packages/lib-ethers/src/PopulatableEthersLiquity.ts index 96b91b64b..193dc173f 100644 --- a/packages/lib-ethers/src/PopulatableEthersLiquity.ts +++ b/packages/lib-ethers/src/PopulatableEthersLiquity.ts @@ -741,6 +741,9 @@ export class PopulatableEthersLiquity ); if (ownAddress) { + // In the case of reinsertion, the address of the Trove being reinserted is not a usable hint, + // because it is deleted from the list before the reinsertion. + // "Jump over" the Trove to get the proper hint. if (prev === ownAddress) { prev = await sortedTroves.getPrev(prev); } else if (next === ownAddress) { @@ -748,6 +751,8 @@ export class PopulatableEthersLiquity } } + // Don't use `address(0)` as hint as it can result in huge gas cost. + // (See https://github.com/liquity/dev/issues/600). if (prev === AddressZero) { prev = next; } else if (next === AddressZero) { diff --git a/packages/lib-ethers/test/Liquity.test.ts b/packages/lib-ethers/test/Liquity.test.ts index eb6c7e453..48cd17665 100644 --- a/packages/lib-ethers/test/Liquity.test.ts +++ b/packages/lib-ethers/test/Liquity.test.ts @@ -988,6 +988,92 @@ describe("EthersLiquity", () => { }); }); + // Test workarounds related to https://github.com/liquity/dev/issues/600 + describe("Hints (adjustTrove)", () => { + let eightOtherUsers: Signer[]; + + before(async () => { + deployment = await deployLiquity(deployer); + eightOtherUsers = otherUsers.slice(0, 8); + liquity = await connectToDeployment(deployment, user); + + await openTroves(eightOtherUsers, [ + { depositCollateral: 30, borrowLUSD: 2000 }, // 0 + { depositCollateral: 30, borrowLUSD: 2100 }, // 1 + { depositCollateral: 30, borrowLUSD: 2200 }, // 2 + { depositCollateral: 30, borrowLUSD: 2300 }, // 3 + // Test 1: 30, 2400 + { depositCollateral: 30, borrowLUSD: 2500 }, // 4 + { depositCollateral: 30, borrowLUSD: 2600 }, // 5 + { depositCollateral: 30, borrowLUSD: 2700 }, // 6 + { depositCollateral: 30, borrowLUSD: 2800 } // 7 + // Test 2: 30, 2900 + // Test 2 (other): 30, 3000 + // Test 3: 30, 3100 -> 3200 + ]); + }); + + // Test 1 + it("should not use extra gas when a Trove's position doesn't change", async () => { + const { newTrove: initialTrove } = await liquity.openTrove({ + depositCollateral: 30, + borrowLUSD: 2400 + }); + + // Maintain the same ICR / position in the list + const targetTrove = initialTrove.multiply(1.1); + + const { rawReceipt } = await waitForSuccess( + liquity.send.adjustTrove(initialTrove.adjustTo(targetTrove)) + ); + + const gasUsed = rawReceipt.gasUsed.toNumber(); + expect(gasUsed).to.be.at.most(250000); + }); + + // Test 2 + it("should not traverse the whole list when bottom Trove moves", async () => { + const bottomLiquity = await connectToDeployment(deployment, eightOtherUsers[7]); + + const initialTrove = await liquity.getTrove(); + const bottomTrove = await bottomLiquity.getTrove(); + + const targetTrove = Trove.create({ depositCollateral: 30, borrowLUSD: 2900 }); + const interferingTrove = Trove.create({ depositCollateral: 30, borrowLUSD: 3000 }); + + const tx = await liquity.populate.adjustTrove(initialTrove.adjustTo(targetTrove)); + + // Suddenly: interference! + await bottomLiquity.adjustTrove(bottomTrove.adjustTo(interferingTrove), {}, { gasPrice: 0 }); + + const { rawReceipt } = await waitForSuccess(tx.send()); + + const gasUsed = rawReceipt.gasUsed.toNumber(); + expect(gasUsed).to.be.at.most(310000); + }); + + // Test 3 + it("should not traverse the whole list when lowering ICR of bottom Trove", async () => { + const initialTrove = await liquity.getTrove(); + + const targetTrove = [ + Trove.create({ depositCollateral: 30, borrowLUSD: 3100 }), + Trove.create({ depositCollateral: 30, borrowLUSD: 3200 }) + ]; + + await liquity.adjustTrove(initialTrove.adjustTo(targetTrove[0])); + // Now we are the bottom Trove + + // Lower our ICR even more + const { rawReceipt } = await waitForSuccess( + liquity.send.adjustTrove(targetTrove[0].adjustTo(targetTrove[1])) + ); + + const gasUsed = rawReceipt.gasUsed.toNumber(); + expect(gasUsed).to.be.at.most(240000); + }); + }); + describe("Gas estimation", () => { const troveWithICRBetween = (a: Trove, b: Trove) => a.add(b).multiply(0.5);