Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method to remove unused sub-stakes in StakingEscrow #2384

Merged
merged 5 commits into from Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/source/guides/network_node/staking_guide.rst
Expand Up @@ -83,6 +83,8 @@ All staking-related operations done by Staker are performed through the ``nucyph
+----------------------+-------------------------------------------------------------------------------+
| ``merge`` | Merge two stakes into one |
+----------------------+-------------------------------------------------------------------------------+
| ``remove-unused`` | Remove unused stake |
+----------------------+-------------------------------------------------------------------------------+

**Stake Command Options**

Expand Down Expand Up @@ -488,6 +490,18 @@ This can help to decrease gas consumption in some operations. To merge two stake
(nucypher)$ nucypher stake merge --hw-wallet


Remove unused sub-stake
***********************

Merging 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.
To remove unused sub-stake:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
To remove unused sub-stake:
To remove unused sub-stakes:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command removes only one sub-stake by each tx


.. code:: bash

(nucypher)$ nucypher stake remove-unused --hw-wallet


Collect rewards earned by the staker
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
26 changes: 26 additions & 0 deletions nucypher/blockchain/eth/actors.py
Expand Up @@ -1275,6 +1275,32 @@ def disable_snapshots(self) -> TxReceipt:
receipt = self._set_snapshots(value=False)
return receipt

@only_me
@save_receipt
def remove_unused_stake(self, stake: Stake) -> TxReceipt:
self._ensure_stake_exists(stake)

# Read on-chain stake and validate
stake.sync()
if not stake.status().is_child(Stake.Status.INACTIVE):
raise ValueError(f"Stake with index {stake.index} is still active")

receipt = self._remove_unused_stake(stake_index=stake.index)

# Update staking cache element
self.refresh_stakes()
return receipt

@only_me
@save_receipt
def _remove_unused_stake(self, stake_index: int) -> TxReceipt:
# TODO #1497 #1358
# if self.is_contract:
# else:
receipt = self.staking_agent.remove_unused_stake(staker_address=self.checksum_address,
stake_index=stake_index)
return receipt

def non_withdrawable_stake(self) -> NU:
staked_amount: NuNits = self.staking_agent.non_withdrawable_stake(staker_address=self.checksum_address)
return NU.from_nunits(staked_amount)
Expand Down
7 changes: 7 additions & 0 deletions nucypher/blockchain/eth/agents.py
Expand Up @@ -711,6 +711,13 @@ def set_snapshots(self, staker_address: ChecksumAddress, activate: bool) -> TxRe
# TODO: Handle SnapshotSet event (see #1193)
return receipt

@contract_api(TRANSACTION)
def remove_unused_stake(self, staker_address: ChecksumAddress, stake_index: int) -> TxReceipt:
contract_function: ContractFunction = self.contract.functions.removeUnusedSubStake(stake_index)
receipt: TxReceipt = self.blockchain.send_transaction(contract_function=contract_function,
sender_address=staker_address)
return receipt

@contract_api(CONTRACT_CALL)
def staking_parameters(self) -> StakingEscrowParameters:
parameter_signatures = (
Expand Down
26 changes: 25 additions & 1 deletion nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol
Expand Up @@ -45,7 +45,7 @@ interface WorkLockInterface {
/**
* @notice Contract holds and locks stakers tokens.
* Each staker that locks their tokens will receive some compensation
* @dev |v5.4.4|
* @dev |v5.5.1|
*/
contract StakingEscrow is Issuer, IERC900History {

Expand Down Expand Up @@ -1048,6 +1048,30 @@ contract StakingEscrow is Issuer, IERC900History {
}
}

/**
* @notice Remove unused sub-stake to decrease gas cost for several methods
*/
function removeUnusedSubStake(uint16 _index) external onlyStaker {
StakerInfo storage info = stakerInfo[msg.sender];

uint256 lastIndex = info.subStakes.length - 1;
SubStakeInfo storage subStake = info.subStakes[_index];
require(subStake.lastPeriod != 0 &&
(info.currentCommittedPeriod == 0 ||
subStake.lastPeriod < info.currentCommittedPeriod) &&
(info.nextCommittedPeriod == 0 ||
subStake.lastPeriod < info.nextCommittedPeriod));

if (_index != lastIndex) {
SubStakeInfo storage lastSubStake = info.subStakes[lastIndex];
subStake.firstPeriod = lastSubStake.firstPeriod;
subStake.lastPeriod = lastSubStake.lastPeriod;
subStake.periods = lastSubStake.periods;
subStake.lockedValue = lastSubStake.lockedValue;
}
info.subStakes.pop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* @notice Withdraw available amount of tokens to staker
* @param _value Amount of tokens to withdraw
Expand Down
57 changes: 56 additions & 1 deletion nucypher/cli/commands/stake.py
Expand Up @@ -73,7 +73,7 @@
INSUFFICIENT_BALANCE_TO_CREATE, PROMPT_STAKE_CREATE_VALUE, PROMPT_STAKE_CREATE_LOCK_PERIODS,
ONLY_DISPLAYING_MERGEABLE_STAKES_NOTE, CONFIRM_MERGE, SUCCESSFUL_STAKES_MERGE, SUCCESSFUL_ENABLE_SNAPSHOTS,
SUCCESSFUL_DISABLE_SNAPSHOTS, CONFIRM_ENABLE_SNAPSHOTS,
CONFIRM_STAKE_USE_UNLOCKED)
CONFIRM_STAKE_USE_UNLOCKED, CONFIRM_REMOVE_SUBSTAKE, SUCCESSFUL_STAKE_REMOVAL)
from nucypher.cli.options import (
group_options,
option_config_file,
Expand Down Expand Up @@ -1068,6 +1068,61 @@ def merge(general_config: GroupGeneralConfig,
paint_stakes(emitter=emitter, staker=STAKEHOLDER)


@stake.command()
@group_transacting_staker_options
@option_config_file
@option_force
@group_general_config
@click.option('--index', help="Index of unused stake to remove", type=click.INT)
def remove_unused(general_config: GroupGeneralConfig,
transacting_staker_options: TransactingStakerOptions,
config_file, force, index):
"""Remove unused stake."""

# Setup
emitter = setup_emitter(general_config)
STAKEHOLDER = transacting_staker_options.create_character(emitter, config_file)
action_period = STAKEHOLDER.staking_agent.get_current_period()
blockchain = transacting_staker_options.get_blockchain()

client_account, staking_address = select_client_account_for_staking(
emitter=emitter,
stakeholder=STAKEHOLDER,
staking_address=transacting_staker_options.staker_options.staking_address,
individual_allocation=STAKEHOLDER.individual_allocation,
force=force)

# Handle stake update and selection
if index is not None: # 0 is valid.
current_stake = STAKEHOLDER.stakes[index]
else:
current_stake = select_stake(staker=STAKEHOLDER, emitter=emitter, stakes_status=Stake.Status.INACTIVE)

if not force:
click.confirm(CONFIRM_REMOVE_SUBSTAKE.format(stake_index=current_stake.index), abort=True)

# Authenticate
password = get_password(stakeholder=STAKEHOLDER,
blockchain=blockchain,
client_account=client_account,
hw_wallet=transacting_staker_options.hw_wallet)
STAKEHOLDER.assimilate(password=password)

# Non-interactive: Consistency check to prevent the above agreement from going stale.
last_second_current_period = STAKEHOLDER.staking_agent.get_current_period()
if action_period != last_second_current_period:
emitter.echo(PERIOD_ADVANCED_WARNING, color='red')
raise click.Abort

# Execute
receipt = STAKEHOLDER.remove_unused_stake(stake=current_stake)

# Report
emitter.echo(SUCCESSFUL_STAKE_REMOVAL, color='green', verbosity=1)
paint_receipt_summary(emitter=emitter, receipt=receipt, chain_name=blockchain.client.chain_name)
paint_stakes(emitter=emitter, staker=STAKEHOLDER)


@stake.command('collect-reward')
@group_transacting_staker_options
@option_config_file
Expand Down
4 changes: 4 additions & 0 deletions nucypher/cli/literature.py
Expand Up @@ -271,6 +271,10 @@

SUCCESSFUL_STAKES_MERGE = 'Successfully Merged Stakes'

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

SUCCESSFUL_STAKE_REMOVAL = 'Successfully Removed Stake'

#
# Rewards
#
Expand Down
15 changes: 14 additions & 1 deletion tests/acceptance/blockchain/actors/test_staker.py
Expand Up @@ -199,7 +199,7 @@ def test_staker_increases_stake(staker, token_economics):
assert stake.value == origin_stake.value + balance


def test_staker_merges_stakes(agency, staker, token_economics):
def test_staker_merges_stakes(agency, staker):
stake_index_1 = 0
stake_index_2 = 3
origin_stake_1 = staker.stakes[stake_index_1]
Expand All @@ -224,6 +224,19 @@ def test_staker_merges_stakes(agency, staker, token_economics):
staker.merge_stakes(stake_1=staker.stakes[1], stake_2=stake)


def test_remove_unused_stake(agency, staker):
stake_index = 3
staker.refresh_stakes()
original_stakes = list(staker.stakes)
unused_stake = original_stakes[stake_index]
assert unused_stake.final_locked_period == 1

staker.remove_unused_stake(stake=unused_stake)

stakes = staker.stakes
assert stakes == original_stakes[:-1]


def test_staker_manages_restaking(testerchain, test_registry, staker):
# Enable Restaking
receipt = staker.enable_restaking()
Expand Down
39 changes: 27 additions & 12 deletions tests/acceptance/blockchain/agents/test_staking_escrow_agent.py
Expand Up @@ -138,7 +138,6 @@ def test_get_swarm(agency, blockchain_ursulas):
assert is_address(staker_addr)



@pytest.mark.usefixtures("blockchain_ursulas")
def test_sample_stakers(agency):
_token_agent, staking_agent, _policy_agent = agency
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this test get relocated somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

staking_agent = ContractAgency.get_agent(StakingEscrowAgent, registry=test_registry)
staker_account, worker_account, *other = testerchain.unassigned_accounts

assert staking_agent.is_restaking(staker_account)
receipt = staking_agent.set_restaking(staker_account, value=False)
assert receipt['status'] == 1
assert not staking_agent.is_restaking(staker_account)


def test_lock_restaking(agency, testerchain, test_registry):
staker_account, worker_account, *other = testerchain.unassigned_accounts
staking_agent = ContractAgency.get_agent(StakingEscrowAgent, registry=test_registry)
Expand Down Expand Up @@ -438,6 +427,33 @@ def test_merge(agency, testerchain, test_registry, token_economics):
assert staking_agent.get_locked_tokens(staker_account, 0) == current_locked_tokens


def test_remove_unused_stake(agency, testerchain, test_registry):
staking_agent = ContractAgency.get_agent(StakingEscrowAgent, registry=test_registry)
staker_account = testerchain.unassigned_accounts[0]

testerchain.time_travel(periods=1)
staking_agent.mint(staker_address=staker_account)
current_period = staking_agent.get_current_period()
original_stakes = list(staking_agent.get_all_stakes(staker_address=staker_account))
assert original_stakes[2].last_period == current_period - 1

current_locked_tokens = staking_agent.get_locked_tokens(staker_account, 0)
next_locked_tokens = staking_agent.get_locked_tokens(staker_account, 1)

receipt = staking_agent.remove_unused_stake(staker_address=staker_account, stake_index=2)
assert receipt['status'] == 1

# Ensure stake was extended by one period.
stakes = list(staking_agent.get_all_stakes(staker_address=staker_account))
assert len(stakes) == len(original_stakes) - 1
assert stakes[0] == original_stakes[0]
assert stakes[1] == original_stakes[1]
assert stakes[2] == original_stakes[4]
assert stakes[3] == original_stakes[3]
assert staking_agent.get_locked_tokens(staker_account, 1) == next_locked_tokens
assert staking_agent.get_locked_tokens(staker_account, 0) == current_locked_tokens


def test_batch_deposit(testerchain,
agency,
token_economics,
Expand All @@ -448,7 +464,6 @@ def test_batch_deposit(testerchain,

amount = token_economics.minimum_allowed_locked
lock_periods = token_economics.minimum_locked_periods
current_period = staking_agent.get_current_period()

stakers = [get_random_checksum_address() for _ in range(4)]

Expand Down
27 changes: 27 additions & 0 deletions tests/acceptance/cli/ursula/test_stakeholder_and_ursula.py
Expand Up @@ -286,6 +286,33 @@ def test_merge_stakes(click_runner,
assert stakes[selection_2].last_period == 1


def test_remove_unused(click_runner,
stakeholder_configuration_file_location,
token_economics,
testerchain,
agency_local_registry,
manual_staker,
stake_value):

staking_agent = ContractAgency.get_agent(StakingEscrowAgent, registry=agency_local_registry)
original_stakes = list(staking_agent.get_all_stakes(staker_address=manual_staker))

selection = 2
assert original_stakes[selection].last_period == 1

stake_args = ('stake', 'remove-unused',
'--config-file', stakeholder_configuration_file_location,
'--staking-address', manual_staker,
'--index', selection,
'--force')
user_input = f'0\n' + f'{INSECURE_DEVELOPMENT_PASSWORD}\n' + YES_ENTER
result = click_runner.invoke(nucypher_cli, stake_args, input=user_input, catch_exceptions=False)
assert result.exit_code == 0

stakes = list(staking_agent.get_all_stakes(staker_address=manual_staker))
assert len(stakes) == len(original_stakes) - 1


def test_stake_bond_worker(click_runner,
testerchain,
agency_local_registry,
Expand Down