Conversation
3c3e30e to
7d0a358
Compare
c8ca534 to
b38cc33
Compare
b033886 to
6589d01
Compare
derekpierre
left a comment
There was a problem hiding this comment.
Haven't looked through the whole PR just yet but one thought comes to mind.
hrac feels like an internal type of representation (within nucypher code) i.e. we derive an hrac and use it for identification purposes. Exposing it as an "hrac" for example in REST requests seems overly complex to me for exposure to users. We could still refer to it as an "id" externally (i.e. policy.id and treasuremap.id), but internally (within nucypher code) we know it is an hrac.
wdyt?
| @@ -56,23 +39,7 @@ def _deserialize(self, value, attr, data, **kwargs): | |||
| raise InvalidInputData(f"Could not parse {self.name}: {e}") | |||
|
|
|||
| def _validate(self, value): | |||
There was a problem hiding this comment.
I would just move this code into _deserialize()so that deserialization returns an EncryptedTreasureMap object - see code change in #2768 - https://github.com/nucypher/nucypher/pull/2768/files#diff-54f7a125485aa11f633996b09e5f6b0468b8b91cd06a04cb097a808b70e14c1eR63
There was a problem hiding this comment.
Where is hrac exposed publicly that gives you concern?
|
@fjarri , @KPrasch any thoughts here - #2773 (review) |
@KPrasch I haven't looked through the entire PR yet - but of particular concern was the Porter REST API using 'hrac' as a REST parameter - https://github.com/fjarri/nucypher/blob/tmaps/tests/integration/porter/control/test_porter_web_control_federated.py#L106 (schema: https://github.com/fjarri/nucypher/blob/tmaps/nucypher/utilities/porter/control/interfaces.py#L71) |
|
@derekpierre , regarding ID/HRAC: my position is that it would be better to have the same name throughout the code. I don't feel particularly strongly whether it's ID or HRAC in the end; HRAC is a little easier to look up in the code. The endpoint you cited is probably going away anyway - @KPrasch is thinking about getting rid of storing the treasure map on Ursula side. In this case, we won't even need to send the HRAC to Ursula. |
|
Regarding the schema changes - would it be better handled in #2768? |
Sure. I guess that would save me from an even more rough rebase 😅 |
@fjarri fair enough. Updated PR looks good - do we need a newsfragment for this PR? Currently, it is empty. |
|
CI fails:
|
Based on top of #2745
Type of PR:
Required reviews:
What this does:
TreasureMapschema and datastore models toEncryptedTreasureMap(because that's what they are)HRACis now an object instead of just a bytestring (seepolicy/hrac.py)Note: versioning was temporarily removed, since the blockchain signature processing does not fit the "
get_splitter()" approach; to be fixed in #2767.Why it's needed:
For reviewers:
EncryptedTreasureMap?PublishedTreasureMap?b'\x00'if no blockchain signature,b'\x01' + signatureotherwise (current version)b'\x00' * 65if no blockchain signaturePolicy.hracattribute? It seems out of place.