haydenshively/nantucket

Flash loan liquidation bot for compound.finance

nantucket's Issues

Less integrationy tests for individual components

Use Human-Readable ABIs for smart contracts

This is technically an ethers.js feature, but we should be able to use ethers to generate the full abi from the human readable one, and then pass the result to Web3 as normal

Typescript everything

Create unique bidding strategies and pick based on profit tier

Could also train a small RL agent to choose the best strategy based on past events, or just make bidding strategy configurable in JSON settings. These will also depend on whether we're tracking events in the mempool or not.

Replace Coinbase Pro oracle with the official Open Price Feed oracles

Right now, asset prices on Compound (the protocol) are updated every once in a while by Compound (the company). After releasing COMP tokens and handing protocol control over to decentralized governance, this price feed is the last piece of centralization in the protocol.

Compound (the company) has proposed that Compound (the protocol) adopt their Open Price Feed. With this system, governance-approved entities like Coinbase and OKex would publish private-key-signed prices at some HTTP endpoint (off-chain), and anybody could submit Ethereum transactions to make the on-chain prices match.

The Open Price Feed smart contract would then check to make sure the prices actually came from an approved source, combine them with moving averages, and save the results.

For liquidators, I think this will be a huge change. Right now everyone must cluster around Compound's (the company) price update transactions by matching their gas price. If I understand correctly[1], the Open Price Feed will allow people to incorporate price update transactions into their own smart contracts. They will then be able to set the gas price as high as they want, leading to gas price bidding wars. If this happens we may need to shift gears -- rather than developing a liquidation bot, we may want to create a smart contract that allows multiple liquidators to share profits (and we take a cut). If nobody does this, the gas price auctions will just suck away everyone's profits. Either way we have to be prepared when governance chooses to adopt the Open Price Feed

[1] There may be a designated waiting period between when the prices are updated and when they go into effect as the new "truth." If that's the case, it could be implemented in two ways. (a) The waiting period is specified in # of blocks or (b) it's specified in # of seconds. Option a would lead to the gas price wars, as described above. Option b would not, since people would have to pick a gas price that places their transaction at an appropriate timestamp within a block.

Explore why we've never gotten a `LiquidateWithPriceUpdate` notification in practice

In theory every liquidation should be a liquidation with price update, but so far that doesn't seem to be the case. Figure out why.

Add partial liquidation capabilities

There are some people with such huge Compound loans that AAVE (the flash loan provider) doesn't have enough liquidity for us to liquidate them all at once. We'd literally be borrowing over 100% of AAVE's funds, which simply isn't possible. In these cases, the FlashLiquidator contract should borrow some MAX amount from AAVE, do part of the liquidation, payoff the loan, and repeat as needed until the entire liquidation is complete.

We don't need to post all prices every time

We only need to post the set of prices that makes our target liquidatable

Begin typescript conversion

Revisit naming scheme after TS basics are merged

We can't really put it into SmartContract because of our workaround with CToken. The implementing class there is actually the intermediary one, not the base CToken class, so its use is a little inconsistent. We could try to rework that too, but given the scale it might be better to put that into a larger refactor PR after the first TS merge.

As for the naming scheme, in general we should try to keep the name as close to the contents as possible. The functionality this provides is the ability to map a network name to an instance of a contract, so I think it would be useful to indicate the "network switching" functionality in the title. That would lead us to MultiEthNetContract, which is a little wordy and reminiscent of Java class names, hence why I shortened it. I guess DeployedContract is alright, but it's a little misleading with the purpose of the interface. Do you have any suggestions that would more tightly represent its contents? I'm kinda stumped.

Originally posted by @adamegyed in #61 (comment)

Write tests for TxManager

It's fairly well-documented but in dire need of tests. wallet.test.js and txqueue.test.js can serve as guides

Can cTokens be seized if they are held but not marked as collateral

Right now, tableusers.js has the following TODO:

// it's possible that the user is holding CTokens but hasn't
// actually entered the market, in which case their supply balance
// shouldn't actually contribute to their collateral computation.
// Also it wouldn't be seizable

Is this true? I'm beginning to doubt it. I think the truth is that the only thing "collateral" or "not collateral" impacts is whether it allows you to borrow against it. I've read through the Comptroller code and there are no checks for whether a cToken is collateral or simply held.

That said, we should double check in the Compound Discord or subreddit.

Find a nice way to remove this hack from Worker

// TODO TxManager isn't hooked into the Database logic, so we have
// to pass along the repay and seize addresses here
if (!String(c.ctokenidpay).startsWith("0x")) {
  const repay = `0x${await this._tCTokens.getAddress(c.ctokenidpay)}`;
  this._candidates[i].ctokenidpay = repay;
}
if (!String(c.ctokenidseize).startsWith("0x")) {
  const seize = `0x${await this._tCTokens.getAddress(c.ctokenidseize)}`;
  this._candidates[i].ctokenidseize = seize;
}

Benchmark Smart Contract Execution across different parameters

Measure gas usage on ethereum mainnet fork for better predictions.

Monitor mempool to track rivals

Improve test coverage

TxManager should send empty transaction if gasPrice starts to grow exponentially

If a transaction repeatedly fails, the current behavior is to multiply the gasPrice by 1.1 and resend it. This results in extremely high tx fees. We should have a fail safe.

Liquidator smart contract is hard-coded to break out of loop with 2M gas

The hard-coded value indicating when to stop trying to liquidate more people is outdated -- from the AAVE days. This number can be decreased to something like 1.5M

Subscribe to supply/borrow events rather than full update in Worker

Even pulling from the local Geth node takes ~2 seconds for 50 accounts. This is horribly slow. If we subscribe to supply and borrow events and update on an as-needed basis, rather than re-loading all data for every account every time, we can speed things up substantially and increase the number of accounts that we're watching (e.g. take it from 50 -> 5000)

Benchmark code that happens on new block

Don't need to re-fetch cToken ID's on every account fetch in user table

IDs in the CTOKEN table should be constant. The only time they change is if an asset is added/removed from Compound, in which case we could just restart the code. This means that the user table code can just store a map of cTokenAddresses -> cTokenIDs and reuse it when populating the user table with accounts. This should cut down on CPU usage since there are thousands of accounts, and we'd be using one less database call for each one.

Migrate IPC messages and channels to gRPC

Using protobufs as a serialization format will lead to better performance, and definined expected types for service via gRPC will lead to better introspection and type safety.

Initial gas price should cause tx fee to lie on max tx fee exponential

To actually reach max tx fee, the initial gas price should be calculated from initial_fee = max_fee / (1.12^N) where N is some positive whole number that gets us near the current market-rate gas price.

Liquidation event subscription duplicates between repay/seize token

Ideally we would only see the notification once per liquidation, but right now it gets triggered once for the repay token and once for the seize token

Make Liquidator's _estimateGas function more precise

    // TODO The following conditional should really have an `or` clause to account
    // for cases where Uniswap has sufficient liquidity to go straight from repay
    // to seize without using Eth as an intermediate, but that's difficult to compute

Add arbitrage capabilities

PriceOracle and CToken addresses can be retrieved from Comptroller

No reason to hard-code these really

Move TxManager logs to Winston

Wrapper around FlashLiquidator that enables Chi token

This should be pretty simple to implement. 1inch provides example code. The function would just check if the contract currently owns any Chi tokens. If it does, it should burn some (amount computed by the 1inch sample code) and pass all args through to the FlashLiquidator contract. Could even implement as a fallback function + delegatecall

Migrate to Redis

A full swap of postgres to redis would move our "single source of truth" to be entirely in-memory, greatly increasing performance.

TxManager can misbehave after 50 blocks

TxManager loses track of what's happening to a transaction after 50 blocks. This can be an issue, especially if a higher nonce tx was updated more recently than a lower tx (or vice versa? I'm not sure) because things get out of order.

A Winston log corresponding to this error:

💸 Transaction | 0xd3fE.65 Failed off-chain: Error: Transaction was not mined within 50 blocks, please make sure your transaction was properly sent. Be aware that it might still be mined!

Go overboard on logging and just make it configurable with config.json

If things go wrong, we at least need to be able to learn from them.

Make TxManager optimize its transaction

Right now the TxManager caches a transaction that contains all of the candidates. However, some candidates may have better profit margins than others, and therefore are profitable under different market gas price conditions. The TxManager should account for this.

This wouldn't be an issue if we were focused on liquidating super-high-value accounts, but right now we have to deal with the low end guys for testing purposes

Add a GitHub Action that starts an experiment

Having a CI is great for quick checks, but changes to this codebase should be evaluated based on their performance on-chain. I've created separate config.json files for production and testing environments. The test config focuses on the lowest-profit accounts.

Task: Create a manually-runable GitHub Action that starts the test version of Nantucket for PR code. Only 1 test instance should be able to run at a time.

Replace tx IPC messages with candidates

Right now, each worker process sends mostly-complete transaction objects to the TxManager process via Node.js IPC. However, multiple worker processes can be matched up with a single TxManager/wallet. If this is the case, and both workers want to submit a liquidation transaction, the TxManager may as well combine the transactions into one. To do this, candidate objects should be sent over IPC instead of transactions.

TxManager only cares about most profitable candidate

See the following in TxManager:

/*
    TODO
    
    Note that this will only force the exponential thing for the _first_ candidate
    that gets sent off to the smart contract. If more candidates are added to
    later bids, the condition no longer necessarily holds.

    To make it apply to those cases as well, (1) the logic would have to be moved
    elsewhere (probably to the `cacheTransaction` function) and (2) upon addition of
    a new candidate, check whether that new candidate is the most profitable
    (idx 0 in the `borrowers` array). If it is, then have some logic that decides
    whether hopping up to a new exponential curve makes sense given how close/far
    we are from `maxFee`
*/

This can be fixed while still only caring about profitability of the best candidate. That said, we should probably look into other ways of optimizing transactions for maximum profit. It may be possible to liquidate a bunch in a single transaction and save on gas, making something profitable that otherwise wouldn't have been. This is especially the case with Chi, because Chi adds too much overhead to be super useful for a single candidate, but that overhead is constant -- so if we liquidate many candidates, the overhead is the same, but the gas savings are much larger.

Note that changing this will require changes to both TxManager.js and Liquidator.js, as both make assumptions based on the "best candidate only" heuristic.

Upon any "revert" error in TxQueue, shut everything down

TxManager should be able to confirm a transaction every single block

The idea behind the price wave / prices posted motif is that when somebody becomes liquidatable (or nearly liquidatable) based on Coinbase prices, we should prepare for them to become liquidatable on-chain. They get added to a list, and the TxManager attempts to maintain a pending transaction that, if confirmed, would try to liquidate the users in the list.

As soon as Compound (the company) updates prices, the code updates[1] the pending transaction to use the same gas price as Compound's transaction. In theory, this would ensure that the transactions happen right after one another, giving us a good chance of "winning" the liquidation.

In practice this doesn't work too well. The only way to ensure that a transaction gets mined in the block it was sent on is to use an incredibly high gas price -- and even then it can sometimes be delayed by 1 block. This is because miners give some priority to transactions that have been waiting longer. This explains why I try to have the TxManager maintain a pending transaction. The issue is that it only maintains 1 at a time. If a pending transaction happens to go through on the block before a price update transaction, the TxManager will replace it with a new one, but that new one will be brand new, and thus it has a very low chance of actually going through soon enough to win the liquidation.

To fix this, we need to have a rolling queue of pending transactions. For example, assume a transaction takes N blocks to be accepted. Instead of sending transactions at blockNumber=0, blockNumber=N, blockNumber=2N... we should send transactions at blockNumber=0, blockNumber=1, blockNumber=2 so that they get confirmed at N, N+1, N+2. This requires some very careful management and heuristics, especially since it gets expensive fast[2].

[1] Note that this update can only happen if the pending transaction has a lower gas price than Compound's transaction (gas prices can only be raised after tx submission, never lowered)
[2] After implementing this, we may find that it's too expensive to do it for all accounts. Nantucket may need to become more targeted: focus on just 1 or 2 accounts that get liquidated often, and ignore the rest. After making some money, we can then reinvest it and broaden Nantucket's scope again

Ganache --fork webthree testing everything

Add script or TxManager functionality for replacing all pending txs with empty txs

If something goes horribly wrong, we may end up in a situation where the TxManager has spammed a bunch of high-gas-price transactions. Since gas price can only ever increase for a given nonce, it'd be nice to be able to replace those transactions with empty transactions (sending 0 ether from self to self). This doesn't reduce gas price, but it does reduce gas, and thus reduces the transaction fee (gasPrice * gas)

Parameterize constants in JSON

Combine *Prices Posted* notifications from each thread into a single one

Takes too long to re-open connection to Infura WS

For fast-paced bids, we either need Infura to upgrade their infrastructure or we need to switch to calling their HTTPS endpoint. Right now we get a bunch of UnhandlePromiseRejections from web3 because the WS connection isn't open when we try to send a transaction

Refactor logging levels to use multiple winston transports

Right now, there is only one winston transport, which sends all logs to a slack hook, and options in the config to adjust what is logged. This presents some added complexity which can be refactored out, and leaves the potential for certain logging events that could be useful in a postmortem analysis to be lost if the config is set to not include certain event types.

A solution to this would be to adjust the logging levels of the winston slackhook, possibly with custom levels, to only report on large-scale, relevant updates, while keeping the low level logging intact. We could add another winston transport that would save the logs to log files, which would give deeper insight to debugging without forcing the slackhook to receive massive amounts of relatively simple operational data.

Additionally, there are certain drawbacks to using slack for logging, such as a scrollback maximum based on the free plan. While it is convenient to view, having a fallback can be very useful in the event of data loss due to Slack message rollover.

Move coinbase oracle to its own thread

There's no need for each worker to make http requests. A single thread should do this (to avoid rate-limiting) and place the results in the UTOKEN database table. The workers can then fetch info from the database rather than the internet

Workers should receive a Web3 provider in their constructor

Right now it's just hard-coded as a global web3 variable:

// lines 82-88
await c.refreshBalances(
  web3,
  Comptroller.mainnet,
  CTokens.mainnet
);

// lines 112-115
if (await c.isLiquidatable(web3, Comptroller.mainnet)) {
  this._candidates[i].msg().broadcast("Liquidate");
  this._candidates.splice(i, 1);
}

Uniswap slippage can eat profits for low-value liquidations

Verify denomination of Compound API and new PriceOracle

The old API and Oracle were denominated in Eth, but that may not still be the case, or it may be about to change. Verify on Discord and make changes if necessary

Race conditions on startup

When the code first starts, each Worker is told to update its list of candidates (that is, N users with A < profitability < B, sorted by liquidity). This list is pulled from postgres, which could be horribly out-of-date -- the first pull from Compound's API doesn't happen until 9 minutes after the code starts.

This on its own is bad, but not a bug. The bug is that on startup each Worker is also told to checkLiquidities() of its candidates. Assuming that the candidates list has been populated by the time this gets called, the code inside Candidate will run init() -- this fetches the candidate's supply and borrow balances from Geth.

Let's say the fetch is happening during block B, so the local Geth node is basing queries off B - 1. In block B, another liquidator happens to liquidate one of the candidates that we're examining, causing the supply/borrow balances returned by Geth to be out-of-date. Now Nantucket thinks that they are liquidatable, but they really aren't.

Under normal operation, Nantucket would (A) be participating in the gas bidding process with the other liquidator or (B) too slow (i.e. unaware of the liquidation potential until block B + 1). In case A, this bug is moot -- we expect to spend money on gas fees. In case B, the subscription to "Liquidate" events should tell the TxManager to stop bidding on a candidate that has already been liquidated[1].

I think one solution to this is to simply not update Worker candidates until the first pull from Compound's API has completed (9 minutes in). More complex things are possible as well, of course

[1] Writing this, I've realized that the Liquidate event should probably remove the Candidate from the Worker's list and the TxManager's list, not just one or the other

Make sure that TxManager & TxQueue don't get screwed by passing tx by reference

Right now, Wallet is safe because the main function (signAndSend()) does tx = {...tx};, but TxManager and TxQueue don't do that, and in some cases are dependent on pass-by-reference (for example, in the dry-run mode of txQueue.replace)

We need to make sure that all such use-cases are intentional, and/or switch them out for more functional-style code. Document whatever is going on rn and whatever changes are made.