Few more fixes for `staker` by vzotova · Pull Request #2170 · nucypher/nucypher

vzotova · 2020-08-09T12:14:39Z

Fixes #2114
Fixes #1739
Fixes #1436
Fixes #2066
Fixes #1545

codecov · 2020-08-09T12:25:25Z

Codecov Report

Merging #2170 into main will increase coverage by 3.78%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #2170      +/-   ##
==========================================
+ Coverage   80.11%   83.89%   +3.78%     
==========================================
  Files         103      103              
  Lines       15172    15188      +16     
==========================================
+ Hits        12155    12742     +587     
+ Misses       3017     2446     -571

Impacted Files	Coverage Δ
nucypher/blockchain/eth/multisig.py	`60.71% <ø> (-0.47%)`	⬇️
nucypher/blockchain/eth/token.py	`90.30% <ø> (+0.60%)`	⬆️
nucypher/cli/literature.py	`100.00% <ø> (ø)`
nucypher/cli/processes.py	`75.22% <ø> (ø)`
nucypher/blockchain/eth/actors.py	`86.76% <87.50%> (+0.41%)`	⬆️
nucypher/cli/painting/status.py	`93.69% <95.83%> (+0.42%)`	⬆️
nucypher/blockchain/eth/agents.py	`92.43% <100.00%> (+1.59%)`	⬆️
nucypher/blockchain/eth/registry.py	`75.00% <100.00%> (+1.51%)`	⬆️
nucypher/cli/commands/stake.py	`90.57% <100.00%> (+13.08%)`	⬆️
nucypher/cli/commands/status.py	`69.31% <100.00%> (-0.35%)`	⬇️
... and 33 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 32e9210...bfa2da2. Read the comment docs.

derekpierre

🎸

KPrasch · 2020-08-17T15:28:39Z

nucypher/blockchain/eth/agents.py


        self.log = Logger(self.__class__.__name__)
-        self.registry = registry
+        self.registry_repr = str(registry)


Heh - slightly confusion name here - since str calls __str__ and not _repr__.

you're right
changed
✔️

KPrasch · 2020-08-17T15:30:39Z

nucypher/cli/commands/stake.py

 @group_staker_options
 @option_config_file
-@click.option('--all', help="List all stakes, including unlocked", is_flag=True)
+@click.option('--show-all', help="List all stakes, including unlocked and inactive", is_flag=True)


I find "show" to be a bit pedantic here and prefer simply --all. There is an additional kwarg for click options to map to an alias to avoid name-shadowing the all function here.

tests/acceptance/cli/ursula/test_stakeholder_and_ursula.py

KPrasch · 2020-08-17T15:33:50Z

tests/unit/test_registry_basics.py

        test_registry.search(contract_address=test_addr)
+
+    # Check id of new registry with the same content
+    new_registry = InMemoryContractRegistry()  # TODO finish this


This TODO Needs an Issue to track?

forgot to remove this
✔️

Co-authored-by: K Prasch <kieranprasch@gmail.com>

KPrasch · 2020-08-18T19:33:00Z

tests/integration/cli/test_stake_cli_functionality.py

+                                                 start_of_period=True)
+            enactment = start_datetime.local_datetime().strftime("%b %d %Y")
+            termination = unlock_datetime.local_datetime().strftime("%b %d %Y")
+            search = f"{index}\\s+│\\s+" \


👍 Sufficiently generic to remain flexible

cygnusv · 2020-08-19T15:41:27Z

nucypher/blockchain/eth/actors.py

 from web3.types import TxReceipt

-from constant_sorrow.constants import FULL, NO_WORKER_BONDED, WORKER_NOT_RUNNING
+from constant_sorrow.constants import FULL, WORKER_NOT_RUNNING


cygnusv · 2020-08-19T15:44:51Z

nucypher/cli/painting/staking.py

        emitter.echo(snippet_with_line, bold=True)
    emitter.echo(f"Staker {staker.checksum_address} ════", bold=True, color='red' if missing else 'green')
-    emitter.echo(f"Worker {staker.worker_address} ════")
+    worker = staker.worker_address if staker.worker_address != NULL_ADDRESS else 'NO_WORKER_BONDED'


Suggested change

worker = staker.worker_address if staker.worker_address != NULL_ADDRESS else 'NO_WORKER_BONDED'

worker = staker.worker_address if staker.worker_address != NULL_ADDRESS else 'not bonded'

So in the next line it's printed Worker not bonded instead of Worker NO_WORKER_BONDED.

I left NO_WORKER_BONDED only to not change all documentation, but if you think it worth it - then I'll do that

Don't worry to change the documentation, the change is too small to justify perfect consistency. We can also leave as it is and merge. As you prefer.

…NULL_ADDRESS instead

…o `--show-all`

…ight immutability of agents

Co-authored-by: K Prasch <kieranprasch@gmail.com> Co-authored-by: David Núñez <david@nucypher.com>

vzotova added the Staking label Aug 9, 2020

vzotova added this to the Before activating StakersEscrow milestone Aug 9, 2020

vzotova self-assigned this Aug 9, 2020

vzotova force-pushed the few-more-stakes branch 8 times, most recently from 320d741 to a46da6c Compare August 16, 2020 10:00

vzotova changed the title ~~[WIP] Few more fixes for staker~~ Few more fixes for staker Aug 16, 2020

vzotova marked this pull request as ready for review August 16, 2020 10:18

vzotova requested review from KPrasch, cygnusv, derekpierre, mswilkison and tuxxy August 16, 2020 10:18

derekpierre approved these changes Aug 17, 2020

View reviewed changes

KPrasch reviewed Aug 17, 2020

View reviewed changes

tests/acceptance/cli/ursula/test_stakeholder_and_ursula.py Show resolved Hide resolved

KPrasch reviewed Aug 17, 2020

View reviewed changes

vzotova added a commit to vzotova/nucypher that referenced this pull request Aug 18, 2020

Apply suggestions from code review nucypher#2170

1aeadfa

Co-authored-by: K Prasch <kieranprasch@gmail.com>

vzotova added a commit to vzotova/nucypher that referenced this pull request Aug 18, 2020

Apply suggestions from code review nucypher#2170

a4a88a3

Co-authored-by: K Prasch <kieranprasch@gmail.com>

vzotova force-pushed the few-more-stakes branch from 1aeadfa to a4a88a3 Compare August 18, 2020 11:29

KPrasch reviewed Aug 18, 2020

View reviewed changes

KPrasch approved these changes Aug 18, 2020

View reviewed changes

cygnusv approved these changes Aug 19, 2020

View reviewed changes

vzotova and others added 7 commits August 20, 2020 00:46

Removed NO_WORKER_BONDED constant from Staker.worker_address, used …

bb6cee4

…NULL_ADDRESS instead

Show staked tokens in current and next periods for status command

ef96651

nucypher stake list: show sub-stake status, rename --all option t…

434b7c6

…o `--show-all`

Fix link to staking guide in nucypher stake create

ea6f3be

Cashing registry id

f85d1f6

Change registry field to representation of registry in Agent to highl…

bf76e2d

…ight immutability of agents

Apply suggestions from code review nucypher#2170

bfa2da2

Co-authored-by: K Prasch <kieranprasch@gmail.com> Co-authored-by: David Núñez <david@nucypher.com>

vzotova force-pushed the few-more-stakes branch from a4a88a3 to bfa2da2 Compare August 19, 2020 21:49

vzotova merged commit 8d092d9 into nucypher:main Aug 19, 2020

vzotova deleted the few-more-stakes branch August 19, 2020 22:01

	worker = staker.worker_address if staker.worker_address != NULL_ADDRESS else 'NO_WORKER_BONDED'
	worker = staker.worker_address if staker.worker_address != NULL_ADDRESS else 'not bonded'

Conversation

vzotova commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

derekpierre left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

cygnusv Aug 19, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vzotova commented Aug 9, 2020 •

edited

Loading

codecov bot commented Aug 9, 2020 •

edited

Loading

cygnusv Aug 19, 2020 •

edited

Loading