Policy refactoring by vzotova · Pull Request #1459 · nucypher/nucypher

vzotova · 2019-11-16T15:05:07Z

Based on #1470
Refs #1188
Closes #1063
Refs #1406
Refs #1492

WARNING: Not upgradeable changes

discrete payment for all policy periods: if policy starts today, ends tomorrow then fee must be for 2 periods
flexible end timestamp
Ursula pays more for confirmActivity (~15k-20k gas) to minimize Alice gas fee (save ~15k per node)
split creator role into creator and owner
revocation using owner's signature

PolicyManager v1.1.2 -> v 2.1.1
StakingEscrow v1.4.1 -> v2.1.1

codecov · 2019-11-17T11:38:27Z

Codecov Report

Merging #1459 into master will increase coverage by 0.17%.
The diff coverage is 90.67%.

@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
+ Coverage   84.31%   84.48%   +0.17%     
==========================================
  Files          72       72              
  Lines       11225    11293      +68     
==========================================
+ Hits         9464     9541      +77     
+ Misses       1761     1752       -9

Impacted Files	Coverage Δ
nucypher/config/characters.py	`96.42% <ø> (-0.05%)`	⬇️
nucypher/characters/lawful.py	`90.05% <ø> (+0.36%)`	⬆️
nucypher/characters/control/specifications.py	`92.72% <ø> (ø)`	⬆️
nucypher/characters/control/interfaces.py	`96.29% <ø> (ø)`	⬆️
nucypher/utilities/sandbox/blockchain.py	`79.22% <100%> (ø)`	⬆️
nucypher/blockchain/eth/agents.py	`91.15% <100%> (ø)`	⬆️
nucypher/blockchain/eth/deployers.py	`89.35% <100%> (+0.86%)`	⬆️
nucypher/cli/deploy.py	`83.41% <100%> (+0.15%)`	⬆️
nucypher/characters/control/serializers.py	`86.29% <100%> (ø)`	⬆️
nucypher/cli/painting.py	`80.11% <100%> (ø)`	⬆️
... and 13 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 61f749e...221c667. Read the comment docs.

nucypher/policy/policies.py

tests/cli/ursula/test_stakeholder_and_ursula.py

derekpierre · 2019-12-30T15:37:43Z

nucypher/blockchain/eth/actors.py

+            duration_periods = calculate_period_duration(now=maya.MayaDT(now),
+                                                         future_time=expiration,
+                                                         seconds_per_period=self.economics.seconds_per_period)
+            duration_periods += 1  # Number of all included periods


Are we using +=1 because the current day counts as one? Should calculate_period_duration make this adjustment, instead of having to manually do it?

Yeah, you're right, calculatePeriodDuration returns periodAt(expiration) - periodAt(now) and we add current period.

Should calculate_period_duration make this adjustment, instead of having to manually do it?

Good question, I'm still not sure what behaviour is more straightforward

It would seem that it should to me, but it may depend on where else it is used, and whether that same +=1 logic applies.

calculatePeriodDuratio used in initialize_stake without adding 1

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com>

derekpierre

🎸 !

tests/blockchain/eth/contracts/integration/test_intercontract_integration.py

jMyles · 2020-01-07T06:21:16Z

nucypher/blockchain/eth/actors.py

            expiration = datetime_at_period(self.staking_agent.get_current_period() + duration_periods,
-                                            seconds_per_period=self.economics.seconds_per_period)
+                                            seconds_per_period=self.economics.seconds_per_period,
+                                            start_of_period=True)


The comment says "expiration date is the last second of the current period", but here we pass start_of_period=True - what am I missing?

We take start of the next period and subtract 1 second (next line)

I'm sorry, maybe I'm being thick here, but I still don't get it. How does subtracting one second from the MayaDT get us to the last second of the current period? And how does start_of_period=True relate to that?

maybe comment is misleading, current period will be only for case when duration_periods==1, sort of example.
In that case start_of_period=True will return zero/first second of the next period and subtracting 1 second we will come to the last second of the previous period.
If duration_periods==2 then start_of_period=True will be period after next period and expiration after subtracting will be last second of the next period
start_of_period=True is sort of rounding, returns the beggining of calculated period

jMyles · 2020-01-07T06:26:42Z

OK, after a first full read-through, here's what I think I'm seeing - correct me where I'm wrong and add what I'm missing:

Policies don't have clients, they have creators
Policy creation and revocation events don't have clients, they have senders
...no wait, policy creation doesn't have a sender, it has a creator and an owner
We use timestamps instead of periods for Policy beginnings and endings
We check revocation by checking the signature of policy owner.

vzotova · 2020-01-07T08:38:34Z

@jMyles All good, I'll add some remarks:

client -> creator and client -> sender are just renames without changing meaning because client initially was not good name for this role
Before the changes, Alice/client/creator could choose whether she would pay for the current period and how much, now always have to pay the same price as for other periods.

docs/source/architecture/contracts.md

cygnusv · 2020-01-07T16:05:45Z

docs/source/architecture/contracts.md

-Payment should be added to the transaction in ETH and the amount is `firstReward * stakers.length + rewardRate * periods * stakers.length`.
-The reward rate must be greater than or equal to the minimum reward for each staker in the list. The first period's reward is not refundable, and can be zero.
+In order to place the fee for a policy, Alice calls the method `PolicyManager.createPolicy(bytes16, address, uint64, address[])`,
+specifying the stakers' addresses, the policy ID (off-chain generation), the policy owner (could be zero address), and the end timestamp of the policy.


The address argument before the policy ID is missing.

Not sure about address, maybe question with order in description? I've put stakers addresses to the end of the sentence

cygnusv · 2020-01-07T16:19:55Z

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

    event PolicyCreated(
        bytes16 indexed policyId,
-        address indexed client
+        address indexed creator,


Rather than creator, it should be something like sponsor, payer, etc.

cygnusv · 2020-01-07T16:38:57Z

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

+    * @return Revocation hash
+    */
+    function getRevocationHash(bytes16 _policyId, address _node) public view returns (bytes32) {
+        return SignatureVerifier.hashEIP191(abi.encodePacked(_policyId, _node), byte(0x45));


I think this line deserves a comment explaining we're using EIP191 version 0x45 ('E') for personal_sign

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

cygnusv · 2020-01-07T16:42:04Z

tests/cli/ursula/test_stake_via_allocation_contract.py

-    expiration = maya.now() + datetime.timedelta(days=3)
+    days = 3
+    now = testerchain.w3.eth.getBlock(block_identifier='latest').timestamp
+    expiration = maya.MayaDT(now).add(days=days-1)


This whole manually subtracting/adding a day is not intuitive at all.

How about expiration = maya.now() - timedelta(days=1)?

how about to rename days to days_to_pay or to something similar?

I think I need to explain it better what is core of these changes:
Before this PR duration of policy was equal to periods for which Alice pays. Also was firstPartialReward, kind of alternative way to pay for current period.
Now duration in seconds and not in periods. But payment still in periods. Sort of border between Alice and staker/worker regarding fees. Also duration includes current period by default (start timestamp is block.timestamp), so we get payment periods == duration periods + 1 (I mean duration periods as ceiling of (endTimestamp - block.timestamp)/secondsPerPeriod). Of course it's possible that payment periods == duration periods in case when block.timestamp == first second of current period and end timestamp == last second of any period but this situation is not used in tests

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

…stamp

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova added the ETH Contracts label Nov 16, 2019

vzotova self-assigned this Nov 18, 2019

vzotova force-pushed the policy branch from b7249dc to 0c421ee Compare November 18, 2019 14:02

vzotova force-pushed the policy branch 2 times, most recently from 6f4f2b4 to 38e9fa3 Compare December 11, 2019 16:24

cygnusv added this to the Testnet v2 milestone Dec 11, 2019

vzotova force-pushed the policy branch 3 times, most recently from 5de4194 to 09d11e3 Compare December 14, 2019 13:05

vzotova changed the title ~~[WIP] Refactoring start/end policy time and fee calculation~~ [WIP] Policy refactoring Dec 14, 2019

vzotova marked this pull request as ready for review December 17, 2019 11:01

vzotova changed the title ~~[WIP] Policy refactoring~~ Policy refactoring Dec 17, 2019

vzotova requested review from KPrasch, arjunhassard, cygnusv, jMyles, michwill, mswilkison and tuxxy December 17, 2019 11:01

vzotova added Alice 👩 Effects the "Alice" development area Enhancement New or improved features labels Dec 17, 2019

vzotova force-pushed the policy branch from 4b4daa6 to 21fac95 Compare December 18, 2019 13:57

vzotova mentioned this pull request Dec 20, 2019

Wind-down parameter #1544

Merged

vzotova force-pushed the policy branch 2 times, most recently from 27193e1 to c3b995f Compare December 26, 2019 10:40

derekpierre reviewed Dec 30, 2019

View reviewed changes

vzotova force-pushed the policy branch from 706fe32 to c4923ba Compare January 3, 2020 11:01

vzotova added a commit to vzotova/nucypher that referenced this pull request Jan 3, 2020

Apply suggestions from code review nucypher#1459

9ec35ea

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com>

vzotova added a commit to vzotova/nucypher that referenced this pull request Jan 3, 2020

Apply suggestions from code review nucypher#1459

42db017

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com>

derekpierre approved these changes Jan 6, 2020

View reviewed changes

jMyles reviewed Jan 7, 2020

View reviewed changes

tests/blockchain/eth/contracts/integration/test_intercontract_integration.py Outdated Show resolved Hide resolved

jMyles reviewed Jan 7, 2020

View reviewed changes

cygnusv reviewed Jan 7, 2020

View reviewed changes

michwill approved these changes Jan 7, 2020

View reviewed changes

vzotova added a commit to vzotova/nucypher that referenced this pull request Jan 9, 2020

Apply suggestions from code review nucypher#1459

b5a32d8

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova force-pushed the policy branch from 3ef6cfa to b5a32d8 Compare January 9, 2020 09:40

cygnusv approved these changes Jan 9, 2020

View reviewed changes

vzotova and others added 16 commits January 10, 2020 13:13

Contracts: discrete payment for all policy periods, flexible end time…

706bde0

…stamp

Tests for new policy start/end timestamp

8a4f079

Inter contract test for new start/end policy time

24d56a0

Agent tests for start/end policy time

513c5a4

Update character layer for new start/end policy time

4c983fc

Fix cli tests with policy creation

3f888c8

Update estimate_gas script for new start/end policy time

2e94f2c

PolicyManager: fix reentrancy test and small optimizations

5acb9ab

Moved some gas spending from createPolicy to confimrAcivity

5fa515d

PolicyManager: split creator role into creator and owner

d0396ac

PolicyManager: revocation using owner's signature

3fd751b

PolicyManager: small optimizations

dd76bb6

Fixes nucypher#1363

9af436c

Fix for test_sampling_distribution

bd7d5ac

Refreshing contract docs

519fa00

Apply suggestions from code review nucypher#1459

35a0dd5

Co-Authored-By: Derek Pierre <derek.pierre@gmail.com> Co-Authored-By: David Núñez <david@nucypher.com>

vzotova force-pushed the policy branch from b5a32d8 to 35a0dd5 Compare January 10, 2020 10:14

cygnusv merged commit 345a09e into nucypher:master Jan 10, 2020

cygnusv mentioned this pull request Jan 10, 2020

Proposal: Policy owner <> Policy sponsor separation #1492

Closed

vzotova deleted the policy branch April 28, 2020 13:06

Conversation

vzotova commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

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!

derekpierre left a comment

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!

jMyles commented Jan 7, 2020

Uh oh!

vzotova commented Jan 7, 2020

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vzotova commented Nov 16, 2019 •

edited

Loading

codecov bot commented Nov 17, 2019 •

edited

Loading