Problem: ibc solo machine integration not tested (fix #613) #757

linfeng-crypto · 2022-04-27T06:11:55Z

No description provided.

codecov · 2022-04-27T06:18:30Z

Codecov Report

Merging #757 (571d97c) into master (ef0bb6d) will decrease coverage by 2.24%.
The diff coverage is n/a.

❗ Current head 571d97c differs from pull request most recent head c02fda3. Consider uploading reports for the commit c02fda3 to get more accurate results

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
- Coverage   20.02%   17.77%   -2.25%     
==========================================
  Files          99       99              
  Lines       10966    10626     -340     
==========================================
- Hits         2196     1889     -307     
- Misses       8291     8297       +6     
+ Partials      479      440      -39

Flag	Coverage Δ
integration_tests	`17.77% <ø> (-0.01%)`	⬇️
unit_tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
x/icaauth/types/types.go	`0.00% <0.00%> (-100.00%)`	⬇️
x/nft/keeper/owners.go	`17.02% <0.00%> (-68.09%)`	⬇️
x/nft/keeper/grpc_query.go	`12.50% <0.00%> (-53.75%)`	⬇️
x/nft/keeper/collection.go	`23.18% <0.00%> (-49.28%)`	⬇️
x/nft/keeper/nft.go	`51.85% <0.00%> (-48.15%)`	⬇️
x/nft/types/msgs.go	`7.40% <0.00%> (-40.75%)`	⬇️
x/nft/client/cli/query.go	`33.51% <0.00%> (-35.20%)`	⬇️
x/nft/keeper/denom.go	`51.28% <0.00%> (-33.34%)`	⬇️
x/nft/client/cli/tx.go	`43.75% <0.00%> (-32.30%)`	⬇️
x/icaauth/types/message_register_account.go	`0.00% <0.00%> (-26.67%)`	⬇️
... and 9 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 ef0bb6d...c02fda3. Read the comment docs.

tomtau · 2022-04-27T09:18:34Z

integration_tests/test_solo_machine.py

+        sysstr = platform.system()
+        if sysstr == "Darwin":
+            url = "https://github.com/crypto-com/ibc-solo-machine/releases/\
+download/v0.1.1/macos-latest-v0.1.1.tar.gz"
+            self.sign_file = os.path.join(self.bin_dir, "libmnemonic_signer.dylib")
+        else:
+            url = "https://github.com/crypto-com/ibc-solo-machine/releases/\
+download/v0.1.1/ubuntu-latest-v0.1.1.tar.gz"
+            self.sign_file = os.path.join(self.bin_dir, "libmnemonic_signer.so")
+        local_filename = url.split("/")[-1]
+        tar_file = f"/tmp/{local_filename}"
+        if os.path.exists(self.bin_file) and os.path.exists(self.sign_file):
+            return
+        with requests.get(url, allow_redirects=True, stream=True) as r:
+            r.raise_for_status()
+            with open(tar_file, "wb") as f:
+                for chunk in r.iter_content(chunk_size=1024 * 8):
+                    f.write(chunk)
+        # uncompress
+        file = tarfile.open(tar_file)
+        file.extractall(self.bin_dir)
+        file.close()
+        # check file exists
+        if not os.path.exists(self.bin_file):
+            raise Exception(f"solomachine binary file {self.bin_file} is not exists")
+        if not os.path.exists(self.sign_file):
+            raise Exception(
+                f"solomachine signature file {self.sign_file} is not exists"
+            )


probably ok, but it seems a bit cumbersome -- it can be replaced by having it fetched in the Nix environment (builtins.fetchTarball) and having the solo machine binary in the environment's PATH

tomtau

looks all right, just two fixes to consider: 1) making the binary download more robust (e.g. by using Nix), 2) additional assertions against the node in the cluster

tomtau · 2022-04-27T09:19:50Z

integration_tests/test_solo_machine.py

+            self.sign_file = os.path.join(self.bin_dir, "libmnemonic_signer.so")
+        local_filename = url.split("/")[-1]
+        tar_file = f"/tmp/{local_filename}"
+        if os.path.exists(self.bin_file) and os.path.exists(self.sign_file):


this should probably check the sha256 checksum of the binaries as well

(or use Nix which will do it by default :))

builtins.fetchTarball { url = "..."; sha256 = "..."; };

tomtau · 2022-04-27T09:23:14Z

integration_tests/test_solo_machine.py

+    assert data["result"] == "success"
+    data = solo_machine.add_chain()
+    assert data["result"] == "success"
+    time.sleep(3)
+    solo_machine.connect_chain()
+    time.sleep(3)
+    balance_0 = int(solo_machine.get_balance())
+    assert balance_0 == 0
+    data = solo_machine.mint("0x20")
+    assert data["result"] == "success"
+    assert data["data"]["amount"] == "0x20"
+    balance_1 = int(solo_machine.get_balance())
+    assert balance_1 == 0x20
+    time.sleep(3)
+    data = solo_machine.burn("0x10")
+    assert data["result"] == "success"
+    assert data["data"]["amount"] == "0x10"
+    time.sleep(3)
+    balance_2 = int(solo_machine.get_balance())
+    assert balance_2 == balance_1 - 0x10
+    assert balance_1 > balance_2


some of these assertions should be against the node directly, i.e. to check that the chain/cluster node indeed received it (if there's a bug in the solo machine CLI or if it connects to a wrong node)

devashishdxt

Can you change the PR to follow the C4 Spec?

A patch commit message MUST consist of a single short (less than 50 characters) line stating the problem (“Problem: …") being solved, followed by a blank line and then the proposed solution (“Solution: …").

integration_tests/test_solo_machine.py

tomtau

the binary setup looks all right now.
there are two minor things to sort out:

not hardcoding the environment config in the test
checking and asserting on-chain denominations

I assume you've read contribution guidelines, so ideally you can also squash your commits after the changes and adjust the commit message as requested before.

tomtau · 2022-05-13T02:21:00Z

.github/workflows/nix.yml

+      - name: Tar debug files
+        if: failure()
+        run: tar cfz debug_files_solomachine.tar.gz -C /tmp/pytest-of-runner .
+      - uses: actions/upload-artifact@v2
+        if: failure()
+        with:
+          name: debug_files_solomachine
+          path: debug_files_solomachine.tar.gz
+          if-no-files-found: ignore


maybe ok, but are these debug files necessary?

I think so, same as other test settings.

tomtau · 2022-05-13T02:25:02Z

integration_tests/test_solo_machine.py

+    def set_env(self):
+        os.environ["SOLO_DB_URI"] = self.db_path
+        os.environ["SOLO_SIGNER"] = self.sign_file
+        os.environ[
+            "SOLO_MNEMONIC"
+        ] = "awesome there minute cash write immune tag \
+reopen price congress trouble reunion south wisdom donate credit below leave \
+wisdom eagle sail siege rice train"
+        os.environ["SOLO_HD_PATH"] = "m/44'/394'/0'/0/0"
+        os.environ["SOLO_ACCOUNT_PREFIX"] = "cro"
+        os.environ["SOLO_ADDRESS_ALGO"] = "secp256k1"
+        os.environ["SOLO_FEE_AMOUNT"] = "1000"
+        os.environ["SOLO_FEE_DENOM"] = "basecro"
+        os.environ["SOLO_GAS_LIMIT"] = "300000"
+        os.environ["SOLO_GRPC_ADDRESS"] = f"http://0.0.0.0:{self.grpc_port}"
+        os.environ["SOLO_RPC_ADDRESS"] = f"http://0.0.0.0:{self.rpc_port}"
+        os.environ["SOLO_TRUSTED_HASH"] = self.trusted_hash
+        os.environ["SOLO_TRUSTED_HEIGHT"] = self.trusted_height


could this be taken from pystarport's yaml config instead of hardcoded here?

tomtau · 2022-05-13T02:26:03Z

integration_tests/test_solo_machine.py

+    # fixme after this PR merged, currently only support hex string amount
+    # https://github.com/crypto-com/ibc-solo-machine/pull/82
+    def mint(self, amount="0x20"):
+        sub_cmd = f"ibc mint {self.chain_id} {amount} solotoken"
+        return self.run_sub_cmd(sub_cmd)
+
+    # fixme fix after this PR merged, currently only support hex string amount
+    # https://github.com/crypto-com/ibc-solo-machine/pull/82


the solo machine was upgraded to 0.1.2, so these comments can be removed and amounts can be passed in a decimal format?

tomtau · 2022-05-13T02:28:37Z

integration_tests/test_solo_machine.py

+    # get the chain balance
+    chain_solo_addr = cluster.address("solo-signer")
+    chain_balance_1 = cluster.balance(chain_solo_addr)
+    assert chain_balance_1 == 1500 * 10 ** 8


the magic number can be extracted into a constant? e.g. CRO_DECIMALS = 10 ** 8 unless it's already defined somewhere else?

tomtau · 2022-05-13T02:30:42Z

integration_tests/test_solo_machine.py

+            raise Exception(result)
+
+    def get_balance(self):
+        sub_cmd = f"chain balance {self.chain_id} solotoken"


I guess "solotoken" is the denomination? Can it be extracted into a constant or an argument/config parameter?

tomtau · 2022-05-13T02:31:44Z

integration_tests/test_solo_machine.py

+    # check the chain balance
+    chain_balance_2 = cli.balance(chain_solo_addr)


besides checking the on-chain balance, it'll be good to assert the denomination is correct as well

tomtau

could you rebase against the latest master, given it's updated the ibc-go module?

tomtau · 2022-05-16T09:05:53Z

integration_tests/test_solo_machine.py

+        os.environ[
+            "SOLO_MNEMONIC"
+        ] = "awesome there minute cash write immune tag \
+reopen price congress trouble reunion south wisdom donate credit below leave \
+wisdom eagle sail siege rice train"


why duplicate it in the test code and not take it from the test config ("solo_machine.yaml")?

I'll fix it.

tomtau

will merge, the other nits can be addressed in a future PR

linfeng-crypto requested review from tomtau, devashishdxt, calvinaco and yihuang as code owners April 27, 2022 06:11

tomtau reviewed Apr 27, 2022

View reviewed changes

tomtau suggested changes Apr 27, 2022

View reviewed changes

devashishdxt requested changes Apr 27, 2022

View reviewed changes

AlphaMale1st approved these changes Apr 29, 2022

View reviewed changes

tomtau requested a review from macong-cdc May 10, 2022 03:00

tomtau reviewed May 12, 2022

View reviewed changes

integration_tests/test_solo_machine.py Outdated Show resolved Hide resolved

linfeng-crypto force-pushed the test_solo_machine branch 2 times, most recently from d059b81 to 571d97c Compare May 12, 2022 10:04

tomtau reviewed May 13, 2022

View reviewed changes

AlphaMale1st approved these changes May 14, 2022

View reviewed changes

linfeng-crypto force-pushed the test_solo_machine branch 4 times, most recently from 36872b2 to c55c070 Compare May 16, 2022 08:51

tomtau suggested changes May 16, 2022

View reviewed changes

linfeng-crypto force-pushed the test_solo_machine branch 6 times, most recently from 807a62c to cbf1574 Compare May 16, 2022 10:12

add ibc solo machine integration(fix crypto-org-chain#613)

c02fda3

linfeng-crypto force-pushed the test_solo_machine branch from cbf1574 to c02fda3 Compare May 16, 2022 14:23

tomtau requested review from tomtau and devashishdxt May 17, 2022 02:24

tomtau approved these changes May 17, 2022

View reviewed changes

tomtau changed the title ~~add ibc solo machine integration(fix #613)~~ Problem: ibc solo machine integration not tested (fix #613) May 17, 2022

tomtau merged commit 3fdc974 into crypto-org-chain:master May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: ibc solo machine integration not tested (fix #613) #757

Problem: ibc solo machine integration not tested (fix #613) #757

linfeng-crypto commented Apr 27, 2022

codecov bot commented Apr 27, 2022 •

edited

Loading

tomtau Apr 27, 2022

linfeng-crypto Apr 28, 2022

tomtau left a comment

tomtau Apr 27, 2022

tomtau Apr 27, 2022 •

edited

Loading

tomtau Apr 27, 2022

devashishdxt left a comment

tomtau left a comment

tomtau May 13, 2022

linfeng-crypto May 16, 2022

tomtau May 13, 2022

tomtau May 13, 2022

tomtau May 13, 2022

tomtau May 13, 2022

tomtau May 13, 2022 •

edited

Loading

tomtau left a comment

tomtau May 16, 2022

linfeng-crypto May 16, 2022

tomtau left a comment

		# check the chain balance
		chain_balance_2 = cli.balance(chain_solo_addr)

Problem: ibc solo machine integration not tested (fix #613) #757

Problem: ibc solo machine integration not tested (fix #613) #757

Conversation

linfeng-crypto commented Apr 27, 2022

codecov bot commented Apr 27, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devashishdxt left a comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau May 13, 2022 • edited Loading

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2022 •

edited

Loading

tomtau Apr 27, 2022 •

edited

Loading

tomtau May 13, 2022 •

edited

Loading