Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Better integration with MoonNet #58

Open
marcelomorgado opened this issue May 30, 2020 · 2 comments
Open

Discussion: Better integration with MoonNet #58

marcelomorgado opened this issue May 30, 2020 · 2 comments

Comments

@marcelomorgado
Copy link
Collaborator

Hi!
I'm interested to keep contributing with this project because I'm studying DeFi protocols and it's a great way to get they better, but one thing is bothering me, the time needed to run the tests, it's making my code/learning slowly since running a forked Ganache node isn't the fastest thing out there... because of that I've also get interested in the MoonNet project because it's trying to solve this issue, but actually, the performance of fork from Infura and fork from MoonNet are almost the same for me (sometimes Infura is performing better).
So, before back my contribution/studies with new protocols, I've spent some time trying to improve the time needed to run the tests and I think that I've found something (that can could also help MoonNet with their users' support):

I've started making some test rounds using the compound.test.ts test suite:

Forked from Infura: 83.418s
Forked from MoonNet: 81.527s
MoonNet (without forking): 29.596s 🥇

Based on these results, I was want to run all test suites using MoonNet without forking it. For make it possible some changes were needed because since the test suites run in parallel, all of them would run against the same node/state making some unexpected behaviors happen (e.g. mint dai in test suite A can make a dai balance assert to fail int test suite B).

The changes that I've made were:

  • Run jest without parallelism (--runInBand param);
  • Since MoonNet node is a ganache mode, we can call evm_revert and evm_snapshot methods. I've changed setup and tearDown functions to do so.

Here are the time results of running all test suites:

Forked from Infura: 462.934s
Forked from MoonNet: 499.884s
MoonNet (without forking): 89.819s 🥇

 PASS  tests/compound.test.ts (27.946s)
  compound
    ✓ enter markets (4305ms)
    ✓ supply 10 ETH (i.e. mint cETH) (3369ms)
    ✓ borrow 20 DAI (4073ms)
    ✓ supply 5 DAI (6321ms)
    ✓ get supply/borrow balances for DAI (1313ms)
    ✓ withdraw 1 ETH from collateral (2777ms)
    ✓ repay 1 DAI of debt (4573ms)

 PASS  tests/aave.test.ts (25.069s)
  aave
    ✓ lend 10 ETH (i.e. mint aETH) (3740ms)
    ✓ borrow 20 DAI (5788ms)
    ✓ lend 5 DAI (5001ms)
    ✓ get supply/borrow balances for DAI (829ms)
    ✓ withdraw 1 ETH from collateral (3035ms)
    ✓ repay 1 DAI of debt (4147ms)
    ✓ retrieve current health factor (1611ms)

 PASS  tests/kyber.test.ts (10.473s)
  kyber
    ✓ buy DAI from Kyber.Network (9542ms)

 PASS  tests/maker.test.ts (9.424s)
  maker
    ✓ create a proxy on Maker (2883ms)
    ✓ open Vault on Maker (5783ms)

 PASS  tests/uniswap.test.ts (7.456s)
  uniswap
    ✓ buy DAI from Uniswap (6689ms)

 PASS  tests/index.test.ts
  initial conditions
    ✓ initial DAI balance of 0 (1255ms)
    ✓ initial ETH balance of 1000 ETH (551ms)

 PASS  tests/medianizer.test.ts
  maker medianizer
    ✓ read ETH price from medianizer (1181ms)

Test Suites: 7 passed, 7 total
Tests:       21 passed, 21 total
Snapshots:   0 total
Time:        89.819s

Some notes:

  • It's less "safe" than fork since the MoonNet node can remain with the changed state after the jest process;
  • Keep the parallelism would be ideal but I think that the most important thing is to get the tests running much faster as possible right?
  • If the coming buidler fork feature is more performatic than the ganache no change would be needed (Do you have some info about it?).
  • For now, it's a discovery/PoC, this way of running tests should be more battle-tested.

WDTY @adrianmcli @kendricktan?

@adrianmcli
Copy link
Member

Thanks for the thoughts @marcelomorgado I think running tests directly on the MoonNet node was @kendricktan's original intention, and it is highly recommended where possible. It really depends on the tests you want to run, but for something like the money-legos repo, the CI usually runs these tests fast enough.

If you're making a dapp, then I would probably run the tests directly on MoonNet. But for this project, I am not sure it's necessary to do that yet. And like you said, we would have the issue of losing parallelism. I wonder if we could spawn a CI that creates Ganache instances in memory that would allow that though.

Definitely a lot to think about. I am not sure what the right answer is. We should keep discussing. @kendricktan any thoughts?

@kendricktan
Copy link
Contributor

@marcelomorgado Thanks for the inputs. @adrianmcli is right - the original goal I had in mind with moonnet was to minimize iteration time, and the largest bottleneck for me was the testing time.

Unfortuntely, Moonnet wasn't designed to be forked from, it was designed specifically as a Ethereum-testnet-in-the-cloud service that had low latency to a pseudo archival node in order to test/integrate with existing mainnet defi protocols.

It's less "safe" than fork since the MoonNet node can remain with the changed state after the jest process

You're right. But as you mention, you can revert back to the original state using evm_revert and evm_snapshot 😄

Keep the parallelism would be ideal but I think that the most important thing is to get the tests running much faster as possible right?

Parallelism can be achieved, but that requires spawning up a new testnet instance for every test suite. Its a tradeoff between time/money/computing power, which is something we might tackle in the future if we have enough time/funding.

If the coming buidler fork feature is more performatic than the ganache no change would be needed (Do you have some info about it?).

Definitely in the words, we will be migrating to builder-evm once it has the forking feature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants