Uniswap deployment #263

liamzebedee · 2019-08-27T16:30:50Z

Closes #129

This issue breaks away work from #160 into something more deliverable:

introduces a script to deploy external systems, based on discussions in Integration with external libraries - discussion #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

Running tests

Integration

cd scripts/
./deploy_uniswap.sh
cd ..
npm run integration-test

Unit

As before:

npm run test

…to onchain-liquidation

gakonst · 2019-09-15T14:29:30Z

implementation/contracts/system/TBTCSystem.sol


    uint256 currentDifficulty = 1;
    uint256 previousDifficulty = 1;
    uint256 oraclePrice = 10 ** 12;

+    address public tbtcUniswapExchange;
+
+    function initialize(address _tbtcUniswapExchange) external onlyOwner {


Why not use a constructor here? Is there going to be any proxy contract deployment using TBTCSystem?

@gakonst previously discussed this here - shame that github doesn't autoexpand all resolved items, otherwise it would've been simple to Ctrl+F the PR.

initialize was chosen as this is the pattern used in the KeepFactory. Since the current test migration setup uses utils.deploySystem, which doesn't deal with constructor params, it is also a bit less work. :)

Yeah, we'll have to revisit this when working on upgrades but I'd say it's fine for now - at least it's consistent with what we have in other places.

pdyraga · 2019-09-16T11:04:06Z

I think we are close, we need to sort out Circle configuration and figure out why integration_tests job still lists some unit tests.

Can we add an instruction to tthe PR description on how to run integration tests locally and how to run unit tests?

liamzebedee · 2019-09-16T11:22:41Z

@pdyraga take one more look at PR desc. - will add this to README if it's good.

pdyraga · 2019-09-16T11:26:07Z

✅ I was able to run unit tests locally without uniswap deployed
✅ I was not able to run integration tests locally without uniswap deployed
✅ I was able to deploy uniswap with deploy_uniswap.sh
🚫 I don't have my integration tests passing

> Compiled successfully using:
   - solc: 0.5.10+commit.5a6ea5b1.Emscripten.clang


  Integration tests
    Contract: Uniswap
      deployment
        ✓ created the tBTC UniswapExchange
      end-to-end trade with tBTC
        ✓ has no liquidity by default

        1) adds liquidity and trades ETH for tBTC

    Events emitted during test:
    ---------------------------

    Transfer(from: <indexed> 0x0000000000000000000000000000000000000000 (address), to: <indexed> 0xAECB81b304068b12D4DF2525C9A59cc2508Fb534 (address), value: 50000000000000000000 (uint256))
    Approval(owner: <indexed> 0xAECB81b304068b12D4DF2525C9A59cc2508Fb534 (address), spender: <indexed> 0x1f7dE91012609539E84768e8433389F23EE637Cb (address), value: 50000000000000000000 (uint256))

(...)

  2 passing (7s)
  1 failing

  1) Integration tests
       Contract: Uniswap
         end-to-end trade with tBTC
           adds liquidity and trades ETH for tBTC:
     Error: Returned error: VM Exception while processing transaction: invalid JUMP at 6050053677bbcd1e6911ca398710e15ed383256ac8424074a6d7e5c53fbf36ab/1f7de91012609539e84768e8433389f23ee637cb:39
      at Object.ErrorResponse (node_modules/truffle/build/webpack:/~/web3-core-requestmanager/~/web3-core-helpers/src/errors.js:29:1)
      at node_modules/truffle/build/webpack:/~/web3-core-requestmanager/src/index.js:140:1
      at node_modules/truffle/build/webpack:/packages/truffle-provider/wrapper.js:112:1
      at XMLHttpRequest.request.onreadystatechange (node_modules/truffle/build/webpack:/~/web3-providers-http/src/index.js:96:1)
      at XMLHttpRequestEventTarget.dispatchEvent (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request-event-target.js:34:1)
      at XMLHttpRequest._setReadyState (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:208:1)
      at XMLHttpRequest._onHttpResponseEnd (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:318:1)
      at IncomingMessage.<anonymous> (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:289:47)
      at endReadableNT (_stream_readable.js:1139:12)
      at processTicksAndRejections (internal/process/task_queues.js:81:17)



npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] integration-test: `NODE_ENV=test INTEGRATION_TEST=true truffle test`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] integration-test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/piotr/.npm/_logs/2019-09-16T11_23_56_729Z-debug.log

implementation/scripts/deploy_uniswap.sh

Shadowfiend · 2019-09-16T11:54:48Z

.circleci/config.yml

@@ -36,7 +36,7 @@ jobs:
              set -ex
              npm install
              npm run sol:lint
-  test_solidity:
+  unit_tests:


If these are still solidity, I suggest we include that (or contract or something) in the job name.

What do you think of leaving this until we do have tests in another language? I agree with the original naming, at the same time, I'm not sure if it adds anything to refer to contracts/solidity here - at least until we have other code (and notably, we've got it in tbtc-dapp for now).

< shrug > ultimately I don't care enough to block this particular PR as we'll border on bikeshedding, but I also don't think it's helpful to avoid detail in general. We already have lint_solidity vs lint_js, for example---doesn't seem crazy to follow the pattern vs trigger another “required jobs” setting dance when we decide we're ready to change the job name.

I take back my original comment - we do have other Kubes code written here, so it's not strictly true that this is only contract code. Will go ahead and change it.

The linting example helped with this ✌️ - linting addresses styling, which we have two targets for (sol/js). Testing addresses functionality, for which we also have two targets - contracts and infrastructure.

Producing a bug for now (possible race condition) and it's not necessarily needed

pdyraga · 2019-09-16T18:57:16Z

Still having the same problem when running integration tests after unit tests:

  Integration tests
    Contract: Uniswap
      deployment
        ✓ created the tBTC UniswapExchange
      end-to-end trade with tBTC
        ✓ has no liquidity by default

        1) adds liquidity and trades ETH for tBTC

    Events emitted during test:
    ---------------------------

    Transfer(from: <indexed> 0x0000000000000000000000000000000000000000 (address), to: <indexed> 0xAECB81b304068b12D4DF2525C9A59cc2508Fb534 (address), value: 50000000000000000000 (uint256))
    Approval(owner: <indexed> 0xAECB81b304068b12D4DF2525C9A59cc2508Fb534 (address), spender: <indexed> 0x9179A545B6A52C44f8d6FA941bFEF04944A6A33e (address), value: 50000000000000000000 (uint256))

(...)

  2 passing (6s)
  1 failing

  1) Integration tests
       Contract: Uniswap
         end-to-end trade with tBTC
           adds liquidity and trades ETH for tBTC:
     Error: Returned error: VM Exception while processing transaction: invalid JUMP at 02cec8c2e544b73319613bf45dca16e87a882815268ffa625de5b0fc37a7b9de/9179a545b6a52c44f8d6fa941bfef04944a6a33e:39
      at Object.ErrorResponse (node_modules/truffle/build/webpack:/~/web3-core-requestmanager/~/web3-core-helpers/src/errors.js:29:1)
      at node_modules/truffle/build/webpack:/~/web3-core-requestmanager/src/index.js:140:1
      at node_modules/truffle/build/webpack:/packages/truffle-provider/wrapper.js:112:1
      at XMLHttpRequest.request.onreadystatechange (node_modules/truffle/build/webpack:/~/web3-providers-http/src/index.js:96:1)
      at XMLHttpRequestEventTarget.dispatchEvent (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request-event-target.js:34:1)
      at XMLHttpRequest._setReadyState (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:208:1)
      at XMLHttpRequest._onHttpResponseEnd (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:318:1)
      at IncomingMessage.<anonymous> (node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:289:47)
      at endReadableNT (_stream_readable.js:1139:12)
      at processTicksAndRejections (internal/process/task_queues.js:81:17)

Scenario 1

started ganache-cli
deployed uniswap with ./deploy_uniswap.sh
ran integration-test, passed
ran integration-test again, passed

Scenario 2

started ganache-cli
ran test, passed
ran test, passed

Scenario 3

started ganache-cli
deployed uniswap with ./deploy_uniswap.sh
ran test, passed
ran integration-test, failed as above

pdyraga · 2019-09-17T16:03:58Z

✅ can run unit tests without uniswap deployed, passing
✅ can not run integration tests without uniswap deployed
✅ passing integration tests
✅ passing integration tests when run twice, and when run after unit tests
✅ passing unit tests when run after integration tests

pdyraga · 2019-09-17T16:05:35Z

Two last outstanding items and I am good to merge:

@nkuba since you requested some changed, do you want to take a look or are you fine with approval just from my side?

nkuba

Nice work! I left some minor comments

implementation/test/integration/UniswapTest.js

mhluongo · 2019-09-21T03:34:16Z

Sorry I'm late to the party, but it looks like the build is broken- any idea what's up?

nkuba · 2019-09-22T09:36:11Z

Sorry I'm late to the party, but it looks like the build is broken- any idea what's up?

Oh, I had the same when I ran it on my local env, but couldn't reproduce it. Seems like it wasn't just my messed up env, but happens on a very first run of the scripts.

I created a ticket for it: #296

mhluongo · 2019-09-22T11:11:05Z

🙇

liamzebedee added 30 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

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

Use setExteroriorAddresses instead of setup

8335067

Add clean command

baf4a9e

Move UniswapHelpers into separate file

cc28f50

gakonst reviewed Sep 15, 2019

View reviewed changes

pdyraga reviewed Sep 16, 2019

View reviewed changes

implementation/scripts/deploy_uniswap.sh Outdated Show resolved Hide resolved

liamzebedee added 2 commits September 16, 2019 12:37

Test edit for CI

117e180

Fix rm uniswap dir

af17265

Shadowfiend reviewed Sep 16, 2019

View reviewed changes

liamzebedee added 3 commits September 16, 2019 17:41

Simpler snapshot RPC call

42165d0

Fix Uniswap test to work with Truffle snapshot behaviour

ea41c9e

Remove snapshot functionality

3bd47dc

Producing a bug for now (possible race condition) and it's not necessarily needed

Fix UniswapHelpers bug with block.timestamp

89b6811

liamzebedee added 2 commits September 17, 2019 17:30

Fix script cleanup

ba9a8e5

More specific CircleCI job names

91514ad

pdyraga previously approved these changes Sep 18, 2019

View reviewed changes

nkuba suggested changes Sep 18, 2019

View reviewed changes

liamzebedee added 2 commits September 18, 2019 13:29

Add invalid JMP to expectThrow

041a709

tBTC -> TBTC (proper naming)

c591014

liamzebedee dismissed pdyraga’s stale review via c591014 September 18, 2019 16:53

nkuba approved these changes Sep 19, 2019

View reviewed changes

nkuba merged commit 46a5dfe into master Sep 19, 2019

nkuba deleted the uniswap-deployment branch September 19, 2019 07:41

liamzebedee mentioned this pull request Sep 19, 2019

Token inheritance fix #269

Merged

prestwich mentioned this pull request Oct 6, 2019

Implement on-chain liquidation #105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uniswap deployment #263

Uniswap deployment #263

liamzebedee commented Aug 27, 2019 •

edited

Loading

gakonst Sep 15, 2019

liamzebedee Sep 16, 2019

pdyraga Sep 16, 2019

pdyraga commented Sep 16, 2019

liamzebedee commented Sep 16, 2019

pdyraga commented Sep 16, 2019

Shadowfiend Sep 16, 2019

liamzebedee Sep 17, 2019

Shadowfiend Sep 17, 2019

liamzebedee Sep 17, 2019

pdyraga commented Sep 16, 2019

pdyraga commented Sep 17, 2019

pdyraga commented Sep 17, 2019

nkuba left a comment

mhluongo commented Sep 21, 2019

nkuba commented Sep 22, 2019

mhluongo commented Sep 22, 2019

Uniswap deployment #263

Uniswap deployment #263

Conversation

liamzebedee commented Aug 27, 2019 • edited Loading

Running tests

Integration

Unit

gakonst Sep 15, 2019

Choose a reason for hiding this comment

liamzebedee Sep 16, 2019

Choose a reason for hiding this comment

pdyraga Sep 16, 2019

Choose a reason for hiding this comment

pdyraga commented Sep 16, 2019

liamzebedee commented Sep 16, 2019

pdyraga commented Sep 16, 2019

Shadowfiend Sep 16, 2019

Choose a reason for hiding this comment

liamzebedee Sep 17, 2019

Choose a reason for hiding this comment

Shadowfiend Sep 17, 2019

Choose a reason for hiding this comment

liamzebedee Sep 17, 2019

Choose a reason for hiding this comment

pdyraga commented Sep 16, 2019

Scenario 1

Scenario 2

Scenario 3

pdyraga commented Sep 17, 2019

pdyraga commented Sep 17, 2019

nkuba left a comment

Choose a reason for hiding this comment

mhluongo commented Sep 21, 2019

nkuba commented Sep 22, 2019

mhluongo commented Sep 22, 2019

liamzebedee commented Aug 27, 2019 •

edited

Loading