Feat/simplify balancer - Githubissues

vic-en commented 2 years ago

This PR mainly removes the caching logic in the balancer code, similar to earlier changes to Uniswap v2 & v3 code. It also add a test script file for Balancer.

For QA:

Pls focus on running the test script on mainnet as there doesn't appear to be pools on Kovan network.
Also, run with AMM-arb strategy on the client. Everything should sill work as before.

PtrckM commented 2 years ago

looping after resetting allowance (tried pairs WETH-USDC / DAI-USDC / WETH-DAI) wait atleast 20 minutes
balancer prices not showing
no arbitrage opportunity but keep showing insufficient eth balance to trade
profitability tab is not showing
order amount zeroes
shows no price for order decimal value

logs and configs: belenser.zip

PtrckM commented 2 years ago

Getting this error below on gateway. Steps done after cloning, yarn then yarn build then copy conf/global_conf.yml.example to conf/global_conf.yml edit it then yarn start or yarn debug

2021-09-09T03:08:40: PM2 log: [--no-daemon] Exit on target PM2 exit pid=21166
03:08:41 0|index  | Error: Cannot find module '/home/ubuntu/simplify_balancer/node_modules/@balancer-labs/sor/dist/index.js'. Please verify that the package.json has a valid "main" entry
03:08:41 0|index  |     at tryPackage (internal/modules/cjs/loader.js:294:19)
03:08:41 0|index  |     at Function.Module._findPath (internal/modules/cjs/loader.js:525:18)
03:08:41 0|index  |     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:781:27)
03:08:41 0|index  |     at Module.Hook._require.Module.require (/home/ubuntu/simplify_balancer/node_modules/require-in-the-middle/index.js:61:29)
03:08:41 0|index  |     at require (internal/modules/cjs/helpers.js:74:18)
03:08:41 0|index  |     at Object.<anonymous> (/home/ubuntu/simplify_balancer/src/services/balancer.js:4:13)
03:08:41 0|index  |     at Module._compile (internal/modules/cjs/loader.js:956:30)
03:08:41 0|index  |     at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
03:08:41 0|index  |     at Module.load (internal/modules/cjs/loader.js:812:32)
03:08:41 0|index  |     at Function.Module._load (internal/modules/cjs/loader.js:724:14) {
03:08:41 0|index  |   code: 'MODULE_NOT_FOUND',
03:08:41 0|index  |   path: '/home/ubuntu/simplify_balancer/node_modules/@balancer-labs/sor/package.json',
03:08:41 0|index  |   requestPath: '@balancer-labs/sor'
03:08:41 0|index  | }

https://user-images.githubusercontent.com/54516858/132617334-8d0e8e1d-22ac-4581-a354-91fafc682b68.mov

node version v12.13.0

vic-en commented 2 years ago

@PtrckM Kindly test again on mainnet. Note that I changed the SOR dependency, so ensure you do yarn install again. In addition, update your global_config.yml file to include BALANCER_VAULT: "0xBA12222222228d8Ba445958a75a0704d566BF2C8". Then start gateway on mainnet and test.

dennisocana commented 2 years ago

For now code review can only be done and testing is not possible for now.

vic-en commented 2 years ago

@mifeng Concerning the issue with the Balancer package, I reported on the Balancer TG group. You can either wait till they update the package or manually build it yourself, just like I did.

vic-en commented 2 years ago

@CoinAlpha/qa-team @mifeng I updated the kovan token list to include custom tokens from Balancer. Note that those tokens have a BAL prefix. And you can mint some for yourself at https://balancer-faucet.on.fleek.co/#/faucet

Also, you'll have to navigate to @Balancer -> SOR directory in node_module of the gateway-api project and manually install then build that package to make it work while Balancer makes an update to the package. Let me know if you have any questions about that.

fengtality commented 2 years ago

Also, you'll have to navigate to @Balancer -> SOR directory in node_module of the gateway-api project and manually install then build that package to make it work while Balancer makes an update to the package. Let me know if you have any questions about that.

I did this to build the package - afterwards, running yarn start from root dir should work

cd node_modules/@balancer-labs/sor
yarn install
yarn build

fengtality commented 2 years ago

For discussion:

The response to /price for buys vs sells on Kovan seems strange.

Price to buy 0.1 in BALWETH-BALDAI pair:

{
    "network": "kovan",
    "timestamp": 1631643496545,
    "latency": 3.688,
    "base": "BALWETH",
    "quote": "BALDAI",
    "amount": "0.1",
    "side": "BUY",
    "expectedAmount": "2.399424212027386063",
    "price": "23.99424212027386063",
    "gasPrice": 92,
    "gasLimit": "3006880",
    "gasCost": 0.27663296,
    "swaps": 4
}

Price to sell 0.1 in BALWETH-BALDAI pair:

{
    "network": "kovan",
    "timestamp": 1631643546375,
    "latency": 3.218,
    "base": "BALWETH",
    "quote": "BALDAI",
    "amount": "0.1",
    "side": "SELL",
    "expectedAmount": "6519.025093052665829154",
    "price": "65190.25093052665829154",
    "gasPrice": 92,
    "gasLimit": "3006880",
    "gasCost": 0.27663296,
    "swaps": 4
}

The much higher price for the sell response indicates there may be a bug here. In a single pool, buy price should always be higher than sell price.

fengtality commented 2 years ago

After I changed BALANCER_MAX_SWAPS to 1, the response for /price seems to make sense:

buy 0.1 BALWETH-BALDAI

{
    "network": "kovan",
    "timestamp": 1631729572132,
    "latency": 2.92,
    "base": "BALWETH",
    "quote": "BALDAI",
    "amount": "0.1",
    "side": "SELL",
    "expectedAmount": "2356.534294410209678274",
    "price": "23565.34294410209678274",
    "gasPrice": 234,
    "gasLimit": "3006880",
    "gasCost": 0.70360992,
    "swaps": [
        {
            "poolId": "0x702605f43471183158938c1a3e5f5a359d7b31ba000100000000000000000070",
            "assetInIndex": 0,
            "assetOutIndex": 1,
            "amount": "100000000000000000",
            "userData": "0x"
        }
    ]
}

sell 0.1 BALWETH-BALDAI

{
    "network": "kovan",
    "timestamp": 1631729572132,
    "latency": 2.92,
    "base": "BALWETH",
    "quote": "BALDAI",
    "amount": "0.1",
    "side": "SELL",
    "expectedAmount": "2356.534294410209678274",
    "price": "23565.34294410209678274",
    "gasPrice": 234,
    "gasLimit": "3006880",
    "gasCost": 0.70360992,
    "swaps": [
        {
            "poolId": "0x702605f43471183158938c1a3e5f5a359d7b31ba000100000000000000000070",
            "assetInIndex": 0,
            "assetOutIndex": 1,
            "amount": "100000000000000000",
            "userData": "0x"
        }
    ]
}

fengtality commented 2 years ago

@phbrgnomo Please test this PR using both Postman and the client on Kovan before we approve. For instance:

changing max_swaps from 1 to 4
using different token combinations from https://balancer-faucet.on.fleek.co/#/ (especially MKR (different ABI) and USDC (different decimals))
using limitPrice parameter
very large and small order amounts
run AMM-ARB strategy from client (approved and not approved)

Also, please read up on the changes that Balancer made from V1 to V2 to get ideas for how we can improve this connector after we migrate this to Gateway-V2: https://docs.balancer.fi/core-concepts/protocol

PtrckM commented 2 years ago

unable to pass token pair due to auto fetch trading pair
- edit ethereum_token_list_url to disable it (need token list url for balancer)

disabletokenlist

Getting this error and unable to show balancer price, also tried different order amount, max_swap and pairs still the same BALWETH-BALDAI / BALDAI-BALUSDC / BALMKR-BALWETH

2021-09-16 11:20:08,149 - 19802 - hummingbot.connector.connector.balancer.balancer_connector - NETWORK - Error getting quote price for BALWETH-BALUSDC  buy order for 0.01 amount.
Traceback (most recent call last):
  File "/Users/patrick/opt/anaconda3/envs/hummingbot/lib/python3.8/site-packages/cachetools/cache.py", line 39, in __getitem__
    return self.__data[key]
KeyError: "((<hummingbot.connector.connector.balancer.balancer_connector.BalancerConnector object at 0x7fe12a3315e0>, 'BALWETH-BALUSDC', True, Decimal('0.01')), {})"

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/patrick/Documents/HB/development/hummingbot/core/utils/__init__.py", line 15, in memoize
    return cache[key]
  File "/Users/patrick/opt/anaconda3/envs/hummingbot/lib/python3.8/site-packages/cachetools/ttl.py", line 84, in __getitem__
    return cache_getitem(self, key)
  File "/Users/patrick/opt/anaconda3/envs/hummingbot/lib/python3.8/site-packages/cachetools/cache.py", line 41, in __getitem__
    return self.__missing__(key)
  File "/Users/patrick/opt/anaconda3/envs/hummingbot/lib/python3.8/site-packages/cachetools/cache.py", line 68, in __missing__
    raise KeyError(key)

getting this on gateway
testscript failing to do trade

same as reported above:

gas limit extra 0
price change when adjusting balancer_max_swaps
logs and configs: belenser.zip

vic-en commented 2 years ago

@PtrckM I pushed an extra commit to resolve the issues with gaslimit. The client shouldn't give those errors anymore.

Also, it's necessary to change the token url to make it fail so the client can be able to use those custom tokens from balancer.

Also note that the extra debug messages about unknown pools and defaulting to zero are from the Balancer package.

fengtality commented 2 years ago

@PtrckM Some of the tests you ran above are great - especially the failing test script and detecting the extra 0 in gasLimit. However, some of things you encountered, like not finding the dummy BALWETH and BALDAI tokens in the tokenList, are not bugs, because this a kovan vs mainnet issue.

So that you don't waste time running those tests and flagging issues which are not relevant, please arrange a call with @vic-en so that he can show you the changes we have made to Gateway and how to test on kovan vs mainnet. Generally, QA and engineers working on the same component should speak on a daily basis.

CoinAlpha / gateway-api

Feat/simplify balancer #143

buy 0.1 BALWETH-BALDAI

sell 0.1 BALWETH-BALDAI