Uniswap on-chain liquidation #265

liamzebedee · 2019-08-28T16:06:43Z

This PR uses our deployed Uniswap to implement on-chain liquidation in attemptToLiquidateOnchain. Another breakaway from a monolithic PR, #160, this changeset is simpler and doesn't change any protocol/liquidation logic.

adds Uniswap exchange stubs
implements onchain liquidation functionality in attemptToLiquidateOnchain
tests attemptToLiquidateOnchain
- using a stub
- using real Uniswap
introduces Ganache snapshots to cleanly revert state between unit tests, without additional resetState stub methods

Refs #105

liamzebedee · 2019-09-02T11:22:56Z

Busy fixing tests

implementation/contracts/deposit/DepositLiquidation.sol

gakonst · 2019-09-03T09:44:01Z

implementation/contracts/deposit/DepositLiquidation.sol

+            return false;
+        }
+
+        uint tbtcAmount = TBTCConstants.getLotSize().add(DepositUtils.beneficiaryReward());


Wouldn't it be better to call ERC20(_d.TBTCToken).balance(address(this)) instead of getLotSize? It can be the case that the Deposit receives TBTC during its lifetime.

Fuaaa - @mhluongo what's our approach for dirty flows like this?

@mhluongo bump on this protocol design question?

Do we design for unexpected behaviour (burn entire balance) or expected behaviour (ignore random tBTC and deal with protocol constants)?

gakonst · 2019-09-03T09:56:26Z

implementation/contracts/deposit/DepositLiquidation.sol

+    /// @notice         Starts signer liquidation due to fraud
+    /// @dev            We first attempt to liquidate on chain, then by auction
+    /// @param  _d      deposit storage pointer
+    function startSignerFraudLiquidation(DepositUtils.Deposit storage _d) public {


Shouldn't this be internal?

Good catch!

gakonst · 2019-09-03T09:56:32Z

implementation/contracts/deposit/DepositLiquidation.sol

+    /// @notice         Starts signer liquidation due to abort or undercollateralization
+    /// @dev            We first attempt to liquidate on chain, then by auction
+    /// @param  _d      deposit storage pointer
+    function startSignerAbortLiquidation(DepositUtils.Deposit storage _d) public {


Shouldn't this be internal?

gakonst · 2019-09-03T09:58:11Z

implementation/contracts/deposit/DepositLiquidation.sol

+            TBTCToken _tbtc = TBTCToken(_d.TBTCToken);
+
+            if (_d.requesterAddress != address(0)) { // redemption
+                _tbtc.transferFrom(address(this), _d.requesterAddress, TBTCConstants.getLotSize());


Same as above, this should probably be the deposit's ERC20 balance instead of getLotSize, in case there's any leftovers (or maybe the lotSize changed somehow)

gakonst · 2019-09-03T10:00:19Z

implementation/contracts/deposit/DepositLiquidation.sol

+
+    /// @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


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

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?

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 😉

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

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?

@gakonst blocking, bump on this

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

gakonst · 2019-09-03T10:01:31Z

implementation/contracts/deposit/DepositLiquidation.sol

+            return false;
+        }
+
+        uint deadline = block.timestamp + 1;


Given that all these transactions get executed together, the +1 shouldn't be required. Also might be good to add a comment so that a reader can understand the purpose of deadline in the context of uniswap (ie to ensure that the order does not get withheld by miners and submitted later)

Yeah good explanation (frontrunning?).

gakonst · 2019-09-03T10:04:48Z

implementation/contracts/deposit/DepositLiquidation.sol

+
+        if(address(this).balance < ethAmount) {
+            return false;
+        }


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?

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?

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

Bump above @gakonst

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.

gakonst · 2019-09-03T10:06:53Z

implementation/contracts/deposit/DepositLiquidation.sol

+
+            TBTCToken _tbtc = TBTCToken(_d.TBTCToken);
+
+            if (_d.requesterAddress != address(0)) { // redemption


I find overloading requesterAddress == 0 to act as a flag to signal the burning of the TBTC to be slightly unclear (& potentially dangerous). Might be better to add a shouldLiquidate flag on the Deposit and prohibit requesterAddress from taking 'invalid' values.

gakonst · 2019-09-03T10:08:07Z

implementation/contracts/deposit/DepositLiquidation.sol

+            _d.distributeBeneficiaryReward();
+
+        } else if (!_liquidated) {
+            _d.liquidationInitiated = block.timestamp;  // Store the timestamp for auction


Will any auction related logic be here, or will it be implemented as separate functionality that checks the liduidationInitiated value?

Yeah I believe it's used in auctionValue

Should we be emitting* some sort of event here to indicate that we are initiating an auction? Doesn’t feel necessary for this PR, but asking while I’m looking at it.

gakonst · 2019-09-03T10:11:43Z

implementation/test/DepositFraudTest.js

+    const uniswapFactory = deployed.UniswapFactoryStub
+    const uniswapExchange = await UniswapExchangeStub.new(tbtcToken.address)
+    await uniswapFactory.setExchange(uniswapExchange.address)
+    await deployed.TBTCSystemStub.setExternalAddresses(uniswapFactory.address)


Are these used anywhere in the test?

gakonst · 2019-09-03T10:16:48Z

implementation/test/contracts/uniswap/UniswapExchangeStub.sol

+        returns (uint256 eth_sold)
+    {
+        deadline;
+        require(msg.value     == ethPrice, "incorrect eth sent");


Suggested change

require(msg.value == ethPrice, "incorrect eth sent");

require(msg.value == ethPrice, "incorrect eth sent");

gakonst · 2019-09-03T10:19:01Z

implementation/test/DepositLiquidationTest.js

+        const ev = evs[1]
+        expect(ev.event).to.equal('Transfer')
+        expect(ev.returnValues.from).to.equal(deposit.address)
+        expect(ev.returnValues.to).to.equal('0x0000000000000000000000000000000000000000')


indentation

nkuba · 2019-09-19T07:43:08Z

@liamzebedee can we update this branch with the latest changes from master?

pdyraga · 2019-09-26T13:35:33Z

implementation/test/integration/UniswapTest.js

@@ -141,4 +141,128 @@ integration('Uniswap', (accounts) => {
      expect(balance).to.eq.BN(TBTC_BUY_AMOUNT)
    })
  })
+
+  // async function addLiquidity(ethAmount, tbtcAmount) {


What's happening here?

It's an additional test from the previous iteration of this PR. Turns out I had blended in unit with integration tests, so moving it out into this file where it belongs

liamzebedee · 2019-09-30T10:34:33Z

@pdyraga @nkuba ready for another pass!

pdyraga · 2019-09-30T11:13:06Z

Is it the last piece we need or do we expect more Uniswap PRs?

Shadowfiend · 2019-10-03T14:13:50Z

implementation/demo/Dockerfile

@@ -0,0 +1,82 @@
+# Mount 3 volumes


What is this file? Looks like it stumbled in during a merge…

Oops! Been fiddling with more Git branches than usual today.

Hey @Shadowfiend - was that the entire review?

No, but don't let that stop you from fixing this issue while I get the time to do the deeper review 😉

Since I'm not fully up on the system design context involving Uniswap here this'll probably take me a touch longer than a normal review. Easiest way to make sure that goes smoothly would be to make sure your PR or referenced issue description clearly lays out the goal, design, etc. Sloan's PRs (e. and g.*) are a solid standard in that department. When I looked through the issue it seemed like a lot of discussion happened but the action plan wasn't clear.

Either way, I'll be circling back Monday with more.

Okay, done my best to do a writeup in the way of Sloan, explaining the context, important touch points and some design tradeoffs. Also touched up the PR description with some more details. Let's get this delivered as smoothly as possible 💯!

Shadowfiend

Did a first pass that included everything except the testS. Shooting this out now so you can start having a look at it, but I’ll be continuing to dig in on the testing site as well. Generally looks good, most of the comments are either cosmetic, grammatical, or designated for future work. I also went ahead and called out a couple of things that were presented as blocking that I think we can punt to future work.

Shadowfiend · 2019-10-09T12:17:08Z

implementation/contracts/deposit/DepositLiquidation.sol

@@ -220,21 +161,21 @@ library DepositLiquidation {

        // Burn the outstanding TBTC
        TBTCToken _tbtcToken = TBTCToken(_d.TBTCToken);
-        require(_tbtcToken.balanceOf(msg.sender) >= TBTCConstants.getLotSize(), "Not enough TBTC to cover outstanding debt");
+        require(_tbtcToken.balanceOf(msg.sender) >= _d.redemptionTBTCAmount(), "Not enough TBTC to cover outstanding debt");


Not sure I understand this commit: 0bceca0

We don’t seem to change redemptionTBTCAmount despite saying we do, and it’s not clear why we switched from lot sizes to the redemption amount* (though it seems like the* obviously right choice). Why was it different before?

We do change it on this line, removing signerFee?

0bceca0#diff-eb5ee13eadec1a8b974e3eba934eb2d7L293

And I don't know why it was different before 🤷‍♂

Hah, whoops. Thought it was a property rather than a method and thus missed the actual change 🤦‍♂️

Usually a good idea to double-check the whys on stuff like this (and then capture it in the commit)---it's a good opportunity to let the original author also be aware of the update, and make sure there isn't some missed understanding that led to the original implementation. Again, not trying to block this PR/not extra-worried here, but flagging it for the future.

Shadowfiend · 2019-10-09T12:21:26Z

implementation/contracts/deposit/DepositLiquidation.sol

+
+    /// @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


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

Shadowfiend · 2019-10-09T13:04:55Z

implementation/contracts/deposit/DepositLiquidation.sol

+    /// @return     True if Liquidated, False otherwise
+    // TODO(liamz): check for re-entry
+    function attemptToLiquidateOnchain(
+        DepositUtils.Deposit storage _d


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.

Shadowfiend · 2019-10-09T13:06:16Z

implementation/contracts/deposit/DepositLiquidation.sol

+    function attemptToLiquidateOnchain(
+        DepositUtils.Deposit storage _d
+    ) internal returns (bool) {
+        // Return early if there is no Uniswap TBTC Exchange


Periods are good on sentences 😉

Shadowfiend · 2019-10-09T13:07:31Z

implementation/contracts/deposit/DepositLiquidation.sol

+        }
+
+        // Only liquidate if we can buy up enough TBTC to burn,
+        // otherwise to go 100% for the falling-price auction


Otherwise go*

Shadowfiend · 2019-10-09T13:37:26Z

implementation/contracts/deposit/DepositLiquidation.sol

+            _d.distributeBeneficiaryReward();
+
+        } else if (!_liquidated) {
+            _d.liquidationInitiated = block.timestamp;  // Store the timestamp for auction


Should we be emitting* some sort of event here to indicate that we are initiating an auction? Doesn’t feel necessary for this PR, but asking while I’m looking at it.

Shadowfiend · 2019-10-09T13:42:23Z

implementation/contracts/deposit/DepositLiquidation.sol

+    /// @param  _d      deposit storage pointer
+    function startSignerFraudLiquidation(DepositUtils.Deposit storage _d) internal {
+        startLiquidation(_d);
+        if(_d.inLiquidated()) {


Not for this PR, but it would make a lot of sense and read more naturally for these functions to start with “is“ rather than “in“.

This is referring to being "in" a state of the state machine as described in the whitepaper.

Yeah, I get that for sure. in<State>State/inState<State> is one common way to express this, is<State> is another (with appropriate naming of the states). in<State> is a not-particularly-clear mishmash of the two, since “in” can just as easily mean “inside of” and there's no additional disambiguation.

It's one of those things that's very easy for someone who's in the code to adapt to, but makes it harder for someone who isn't in the code frequently to grok. The less hard it can be, the better overall, especially if it doesn't make it particularly harder for the day-to-day developers.

Yeah that's a good explanation of it. Notably, I just found we do adopt this pattern in the Deposit codebase, eg. inRedeemableState.

😬

Obviously I lean hard towards is<State> as a pattern, but I ain't lookin' to die on that hill. Mostly, consistency is key.

And either way, we can handle it in a follow-up.

Shadowfiend · 2019-10-09T13:44:04Z

implementation/contracts/deposit/DepositLiquidation.sol

+    }
+
+    /// @notice         Starts signer liquidation due to abort or undercollateralization
+    /// @dev            We first attempt to liquidate on chain, then by auction


It would be useful for these docs to describe the differences between signer abort and fraud liquidation, in particular with regards to who gets any remaining balance and why.

Shadowfiend · 2019-10-09T13:47:30Z

implementation/contracts/deposit/DepositRedemption.sol

@@ -59,7 +59,7 @@ library DepositRedemption {
        uint256 _red = _d.redemptionTBTCAmount();
        require(_bal >= _red, "Not enough TBTC to cover outstanding debt");
        _tbtc.burnFrom(msg.sender, TBTCConstants.getLotSize());
-        _tbtc.transferFrom(msg.sender, address(this), DepositUtils.signerFee().add(DepositUtils.beneficiaryReward()));
+        _tbtc.transferFrom(msg.sender, address(this), DepositUtils.beneficiaryReward());


Commits really need to start including reasoning (not links to reasoning, mind you, but distilled summaries of reasoning for the change).

Good point - am getting better at this

No worries---we're inconsistent across the team on this front, and I usually call it out when I go to a commit to figure out why a change happened and… Can't 😁

Shadowfiend · 2019-10-09T13:50:59Z

implementation/contracts/deposit/DepositUtils.sol

    /// @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
+    // TODO(liamz): consider if redundant


Redundant with? In general it’s best to capture todos like this one in your personal task list or in an issue, depending on whether you are intending on tackling it yourself in the very near future.

Agreed, will remove.

This commit breaks apart work from the tbtc#265 PR, hence the lack of semantic Git commit history. Not complete. Tests for DepositLIquidation and Uniswap integration are passing only.

…temptToLiquidateOnchain

Shadowfiend

Ok, went through tests and left a bunch of additional thoughts.

One big theme: there are comments that aren't adding much over what the code already says. Let's avoid those, as they tend to become outdated and then give mixed signals.

Also on the comment front, if you've got comments on their own line, aim to make them complete, grammatically correct sentences. Only if they're at the end of a line for some very short clarification should they be fragments. This tends to track with longer comments wanting more space and needing more to be more clear in the future.

Shadowfiend · 2019-10-15T19:40:55Z

implementation/test/DepositFraudTest.js

+    await tbtcSystemStub.initialize(
+      keepRegistry.address,
+      uniswapExchange.address
+    )


Did this become necessary due to code from elsewhere?

Shadowfiend · 2019-10-15T19:41:41Z

implementation/test/helpers/assertBalance.js

+const bnChai = require('bn-chai')
+chai.use(bnChai(BN))
+
+export class AssertBalanceHelpers {


Given that this file strictly exports a single class, it feels right for the filename to match the class name.

On the other hand, I wonder if we need to encapsulate this in a class, vs passing the instance to the helpers directly. Slightly more typing, but it could also be clearer what you're doing at the call site, and make scenarios where you want to test against different TBTC instances (for example, to group tests for certain behaviors across different TBTC states together) more straightforward.

Don't think we need to move on the class encapsulation in this PR, but I do think we should make sure the filename is aligned properly with the current usage.

make scenarios where you want to test against different TBTC instances (for example, to group tests for certain behaviors across different TBTC states together) more straightforward

Could you elaborate a little bit more on this? I'm trying to imagine a scenario when we would have multiple instances of TBTC that are are testing in unison.

Reconsidering this, I think my real issue here is that it feels like we're using a class to approximate partial application, which we can do instead with Function.bind. There is nothing relating these helpers other than that they happen to both depend on a tbtcInstance.

To address your question directly though, I think any situation where we might run multiple tests in parallel against the tBTC system that expect different system states could use multiple instances.

Shadowfiend · 2019-10-15T20:10:23Z

implementation/test/helpers/snapshot.js

+      if (err) rej(err)
+      else res(result.result)
+    })
+  })


Given this is an async function, this feels like it could just be:

return await web3.currentProvider.send(...)

No? Or if we're want result.result, we can just do:

export ... { const result = await web3.currentProvider.send... return result.result }

The await will throw if there's an error in the send call, but that will be wrapped in a failed Promise by the function's async designation.

Ah if it is, then much obliged. 😄 Truffle Web3 has changed over the course of my using it - I'll go ahead and try testing with the newer syntax.

Just for completeness, async/await isn't Web3 but rather ECMAScript/JavaScript syntax. This particular aspect of async/await isn't immediately obvious, and is complicated further by the fact that a Promise return of an async function behaves the same as a non-Promise return for the caller.

It's great 😒

Wait till you see PromiEventEmitter's - event emitters that are also promises.

I tried it but it appears .send doesn't return a promise. Given that we've pinned Truffle in our config, how about we leave it for now.

Got it! Yep, I'm down. I won't block this PR, but it would be nice to drop a quick note in the file to indicate that our current version of Truffle doesn't return proper Promise objects and that's why we're jumping through hoops.

Shadowfiend · 2019-10-15T20:10:37Z

implementation/test/helpers/snapshot.js

+ * @param {string} snapshotId the snapshot ID (hex)
+ */
+export async function restoreSnapshot(snapshotId) {
+  return await new Promise((res, rej) => {


Same note as above about the Promise indirection.

Shadowfiend · 2019-10-15T20:12:42Z

implementation/test/DepositFraudTest.js

@@ -463,15 +471,13 @@ contract('DepositFraud', (accounts) => {
        utils.bytes32zero
      )

-      // Provide proof of a transaction where output is sent to a requestor, with
+      // Provide proof of a transaction where output is sent to a redeemer, with


Most of this comment feels like a restatement of the test description, which I don't love. Not really for this PR, and didn't get introduced here, but wanted to call it out.

Shadowfiend · 2019-10-15T20:51:26Z

implementation/test/helpers/txCost.js

+/**
+ * Gets the cost of a transaction in ETH.
+ *
+ * This is often useful in the cases when you're trying to assert some eth was refunded to `msg.sender`.


“some ETH”. Also don't forget your 80-character wrap!

Shadowfiend · 2019-10-15T20:57:51Z

implementation/test/integration/UniswapTest.js

+        await UniswapHelpers.addLiquidity(accounts[0], uniswapExchange, tbtcToken, ethAmount, tbtcAmount)
+
+        const minTbtcAmount = '100100000'
+        // hard-coded from previous run


Previous run of what?

From a previous run of Uniswap, supplied with the same liquidity.

Would that extra note be good?

Shadowfiend · 2019-10-15T21:00:02Z

implementation/test/integration/UniswapTest.js

+        await assertBalance.eth(deposit.address, expectedPrice.toString())
+
+        const price = await uniswapExchange.getEthToTokenOutputPrice.call(minTbtcAmount)
+        expect(price).to.eq.BN(expectedPrice)


This feels like we're testing Uniswap's behavior rather than our own, no?

True, we can remove this portion.

Shadowfiend · 2019-10-15T21:01:17Z

implementation/test/integration/helpers/uniswap.js

+    await tbtcToken.approve(uniswapExchange.address, tbtcSupply, { from: account })
+
+    // Uniswap requires a minimum of 1000000000 wei for the initial addLiquidity call
+    const UNISWAP_MINIMUM_INITIAL_LIQUIDITY_WEI = new BN('1000000000')


Is this a constant that exists inside Uniswap? Is it exported in some way we can see, or alternatively is there something we can reference that indicates this minimum?

We can reference the exact line of code. The constant wasn't documented anywhere, I discovered it myself through running up against the "revert" brick wall and reading the code.

Here it is - https://github.com/Uniswap/contracts-vyper/blob/master/contracts/uniswap_exchange.vy#L65

Good idea, I'll add it in.

I discovered it myself through running up against the "revert" brick wall and reading the code.

… Is there any chance we could work with them to expose this as a constant? Obviously we'd pursue that independent of this PR.

Shadowfiend · 2019-10-15T21:01:33Z

implementation/test/integration/helpers/uniswap.js

+
+  /*
+   * Add TBTC and ETH liquidity to the Uniswap exchange.
+   * This helper abstracts away detail in test code.


Don't really need this line.

Shadowfiend · 2019-10-15T21:06:30Z

Oh, also… Most of my remarks don't need to be handled in this PR, but can go into follow-up work. The key ones I think we should deal with (either by discussing or by making the changes) right now, in addition to remaining bits from the previous review, are:

Matches the filename of where the class is implemented.

We don't need to test price here, as its behaviour of Uniswap, tested separately in UniswapTest.

These changes were introduced as part of properly testing the beneficiary reward, in liquidation flow tests. The changes should've been removed in e56f394 but were missed. They will be reintroduced in a later PR.

…chain-liquidation

liamzebedee · 2019-10-18T15:51:28Z

@Shadowfiend cleaned up, tests passing, ready for another review.

Shadowfiend

All righty, let's rock and roll.

Shadowfiend · 2019-10-18T19:05:50Z

One thing remains here: let's do a final pass through comments and make sure everything that needs follow-up is captured in an issue or a personal todo for the next PR. In particular, we need issues for any remaining protocol questions. @liamzebedee can you take a pass on Monday?

Almost made it to two months, but we finally wrangled it down 😉

This was referenced Aug 29, 2019

Uniswap integration #160

Closed

Uniswap deployment #263

Merged

liamzebedee marked this pull request as ready for review August 31, 2019 17:45

liamzebedee requested review from prestwich, gakonst and nkuba August 31, 2019 17:46

gakonst reviewed Sep 3, 2019

View reviewed changes

implementation/contracts/deposit/DepositLiquidation.sol Show resolved Hide resolved

gakonst reviewed Sep 3, 2019

View reviewed changes

mhluongo added tbtc dev labels Sep 13, 2019

pdyraga assigned nkuba Sep 16, 2019

liamzebedee mentioned this pull request Sep 19, 2019

Refactor IKeep/KeepBridge #268

Merged

pdyraga reviewed Sep 26, 2019

View reviewed changes

pdyraga assigned pdyraga and unassigned nkuba Sep 30, 2019

pdyraga assigned Shadowfiend and unassigned pdyraga Oct 3, 2019

Shadowfiend reviewed Oct 3, 2019

View reviewed changes

liamzebedee force-pushed the uniswap-onchain-liquidation branch from 6e433f6 to 735892c Compare October 6, 2019 21:53

liamzebedee mentioned this pull request Oct 6, 2019

Consider renaming requester to redeemer #311

Closed

Shadowfiend mentioned this pull request Oct 7, 2019

Implement on-chain liquidation #105

Closed

Shadowfiend suggested changes Oct 9, 2019

View reviewed changes

liamzebedee added 2 commits October 9, 2019 19:10

Implement attemptToLiquidateOnchain

e56f394

This commit breaks apart work from the tbtc#265 PR, hence the lack of semantic Git commit history. Not complete. Tests for DepositLIquidation and Uniswap integration are passing only.

Touch up attemptToLiquidateOnchain comments

3f70978

liamzebedee mentioned this pull request Oct 10, 2019

Implement attemptToLiquidateOnchain #320

Closed

Merge branch 'master' of github.com:keep-network/tbtc into uniswap-at…

6cfa741

…temptToLiquidateOnchain

Shadowfiend suggested changes Oct 15, 2019

View reviewed changes

liamzebedee added 6 commits October 17, 2019 16:35

Revert naming changes for token balances

5029018

Contextualise undocumented constant in Uniswap with link to code

a13ad92

Rename AssertBalanceHelper to AssertBalance

11ac9e4

Matches the filename of where the class is implemented.

Minor touchups to docs

5d2daad

Remove dead code

56479fb

We don't need to test price here, as its behaviour of Uniswap, tested separately in UniswapTest.

Minor formatting touchups

a22db06

liamzebedee force-pushed the uniswap-onchain-liquidation branch 3 times, most recently from 04b7d2e to a22db06 Compare October 18, 2019 13:34

liamzebedee added 3 commits October 18, 2019 15:09

Revert changes to purchaseSignerBondsAtAuction

c82dd33

These changes were introduced as part of properly testing the beneficiary reward, in liquidation flow tests. The changes should've been removed in e56f394 but were missed. They will be reintroduced in a later PR.

Fix snapshot restoration failing when snapshotId is re-used

f307e41

Merge branch 'master' of github.com:keep-network/tbtc into uniswap-on…

474f814

…chain-liquidation

Shadowfiend approved these changes Oct 18, 2019

View reviewed changes

Shadowfiend merged commit 246c330 into master Oct 18, 2019

Shadowfiend deleted the uniswap-onchain-liquidation branch October 18, 2019 19:04


		TBTCToken _tbtc = TBTCToken(_d.TBTCToken);

		if (_d.requesterAddress != address(0)) { // redemption

	require(msg.value == ethPrice, "incorrect eth sent");
	require(msg.value == ethPrice, "incorrect eth sent");

Uniswap on-chain liquidation #265

Uniswap on-chain liquidation #265

Conversation

liamzebedee commented Aug 28, 2019 • edited Loading

liamzebedee commented Sep 2, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Oct 9, 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

nkuba commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee commented Sep 30, 2019

pdyraga commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Shadowfiend Oct 9, 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

Shadowfiend Oct 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee commented Aug 28, 2019 •

edited

Loading

gakonst Sep 3, 2019 •

edited

Loading

Shadowfiend Oct 9, 2019 •

edited

Loading

Shadowfiend Oct 4, 2019 •

edited

Loading

Shadowfiend Oct 9, 2019 •

edited

Loading

Shadowfiend Oct 9, 2019 •

edited

Loading