CoinAlpha / gateway-api

Apache License 2.0
47 stars 25 forks source link

Test/add test script for uniswap v2 #120

Closed vic-en closed 2 years ago

james-hummingbot commented 2 years ago

@vic-en It looks good to me, but maybe let's wait for @mifeng to approve it since he was leading these changes. Also, is this something that @CoinAlpha/qa-team will be running? Do they have enough details here to run it themselves?

fengtality commented 2 years ago

The eth/poll route still doesn't work. I think I found what the issue, which can be seen in the logs emitted by the process:

15:05:30 0|index  | info:   Request from IP: ::ffff:127.0.0.1 POST /eth/poll {"timestamp":"2021-08-18T22:05:30.822Z"}
15:05:31 0|index  | TypeError: Cannot read property 'gasUsed' of null
15:05:31 0|index  |     at EthereumService.<anonymous> (/Users/feng/Code/gateway-api/src/services/ethereum.ts:278:21)
15:05:31 0|index  |     at Generator.next (<anonymous>)
15:05:31 0|index  |     at fulfilled (/Users/feng/Code/gateway-api/dist/services/ethereum.js:5:58)
15:05:31 0|index  |     at processTicksAndRejections (internal/process/task_queues.js:93:5)

There's a bug in the response handler which tries to convert gasUsed to a number even though it may be null. This prevents the await from resolving. I'll push a fix.

vic-en commented 2 years ago

The eth/poll route still doesn't work. I think I found what the issue, which can be seen in the logs emitted by the process:

15:05:30 0|index  | info:     Request from IP: ::ffff:127.0.0.1 POST /eth/poll {"timestamp":"2021-08-18T22:05:30.822Z"}
15:05:31 0|index  | TypeError: Cannot read property 'gasUsed' of null
15:05:31 0|index  |     at EthereumService.<anonymous> (/Users/feng/Code/gateway-api/src/services/ethereum.ts:278:21)
15:05:31 0|index  |     at Generator.next (<anonymous>)
15:05:31 0|index  |     at fulfilled (/Users/feng/Code/gateway-api/dist/services/ethereum.js:5:58)
15:05:31 0|index  |     at processTicksAndRejections (internal/process/task_queues.js:93:5)

There's a bug in the response handler which tries to convert gasUsed to a number even though it may be null. This prevents the await from resolving. I'll push a fix.

I thought I fixed this. Can you confirm you're on the correct branch again?

fengtality commented 2 years ago

@vic-en Yes I was on your latest brach. I think the root cause of this issue is converting gasUsed to a number, which causes an error when that field doesn't exist on the transaction response. Looks like we do that to pass a type check, so I removed the type check entirely in my latest commit.

Afterwards, here's what I get from running the test. Now it is actually polling and assessing the test. Since the status field is 0, it fails the test which is the expected result.

Starting Uniswap tests
***************************************************
Starting Uniswap v2 on pair WETH-DAI...
{
  network: 'kovan',
  timestamp: 1629326446443,
  latency: 0.731,
  success: true,
  pairs: [ 'WETH-DAI' ],
  gasPrice: 38,
  gasLimit: 150688,
  gasCost: 0.005726144
}
Calling uniswap v2 gas-limit endpoint...
{ network: 'kovan', gasLimit: 150688, timestamp: 1629326447195 }
Checking buy price for WETH-DAI...
Buy price: 997.60013
Checking sell price for WETH-DAI...
Sell price: 987.44954
Executing buy trade on WETH-DAI with 0.01 amount...
Buy hash - 0xbf5244a9e973794a52c038d990c83c84764bbf4c371e0527f0238861a1cbe062
Polling...
{
  network: 'kovan',
  timestamp: 1629326450726,
  latency: 0.597,
  txHash: '0xbf5244a9e973794a52c038d990c83c84764bbf4c371e0527f0238861a1cbe062',
  confirmed: false,
  receipt: {}
}
{
  network: 'kovan',
  timestamp: 1629326451327,
  latency: 0.613,
  txHash: '0xbf5244a9e973794a52c038d990c83c84764bbf4c371e0527f0238861a1cbe062',
  confirmed: false,
  receipt: {}
}
{
  network: 'kovan',
  timestamp: 1629326451946,
  latency: 0.864,
  txHash: '0xbf5244a9e973794a52c038d990c83c84764bbf4c371e0527f0238861a1cbe062',
  confirmed: true,
  receipt: {
    gasUsed: { type: 'BigNumber', hex: '0x96fd' },
    blockNumber: 26836413,
    confirmations: 1,
    status: 0,
    logs: []
  }
}
(node:7725) UnhandledPromiseRejectionWarning: AssertionError: Buy trade reverted.: expected 0 to equal 1
    at unitTests (/Users/feng/Code/gateway-api/tests/scripts/uniswap.v2.test.js:218:10)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /Users/feng/Code/gateway-api/tests/scripts/uniswap.v2.test.js:268:3
(Use `node --trace-warnings ...` to show where the warning was created)
(node:7725) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:7725) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
fengtality commented 2 years ago

@vic-en I think the test works now. Can you please run it?

Note that I merged in development and then reverted the merge. That is because I am getting a weird error when running the current version of development, so I wanted to get the test working first. When we merge this into development, we should issue a clean PR based on development

fengtality commented 2 years ago

I found the issue - there was an incorrect entry in package.json that sneaked into development. It's the same issue that affects #132