New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retrieval protocol #2730
Retrieval protocol #2730
Conversation
6d206f4
to
7b60dac
Compare
84bbcc0
to
5f9b6d1
Compare
515ea0b
to
e1fcf4c
Compare
cf8cf61
to
132f62a
Compare
132f62a
to
d93dfe9
Compare
…ting_key" mandatory.
…evalClient.retrieve_cfrags()
c81e6d9
to
bb31fac
Compare
7a0d4cb
to
1a10ddf
Compare
cdd8166
to
e2a6d75
Compare
@@ -25,3 +25,4 @@ | |||
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is one, not even in eth_utils
or eth_typings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉 Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). 👍🏻
Type of PR:
Required reviews:
What this does:
bob
parameter ofTreasureMap.construct_by_publisher()
(introduced in Immutable treasure maps #2773)Bob.matching_nodes_among()
EncryptedTreasureMap
datastore modelUmbralMessageKit
alias ofPolicyMessageKit
TLSHostingPower
topowers.py
fromserver.py
(a proper place for it, also helps avoid some import cycles)PolicyMessageKit
toMessageKit
. This is the externally exposed message-for-reencryption, so I think it makes sense to avoid unnecessary qualifiers - previouslyMessageKit
wasn't even instantiated anywhere.kits.py
topolicies/
(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 returnsMessageKit
and not a tuple of the kit and the signature - the latter is already a part of the kitPolicyMessageKit
objects)ReencryptionRequest
andReencryptionResponse
encrypted_treasure_map
is now required inretrieve()
;label
andpublisher_verifying_key
are not neededenrico
parameter is removed fromretrieve
;policy_encrypting_key
is mandatoryBob.follow_treasure_map
). Fixes MakeBob.follow_treasure_map()
less obscure #2791PolicyMessageKit
objects? For Bob to store them somewhere.retrieve()
,retrieve_cfrags()
,PolicyMessageKit
,RetrievalResult
are the main offenders.The whole retrieval procedure has been significantly simplified. Bob creates a
RetrievalPlan
plan 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()
?RetrievalClient
be a class? It is pretty much stateless and could probably be rewritten as a function.