Uniswap integration #160

liamzebedee · 2019-06-09T19:31:14Z

Closes #105, closes #129

This deploys a private Uniswap Factory/Exchange as part of a Truffle migration, as well as implementing onchain liquidation.

It also refactors some liquidation code, introducing startLiquidation. There is also some changes to the liquidation logic, which are discussed in #105

--

Implementor: @liamzebedee
Feature buddy: @mhluongo

mhluongo · 2019-06-11T03:13:02Z

@liamzebedee does this also touch #129? Looks like you're deploying non-stub versions of Uniswap 😃

mhluongo

Eine kleine Frage

mhluongo · 2019-06-11T03:14:33Z

implementation/migrations/2_deploy_contracts.js

+const child_process = require('child_process')
+
+async function deployUniswap() {
+  if(uniswap.getDeployments().Factory != "") {


Is this enough to test for a deployment on mainnet, or just testnet? If so, we need to make sure we have a repeatable build, right?

Originally for testnet - this was just intended to work with unit tests (since Truffle redeploys for every test). Will get back to you on how suitable it is for mainnet.

liamzebedee · 2019-06-11T12:29:08Z

@liamzebedee does this also touch #129? Looks like you're deploying non-stub versions of Uniswap 😃

Haha was that not the intention? In which case, it would be closing #129 too. 🌳

…to onchain-liquidation

mhluongo · 2019-07-19T02:24:35Z

@liamzebedee where are we on a non-draft PR? This is on my list for tomorrow.

@prestwich can you give this another look? I'm going to double check on your requestor reward question

liamzebedee · 2019-07-19T09:59:14Z

@mhluongo last touches/cleanups, give it one hour. ⏳

- new ganache-cli has more detailed error msg's?

liamzebedee · 2019-07-19T12:56:24Z

Ready for design review.

Couple things still before we can undraft the PR:

there are ~10 tests failing now because attemptToLiquidateOnchain doesn't just return true like before, and balances are slightly different.
test attemptToLiquidateOnchain with real uniswap deployment - I forgot I swapped it out with a stub 😅. This won't take long, just have to add the 3% fee.
fix stupid Truffle things with Uniswap deployment (Migrating multiple contracts where a subsequent contract is deployed in a .then clause or after an await results in only first contract being deployed trufflesuite/truffle#650) - also a small task
merge from master, changing some paths

mhluongo

A couple JS changes, but the big issue is removing the unrelated stuff from the PR that's making review difficult- eg around funding proofs, pulling in the token contract, etc. Has enough of that stuff been merged to master for a clean-up rebase? If not, is there another branch that's a better base for this PR?

mhluongo · 2019-07-19T13:13:59Z

implementation/test/helpers/uniswap.js

+
+  static getDeadline() {
+    const DEADLINE_FROM_NOW = 300 // TX expires in 300 seconds (5 minutes)
+    const deadline = Math.ceil(Date.now() / 1000) + DEADLINE_FROM_NOW


This is a little obtuse- it looks like you're converting Date.now() to seconds to get a future deadline in seconds, but considering the proliferation of date, time, and math libs and approaches in JS I'd leave a comment about what deadline means

Mmmm true. Will doc it returns a seconds-since-epoch timestamp

mhluongo · 2019-07-19T13:15:02Z

implementation/test/helpers/assert_balance.js

+
+export class AssertBalanceHelpers {
+  constructor(tbtc) {
+    this.tbtcInstance = tbtc


The difference in constructor param name vs instance var name.. irks

I also don't personally think using the suffix instance anywhere is useful unless you're distinguishing it from a constructor, factory, or collection, though maybe in this case tbtcInstance is a lazy pseudo "type", reminding me sadly of Hungarian notation

Definitely Hungarian. Renaming constructor param sounds good.

mhluongo · 2019-07-19T13:20:08Z

implementation/test/UniswapDeploymentTest.js

+/**
+
+
+Some notes on the cryptoeconomics of Uniswap and TBTC, time arbitrage between chains


Oh my, I hope we have a better place for some of this- low discoverability

I think it'd be better to contribute to the Uniswap docs in that case. Had to read the Uniswap frontend JS to even get to this understanding... 😮

mhluongo · 2019-07-19T13:21:52Z

implementation/test/UniswapDeploymentTest.js

+  a. Uniswap 100% of ETH
+  b. Uniswap 50% ETH and 50% falling price auction
+
+I think it's worth documenting the additional complexity stemming from automatic liquidation. 3a won't necessarily buy up enough TBTC to burn, depending on the pool size, but the arbitrage incentives are going to keep the price stable to its price-oracle reported value (with a little delta).


3a won't necessarily buy up enough TBTC to burn, depending on the pool size

The plan was to only Uniswap if we can buy up enough TBTC to burn, otherwise to go 100% for the falling-price auction. Thoughts?

mhluongo · 2019-07-19T13:23:08Z

implementation/test/UniswapDeploymentTest.js

+
+
+      tbtcExchange = await IUniswapExchange.at(tbtcExchangeAddr)
+      /* eslint-enable no-unused-vars */


Why uh.. why?

If you just need the side-effect can't you await as a statement?

It's to create the contract from the interface ABI. IUniswapExchange is the interface contract. :)

mhluongo · 2019-07-19T13:28:22Z

Ready for design review.

I'll circle back to this when it's a little tidier- don't want to stick you with a bunch of irrelevant comments

mhluongo · 2019-07-23T21:46:14Z

@gakonst you might find this interesting

gakonst · 2019-08-15T17:13:36Z

What's the status of this PR?

mhluongo · 2019-08-15T17:15:15Z

Languishing while we finish up https://github.com/keep-network/tbtc-dapp @gakonst - would love review

liamzebedee · 2019-08-16T10:05:48Z

@gakonst warning, this one's a bit of a nakamofo. Design (esp tests) needs to be reduced down a bit more.

pdyraga · 2019-08-19T12:42:28Z

It looks like we have some rebase work to be done here. Can we do that before anyone jumps into review?

gakonst · 2019-08-22T08:17:54Z

Ping, please let me know when I should review this PR (@pdyraga mentions above that it should first be rebased)

pdyraga · 2019-08-22T08:38:09Z

@mhluongo Also interested if this is affected by the recent oracle discussions - should we review/merge or do we need something completely else?

mhluongo · 2019-08-23T10:32:24Z

This should not be impacted by the new oracle design. Let's rebase and clean up so @gakonst can review @liamzebedee

liamzebedee · 2019-08-26T12:15:24Z

Yep going to give this a cleanup on the plane today and we can push forward

liamzebedee · 2019-08-27T16:54:00Z

Ok @gakonst this issue spiralled out into a couple different changes, which I'm splitting into multiple PR's.

The 1st is the Uniswap deployment, which is ready for review here #263. The 2nd will include the rest of liquidation implementation. Was a bit sick, hence the delay.

liamzebedee · 2019-08-29T16:03:53Z

Closing this PR - the rest of work has been split into #265, which so far is a much simpler implementation with hindsight applied 😉

This issue breaks away work from #160 into something more deliverable: - introduces a script to deploy external systems, based on discussions in #270. Uniswap is deployed from a newly-created package at https://github.com/keep-network/uniswap - updates README with respect to this - includes Uniswap Solidity interfaces - separates unit and integration testing into two suites, new circleci flows for this - includes an E2E test with our TBTCToken of a Uniswap trade

liamzebedee added 13 commits June 9, 2019 21:18

Add Uniswap contracts as git submodule

8e774e7

Add README and Seth deploy script

b1489aa

Added Uniswap interface definitions in Sol

6e15c3b

Deploy script now uses separate files for addresses

46a8c96

Add TBTCSystemStub.setup and save uniswap factory

8b2ee5f

Rename Uniswap interfaces to more be more terse

f6fa7b3

Add TBTC ERC20 token and createExchange

9f89aa5

Uniswap deployment now automatic in Truffle migration

756b93f

Add openzeppelin 2.0 for token contracts

88eb55e

Extend TBTC from OpenZeppelin contracts and deploy

1fa9333

Fix TBTCSystemStub.setup (bad git- iquette)

8af7289

Fixed Uniswap migration to work with truffle tests

e47cb93

Test Uniswap and TBTC is deployed

63eb812

mhluongo reviewed Jun 11, 2019

View reviewed changes

liamzebedee added 14 commits June 11, 2019 21:29

Test for Uniswap e2e trade with TBTC

393cd00

Fix Uniswap deploy script env vars

f80d08e

Refactor uniswap deploys separate from TBTCSystem

8f49752

Add Remix command for debugging

48a16c6

Better design for storing/testing Uniswap deploys

603c3c9

Merge branch 'onchain-liquidation' of github.com:keep-network/tbtc in…

01fc708

…to onchain-liquidation

Uniswap migrated purely through Truffle now

3f2a224

KeepStub should implement IKeep

a2ca3f7

Add _d Deposit param to attemptToLiquidateOnchain

064fa4c

Expose attemptToLiquidateOnchain

5d3dec3

Failing test for attemptToLiquidateOnchain

d29d02b

Remember to include changes to Uniswap deployment

56445a8

Use TBTC instead of TBTCStub in uniswap test

9edf4d6

Create exchange in deployment only

0f5ea65

liamzebedee added 7 commits July 19, 2019 12:54

Leftover eth from liquidation

384b877

Add getTxCost helper

d2a01f2

Asserting that maintainer receives eth refund

7688c51

Cleanup

0cd33ad

Define maintainer for all tests

3378a52

Fix BN assertion

0a2f0de

Fix "has no liquidity by default" test

922bd68

- new ganache-cli has more detailed error msg's?

mhluongo suggested changes Jul 19, 2019

View reviewed changes

Introduced real Uniswap exchange in tests, calc fees

091bbc9

mhluongo requested a review from gakonst August 15, 2019 17:15

liamzebedee mentioned this pull request Aug 27, 2019

Uniswap deployment #263

Merged

liamzebedee mentioned this pull request Aug 28, 2019

Uniswap on-chain liquidation #265

Merged

liamzebedee closed this Aug 29, 2019

liamzebedee deleted the onchain-liquidation branch September 26, 2019 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uniswap integration #160

Uniswap integration #160

liamzebedee commented Jun 9, 2019 •

edited

Loading

mhluongo commented Jun 11, 2019

mhluongo left a comment

mhluongo Jun 11, 2019

liamzebedee Jun 11, 2019

liamzebedee commented Jun 11, 2019

mhluongo commented Jul 19, 2019

liamzebedee commented Jul 19, 2019

liamzebedee commented Jul 19, 2019 •

edited

Loading

mhluongo left a comment

mhluongo Jul 19, 2019

liamzebedee Jul 24, 2019 •

edited

Loading

mhluongo Jul 19, 2019

mhluongo Jul 19, 2019

liamzebedee Jul 24, 2019

mhluongo Jul 19, 2019

liamzebedee Jul 24, 2019

mhluongo Jul 19, 2019

mhluongo Jul 19, 2019

liamzebedee Jul 24, 2019

mhluongo commented Jul 19, 2019

mhluongo commented Jul 23, 2019

gakonst commented Aug 15, 2019

mhluongo commented Aug 15, 2019

liamzebedee commented Aug 16, 2019

pdyraga commented Aug 19, 2019

gakonst commented Aug 22, 2019

pdyraga commented Aug 22, 2019

mhluongo commented Aug 23, 2019 •

edited

Loading

liamzebedee commented Aug 26, 2019

liamzebedee commented Aug 27, 2019

liamzebedee commented Aug 29, 2019

		/**


		Some notes on the cryptoeconomics of Uniswap and TBTC, time arbitrage between chains



		tbtcExchange = await IUniswapExchange.at(tbtcExchangeAddr)
		/* eslint-enable no-unused-vars */

Uniswap integration #160

Uniswap integration #160

Conversation

liamzebedee commented Jun 9, 2019 • edited Loading

mhluongo commented Jun 11, 2019

mhluongo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee commented Jun 11, 2019

mhluongo commented Jul 19, 2019

liamzebedee commented Jul 19, 2019

liamzebedee commented Jul 19, 2019 • edited Loading

mhluongo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhluongo commented Jul 19, 2019

mhluongo commented Jul 23, 2019

gakonst commented Aug 15, 2019

mhluongo commented Aug 15, 2019

liamzebedee commented Aug 16, 2019

pdyraga commented Aug 19, 2019

gakonst commented Aug 22, 2019

pdyraga commented Aug 22, 2019

mhluongo commented Aug 23, 2019 • edited Loading

liamzebedee commented Aug 26, 2019

liamzebedee commented Aug 27, 2019

liamzebedee commented Aug 29, 2019

liamzebedee commented Jun 9, 2019 •

edited

Loading

liamzebedee commented Jul 19, 2019 •

edited

Loading

liamzebedee Jul 24, 2019 •

edited

Loading

mhluongo commented Aug 23, 2019 •

edited

Loading