Add `stake delegate` functionality #121

ngrinkevich · 2018-05-18T15:09:31Z

Stake delegate alternative approach to the previous one

Staker account A (delegator) is now able to say that account B (operator) is allowed to operate a node using A's stake. This doesn't affect the rest of the logic i.e. withdrawals, punishments etc... those are are still on A's stake.

Related issue #92

ngrinkevich · 2018-05-18T15:26:08Z

@Shadowfiend ok, I think I found a good way to avoid duplication in staking contracts, they now both inherit from StakeDelegatable. I haven't put events yet, should I go in detail like in the previous PR, and fire them for every case? i.e. operator staked, delegation removed ... or we add them later as we go

also I'm thinking to put delegateStakeTo and removeDelegate methods on staking proxy, a single function that will call those on every child contract

Shadowfiend · 2018-05-21T13:27:42Z

Will be having a look at this today.

Shadowfiend

Left some comments. This definitely feels cleaner than the previous pass! It feels like we're missing a couple of corner cases/potential nuisance attacks, so I'll think about those and try to put them in the PR later today or tomorrow morning.

Shadowfiend · 2018-05-21T15:53:49Z

contracts/solidity/contracts/StakeDelegatable.sol

+
+    mapping(address => uint256) public stakeBalances;
+    mapping(address => address) public delegatorFor;
+    mapping(address => address) public operatorFor;


Thoughts on stakerToOperator and operatorToStaker as these names?

Shadowfiend · 2018-05-21T15:54:43Z

contracts/solidity/contracts/StakeDelegatable.sol

+
+    /**
+     * @dev Gets the stake balance of the specified address.
+     * @param _staker The address to query the balance of.


This isn't necessarily the “staker” though, right? Should we name this _stakerOrOperator? Or even simply _address, since this parameter can be any address, staker, operator, or random?

Shadowfiend · 2018-05-21T15:55:09Z

contracts/solidity/contracts/StakeDelegatable.sol

+     * @param _staker The address to query the balance of.
+     * @return The amount staked by the passed address.
+     */
+    function stakeBalanceOf(address _staker) public view returns (uint256) {


Should we be checking for a 0 address?

Shadowfiend · 2018-05-21T15:57:10Z

contracts/solidity/contracts/StakeDelegatable.sol

+    }
+
+    /**
+     * @dev Delegates your stake balance to a specified address.


A little explanation of the implications would be good here (e.g., one address can only be an operator for one other address, delegating an address's stake means that address can no longer use the stake unless the delegate is removed, etc).

Shadowfiend · 2018-05-21T19:26:40Z

contracts/solidity/contracts/StakeDelegatable.sol

+/**
+ * @title Stake Delegatable
+ * @dev A contract that allows delegate your stake balance to any address
+ * that is not already a staker.


It's probably worth noting in this doc that delegator refers to a staker who has delegated their stake to another address, while operator refers to an address that has had a stake delegated to it.

Shadowfiend · 2018-05-21T19:29:48Z

contracts/solidity/contracts/StakeDelegatable.sol

+        require(previousDelegator == address(0));
+
+        // Release previous operator address when you delegate stake to a new one.
+        address previousOperator = operatorFor[msg.sender];


We'll need to consider how this interacts with the client. We may want it require a separate transaction to remove an existing operator…

Shadowfiend · 2018-05-21T19:31:45Z

contracts/solidity/test/TestStakeDelegate.js

+    await exceptThrow(stakingContract.delegateStakeTo(account_two));
+  });
+
+  it("should be able to delegate stake to an 'operator' address to represet your stake balance", async function() {


Shadowfiend · 2018-05-21T19:32:07Z

contracts/solidity/test/TestStakeDelegate.js

+    assert.equal(await stakingProxy.balanceOf(account_four), 200, "Updated operator account should represent delegator's stake balance.");
+  });
+
+  it("should be able to remove delagated operator address that represent your stake balance", async function() {


delegated* and represents*

Shadowfiend

Okay, so I wrote out some of the scenarios here. Here are a few situations I want to make sure we have tests/solutions for:

Ensuring changes to the parent stake are reflected in an operator (e.g., in the case of a grant revocation, stake withdrawal, or additional staking).
If an operator address stakes, what do we do? Right now we drop the operator status of that address, but this could result in adverse effects for the address the operator is operating for. What happens if operator B is using their stake? If operator B now stakes an amount of KEEP that is less than what address A had staked, any system behavior based on total stake amount is no longer guaranteed to be accurate. We should probably discuss this some more on Flowdock.
If a staker address drops another address's operator status directly, what do we do? Right now this happens instantaneously, and the operator is simply cut off. For the threshold relay, this has no adverse effects; however, if the operator is running keeps, they may have ETH bonded for those keeps. What happens to the data in those keeps?

I think we may need a handshake for both delegating and undelegating stakes… But a handshake for undelegating stakes is a potential attack vector (delegate stake A to operator B, try to undelegate because staker A wants to profit from their stake directly, operator B will not complete the handshake), so we may want undelegating to come with a time window (e.g., undelegating completes after two weeks unless the operator agrees to it before then).

Shadowfiend · 2018-05-25T13:22:55Z

contracts/solidity/contracts/StakeDelegatable.sol

    }

    /**
     * @dev Delegates your stake balance to a specified address.
-     * @param _operator Address to where you want to delegate your balance.
+     * @param _operator Address to where you want to delegate your


Let's move this into the dev docs. Parameter docs are generally short and sweet.

Shadowfiend · 2018-05-25T13:23:18Z

contracts/solidity/contracts/StakeDelegatable.sol

+     * @param _operator Address to where you want to delegate your
+     * balance. An address can only have one operator address. You can
+     * delegate stake to any ethereum address as long as it's not an
+     * actual staker or is being used as an operator by someone already.


Not a current staker*. In fact, perhaps “as long as it isn't currently staking or operating someone else's stake”?

Shadowfiend · 2018-05-25T13:24:52Z

contracts/solidity/contracts/StakeDelegatable.sol

+     * delegate stake to any ethereum address as long as it's not an
+     * actual staker or is being used as an operator by someone already.
+     * In the current implementation delegating stake doesn't hide the
+     * stake balance on your main stake address.


Given this, should we also have some sort of hasOperator or something that would allow checking whether a given address's staking balance is being operated by a different address?

Do you think it will be useful? Is it going to be in the Keep Client somewhere to check this value?

My initial thoughts were that Keep Client doesn't care if it's a staker or operator. In case of new operator assigned, Keep Client is just informed by unstaked(for staker) and staked(for operator) events and when operator is removed the reverse unstaked(for operator) and staked(for staker).

So it's reusing existing events, and if we decide to remove operator functionality completely there is no need to update Keep Client software

Staking and operation are the kinds of things that I would expect some people will want to interact with outside of the client. That is, enabling them to understand their current staker/operator situation feels like a valuable thing to do, because I might stake, then set up an operator, then walk away for a few months, then come back and not remember my setup, but want to know what it is.

Perhaps a good way to look at it is “we may want to put a basic UI on top of this”, alongside the staking UI.

ah I see, yeah I'll make sure to update the staking UI for this, as I remember there are automatic getters for mappings, so probably can just read operatorToStaker mapping

Keep Client is just informed by unstaked(for staker) and staked(for operator) events and when operator is removed the reverse unstaked(for operator) and staked(for staker).

btw do you have any concerns with this? I'm about to update staking contracts with some additional "ifs" to emit events with operator address if it is set. It doesn't look elegant but not sure if there is better way

I think we may need a handshake for both delegating and undelegating stakes… But a handshake for undelegating stakes is a potential attack vector (delegate stake A to operator B, try to undelegate because staker A wants to profit from their stake directly, operator B will not complete the handshake), so we may want undelegating to come with a time window (e.g., undelegating completes after two weeks unless the operator agrees to it before then).

I'm pretty sure we're ok with just a handshake for delegating to make sure you can't add someone else address.
I doubt anyone would want to add an operator address that they don't have ownership for.

I don't think they would add an operator they don't own, but I'd like to be prepared when someone inevitably does it out of silliness heh (or to be predatory).

The problem with unstaking being the same as undelegating is, when account A stakes and then bonds ETH, they bond their ETH. If they unstake and their ETH is lost, that's fine. If account A delegates to account B, and B bonds ETH, they're not bonding A's ETH. So if A now undelegates, particularly if it's instant, B's ETH is gone, and A has suffered no harm.

@mhluongo thoughts here?

when account A stakes and then bonds ETH, they bond their ETH.

ah I think i misunderstood, how stakers bond their ETH? they don't send any ETH to participate in the relay, right?

I thought it's only people who request a service send ETH to pay

For the relay, requestors just pay ETH. For keeps, requestors also pay ETH, but they can require that a keep provider bonds ETH in addition to their staked KEEP.

A delegates to account B, and B bonds ETH, they're not bonding A's ETH. So if A now undelegates, particularly if it's instant, B's ETH is gone, and A has suffered no harm.

I guess we can make this additional "ETH bonding" feature into a separate contract and have a more flexible setup there, for example that operator account B doesn't have to bond its own ETH and can ask delegator A to bond ETH instead?

My feeling we should not worry about this until we start working implementing keeps functionality. A separate contract should be flexible enough to route everything for every case nicely

ngrinkevich · 2018-06-08T12:29:31Z

@Shadowfiend let me know if we're missing anything else here, would be good to merge asap ideally so I can update the dashboard as well

pls see my latest comment in regard to ETH bonding

Shadowfiend · 2018-06-08T17:15:23Z

I'm not wanting to do anything else to the dashboard until we've had a chance to review what's there.

We also need to split that PR up.

But! That doesn't mean I don't want to merge this PR :) Let's force @mhluongo to look at it when we can tie him to a chair next week <_<

ngrinkevich · 2018-06-15T18:07:41Z

@mhluongo I think I'm ready here, pls have a look

Staker account A is now able to say that account B is allowed to operate a node using A's stake. This doesn't affect the rest of the logic i.e. withdrawals, punishments etc... those are are still on A's stake.

…erator

Add two step approach (Request/Approve) to simplify and secure delegate stake functionality between a staker and an operator.

Shadowfiend · 2018-08-02T20:39:25Z

Chair tying was a catastrophic failure, my apologies...

@ngrinkevich is this PR salvageable with its conflicts? If so, I think let's resolve conflicts and let @pschlump merge if he is satisfied.

ngrinkevich · 2018-08-03T15:53:05Z

@Shadowfiend Merge conflicts solved @pschlump could you pls have a look at this one?

Shadowfiend · 2018-08-03T16:14:29Z

Looks like we're still getting some out of gas errors here. I'm dismissing my review to let Philip take over from here.

@pschlump

Outdated. @pschlump is on.

Split heavy ones and remove duplicates

ngrinkevich · 2018-08-03T17:24:37Z

Cleaned up tests, @pschlump pls check

mhluongo

Took a first pass. I see why you weren't a fan of delegate and delegator as terms- they're incredibly confusing, especially as "delegate" is both a verb and noun.

I haven't run this yet, or fully grokked the tests- working on it.

mhluongo · 2018-08-13T00:43:20Z

contracts/solidity/contracts/StakeDelegatable.sol

+
+/**
+ * @title Stake Delegatable
+ * @dev A contract that allows delegate your stake balance to any address


A contract that allows delegate

I suspect this should read "that allows a staker to delegate their staked balance to any..."?

mhluongo · 2018-08-13T00:46:53Z

contracts/solidity/contracts/StakeDelegatable.sol

+     * @dev Only non staker addresses can be passed to the functions with this modifier.
+     */
+    modifier notStaker(address _address) {
+        require(stakeBalances[_address] == 0, "Provided address is not a staker.");


I'd use <= rather than == as a stronger guarantee against future code mistakes. I understand that stakeBalances is a uint256 mapping of course, but in case it's ever signed in a future code update, you want to cover the negative case.

mhluongo · 2018-08-13T00:47:32Z

contracts/solidity/contracts/StakeDelegatable.sol

+    mapping(address => address) public delegateToDelegator;
+
+    /**
+     * @dev Gets the stake balance of the specified address.


Doc nit - "staked balance" rather than "stake balance"

mhluongo · 2018-08-13T00:48:28Z

contracts/solidity/contracts/StakeDelegatable.sol

+     * @param _address The address to query the balance of.
+     * @return The amount staked by the passed address.
+     */
+    function stakeBalanceOf(address _address)


stakeBalanceOf() sounds like a verb for staking tokens. Consider eg getStakedBalance() or stakedBalanceOf()

mhluongo · 2018-08-13T00:51:36Z

contracts/solidity/contracts/StakeDelegatable.sol

+        // If provided address is a delegator return zero balance since the 
+        // balance is delegated to a delegate.
+        address delegate = delegateToDelegator[_address];
+        if (delegate != address(0) && delegatorToDelegate[delegate] == _address) {


It appears this branch will never be hit- it's the same logic as the branch above, with a single variable name changed (delegator -> delegate).

Ah, now I see. This terminology continues to be difficult to parse.

mhluongo · 2018-08-13T01:00:11Z

contracts/solidity/contracts/StakeDelegatable.sol

+        public
+        notNull(_address)
+    {
+        delegatorToDelegate[msg.sender] = _address;


It seems like this would be safer if it confirmed the delegator has already requested this address be a delegate?

I'm also seriously afraid we're switching delegate and delegator in these functions. Can we get some solid term definitions somewhere?

I'm also seriously afraid we're switching delegate and delegator in these functions.

We're not actually switching, it's like a handshake, if those two mapping records match then delegation works, if any of them changes, delegation stops straight away.

It seems like this would be safer if it confirmed the delegator has already requested this address be a delegate?

with the current logic delegating won't work until both delegator and delegate add each other,
but I guess yeah would be safer to force delegator approve delegate address first (handshake only can be initiated by delegator)

Can we get some solid term definitions somewhere?

I'm also gonna look at ethereum/EIPs#1207 @Shadowfiend found, maybe we could use terms and concepts from there

contracts/solidity/contracts/StakeDelegatable.sol

mhluongo · 2018-08-13T01:03:57Z

contracts/solidity/contracts/StakeDelegatable.sol

+    /**
+     * @dev Requests to delegate stake for a specified address.
+     * An address must approve you first to delegate by calling
+     * requestDelegateFor() method.


The second line of the doc here looks like copy-pasta from approveDelegateAt()

mhluongo · 2018-08-13T01:06:36Z

contracts/solidity/contracts/StakeDelegatable.sol

+    }
+
+    /**
+     * @dev Revert if a delegate try to stake.


mhluongo · 2018-08-13T01:07:23Z

contracts/solidity/contracts/StakeDelegatable.sol

+    }
+
+    /**
+     * @dev Removes delegate for your stake balance.


Can you explain why you want would to do this? Also... should you ever be allowed to, considering our threat model?

I'm thinking first case would be: if a delegate account is compromised then you definitely would like to remove it right? Even though delegate doesn't have any access to transfer balances, withdraw stakes etc.. it still can behave bad in the network and you as the delegator would be punished for that?

This is to cover cases with potential signed integers in the future.

pschlump

Just a couple of tiny changes.

pschlump · 2018-10-31T11:43:40Z

contracts/solidity/test/TestTokenGrantLookup.sol

@@ -0,0 +1,38 @@
+pragma solidity ^0.4.21;


I know that this is a little detail - but let's get all of our contracts and tests up to version 0.4.24
so that they all work the same way.

pschlump · 2018-10-31T11:43:58Z

contracts/solidity/test/TestTokenGrantLookupVesting.sol

@@ -0,0 +1,38 @@
+pragma solidity ^0.4.21;


See Above, 0.4.24.

Shadowfiend · 2018-11-26T17:39:07Z

Closing this for now, we'll be specing this out before continuing (see this flowdock thread)_. Leaving the branch intact so we can work off of it if we choose to.

ngrinkevich requested review from Shadowfiend, mhluongo and pschlump May 21, 2018 13:25

Shadowfiend previously requested changes May 21, 2018

View reviewed changes

Shadowfiend reviewed May 29, 2018

View reviewed changes

ngrinkevich changed the title ~~Add stake delegate functionality (Alternative approach)~~ Add stake delegate functionality Jun 25, 2018

ngrinkevich added 17 commits June 25, 2018 11:52

Add stake delegate functionality to staking contracts

669154b

Staker account A is now able to say that account B is allowed to operate a node using A's stake. This doesn't affect the rest of the logic i.e. withdrawals, punishments etc... those are are still on A's stake.

Split tests to avoid out of gas issue

4e7927d

Add stake delegate tests

0af3219

Improve doc comments + minor typos fixes

fd508eb

Fix doc string

69011c7

Emit staked/unstaked events via operator if staker works via operator

53c9654

Add test to ensure changes to the parent stake are reflected in an op…

ae58bb7

…erator

Refactor code with a modifier for null address check

92069a0

Refactor code with a modifier for non staker check

ad36803

Refactor delegate stake with the "handshake" approach

713dba6

Add two step approach (Request/Approve) to simplify and secure delegate stake functionality between a staker and an operator.

Fix broken modifiers

e1a09a9

Update tests

0838359

Split tests to avoid out of gas issue

8089c06

Add a test for balance check if operator becomes a staker

c15f280

Add revert to prevent a delegate to stake

2e03659

Rename staker/operator to delegator/delegate for more clarity

06fe284

Make sure delegator stake balance is zero when delegating

c265be3

ngrinkevich force-pushed the delegate-stake-to-operator-alternative branch from 93097ab to c265be3 Compare June 25, 2018 16:53

Shadowfiend added the ⛓️ solidity label Aug 2, 2018

ngrinkevich added 3 commits August 2, 2018 17:00

Merge branch 'master' into delegate-stake-to-operator-alternative

e73e240

Split token grant tests

1868900

Remove duplicated test

237e2a5

Provide error messages for require and revert

1dc0163

Cleanup solidity tests

7f4e5f7

Split heavy ones and remove duplicates

mhluongo reviewed Aug 13, 2018

View reviewed changes

ngrinkevich added 3 commits August 30, 2018 22:10

Improve doc strings and fix typos

1d2d17f

Rename stakeBalanceOf for better clarity

c5ec90c

Switch equal to a safer equal or less check

5f2c3ce

This is to cover cases with potential signed integers in the future.

ngrinkevich force-pushed the delegate-stake-to-operator-alternative branch from 3f87615 to 5f2c3ce Compare August 31, 2018 08:54

pschlump suggested changes Nov 1, 2018

View reviewed changes

Shadowfiend closed this Nov 26, 2018

madxor mentioned this pull request Nov 29, 2018

[WIP] and RFC stake delegation contract implementation in Solidity #431

Closed

ngrinkevich mentioned this pull request Dec 12, 2018

RFC 3: Stake delegation #443

Merged

Shadowfiend deleted the delegate-stake-to-operator-alternative branch May 15, 2019 01:01

Add stake delegate functionality #121

Add stake delegate functionality #121

Conversation

ngrinkevich commented May 18, 2018 • edited Loading

ngrinkevich commented May 18, 2018

Shadowfiend commented May 21, 2018

Shadowfiend 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend 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

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

ngrinkevich commented Jun 8, 2018

Shadowfiend commented Jun 8, 2018

ngrinkevich commented Jun 15, 2018

Shadowfiend commented Aug 2, 2018

ngrinkevich commented Aug 3, 2018

Shadowfiend commented Aug 3, 2018

ngrinkevich commented Aug 3, 2018

mhluongo 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

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

pschlump left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend commented Nov 26, 2018

Add `stake delegate` functionality #121

Add `stake delegate` functionality #121

ngrinkevich commented May 18, 2018 •

edited

Loading