Maintain Gas Estimation Metrics #594

KPrasch · 2018-12-03T18:37:07Z

Restores the estimate_gas.py scenario-based metrics script. Uses the twisted logger facilities to collect, store, and view gas estimation data. Log files can possibly be used as a build artefact, or extended for further analysis. Fixes #602

Twisted based logging and data collection
Save timestamped JSON logs and result reports
CircleCI build automation and storage of the resulting artefacts

Example Build: https://circleci.com/gh/nucypher/nucypher/8175#artifacts/containers/0

Example Artefact: https://8175-100446516-gh.circle-artifacts.com/0/home/circleci/nucypher-depends/tests/metrics/results/1544742453-estimate-gas.json

KPrasch · 2018-12-10T16:59:52Z

Do you think it's useful to add this script as a build to our CI workflow as its own job?

One benefit of this is that It will introduce an automation step for this script, preventing it from growing stale with future development.

vzotova · 2018-12-11T11:23:22Z

It will help to keep up-to-date this script, so I like idea with CI

tuxxy

Looks great! 🐧

tuxxy · 2018-12-13T20:55:10Z

tests/metrics/estimate_gas_tester.py

-                                             providers=(pyevm_provider,))  # use custom test provider
+    solidity_compiler = SolidityCompiler(test_contract_dir=CONTRACTS_DIR)
+    memory_registry = InMemoryEthereumContractRegistry()
+    circumflex = BlockchainDeployerInterface(provider_uri="tester://pyevm",


circumflex circumflexing

tuxxy · 2018-12-13T20:55:51Z

tests/metrics/estimate_gas_tester.py

+    def __call__(self, event, *args, **kwargs):
+        if event.get('log_namespace') == LOG_NAME:
+            message = event.get("log_format")
+            if not re.match(r'.*\s=\s\d+$', message):


tuxxy

🐧

cygnusv

Great work!
My main comment is that I think there's room for improvement re: code verbosity and repeated patterns.

cygnusv · 2018-12-14T14:47:14Z

tests/metrics/estimate_gas_tester.py

        .transact({'from': ursula2})
    testerchain.wait_for_receipt(tx)
-    tx = token_agent.contract.functions.approve(miner_agent.contract_address, constants.MIN_ALLOWED_LOCKED * 6)\
+    tx = token_agent.contract.functions.approve(miner_agent.contract_address, MIN_ALLOWED_LOCKED * 6)\
        .transact({'from': ursula3})
    testerchain.wait_for_receipt(tx)


I think that the clarity of previous lines would benefit from having a variable functions = token_agent.contract.functions (or some other similar name), so lines length can be reduced reduced, and hopefully, become one-liners.

I tried only to maintain and not change the logic of this script at all during the course of this PR.

Of course this is not a pubic API, we typically use higher-level methods to wrap these contract calls - are made at one of the lowest levels in the stack, executing EVM contract code.

I named the contract function callers ✔️

cygnusv · 2018-12-14T14:47:41Z

tests/metrics/estimate_gas_tester.py

              .estimateGas({'from': ursula3})))
-    tx = miner_agent.contract.functions.deposit(constants.MIN_ALLOWED_LOCKED * 2, int(constants.MIN_LOCKED_PERIODS))\
+    tx = miner_agent.contract.functions.deposit(MIN_ALLOWED_LOCKED * 2, int(MIN_LOCKED_PERIODS))\


Same, but for miner_agent.contract.functions.

cygnusv · 2018-12-14T14:49:30Z

tests/metrics/estimate_gas_tester.py

              .estimateGas({'from': ursula1})))
    tx = miner_agent.contract.functions.confirmActivity().transact({'from': ursula1})
    testerchain.wait_for_receipt(tx)
    print("Divide stake (next period is confirmed) = " +
-          str(miner_agent.contract.functions.divideStake(0, int(constants.MIN_ALLOWED_LOCKED), 2)
+          str(miner_agent.contract.functions.divideStake(0, int(MIN_ALLOWED_LOCKED), 2)
              .estimateGas({'from': ursula1})))



There seems to be a lot of repeated logic for Ursulas 1, 2, and 3. This can be error-prone. Perhaps do this more generically?

I'm interested in your suggestions - What can we do better here?

We do want to keep this script as simple as possible and not let it become a time sink.

Tagging @szotov - the original author

This script is some common cases and some edge cases that shows where optimization goes. Almost each line depends on executing previous code because each function changes storage of contracts. So it's why this looks like repeated functions but it's not

But is it possible to make Ursula a parameter here? And then reuse the code for Ursulas 1, 2 and 3.

tests/metrics/estimate_gas.py

cygnusv · 2018-12-14T15:02:51Z

tests/metrics/estimate_gas_tester.py

+                return
+            label, gas = (s.strip() for s in message.split('='))
+            print('{label} {dots} {gas:,}'.format(label=label,
+                                                  dots='.' * (65 - len(label)),


Instead of explicitly computing the number of dots, you can just right-pad the label with '*':

label.ljust(65, '*')

Nice! ✔️

Co-Authored-By: KPrasch <kieranprasch@gmail.com>

New commits address RFC

jMyles

I can't say I've fine-toothed it, but I don't see anything totally outrageous.

KPrasch force-pushed the estimate-gas branch 8 times, most recently from c9313a7 to e997104 Compare December 8, 2018 21:09

KPrasch self-assigned this Dec 8, 2018

KPrasch force-pushed the estimate-gas branch from e997104 to 51a6b35 Compare December 8, 2018 21:31

KPrasch added the Enhancement New or improved features label Dec 8, 2018

KPrasch changed the title ~~[WIP] Maintain Gas Estimation Metrics~~ Maintain Gas Estimation Metrics Dec 10, 2018

KPrasch requested review from jMyles, cygnusv, tuxxy and vzotova December 10, 2018 16:48

vzotova previously approved these changes Dec 11, 2018

View reviewed changes

michwill previously approved these changes Dec 13, 2018

View reviewed changes

vzotova mentioned this pull request Dec 13, 2018

Slashing in the MinersEscrow #591

Merged

tuxxy previously approved these changes Dec 13, 2018

View reviewed changes

KPrasch dismissed stale reviews from tuxxy, michwill, and vzotova via 1657c4c December 13, 2018 20:59

KPrasch force-pushed the estimate-gas branch from 51a6b35 to 1657c4c Compare December 13, 2018 20:59

tuxxy previously approved these changes Dec 13, 2018

View reviewed changes

KPrasch dismissed tuxxy’s stale review via 53e67f2 December 13, 2018 22:56

tuxxy approved these changes Dec 13, 2018

View reviewed changes

Maintains estimate gas tester metrics script; Update to latest API.

d8b9a7e

KPrasch added 2 commits December 13, 2018 16:01

Use twisted loggers to collect and analyze gas estimation data

5002639

Class-based refactor; Timestamped reports; Automation job.

bf62f92

KPrasch force-pushed the estimate-gas branch from 53e67f2 to bf62f92 Compare December 14, 2018 00:01

cygnusv previously requested changes Dec 14, 2018

View reviewed changes

cygnusv and others added 2 commits December 14, 2018 08:06

Update tests/metrics/estimate_gas.py

9f32f76

Co-Authored-By: KPrasch <kieranprasch@gmail.com>

Respond to RFCs: Code Style and Readability Improvements

0c7c5a9

Use Regex.compile; Docstrings; Even more cleanup

6f69704

jMyles approved these changes Dec 14, 2018

View reviewed changes

cygnusv approved these changes Dec 16, 2018

View reviewed changes

KPrasch merged commit 3051b10 into nucypher:master Dec 16, 2018

KPrasch deleted the estimate-gas branch July 21, 2020 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain Gas Estimation Metrics #594

Maintain Gas Estimation Metrics #594

KPrasch commented Dec 3, 2018 •

edited

KPrasch commented Dec 10, 2018

vzotova commented Dec 11, 2018

tuxxy left a comment

tuxxy Dec 13, 2018

tuxxy Dec 13, 2018

tuxxy left a comment

cygnusv left a comment

cygnusv Dec 14, 2018

KPrasch Dec 14, 2018

KPrasch Dec 14, 2018

cygnusv Dec 14, 2018

KPrasch Dec 14, 2018

cygnusv Dec 14, 2018

KPrasch Dec 14, 2018 •

edited

KPrasch Dec 14, 2018

vzotova Dec 14, 2018

cygnusv Dec 16, 2018

cygnusv Dec 14, 2018

KPrasch Dec 14, 2018

jMyles left a comment

Maintain Gas Estimation Metrics #594

Maintain Gas Estimation Metrics #594

Conversation

KPrasch commented Dec 3, 2018 • edited

KPrasch commented Dec 10, 2018

vzotova commented Dec 11, 2018

tuxxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxxy left a comment

Choose a reason for hiding this comment

cygnusv 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

KPrasch Dec 14, 2018 • edited

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

jMyles left a comment

Choose a reason for hiding this comment

KPrasch commented Dec 3, 2018 •

edited

KPrasch Dec 14, 2018 •

edited