Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Uniswap on-chain liquidation #265

Merged
merged 12 commits into from
Oct 18, 2019
42 changes: 33 additions & 9 deletions implementation/contracts/deposit/DepositLiquidation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {TBTCConstants} from "./TBTCConstants.sol";
import {IBondedECDSAKeep} from "../external/IBondedECDSAKeep.sol";
import {OutsourceDepositLogging} from "./OutsourceDepositLogging.sol";
import {TBTCToken} from "../system/TBTCToken.sol";
import {IUniswapExchange} from "../external/IUniswapExchange.sol";
import {ITBTCSystem} from "../interfaces/ITBTCSystem.sol";

library DepositLiquidation {

Expand All @@ -20,13 +22,6 @@ library DepositLiquidation {
using DepositStates for DepositUtils.Deposit;
using OutsourceDepositLogging for DepositUtils.Deposit;

/// @notice Tries to liquidate the position on-chain using the signer bond
/// @dev Calls out to other contracts, watch for re-entrance
/// @return True if Liquidated, False otherwise
function attemptToLiquidateOnchain() public pure returns (bool) {
return false;
}

/// @notice Notifies the keep contract of fraud
/// @dev Calls out to the keep contract. this could get expensive if preimage is large
/// @param _d deposit storage pointer
Expand Down Expand Up @@ -92,7 +87,7 @@ library DepositLiquidation {
return;
}

bool _liquidated = attemptToLiquidateOnchain();
bool _liquidated = attemptToLiquidateOnchain(_d);

if (_liquidated) {
_d.distributeBeneficiaryReward();
Expand All @@ -115,7 +110,7 @@ library DepositLiquidation {
_d.redemptionTeardown();
_d.seizeSignerBonds();

bool _liquidated = attemptToLiquidateOnchain();
bool _liquidated = attemptToLiquidateOnchain(_d);

if (_liquidated) {
_d.distributeBeneficiaryReward();
Expand Down Expand Up @@ -329,4 +324,33 @@ library DepositLiquidation {
_d.logCourtesyCalled();
_d.courtesyCallInitiated = block.timestamp;
}

/// @notice Tries to liquidate the position on-chain using the signer bond
/// @dev Calls out to other contracts, watch for re-entrance
/// @return True if Liquidated, False otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of returning bool, it could return how much ETH it sent to Uniswap and/or how much TBTC it got in return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is @prestwich's code not mine - but I believe the intention is to decide whether it was successful within this block. The naming (attempt) is supposed to signal that we're attempting to instantly liquidate the entire position (as opposed to the ongoing auction flow), and such the binary RV of successful attempt or failure. What do you think, should it be improved?

Copy link
Member

Choose a reason for hiding this comment

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

This is our code - we propose it in the PR, we review it, and we accept or reject it. We all need to understand what is it doing 😉

Copy link
Member

Choose a reason for hiding this comment

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

Strongly agree with @pdyraga, though I imagine the man himself will take a peek when it's a little later PT

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 need to learn not to be more attentive with words haha - I like how this is implemented for now. @gakonst could you elaborate on why returning the amount might be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gakonst blocking, bump on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s not block on this, but capture it in a speculative issue and discuss/implement/close there.

// TODO(liamz): check for re-entry
function attemptToLiquidateOnchain(
DepositUtils.Deposit storage _d
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we’ve made a habit of this, but if so it’s time to break it: let’s use full words, not single letters, for our variable names.

) internal returns (bool) {
// Return early if there is no Uniswap TBTC Exchange.
IUniswapExchange exchange = IUniswapExchange(ITBTCSystem(_d.TBTCSystem).getTBTCUniswapExchange());
if(address(exchange) == address(0x0)) {
liamzebedee marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

// Only liquidate if we can buy up enough TBTC to burn,
// otherwise go 100% for the falling-price auction
uint tbtcAmount = _d.liquidationTBTCAmount();
uint ethAmount = exchange.getEthToTokenOutputPrice(tbtcAmount);
Copy link
Contributor

Choose a reason for hiding this comment

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

requiredEth or ethToLiquidate, perhaps? Not a huge deal.


if(address(this).balance < ethAmount) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use Uniswap only when we have enough funds to buy 1.0 TBTC? Can it be desirable that we buy some from Uniswap before slippage starts to become significant, and buy the rest of the TBTC via auction?

Copy link
Member

Choose a reason for hiding this comment

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

It's unclear what is a "good" price on Uniswap from our perspective, so this is a little tricky- can you propose pseudocode to make that decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump @gakonst, would like to hear your answer to this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump above @gakonst

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s punt here as well. Either we can capture it as a checkbox on #105, or as a separate issue, but I don’t think it needs to block this PR.


// Leverage uniswap’s frontrunning mitigation functionality.
uint deadline = block.timestamp;
exchange.ethToTokenSwapOutput.value(ethAmount)(tbtcAmount, deadline);

return true;
}
}
6 changes: 6 additions & 0 deletions implementation/contracts/deposit/DepositUtils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ library DepositUtils {
}
}

/// @notice Determines the threshold amount of TBTC necessary to liquidate
/// @return The amount of TBTC to buy during liquidation
function liquidationTBTCAmount(Deposit storage _d) public view returns (uint256) {
return TBTCConstants.getLotSize().add(beneficiaryReward());
}

/// @notice Determines the amount of TBTC accepted in the auction
/// @dev If requesterAddress is non-0, that means we came from redemption, and no auction should happen
/// @return The amount of TBTC that must be paid at auction for the signer's bond
Expand Down
1 change: 1 addition & 0 deletions implementation/contracts/interfaces/ITBTCSystem.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ interface ITBTCSystem {
function fetchRelayCurrentDifficulty() external view returns (uint256);
function fetchRelayPreviousDifficulty() external view returns (uint256);

function getTBTCUniswapExchange() external view returns (address);
}
89 changes: 88 additions & 1 deletion implementation/test/DepositLiquidationTest.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import expectThrow from './helpers/expectThrow'
import {
createSnapshot,
restoreSnapshot,
} from './helpers/snapshot'
import { AssertBalance } from './helpers/assertBalance'

const BytesLib = artifacts.require('BytesLib')
const BTCUtils = artifacts.require('BTCUtils')
Expand All @@ -13,13 +18,16 @@ const DepositRedemption = artifacts.require('DepositRedemption')
const DepositLiquidation = artifacts.require('DepositLiquidation')

const ECDSAKeepStub = artifacts.require('ECDSAKeepStub')
const KeepRegistryStub = artifacts.require('KeepRegistryStub')
const TestToken = artifacts.require('TestToken')
const TBTCSystemStub = artifacts.require('TBTCSystemStub')

const TestTBTCConstants = artifacts.require('TestTBTCConstants')
const TestDeposit = artifacts.require('TestDeposit')
const TestDepositUtils = artifacts.require('TestDepositUtils')

const UniswapExchangeStub = artifacts.require('UniswapExchangeStub')

const BN = require('bn.js')
const utils = require('./utils')
const chai = require('chai')
Expand Down Expand Up @@ -58,6 +66,15 @@ contract('DepositLiquidation', (accounts) => {
let beneficiary
let tbtcToken
let tbtcSystemStub
let uniswapExchange

before(async () => {
await createSnapshot()
})

after(async () => {
await restoreSnapshot()
})

before(async () => {
beneficiary = accounts[4]
Expand All @@ -73,11 +90,24 @@ contract('DepositLiquidation', (accounts) => {
testInstance.setExteriorAddresses(tbtcSystemStub.address, tbtcToken.address)

tbtcSystemStub.forceMint(beneficiary, web3.utils.toBN(deployed.TestDeposit.address))


const keepRegistry = await KeepRegistryStub.new()
uniswapExchange = await UniswapExchangeStub.new(tbtcToken.address)
await tbtcSystemStub.initialize(
keepRegistry.address,
uniswapExchange.address
)
})

beforeEach(async () => {
await testInstance.reset()
await testInstance.setKeepAddress(deployed.ECDSAKeepStub.address)
await createSnapshot()
})

afterEach(async () => {
await restoreSnapshot()
})

describe('purchaseSignerBondsAtAuction', async () => {
Expand Down Expand Up @@ -149,7 +179,6 @@ contract('DepositLiquidation', (accounts) => {

const finalTokenBalance = await tbtcToken.balanceOf(beneficiary)
const tokenCheck = new BN(initialTokenBalance).add(new BN(beneficiaryReward))

expect(finalTokenBalance, 'tokens not returned to beneficiary correctly').to.eq.BN(tokenCheck)
})

Expand Down Expand Up @@ -489,4 +518,62 @@ contract('DepositLiquidation', (accounts) => {
)
})
})

describe('#attemptToLiquidateOnchain', async () => {
let assertBalance
let deposit

beforeEach(async () => {
deposit = testInstance
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this assignment can't happen at the top level of the describe?


/* eslint-disable no-multi-spaces */
const ethSupply = web3.utils.toWei('0.2', 'ether') // 0.2 ETH
const tbtcSupply = new BN('1000000000') // 10 TBTC
/* eslint-enable */
await uniswapExchange.addLiquidity(
ethSupply, tbtcSupply, '0',
{ from: accounts[0], value: ethSupply }
)

// Helpers
assertBalance = new AssertBalance(tbtcToken)
})

it('returns false if address(exchange) = 0x0', async () => {
await tbtcSystemStub.reinitialize('0x0000000000000000000000000000000000000000')
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 be a revert rather than a false return? Not the test, but the actual behavior.


const retval = await deposit.attemptToLiquidateOnchain.call()
expect(retval).to.be.false
})

it('liquidates using Uniswap successfully', async () => {
const minTbtcAmount = '100100000'
const expectedPrice = new BN('100000000')

await assertBalance.eth(deposit.address, '0')
await assertBalance.tbtc(deposit.address, '0')
await deposit.send(expectedPrice, { from: accounts[0] })
await assertBalance.eth(deposit.address, expectedPrice.toString())

const retval = await deposit.attemptToLiquidateOnchain.call()
expect(retval).to.be.true
await deposit.attemptToLiquidateOnchain()

await assertBalance.tbtc(deposit.address, minTbtcAmount)
await assertBalance.eth(deposit.address, '0')
})

it('returns false if cannot buy up enough tBTC', async () => {
const expectedPrice = new BN('100000000')
const depositEthFunding = expectedPrice.sub(new BN(100))

await assertBalance.eth(deposit.address, '0')
await assertBalance.tbtc(deposit.address, '0')
await deposit.send(depositEthFunding, { from: accounts[0] })
await assertBalance.eth(deposit.address, depositEthFunding.toString())

const retval = await deposit.attemptToLiquidateOnchain.call()
expect(retval).to.be.false
})
})
})
6 changes: 6 additions & 0 deletions implementation/test/contracts/deposit/TBTCSystemStub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ contract TBTCSystemStub is TBTCSystem {
// solium-disable-previous-line no-empty-blocks
}

// Bypasses the ACL on TBTCSystem.initialize
// and allows repeat initialization.
function reinitialize(address _tbtcUniswapExchange) external {
tbtcUniswapExchange = _tbtcUniswapExchange;
}

function setOraclePrice(uint256 _oraclePrice) external {
oraclePrice = _oraclePrice;
}
Expand Down
4 changes: 4 additions & 0 deletions implementation/test/contracts/deposit/TestDeposit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,8 @@ contract TestDeposit is Deposit {
function pushFundsToKeepGroup(uint256 _ethValue) public returns (bool) {
return self.pushFundsToKeepGroup(_ethValue);
}

function attemptToLiquidateOnchain() public returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this and below are just publicly exposing private functionality?

If so, I wonder if we could come up with a naming convention for these to make reviews easier (not for this PR though).

return self.attemptToLiquidateOnchain();
}
}
50 changes: 50 additions & 0 deletions implementation/test/contracts/uniswap/UniswapExchangeStub.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
pragma solidity ^0.5.10;

import {TestToken} from '../deposit/TestToken.sol';
import {IUniswapExchange} from '../../../contracts/external/IUniswapExchange.sol';

contract UniswapExchangeStub is IUniswapExchange {
TestToken tbtc;

// The below returns an absurdly large price for tBTC
// such that attemptToLiquidateOnchain will return early, from not being funded enough
uint256 ethPrice = 10**8;

constructor(address _tbtc) public {
tbtc = TestToken(_tbtc);
}

function setEthPrice(uint256 _ethPrice) public {
ethPrice = _ethPrice;
}

function addLiquidity(uint256 min_liquidity, uint256 max_tokens, uint256 deadline)
external payable
returns (uint256)
{
require(msg.value > 0, "ETH missing from addLiquidity");
tbtc.forceMint(address(this), max_tokens);
// Stub doesn't implement the internal Uniswap token (UNI),
// so return 0 here for total minted UNI.
return 0;
}

function getEthToTokenOutputPrice(uint256 tokens_sold)
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing the under_scores here are a Uniswap disaster choice?

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 didn't respond here - this is actually because of Uniswap being coded in Vyper, a Pythonic language. Now that I'm thinking about it - I should've changed the style, since while the method name is part of the ABI definition, the parameter names aren't 🤦‍♂🤦‍♂🤦‍♂

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, that makes a lot of sense.

If you feel like it's fixable, we can always make that adjustment in one of your follow-up PRs.

external view
returns (uint256)
{
tokens_sold;
return ethPrice;
}

function ethToTokenSwapOutput(uint256 tokens_bought, uint256 deadline)
external payable
returns (uint256 eth_sold)
{
deadline;
require(msg.value == ethPrice, "incorrect eth sent");
require(tbtc.balanceOf(address(this)) >= tokens_bought, "not enough TBTC liquidity mocked");
tbtc.transfer(msg.sender, tokens_bought);
return msg.value;
}
}
21 changes: 21 additions & 0 deletions implementation/test/helpers/assertBalance.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const BN = require('bn.js')
const chai = require('chai')
const expect = chai.expect
const bnChai = require('bn-chai')
chai.use(bnChai(BN))

export class AssertBalance {
constructor(tbtc) {
this.tbtcInstance = tbtc
}

async tbtc(account, amount) {
const balance = await this.tbtcInstance.balanceOf(account)
expect(balance).to.eq.BN(amount)
}

async eth(account, amount) {
const balance = await web3.eth.getBalance(account)
expect(balance).to.equal(amount)
}
}
Loading