Improve `nucypher-deploy allocations` by cygnusv · Pull Request #1319 · nucypher/nucypher

cygnusv · 2019-09-12T10:13:04Z

Provides a minimal UX for nucypher-deploy allocations. Here's an example of the allocation process.

Steps to reproduce:

# 1. Inspect published registry
nucypher-deploy --provider /path/to/geth.ipc --poa inspect

# 2. Deploy UserEscrowProxy
(Download registry manually first and use it as input next)
nucypher-deploy --provider /path/to/geth.ipc --poa contracts --deployer-address 0xfoobar --contract-name UserEscrowProxy --registry-infile contract_registry.json

# 3. Inspect new registry
nucypher-deploy --provider /path/to/geth.ipc --poa inspect --registry-infile contract_registry.json

# 4. Deploy allocations
nucypher-deploy --provider /path/to/geth.ipc --poa allocations --allocation-infile allocations.json --registry-infile contract_registry.json

Steps 1 and 2 are needed only because our current testnet doesn't have a UserEscrowProxy.
You can skip these by using this registry, and go directly to steps 3 and 4, to try allocations. Here's an example of allocation file.

…ficiary()

…r confirmation

codecov · 2019-09-13T19:57:55Z

Codecov Report

Merging #1319 into biznaga will decrease coverage by 2.12%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           biznaga    #1319      +/-   ##
===========================================
- Coverage    82.48%   80.36%   -2.13%     
===========================================
  Files           72       72              
  Lines        10268    10343      +75     
===========================================
- Hits          8470     8312     -158     
- Misses        1798     2031     +233

Impacted Files	Coverage Δ
nucypher/characters/control/emitters.py	`87.69% <ø> (ø)`	⬆️
nucypher/cli/characters/stake.py	`81.36% <ø> (ø)`	⬆️
nucypher/blockchain/eth/deployers.py	`92.3% <100%> (-0.15%)`	⬇️
nucypher/cli/deploy.py	`72.95% <66.66%> (-1.72%)`	⬇️
nucypher/blockchain/eth/actors.py	`84.66% <83.63%> (-0.49%)`	⬇️
nucypher/blockchain/eth/registry.py	`71.64% <87.5%> (-4.59%)`	⬇️
nucypher/cli/painting.py	`79.15% <88.46%> (+0.86%)`	⬆️
nucypher/keystore/keystore.py	`59.03% <0%> (-37.35%)`	⬇️
nucypher/crypto/utils.py	`68.51% <0%> (-24.08%)`	⬇️
nucypher/config/base.py	`85.39% <0%> (-13.49%)`	⬇️
... and 18 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 5546055...f4214cd. Read the comment docs.

nucypher/blockchain/eth/deployers.py

nucypher/blockchain/eth/sol/source/contracts/UserEscrow.sol

nucypher/cli/painting.py

nucypher/blockchain/eth/actors.py

derekpierre · 2019-09-16T13:20:19Z

nucypher/blockchain/eth/registry.py

            allocation_data = self.read()
        except self.RegistryError:
-            self.log.info("Blank allocation registry encountered: enrolling {}:{}".format(beneficiary_address, contract_address))
+            self.log.info(f"Blank allocation registry encountered: enrolling {beneficiary_address}:{contract_address}")


Suggested change

self.log.info(f"Blank allocation registry encountered: enrolling {beneficiary_address}:{contract_address}")

self.log.info(f"Blank allocation registry encountered when enrolling {beneficiary_address}:{contract_address}")

^ does RegistryError only occur if blank - is it possible there is something but the format is incorrect?

Yeah, it's possible since were catching a base class that another condition has occurred. Needs an eye for review - The addition of class BlankRegistry(RegistryError) will help to alleviate this.

Apart from the code suggestion, not sure how to proceed here.
@KPrasch are you suggesting to add class BlankRegistry(RegistryError) now?

My take would be to update the message to be more general. Blank allocation doesn't seem to be the only condition for the Error. If you want to differentiate between Blank registry and some other error scenario, then you can do the subclass thing.

tuxxy

See the comment on classmethod vs staticmethod.

nucypher/blockchain/eth/actors.py

nucypher/blockchain/eth/registry.py

nucypher/cli/deploy.py

nucypher/cli/painting.py

KPrasch · 2019-09-16T15:40:38Z

nucypher/blockchain/eth/actors.py

+
+            if failed:
+                # TODO: More with these failures: send to isolated logfile, and reattempt
+                self.log.critical(f"FAILED TOKEN ALLOCATION - {len(failed)} Allocations failed.")


Let's write the failed entries to a separate JSON file for easier re-attempt.

nucypher/characters/control/emitters.py

KPrasch · 2019-09-16T15:46:25Z

nucypher/cli/deploy.py

+
+    if action == "contracts":
+        if contract_name and registry_infile is None:  # There's a current deployment and you want to deploy a new contract on it.
+            message = "--registry-infile is required when deploying a contract to an existing registry."


Adding a command to download the latest registry to disk will help prevent accidents.

KPrasch · 2019-09-16T15:49:09Z

nucypher/cli/painting.py

+        name = allocation.get('name', 'No name provided')
+        emitter.echo(f"{beneficiary} | {name:20} | {contract_address}")
+    for allocation in failed:
+        beneficiary = allocation['beneficiary_address']


How can we get test coverage in this area , 🤔 ? We need a way to mock a transaction failed part of the way through.

nucypher/cli/deploy.py

nucypher/blockchain/eth/registry.py

cygnusv added 9 commits September 10, 2019 22:48

Handle receipts in allocation methods, not txhashes. Better log messages

2cb2c7d

Use @validate_checksum_address in UserEscrowProxyDeployer.assign_bene…

b449259

…ficiary()

Paint allocation data prior deploying UserEscrow contracts and ask fo…

2ade7d9

…r confirmation

Allow empty message in StdoutEmitter.echo()

01a4807

First version of interactive and verbose allocation deployment

4a9f88f

Paint deployed allocations after nucypher-deploy allocations

5a01791

Support optional "names" for allocations

221cf84

Save allocation receipts after deployment

4724bf8

Touches to allocation tests

44a14a7

cygnusv mentioned this pull request Sep 13, 2019

[EPIC] Biznaga: Integration of testnet with staking contracts #1327

Merged

4 tasks

Touches to nucypher-deploy: checks on registry input/output filepaths

849ad0b

cygnusv force-pushed the allocations branch from 3640c61 to 849ad0b Compare September 13, 2019 21:00

cygnusv requested review from KPrasch, derekpierre, fjarri, jMyles, michwill, mswilkison, tuxxy, vepkenez and vzotova September 13, 2019 21:10

mswilkison reviewed Sep 13, 2019

View reviewed changes

nucypher/blockchain/eth/deployers.py Outdated Show resolved Hide resolved

michwill approved these changes Sep 13, 2019

View reviewed changes

vzotova approved these changes Sep 14, 2019

View reviewed changes

nucypher/blockchain/eth/sol/source/contracts/UserEscrow.sol Show resolved Hide resolved

nucypher/cli/painting.py Show resolved Hide resolved

derekpierre reviewed Sep 16, 2019

View reviewed changes

tuxxy reviewed Sep 16, 2019

View reviewed changes

KPrasch reviewed Sep 16, 2019

View reviewed changes

nucypher/cli/painting.py Show resolved Hide resolved

KPrasch reviewed Sep 16, 2019

View reviewed changes

nucypher/characters/control/emitters.py Show resolved Hide resolved

KPrasch reviewed Sep 16, 2019

View reviewed changes

nucypher/cli/deploy.py Show resolved Hide resolved

KPrasch reviewed Sep 16, 2019

View reviewed changes

nucypher/blockchain/eth/registry.py Outdated Show resolved Hide resolved

Addressing RFCs: better exception and log messages, painting, etc.

f4214cd

derekpierre approved these changes Sep 16, 2019

View reviewed changes

KPrasch approved these changes Sep 17, 2019

View reviewed changes

KPrasch merged commit dc725e9 into nucypher:biznaga Sep 17, 2019

This was referenced Sep 17, 2019

Write failed allocation entries to a separate JSON file for easier re-attempt #1340

Closed

Add command to download the latest registry to disk #1341

Closed

Test failed allocations #1342

Closed

cygnusv mentioned this pull request Sep 24, 2019

CLI staking via UserEscrow #1227

Merged

	self.log.info(f"Blank allocation registry encountered: enrolling {beneficiary_address}:{contract_address}")
	self.log.info(f"Blank allocation registry encountered when enrolling {beneficiary_address}:{contract_address}")

Conversation

cygnusv commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuxxy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cygnusv commented Sep 12, 2019 •

edited

Loading

codecov bot commented Sep 13, 2019 •

edited

Loading

tuxxy left a comment •

edited

Loading