Method to remove unused sub-stakes in StakingEscrow #2384

vzotova · 2020-10-18T12:22:47Z

No description provided.

nucypher/cli/commands/stake.py

nucypher/cli/literature.py

nucypher/cli/commands/stake.py

docs/source/guides/network_node/staking_guide.rst

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

derekpierre

🎸

arjunhassard

💹

arjunhassard · 2020-10-20T15:57:29Z

docs/source/guides/network_node/staking_guide.rst

+Remove unused sub-stake
+***********************
+
+Unused sub-stakes still cost some amount of gas during operations. 


Suggested change

Unused sub-stakes still cost some amount of gas during operations.

Combining or editing sub-stakes can lead to 'unused', inactive sub-stakes remaining on-chain. These unused sub-stakes add unnecessary gas costs to daily operations.

Technicaly this is wrong, because merging stakes will produce two sub-stakes and sometimes both of them will be active at least one period (when node is online at least 2 periods), also inactive sub-stakes will be shown if special flag will be specified. Anyway I'd prefer not to have here info about merge stake

I had another go! But no pressure to make this change, I just think some context is required.

✔️(changed one word)

arjunhassard · 2020-10-20T15:57:52Z

docs/source/guides/network_node/staking_guide.rst

+***********************
+
+Unused sub-stakes still cost some amount of gas during operations. 
+To remove unused sub-stake:


Suggested change

To remove unused sub-stake:

To remove unused sub-stakes:

Command removes only one sub-stake by each tx

nucypher/cli/literature.py

arjunhassard · 2020-10-20T16:10:06Z

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

+            subStake.periods = lastSubStake.periods;
+            subStake.lockedValue = lastSubStake.lockedValue;
+        }
+        info.subStakes.pop();


Does using pop mean that the function can only remove one unused sub-stake at a time? Unless I'm mistaken this just deletes one element from the end of an array.

If so, how does one keep track of their removal of multiple sub-stakes? It doesn't show you unused sub-stakes when you check via nucypher stake list

You're right - this deletes last element in array but before this - copies last element to place of removed unused sub-stake.
To show inactive sub-stakes you can use option --all for nucypher stake list or interactive mode for nucypher stake remove-unused

Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: Arjun Hassard <arjunhassard@gmail.com>

KPrasch · 2020-10-21T01:46:28Z

docs/source/guides/network_node/staking_guide.rst

+Remove unused sub-stake
+***********************
+
+Combining or editing sub-stakes can lead to 'unused', inactive sub-stakes remaining on-chain. 


What guidance can you offer as to who and when this operation needs to be performed, after every stake editing operation?

Usage of the word "combine" here makes me want to rename "merge".

Remove all inactive (shown using by nucypher stake list --all) allways if you want pay less as possible for gas,
unlock, increase, merge, slashing could create temporary sub-stakes (which will be inactive soon) or make some sub-stakes inactive, so ideally do some operations and only after all of them - check inactive and potentially inactive sub-stakes (last period active)

✔️

KPrasch · 2020-10-21T01:48:41Z

nucypher/cli/literature.py

@@ -271,6 +271,10 @@

 SUCCESSFUL_STAKES_MERGE = 'Successfully Merged Stakes'

+CONFIRM_REMOVE = "Publish removal of {stake_index} stake?"


Suggested change

CONFIRM_REMOVE = "Publish removal of {stake_index} stake?"

CONFIRM_REMOVE_SUBSTAKE = "Publish removal of {stake_index} stake?"

KPrasch · 2020-10-21T01:50:05Z

tests/acceptance/blockchain/agents/test_staking_escrow_agent.py

@@ -264,16 +263,6 @@ def test_deposit_and_increase(agency, testerchain, test_registry, token_economic
    assert staking_agent.get_locked_tokens(staker_account, 1) == locked_tokens + amount


-def test_disable_restaking(agency, testerchain, test_registry):


Did this test get relocated somewhere?

Few lines below - same method with the same name and this one was already disabled

Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: Arjun Hassard <arjunhassard@gmail.com> Co-authored-by: Kieran Prasch <kieran@nucypher.com>

vzotova added ETH Contracts CLI This effects the nucypher CLI labels Oct 18, 2020

vzotova added this to the Mainnet Second-tier (previously "Mainnet Month 1") milestone Oct 18, 2020

vzotova self-assigned this Oct 18, 2020

vzotova marked this pull request as ready for review October 19, 2020 17:54

vzotova changed the title ~~[WIP] Method to remove unused sub-stakes in StakingEscrow~~ Method to remove unused sub-stakes in StakingEscrow Oct 19, 2020

vzotova requested review from cygnusv, derekpierre, KPrasch, arjunhassard, mswilkison and tuxxy October 19, 2020 17:54

derekpierre reviewed Oct 19, 2020

View reviewed changes

nucypher/cli/commands/stake.py Outdated Show resolved Hide resolved

nucypher/cli/literature.py Outdated Show resolved Hide resolved

nucypher/cli/commands/stake.py Outdated Show resolved Hide resolved

docs/source/guides/network_node/staking_guide.rst Outdated Show resolved Hide resolved

vzotova added a commit to vzotova/nucypher that referenced this pull request Oct 19, 2020

Apply suggestions from code review nucypher#2384

9348a53

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

derekpierre approved these changes Oct 19, 2020

View reviewed changes

arjunhassard approved these changes Oct 20, 2020

View reviewed changes

vzotova added 4 commits October 20, 2020 20:33

StakingEscrow: method to remove unused sub-stakes

30a06fe

Method to remove unused sub-stakes for agent and actor

b443ab7

CLI method to remove unused sub-stake

d1ede31

Small docs update

5741182

vzotova added a commit to vzotova/nucypher that referenced this pull request Oct 20, 2020

Apply suggestions from code review nucypher#2384

fc08be8

Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: Arjun Hassard <arjunhassard@gmail.com>

vzotova force-pushed the remove-sub-stakes branch from 9348a53 to fc08be8 Compare October 20, 2020 17:33

vzotova added a commit to vzotova/nucypher that referenced this pull request Oct 20, 2020

Apply suggestions from code review nucypher#2384

e38594b

Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: Arjun Hassard <arjunhassard@gmail.com>

vzotova force-pushed the remove-sub-stakes branch from fc08be8 to e38594b Compare October 20, 2020 21:40

KPrasch reviewed Oct 21, 2020

View reviewed changes

Apply suggestions from code review nucypher#2384

8c80b03

Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: Arjun Hassard <arjunhassard@gmail.com> Co-authored-by: Kieran Prasch <kieran@nucypher.com>

vzotova force-pushed the remove-sub-stakes branch from e38594b to 8c80b03 Compare October 21, 2020 07:06

KPrasch approved these changes Oct 21, 2020

View reviewed changes

KPrasch merged commit e531035 into nucypher:main Oct 21, 2020

KPrasch mentioned this pull request Oct 23, 2020

Enroll bare deployment of StakingEscrow 5.5.1 #2402

Merged

vzotova deleted the remove-sub-stakes branch December 7, 2020 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Method to remove unused sub-stakes in StakingEscrow #2384

Method to remove unused sub-stakes in StakingEscrow #2384

vzotova commented Oct 18, 2020

derekpierre left a comment

arjunhassard left a comment

arjunhassard Oct 20, 2020 •

edited

vzotova Oct 20, 2020

arjunhassard Oct 20, 2020

vzotova Oct 20, 2020

arjunhassard Oct 20, 2020

vzotova Oct 20, 2020

arjunhassard Oct 20, 2020

vzotova Oct 20, 2020

KPrasch Oct 21, 2020 •

edited

vzotova Oct 21, 2020 •

edited

KPrasch Oct 21, 2020

vzotova Oct 21, 2020

KPrasch Oct 21, 2020

vzotova Oct 21, 2020

	Unused sub-stakes still cost some amount of gas during operations.
	Combining or editing sub-stakes can lead to 'unused', inactive sub-stakes remaining on-chain. These unused sub-stakes add unnecessary gas costs to daily operations.

		@@ -271,6 +271,10 @@

		SUCCESSFUL_STAKES_MERGE = 'Successfully Merged Stakes'

		CONFIRM_REMOVE = "Publish removal of {stake_index} stake?"

	CONFIRM_REMOVE = "Publish removal of {stake_index} stake?"
	CONFIRM_REMOVE_SUBSTAKE = "Publish removal of {stake_index} stake?"

		@@ -264,16 +263,6 @@ def test_deposit_and_increase(agency, testerchain, test_registry, token_economic
		assert staking_agent.get_locked_tokens(staker_account, 1) == locked_tokens + amount


		def test_disable_restaking(agency, testerchain, test_registry):

Method to remove unused sub-stakes in StakingEscrow #2384

Method to remove unused sub-stakes in StakingEscrow #2384

Conversation

vzotova commented Oct 18, 2020

derekpierre left a comment

Choose a reason for hiding this comment

arjunhassard left a comment

Choose a reason for hiding this comment

arjunhassard Oct 20, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Oct 21, 2020 • edited

Choose a reason for hiding this comment

vzotova Oct 21, 2020 • 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

arjunhassard Oct 20, 2020 •

edited

KPrasch Oct 21, 2020 •

edited

vzotova Oct 21, 2020 •

edited