Problem: there are duplicated hard coded informations in integration tests #233

damoncro · 2021-11-30T03:28:31Z

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

Close #10
This PR adds the following features:

Add a .env file which consolidates all keys/mnemonic words/address etc. Scripts, python files, and all yamls can read the .env file to expand the keys/mnemonic words/address etc
The yaml expansion job is done by pystarport.

Please merge crypto-com/pystarport#37 first.

PR Checklist:

Thank you for your code, it's appreciated! :)

damoncro · 2021-11-30T03:31:04Z

I will fix the the bugs and lint asap. But it is ok to review first.

codecov · 2021-11-30T03:31:52Z

Codecov Report

Merging #233 (3416c76) into main (3ea70c5) will increase coverage by 3.77%.
The diff coverage is 38.97%.

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   21.51%   25.29%   +3.77%     
==========================================
  Files          27       34       +7     
  Lines        1729     2554     +825     
==========================================
+ Hits          372      646     +274     
- Misses       1324     1858     +534     
- Partials       33       50      +17

Impacted Files	Coverage Δ
app/prefix.go	`0.00% <0.00%> (ø)`
app/test_helpers.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/gravity_hooks.go	`0.00% <ø> (ø)`
x/cronos/keeper/grpc_query.go	`0.00% <ø> (ø)`
x/cronos/keeper/ibc.go	`83.20% <ø> (+5.01%)`	⬆️
x/cronos/keeper/ibc_hooks.go	`50.00% <ø> (-8.83%)`	⬇️
x/cronos/keeper/keeper.go	`64.89% <ø> (-29.45%)`	⬇️
x/cronos/keeper/msg_server.go	`4.76% <ø> (-1.69%)`	⬇️
x/cronos/module.go	`57.62% <ø> (-4.20%)`	⬇️
x/cronos/proposal_handler.go	`6.25% <ø> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a4283...3416c76. Read the comment docs.

thomas-nguy · 2021-11-30T05:07:49Z

there are still linter issues

scripts/devnet.yaml

yihuang · 2021-11-30T06:34:26Z

For the lint issues, you can configure black and isort as plugins in your editor, to auto-format it when save @chenyanming

damoncro · 2021-11-30T06:44:38Z

@yihuang I just configured it. Should be ok now.

yihuang

I think we should support the expansion feature in pystarport directly, so people can still use pystarport serve --config scripts/devnet.yaml in cli directly.

damoncro · 2021-11-30T06:54:58Z

ok, I'll change a little bit. By the way the PR here crypto-com/pystarport#37

yihuang · 2021-11-30T06:59:10Z

ok, I'll change a little bit. By the way the PR here crypto-com/pystarport#37

If that one is merged, we don't need to do the expansion here right?

damoncro · 2021-11-30T07:02:23Z

@yihuang Yes, but .env can not work in our nix setting so far if it is relative path, crypto-com/pystarport#37 only works in cli usage, which I tested.

I still need to do some modifications on this PR.

yihuang · 2021-11-30T07:23:10Z

@yihuang Yes, but .env can not work in our nix setting so far if it is relative path, crypto-com/pystarport#37 only works in cli usage, which I tested.

why?

I still need to do some modifications on this PR.

damoncro · 2021-11-30T07:31:35Z

the scripts.nix would copy the yaml files to nix store, if the .env is relative path, it is under /nix/store, of course, the .env is not there. But I can update default.nix and scripts.nix by not copying to nix store.

yihuang · 2021-11-30T07:39:57Z

the scripts.nix would copy the yaml files to nix store, if the .env is relative path, it is under /nix/store, of course, the .env is not there. But I can update default.nix and scripts.nix by not copying to nix store.

This can be solved by overriding the dotenv path with a parameter.

damoncro · 2021-11-30T07:43:50Z

Good idea, I can also try parameterise it.

damoncro · 2021-11-30T09:03:54Z

Now change the expansion code to pystarport. But The commit uses the test version of pystarport, need to finish and merge crypto-com/pystarport#37, before continuing.

1. Scritps 2. yaml 3. Python files

After crypto-com/pystarport#37, would change back to crypto-com/pystarport.

Test the expansion feature

integration_tests/pyproject.toml

integration_tests/utils.py

yihuang · 2021-12-03T04:19:44Z

nix/default.nix

-          cronos-config = ../scripts/cronos-devnet.yaml;
-          hermes-config = ../scripts/hermes.toml;
-          geth-genesis = ../scripts/geth-genesis.json;
+          chainmain-config = builtins.toString ../scripts/chainmain-devnet.yaml;


Why the builtins.toString call?

It is necessary to get the full path of the file (string), instead of copying to nix store. https://github.com/crypto-org-chain/cronos/blob/main/nix/scripts.nix#L15: if we use ../scripts/cronos-devnet.yaml and .env, then quote it with ${config.cronos-config} will copy to nix store.

Not copying to nix sotre has following benefits:

Read .env with relative path just under the project script folder.

In nix-shell environment, if we modify the cronos-devnet.yaml, we can call start-cronos directly without exiting the nix-shell and re-enter the nix-shell.

start-cronos Before:

#!/nix/store/c9cxq1583a85bsq76q2rbnbiwwp7ygxr-bash-4.4-p23/bin/bash # rely on environment to provide cronosd export PATH=/nix/store/f347n055kzsyq30m41x481dpsq0zbip1-python3.8-pystarport-0.2.2/bin:$PATH source /nix/store/ym2jzjqjmzf7azjkc8m9jd4vy0qjrx5p-dotenv /nix/store/zhicf2wijlr36315xx1nxq5bhh6q90qs-start-cronos /nix/store/k0mfjv85j0sjbfjj6lisriwq3pkzcsqi-cronos-devnet.yaml $@

After:

#!/nix/store/c9cxq1583a85bsq76q2rbnbiwwp7ygxr-bash-4.4-p23/bin/bash # rely on environment to provide cronosd export PATH=/nix/store/fjm893gw3z5yd3f3vsabb5vw2my1dvd2-python3.8-pystarport-0.2.3/bin:$PATH source /Users/damonchen/Documents/cronos/scripts/.env /nix/store/zhicf2wijlr36315xx1nxq5bhh6q90qs-start-cronos /Users/damonchen/Documents/cronos/scripts/cronos-devnet.yaml $@

Then you don't get the reproducibility with these scripts.
In development, you can always run ./scripts/start-cronos config data explicitly.

I see, treat it like a fixed binary. Let me think a more thoughtful idea.

But seems no documents here, user would not know start-cronos exists. The binary is not useful because it only works in nix-shell. Even me, I don't use it at all.

Of course, it is important in integration test. Just not that useful for users, assuming they do not have nix installed.

start-cronos and start-geth are only used to start cronos and geth devnets with the identical environment, to run cronos devnet with custom config, one can always use pystarport directly, the scripts/start-cronos is only a thin wrapper over that.

Two TODOs here:

Make all ShellScriptBin reproducible. And add documents if possible.

Improve all scripts. E.g. So that user knows they can use dotenv option as well.

These scripts are not intended to be used by user, so just keep it simple here, only start-geth is useful for normal user, the other scripts are thin wrappers of pystarport.
We can even remove start-chainmain and start-cronos, and simply use pystarport directly.

nix/sources.json

scripts/.env

and remove deepdiff dependency.

It is more general and work with full path as well

integration_tests/pyproject.toml

This makes these two script-bins defined in `scripts.nix` always use the same yaml and dotenv file, for metting the "re-producibility" requirement during one/same `nix-shell` environment. These script-bins are wrappers of pystarport with fixed yaml and dotenv. If you want to create the cronos/chainmain instance with different configurations, you should avoid using the script-bins. Please run the scripts directly, or use `pystarport` step by step. Besides, if you change the yaml or dotenv, you need to exit and re-enter the nix-shell. So that the modified yaml or dotenv are copied to nix store, and their paths can be rewritten into these two script-bins.

damoncro requested a review from a team as a code owner November 30, 2021 03:28

damoncro requested review from yihuang and thomas-nguy and removed request for a team November 30, 2021 03:28

damoncro force-pushed the feature/expansion branch from fb82424 to 900240b Compare November 30, 2021 04:24

yihuang reviewed Nov 30, 2021

View reviewed changes

scripts/devnet.yaml Show resolved Hide resolved

yihuang self-requested a review November 30, 2021 06:47

yihuang reviewed Nov 30, 2021

View reviewed changes

damoncro force-pushed the feature/expansion branch from cfd4bbf to 68675b5 Compare November 30, 2021 08:55

damoncro force-pushed the feature/expansion branch 2 times, most recently from 6c2a29e to 4d63980 Compare December 2, 2021 03:24

damoncro added 6 commits December 2, 2021 14:10

Add .env

850c205

Load dotenv and expand it in all related files

8d4d671

1. Scritps 2. yaml 3. Python files

Update to read full path of config files

249abcd

Test: Replace with test pystarport at first

bc2ae1a

After crypto-com/pystarport#37, would change back to crypto-com/pystarport.

Test expansion

479eca8

Test the expansion feature

Update the test pystarport

dd5126e

damoncro added 2 commits December 2, 2021 14:10

Update test pystarport

1298770

Update test_expansion

f85ee81

damoncro force-pushed the feature/expansion branch from 3c285d4 to f85ee81 Compare December 2, 2021 06:11

Move the test_expansion to pystarport

24f4e58

thomas-nguy approved these changes Dec 2, 2021

View reviewed changes