You Sign Me (Geth EIP-191 "E" Support); Controller, CLI Bug Fixes by KPrasch · Pull Request #1031 · nucypher/nucypher

KPrasch · 2019-05-30T17:37:48Z

Implements geth client signing via w3.eth.sign in NuCypher client abstractions
Verify wallet signature as part of standard node verification
Labels are Unicode, not Base64
Remove keccack of hexadecimal map ID in requests
Handle internal geth process management in-test for signature integration
Remove "pubkey_sig" from the codebase
Use common deploy interactive input fixture

codecov · 2019-05-31T15:29:22Z

Codecov Report

Merging #1031 into hawksbeard will increase coverage by 10.14%.
The diff coverage is 91.89%.

@@               Coverage Diff               @@
##           hawksbeard    #1031       +/-   ##
===============================================
+ Coverage       72.88%   83.03%   +10.14%     
===============================================
  Files              67       67               
  Lines            8642     8658       +16     
===============================================
+ Hits             6299     7189      +890     
+ Misses           2343     1469      -874

Impacted Files	Coverage Δ
nucypher/cli/characters/alice.py	`80% <ø> (+44.58%)`	⬆️
nucypher/utilities/sandbox/ursula.py	`98.07% <ø> (-0.14%)`	⬇️
nucypher/characters/control/serializers.py	`87.6% <ø> (+6.45%)`	⬆️
nucypher/crypto/signing.py	`85.71% <ø> (+2.85%)`	⬆️
nucypher/cli/main.py	`91.93% <0%> (+3.22%)`	⬆️
nucypher/blockchain/eth/actors.py	`76.83% <100%> (+8.99%)`	⬆️
nucypher/crypto/kits.py	`87.27% <100%> (ø)`	⬆️
nucypher/blockchain/eth/interfaces.py	`70.27% <100%> (+4.95%)`	⬆️
nucypher/network/server.py	`87.96% <100%> (+0.05%)`	⬆️
nucypher/keystore/db/models.py	`94.23% <100%> (+5.76%)`	⬆️
... and 46 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 60ed5de...1ee37a4. Read the comment docs.

… strings in EthereumTester and Parity.

… and Powers.

…tion

…nal test bug fixes, found missing assertions.

…ke Ursula display name a class attr

…bugging ease.

…ockchain-alice node discovery test.

…n CI until a better solution is found.

…e in reward integration cli test.

…ction defaults.

derekpierre · 2019-06-03T14:14:33Z

nucypher/blockchain/eth/clients.py

+        client_kwargs = {
+            'node_technology': node_technology,
+            'version': client_data[1],
+            'backend': client_data[-1],


Nice logic!

vepkenez · 2019-06-03T15:19:05Z

nucypher/blockchain/eth/clients.py

-        raise NotImplementedError
-
-    def unlockAccount(self, address, password):
+    def unlock_account(self, address, password) -> bool:


I like this convention of using underscore_methods to make them obviously distinct from w3 calls

vepkenez · 2019-06-03T15:20:14Z

nucypher/characters/base.py


    def __repr__(self):
-        r = "({})⇀{}↽ ({})"
+        r = self._display_name_template


vepkenez · 2019-06-03T15:21:27Z

nucypher/characters/control/interfaces.py


    def derive_policy_encrypting_key(self, label: bytes) -> dict:
-        policy_encrypting_key = self.character.get_policy_pubkey_from_label(label)
+        policy_encrypting_key = self.character.get_policy_encrypting_key_from_label(label)


these name changes make it much clearer to me what is going on here!
💯

vepkenez · 2019-06-03T15:22:41Z

nucypher/cli/characters/alice.py

            'bob_encrypting_key': bob_encrypting_key,
            'bob_verifying_key': bob_verifying_key,
-            'label': b64encode(label.encode()).decode(),
+            'label': label,


🥂 indeed

vepkenez · 2019-06-03T15:23:58Z

nucypher/policy/models.py

        """
-        return keccak_digest(bytes(self._verifying_key) + bytes(self._hrac)).hex()
+        _id = keccak_digest(bytes(self._verifying_key) + bytes(self._hrac)).hex()
+        return _id


to new standards! 🥂

vepkenez

No new features, tons of cleanup and readability improvements.
Awesome.

cygnusv · 2019-06-03T16:50:26Z

@KPrasch I have an ugly workaround for the finnegan's wake issue. It's in here, in case you want to cherry-pick and have this PR with tests passing.
ed65dce

cygnusv

Awesome, @KPrasch !

cygnusv · 2019-06-03T17:02:37Z

nucypher/characters/base.py

+        # Needed for on-chain verification
+        if not self.federated_only:
+            self.blockchain = blockchain or Blockchain.connect()
+            self.miner_agent = MinerAgent(blockchain=blockchain)


This logic is somewhat weird, since all characters, even Enrico will get a miner agent. But for the moment I guess it's OK!

Perhaps a TODO?

nucypher/characters/base.py

cygnusv · 2019-06-03T17:09:18Z

nucypher/characters/base.py

+                raise InvalidSignature("Signature for message isn't valid: {}".format(signature_to_use))
        else:
-            raise self.InvalidSignature("No signature provided -- signature presumed invalid.")
+            raise InvalidSignature("No signature provided -- signature presumed invalid.")


Why are we changing here the type of InvalidSignature? Previous one was our own, and now it's the one from the cryptography, even if the Signature is ours.

See related PR #1040

cygnusv · 2019-06-03T17:17:23Z

nucypher/characters/lawful.py

+            signature=message_kit.signature,
+            decrypt=True,
+            label=label
+        )]


Why are we returning here a list of cleartexts, when message_kit always corresponds to a single message?
I know that Bob.retrieve() returns a list of cleartexts, but only because it's associated to WorkOrders, which may have more than one capsule (although currently they don't).
This may not be an issue to solve here, but perhaps we should consider a better API for retrieve/decrypt methods: it doesn't make any sense to have methods that accept only a single capsule, but that output a list of cleartexts (of length 1). Either we make the input also a list, or we return the only cleartext.

That's true: some hackathon participants wondered about the same thing

cygnusv · 2019-06-03T17:18:16Z

nucypher/cli/characters/alice.py

            'bob_encrypting_key': bob_encrypting_key,
            'bob_verifying_key': bob_verifying_key,
-            'label': b64encode(label.encode()).decode(),
+            'label': label,


🥂 indeed

cygnusv · 2019-06-03T17:21:52Z

nucypher/network/server.py

            # TODO 341 - what if we already have this TreasureMap?
-            this_node.treasure_maps[keccak_digest(binascii.unhexlify(treasure_map_id))] = treasure_map
+            treasure_map_index = bytes.fromhex(treasure_map_id)
+            this_node.treasure_maps[treasure_map_index] = treasure_map


cygnusv · 2019-06-03T17:54:35Z

tests/fixtures.py

                                checksum_public_address=testerchain.alice_account,
                                network_middleware=MockRestMiddleware(),
-                                known_nodes=blockchain_ursulas,
+                                known_nodes=blockchain_ursulas[:-1],  # TODO: 1035


[:-1] <-- nice guy!

tests/learning/test_fault_tolerance.py

Co-Authored-By: David Núñez <david@nucypher.com>

KPrasch mentioned this pull request May 31, 2019

Abstract/Cleanup BlockChainInterface #1016

Merged

KPrasch and others added 14 commits May 31, 2019 13:44

Supporting EIP-191 Geth message signatures

b845224

Inheretence improvements for web3 abstractions, Handle 4-part version…

ceff996

… strings in EthereumTester and Parity.

Use EIP-191 Signatures on Geth and PyEVM backends

2898856

Recompose verify_eip_191 into crypto.api, off of Interfaces, Clients,…

0529030

… and Powers.

Give characters blockchain and miner agency on init by default

44fa3fe

Perform on/off chain EIP 191 verification during node discovery.

adfb0c5

Adjust tests to accomodate new eip 191 and on-chain staking checks.

43b1a42

Fix eth clients bugs. Improved tests for clients.

681b2df

Test integration for geth client signing and nucypher.crypto verifica…

403f348

…tion

Handle geth --dev process as a fixture to stop when finished. Additio…

24cdae7

…nal test bug fixes, found missing assertions.

Handle non-staking Ursula in-tests; Annotations for failing tests, Ma…

c3cc6d1

…ke Ursula display name a class attr

Update deployment interactive input to match the latest CLI design.

bb6c022

Removes base64 label encoding via CLI interface. Style updates for de…

97d509d

…bugging ease.

Remove outermost keccack digest for map id construction, Restore a bl…

45a6df2

…ockchain-alice node discovery test.

KPrasch force-pushed the you-sign-me branch from 253f207 to ab4a4be Compare May 31, 2019 20:45

KPrasch mentioned this pull request May 31, 2019

[EPIC] Decentralized Characters & Integrated Ethereum Node Providers #951

Merged

20 tasks

KPrasch force-pushed the you-sign-me branch from ab4a4be to 117f903 Compare May 31, 2019 20:56

New client test module; Skip geth client signature integration test o…

ebc1333

…n CI until a better solution is found.

KPrasch force-pushed the you-sign-me branch from 117f903 to ebc1333 Compare May 31, 2019 21:16

KPrasch requested review from cygnusv, derekpierre, fjarri, jMyles, michwill and vepkenez May 31, 2019 21:28

KPrasch changed the title ~~You Sign Me (EIP-191 Support)~~ You Sign Me (Geth EIP-191 Support); CLI Bug Fixes May 31, 2019

KPrasch changed the title ~~You Sign Me (Geth EIP-191 Support); CLI Bug Fixes~~ You Sign Me (Geth EIP-191 Support); Controller, CLI Bug Fixes May 31, 2019

Ensure alice and bob remember the manually controlled non-staking nod…

34bdc31

…e in reward integration cli test.

KPrasch added 3 commits May 31, 2019 15:21

Straggler invalid interactive input at deploy-time

1ef56fc

Understand labels to be handled as unicode in interfaces.

3ce1904

Removes 'pubkey_sig' from the codebase, In favor of 'verifying_key'.

92f23b5

KPrasch force-pushed the you-sign-me branch from 49d5dde to 9ba338f Compare June 1, 2019 10:48

KPrasch marked this pull request as ready for review June 1, 2019 10:51

Implement deploy_user input fixture for CLI deployments in-test.

688321d

KPrasch force-pushed the you-sign-me branch from 9ba338f to 688321d Compare June 1, 2019 10:57

Adding domains to Alice and Bob in examples, to aviod using the produ…

33d2ca8

…ction defaults.

derekpierre reviewed Jun 3, 2019

View reviewed changes

vepkenez reviewed Jun 3, 2019

View reviewed changes

nucypher/characters/base.py

def __repr__(self):

r = "({})⇀{}↽ ({})"

r = self._display_name_template

Copy link

Contributor

vepkenez Jun 3, 2019

Choose a reason for hiding this comment

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

👍

vepkenez reviewed Jun 3, 2019

View reviewed changes

vepkenez approved these changes Jun 3, 2019

View reviewed changes

derekpierre approved these changes Jun 3, 2019

View reviewed changes

cygnusv approved these changes Jun 3, 2019

View reviewed changes

michwill approved these changes Jun 3, 2019

View reviewed changes

cygnusv and others added 2 commits June 3, 2019 21:48

Testing dirty hack for finnegans wake demo on CI

04980a9

Apply suggestions from code review

1ee37a4

Co-Authored-By: David Núñez <david@nucypher.com>

KPrasch merged commit 2e4bff7 into nucypher:hawksbeard Jun 3, 2019

cygnusv mentioned this pull request Jun 4, 2019

Consistent API for retrieve/decrypt methods #1043

Closed

KPrasch changed the title ~~You Sign Me (Geth EIP-191 Support); Controller, CLI Bug Fixes~~ You Sign Me (Geth EIP-191 "E" Support); Controller, CLI Bug Fixes Jun 9, 2019

cygnusv mentioned this pull request Jun 24, 2019

Finalize integration of Adjudicator to new Worker/Staker model #1093

Merged

KPrasch deleted the you-sign-me branch July 21, 2020 02:31

Conversation

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

Uh oh!

Uh oh!

codecov bot commented May 31, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

cygnusv commented Jun 3, 2019

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KPrasch commented May 30, 2019 •

edited

Loading

codecov bot commented May 31, 2019 •

edited

Loading