Merged
Conversation
fjarri
added a commit
to fjarri/nucypher
that referenced
this pull request
Jun 26, 2021
6d206f4 to
7b60dac
Compare
fjarri
added a commit
to fjarri/nucypher
that referenced
this pull request
Jun 26, 2021
84bbcc0 to
5f9b6d1
Compare
fjarri
added a commit
to fjarri/nucypher
that referenced
this pull request
Jul 10, 2021
515ea0b to
e1fcf4c
Compare
cf8cf61 to
132f62a
Compare
derekpierre
reviewed
Jul 12, 2021
jMyles
reviewed
Jul 12, 2021
jMyles
reviewed
Jul 12, 2021
fjarri
added a commit
to fjarri/nucypher
that referenced
this pull request
Jul 12, 2021
132f62a to
d93dfe9
Compare
fjarri
added a commit
to fjarri/nucypher
that referenced
this pull request
Aug 6, 2021
derekpierre
reviewed
Sep 7, 2021
derekpierre
approved these changes
Sep 7, 2021
derekpierre
reviewed
Sep 8, 2021
…ting_key" mandatory.
…evalClient.retrieve_cfrags()
derekpierre
reviewed
Sep 8, 2021
| capsule_splitter = BytestringSplitter((Capsule, Capsule.serialized_size())) | ||
| cfrag_splitter = BytestringSplitter((CapsuleFrag, CapsuleFrag.serialized_size())) | ||
| kfrag_splitter = BytestringSplitter((KeyFrag, KeyFrag.serialized_size())) | ||
| checksum_address_splitter = BytestringSplitter((bytes, 20)) # TODO: is there a pre-defined constant? |
Contributor
There was a problem hiding this comment.
I don't think there is one, not even in eth_utils or eth_typings
piotr-roslaniec
approved these changes
Sep 13, 2021
Contributor
piotr-roslaniec
left a comment
There was a problem hiding this comment.
LGTM 🎉 Great work!
KPrasch
approved these changes
Sep 14, 2021
Member
KPrasch
left a comment
There was a problem hiding this comment.
Thanks for the walkthrough in-person. This PR represents the introduction of many new internal objects and altered system metaphors, it will take me a while to get used to this version of the codebase. Overall I think we're introducing some complexity to gain immutability and reduce the number of invalid instance states (as far as I understand this is precisely your goal). 👍🏻
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of PR:
Required reviews:
What this does:
bobparameter ofTreasureMap.construct_by_publisher()(introduced in Immutable treasure maps #2773)Bob.matching_nodes_among()EncryptedTreasureMapdatastore modelUmbralMessageKitalias ofPolicyMessageKitTLSHostingPowertopowers.pyfromserver.py(a proper place for it, also helps avoid some import cycles)PolicyMessageKittoMessageKit. This is the externally exposed message-for-reencryption, so I think it makes sense to avoid unnecessary qualifiers - previouslyMessageKitwasn't even instantiated anywhere.kits.pytopolicies/(seems like the proper place for it, the objects there are all policy-related)encrypt_and_sign()a classmethod constructor ofMessageKit(renamed toauthor())author()from above only returnsMessageKitand not a tuple of the kit and the signature - the latter is already a part of the kitPolicyMessageKitobjects)ReencryptionRequestandReencryptionResponseencrypted_treasure_mapis now required inretrieve();labelandpublisher_verifying_keyare not neededenricoparameter is removed fromretrieve;policy_encrypting_keyis mandatoryBob.follow_treasure_map). Fixes MakeBob.follow_treasure_map()less obscure #2791PolicyMessageKitobjects? For Bob to store them somewhere.retrieve(),retrieve_cfrags(),PolicyMessageKit,RetrievalResultare the main offenders.The whole retrieval procedure has been significantly simplified. Bob creates a
RetrievalPlanplan based on Ursulas from the treasure map, the capsules in the kits, and possibly the attached cfrags in the kits. From it work orders for Ursulas are spawned and the results saved back into the plan.You can see the new usage in
test_bob_handles_frags.py. A lot of tests there were removed (because they tested the internal workings ofretrieve()) and replaced with new ones.For reviewers:
retrieve()is a good name? Mayberetrieve_and_decrypt()?RetrievalClientbe a class? It is pretty much stateless and could probably be rewritten as a function.