Introduce Stakeholder by KPrasch · Pull Request #1064 · nucypher/nucypher

KPrasch · 2019-06-08T19:59:55Z

PR goes into epic. Introduces new Issue #1119

codecov · 2019-06-11T15:45:58Z

Codecov Report

Merging #1064 into sekanjabin will decrease coverage by 1.47%.
The diff coverage is 36.1%.

@@              Coverage Diff              @@
##           sekanjabin   #1064      +/-   ##
=============================================
- Coverage       83.08%   81.6%   -1.48%     
=============================================
  Files              70      70              
  Lines            9252    9521     +269     
=============================================
+ Hits             7687    7770      +83     
- Misses           1565    1751     +186

Impacted Files	Coverage Δ
nucypher/blockchain/eth/deployers.py	`91.24% <100%> (ø)`	⬆️
nucypher/crypto/powers.py	`95.09% <100%> (+0.06%)`	⬆️
nucypher/config/node.py	`90.16% <100%> (+0.44%)`	⬆️
nucypher/blockchain/eth/actors.py	`58.63% <26.69%> (-22.43%)`	⬇️
nucypher/blockchain/eth/clients.py	`57.89% <41.17%> (-0.84%)`	⬇️
nucypher/blockchain/eth/agents.py	`89.85% <50%> (-2.25%)`	⬇️
nucypher/blockchain/eth/interfaces.py	`80.6% <73.68%> (-1.29%)`	⬇️
nucypher/config/base.py	`96.62% <75%> (-2.16%)`	⬇️
nucypher/blockchain/eth/token.py	`87.62% <87.5%> (+0.8%)`	⬆️
nucypher/network/nodes.py	`82.3% <0%> (+0.61%)`	⬆️
... and 1 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 65f6fcd...34abab8. Read the comment docs.

…, Geth tranction formatting and signging.

…e same address multiple times.

…hwill

…hwill's funding account recipt fix; Touch Ups.

cygnusv · 2019-07-16T20:35:26Z

nucypher/blockchain/eth/actors.py

+            self.__worker_address = worker_address
+            if self.__worker_address is BlockchainInterface.NULL_ADDRESS:
+                return NO_WORKER_ASSIGNED
+            return self.__worker_address


Two issues here:

If there's no worker set and you call worker_address twice in a row, the first time you will obtain NO_WORKER_ASSIGNED, and the second time you will get NULL_ADDRESS.

You're using is again to test string equality!

✔️ (right?)

nucypher/blockchain/eth/clients.py

cygnusv · 2019-07-16T20:53:19Z

nucypher/blockchain/eth/token.py

        # Agency
        self.staking_agent = None
-        self.token_agent = NucypherTokenAgent()
+        self.token_agent = NucypherTokenAgent()  # TODO: Use Agency


Initially, I was OK with simply using the constructor of each agent, but I now agree with you about that API can be misleading, and I think centralizing everything in the Agency is good.

cygnusv · 2019-07-16T21:17:00Z

nucypher/blockchain/eth/actors.py

+            stakes = list(self.staking_agent.get_all_stakes(staker_address=account))
+            if stakes:
+                staker = Staker(is_me=True, checksum_address=account, blockchain=self.blockchain)
+                self.__stakers[staker] = staker.stakes


You're using Staker as dict key here, but as far as I can tell, this class is not Hashable. Can this be problematic somehow?

Yes. Fixed here: 68426a32076f60be232a5b50b458f5da56d06f31

nucypher/blockchain/eth/actors.py

cygnusv · 2019-07-16T21:30:17Z

nucypher/blockchain/eth/actors.py

+                raise self.ConfigurationError("No password supplied to create new staking account.")
+            new_account = self.blockchain.client.new_account(password=password)
+        self.__accounts.append(new_account)
+        staker = Staker(blockchain=self.blockchain, is_me=True, checksum_address=new_account)


Shouldn't this staker be added to self.__stakers too?

Also fixed in 68426a32076f60be232a5b50b458f5da56d06f31

to me, these little fixes are a reason not to do the whole

@property def thing(self) def set_thing(self, thing) self.__thing = thing def get_thing(self, thing) return self.__thing

just another bug to have

Heh - Just generally prefer statelessness?

python objects already have a thing called __dict__ and methods called setattr and getattr that help manage state.

self.thing = new_value
print (self.thing)

that is state enough no?
I guess if we want to do some kind of validation on the attribute being set it makes sense, but we're not really.

Also, there is nothing stopping a developer from directly modifying these attributes anyway.

Sometimes I want to perform some logic during object.attr = value. In python this can be implemented with the @property.getter and @property.setter methods, but I typically prefer to use named methods for this sort of operation. object.setter(value)

In any case, private attributes with public interfaces make it much harder to "shoot yourself in the foot" if executed well.

Another Item I want to check here is that the user does indeed have control of the new funding account. It's a bit awkward to set it outside of the configuration API.

cygnusv · 2019-07-16T21:30:57Z

nucypher/blockchain/eth/actors.py

+                                                            federated_only=False,
+                                                            provider_uri=self.blockchain.provider_uri,
+                                                            **configuration)
+        return worker_configuration


cygnusv · 2019-07-16T21:35:15Z

nucypher/blockchain/eth/actors.py

+        self.funding_power.activate()
+
+        # Send new staker account ETH
+        tx = {'to': staker.checksum_address, 'from': self.funding_account, 'value': self.eth_funding}


Why does it have to send a default amount of ETH?

The new staker account needs to perform transactions, and pay for gas.

cygnusv · 2019-07-16T21:44:43Z

nucypher/blockchain/eth/actors.py

+        receipts = dict()
+        for staker in self.stakers:
+            self.attach_transacting_power(checksum_address=staker.checksum_address)
+            receipt = self.collect_rewards(staker_address=staker.checksum_address)


No password is passed to self.attach_transacting_power() and to self.collect_rewards(), which means this assumes that the transacting power is already cached with a password; otherwise, this won't work. Perhaps you can pass a list of passwords as input to sweep()? (I know I know.....yuck... 🤢 )

Sweep is deprecated (for now) in 68426a32076f60be232a5b50b458f5da56d06f31

cygnusv

👍
Just the minor details I described above.

…hwill

…hwill's funding account recipt fix; Touch Ups.

vepkenez · 2019-07-16T22:44:19Z

nucypher/blockchain/eth/actors.py

        blockchain.transacting_power.activate()
        self.log = Logger("Deployment-Actor")

+        # TODO: Does this class want to be a Character implementing PowerUp consumption?


Would would it mean to make this change? Moving this class into either chaotic.py or lawful.py (and deciding which one)?

Yeah - I think into lawful.py - However, I'm not fully convinced.

vepkenez · 2019-07-16T22:45:11Z

nucypher/blockchain/eth/actors.py

+        self.log = Logger(f"stakeholder")
+        self.blockchain = blockchain
+        self.offline_mode = offline_mode
+        self.eth_funding = Web3.toWei('0.1', 'ether')  # TODO: How much ETH to use while funding new stakes.


this should be a constant/cli arg/envvar right?

Yes to all. Not sure what makes sense as a default. Perhaps an amount based on the duration or another way to estimate future gas consumption?

well that's all the more reason to make it modifiable without editing the code eh?

Indeed. I want to bring this forth for discussion before implementing it. If there's not already an open Issue, lets make one.

Loosely Related to #842

vepkenez · 2019-07-16T22:45:59Z

nucypher/blockchain/eth/actors.py

+        return payload
+
+    @classmethod
+    def from_configuration_file(cls, filepath: str = None, **overrides) -> 'StakeHolder':


we already have this method in node.py don't we? I guess this is how we know if it should be a Character?

It's one way for sure, but not the most defining characteristic (no pun indented) is the usage of TransactingPower in the StakeHolder.__init__, and the analogous layers to that of Worker -> Ursula & Staker -> StakeHolder

vepkenez · 2019-07-16T22:46:42Z

nucypher/blockchain/eth/actors.py

+        instance = cls(filepath=filepath, **payload)
+        return instance
+
+    @validate_checksum_address


vepkenez · 2019-07-16T22:49:20Z

nucypher/blockchain/eth/actors.py

+        self.__accounts.extend(accounts)
+
+    def set_funding_account(self, address: str):
+        self.__funding_account = address


these getters and setters are a little... why have them instead of
self.funding_account = <whatever>

I want to perform checksum address validation on setter here.

vepkenez · 2019-07-16T22:50:31Z

nucypher/blockchain/eth/actors.py

+            raise self.ConfigurationError(f"{self.funding_account} has no ETH")
+
+        # Transfer NU
+        self.funding_power.activate()


didn't we already activate this in the __init__?

Yes, however, its possible through normal usage to switch TransactingPowers with one belonging to another account, like a Staker. We might be able to remove this with some careful inspection, but this is ensuing that we indeed have the funding power attached.

vepkenez · 2019-07-16T22:54:30Z

nucypher/blockchain/eth/actors.py

+
+    @validate_checksum_address
+    def attach_transacting_power(self, checksum_address: str, password: str = None) -> None:
+        try:


I feel like the right way to do this is more like

if checksum_addresss in self.__transacting_powers: # or self.__transacting_powers.get(checksum_address) transacting_power = self.__transacting_powers[checksum_address] else: transacting_power = TransactingPower(blockchain=self.blockchain, account=checksum_address) self.__transacting_powers[checksum_address] = transacting_power transacting_power.activate(password=password)

I guess it's an ongoing debate...
https://softwareengineering.stackexchange.com/questions/351110/are-exceptions-for-flow-control-best-practice-in-python

Heh - Yeah I even say that I prefer the exception handling as flow control, from the old idiom "Don't look before you leap" / "Better to ask for forgiveness than permission"

vepkenez · 2019-07-16T22:59:43Z

nucypher/blockchain/eth/agents.py


+    @classmethod
+    def get_agent(mcs, cls):
+        try:


what is an "mcs"?

"metaclass". it's a conventional first-arg name for a meta-classmethod.

vepkenez · 2019-07-16T23:01:22Z

nucypher/blockchain/eth/clients.py

@@ -197,20 +197,20 @@ def coinbase(self) -> str:
        return self.w3.eth.coinbase


I don't see any tests for a lot of the new methods in this file

Which new methods? I do see one: new_account.

Yes that one, but I am mode thinking that some of these will fail to work in some of the various clients which will necessitate another fix-ganache-issues branch.

vepkenez

I say merge it after a test for new_account.

…hwill

…hwill's funding account recipt fix; Touch Ups.

KPrasch changed the base branch from master to alhambra-verde June 9, 2019 19:26

cygnusv force-pushed the alhambra-verde branch 4 times, most recently from ffdb6f6 to e2a3ec9 Compare June 10, 2019 10:02

KPrasch force-pushed the stakeholder branch from 53b8ad6 to 7e02bee Compare June 10, 2019 17:56

cygnusv force-pushed the alhambra-verde branch 2 times, most recently from 4eca13d to b1572a7 Compare June 11, 2019 08:05

KPrasch force-pushed the stakeholder branch from 7e02bee to 79062c8 Compare June 11, 2019 15:45

KPrasch force-pushed the stakeholder branch from 79062c8 to 8c9b6e6 Compare June 11, 2019 16:06

KPrasch changed the title ~~[Concept-Only] Stakeholder~~ Stakeholder Jun 11, 2019

KPrasch force-pushed the stakeholder branch from c023651 to e29dc76 Compare June 12, 2019 00:29

cygnusv force-pushed the alhambra-verde branch from b1572a7 to 9738aeb Compare June 13, 2019 18:33

KPrasch added the API label Jun 13, 2019

KPrasch force-pushed the stakeholder branch 3 times, most recently from 3c0cf99 to 3a1d3cc Compare June 15, 2019 05:51

KPrasch force-pushed the alhambra-verde branch from 9720e49 to d3fcff5 Compare June 15, 2019 06:02

KPrasch force-pushed the stakeholder branch from 3a1d3cc to 799f1f3 Compare June 15, 2019 06:03

KPrasch mentioned this pull request Jun 15, 2019

[EPIC] Alhambra Verde - Introduce Staker and Worker Roles #1029

Merged

8 tasks

cygnusv force-pushed the alhambra-verde branch 2 times, most recently from 0dc5f37 to ddf1d08 Compare June 20, 2019 13:58

KPrasch force-pushed the stakeholder branch from 799f1f3 to 4071938 Compare June 21, 2019 16:41

KPrasch mentioned this pull request Jun 21, 2019

Extract Stakeholder CLI and Deploy Actions #1056

Merged

cygnusv force-pushed the alhambra-verde branch from 0cb8025 to 407aaae Compare June 26, 2019 16:19

KPrasch mentioned this pull request Jun 27, 2019

[WIP] [EPIC] Sekanjabin - StakeHolder & Worker-Ursula Integration #1107

Closed

6 tasks

KPrasch force-pushed the stakeholder branch from 4071938 to 0365767 Compare June 27, 2019 19:53

KPrasch changed the base branch from alhambra-verde to sekanjabin June 27, 2019 19:58

KPrasch changed the title ~~Stakeholder~~ Introduce Stakeholder Jun 27, 2019

KPrasch added 7 commits July 15, 2019 15:58

Stakeholder power-tracking, and quasi-character blockchain persistence

8c923fd

Include worker address sync in Stake; Blockchain to/from dict payload…

9f68675

…, Geth tranction formatting and signging.

Ensure access to geth accounts pre-character-init

65749b0

Optional allowance detection for TokenAgency to ease stake init on th…

f22b91e

…e same address multiple times.

Improve Stakeholder API tests.

336c4fc

xAdvance Stakeholder rewards integration testing.

6efb86b

BYOK for stakeholder and worker - Passing collect inflation reward test.

5b642f0

KPrasch force-pushed the stakeholder branch from cfec68b to 86ddae2 Compare July 15, 2019 22:59

KPrasch added a commit to KPrasch/nucypher that referenced this pull request Jul 15, 2019

Respond to RFCs in PR nucypher#1064 - Staker to/from blockchain, @mic…

86ddae2

…hwill's funding account recipt fix; Touch Ups.

cygnusv requested changes Jul 16, 2019

View reviewed changes

cygnusv approved these changes Jul 16, 2019

View reviewed changes

Respond to RFCs in PR nucypher#1064 - Staker to/from blockchain, @mic…

34abab8

…hwill's funding account recipt fix; Touch Ups.

KPrasch force-pushed the stakeholder branch from 86ddae2 to 34abab8 Compare July 16, 2019 22:02

vepkenez reviewed Jul 16, 2019

View reviewed changes

nucypher/blockchain/eth/actors.py

instance = cls(filepath=filepath, **payload)

return instance

@validate_checksum_address

Copy link

Contributor

vepkenez Jul 16, 2019

Choose a reason for hiding this comment

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

👍

vepkenez reviewed Jul 16, 2019

View reviewed changes

vepkenez suggested changes Jul 16, 2019

View reviewed changes

Adds integration test for creating new geth accounts over RPC clients.

ca0715f

KPrasch requested a review from vepkenez July 16, 2019 23:46

vepkenez approved these changes Jul 16, 2019

View reviewed changes

KPrasch merged commit 04122fe into nucypher:sekanjabin Jul 16, 2019

KPrasch mentioned this pull request Jul 16, 2019

[EPIC] Sekanjabin - StakeHolder & Worker-Ursula Integration, Deployer Coverage, UX Baseline. #1124

Merged

13 tasks

KPrasch added a commit to KPrasch/nucypher that referenced this pull request Jul 25, 2019

Respond to RFCs in PR nucypher#1064 - Staker to/from blockchain, @mic…

362aff4

…hwill's funding account recipt fix; Touch Ups.

KPrasch deleted the stakeholder branch March 22, 2021 18:41

		@@ -197,20 +197,20 @@ def coinbase(self) -> str:
		return self.w3.eth.coinbase

Conversation

KPrasch commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

cygnusv left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KPrasch commented Jun 8, 2019 •

edited

Loading

codecov bot commented Jun 11, 2019 •

edited

Loading

KPrasch Jul 16, 2019 •

edited

Loading

vepkenez Jul 16, 2019 •

edited

Loading

KPrasch Jul 16, 2019 •

edited

Loading

vepkenez Jul 16, 2019 •

edited

Loading

vepkenez Jul 16, 2019 •

edited

Loading

KPrasch Jul 16, 2019 •

edited

Loading

KPrasch Jul 16, 2019 •

edited

Loading

vepkenez Jul 16, 2019 •

edited

Loading