Conversation
Co-authored-by: David Núñez <david@nucypher.com>
Codecov Report
@@ Coverage Diff @@
## master #1339 +/- ##
==========================================
- Coverage 83.16% 82.82% -0.34%
==========================================
Files 72 72
Lines 10226 10347 +121
==========================================
+ Hits 8504 8570 +66
- Misses 1722 1777 +55
Continue to review full report at Codecov.
|
cygnusv
left a comment
There was a problem hiding this comment.
Just some minor comments wrt to docs.
|
|
||
| As your Ursula performs work, you can optionally enable the automatic addition of | ||
| all rewards to your existing stake to optimize earnings. By default this feature is disabled, | ||
| to enable it run: |
There was a problem hiding this comment.
I think we talked about leaving re-stake on by default, since that would be the most profitable and common scenario for stakers. Also, use "re-stake" with hyphen in the section title and in the next lines.
There was a problem hiding this comment.
Yeah - I agree that it makes sense to have restaking enabled by default - just documenting the current behavior.
Do we want to control this default at the contract-level? If not we can add a re-stake enable transaction to the nucypher stake create command. Is this something we want to introduce in this PR?
There was a problem hiding this comment.
We can just leave this as an open issue, because it does make sense to do it at the contract level. What do you think @szotov ?
There was a problem hiding this comment.
unfortunately no such thing like default value in contract, because all values are zero in the beginning and only can be set in functions. So we can set value in first deposit if we want, but I'm not sure is it good decision to do it implicitly
There was a problem hiding this comment.
The value can be set when initializing the contract though?
There was a problem hiding this comment.
Not really, because it's individual structure for each staker
I can do a trick - use something like notReStake field. But also issue here: to keep upgradability this should be a new field and old reStake wil be as a garbage.
So I prefer to set reStake on the first call of deposit
There was a problem hiding this comment.
Or, perhaps, we can have it explicitly set as a required parameter when we start staking (and then the default can be set in Python)
cygnusv
left a comment
There was a problem hiding this comment.
Oh, another commend wrt to the commands: if --lock and --release-period are always used together, why not just using one? I always envisioned something like --lock-until PERIOD.
|
Why past tense? :-) |
cygnusv
left a comment
There was a problem hiding this comment.
Good stuff. My only major requested change is to remove one of the duplicated methods of StakingEscrowAgent
tests/blockchain/eth/entities/agents/test_staking_escrow_agent.py
Outdated
Show resolved
Hide resolved
…ods, Docs touchups; ContractAgency, restaing agency layer assertions.
…ods, Docs touchups; ContractAgency, restaing agency layer assertions.
…ods, Docs touchups; ContractAgency, restaing agency layer assertions.
|
Only outstanding question I have here (perhaps best addressed in a follow-up PR) - Do we want to perform a re-staking enable transaction as part of stake initialization? |
|
@KPrasch yeah, my vote on that is to, ideally, have a |
|
Follow-up #1349 |
Co-Author: @cygnusv
Fixes #1269, Includes extracted changes from #1227
New Staker CLI Commands
nucypher stake restake --enablenucypher stake restake --disablenucypher stake restake --lock-until <PERIOD>Example