Conversation
f098aac to
6070910
Compare
| if writ_hrac != work_order.hrac: # Funky Workorder | ||
| raise Policy.Unauthorized # Bob, what the *hell* are you doing? | ||
|
|
||
| kfrag_checksum = keccak_digest(bytes(kfrag))[:WRIT_CHECKSUM_SIZE] |
There was a problem hiding this comment.
Any particular reason we're using a separate constant here instead of KECCAK_DIGEST_LENGTH?
There was a problem hiding this comment.
This gives the writ checksum potential flexibility aside from other keccak digests we may be using. Do you think it's superfluous?
KPrasch
left a comment
There was a problem hiding this comment.
I am one of the author's of this PR and I approve! 😅
Seriously, it's good to get a merge on this long-awaiting and impactful PR so we can begin to work with the new design assumptions.
We will transition to a better serialization approach (e.g., protobuf) so versioning is simpler to manage
Updated both server and client sides.
nucypher/blockchain/eth/agents.py
Outdated
| StakerInfo, | ||
| PeriodDelta, | ||
| StakingEscrowParameters, | ||
| Policy |
There was a problem hiding this comment.
I like that ArrangementInfo is defined in PolicyManagerAgent, and find it a little strange that we have some other datatypes that are specific to contracts declared somewhere other than in the respective agent eg. can Policy be declared in PolicyAgent instead of in nucypher/types.py; can StakingEscrowParameters be declared in StakingEscrowAgent instead of in nucypher/types.py...?
May not apply to all classes defined in nucypher/types.py, but perhaps some/most?
There was a problem hiding this comment.
Also, in keeping with the naming of other similar classes - wdyt about PolicyInfo instead of Policy?
There was a problem hiding this comment.
In terms of composition, this might get a bit bulky. In StakingEscrow alone we have: StakingEscrowParameters, SubStakeInfo, RawSubStakeInfo, Downtime, StakerFlags, and StakerInfo. While it's strictly good encapsulation I don't think nesting so many class definitions will be very readable.
Do you have a preference for consistency? If it's the namepacing that matters most to you perhaps we make a typing module per contract?
There was a problem hiding this comment.
I am with Derek here regarding PolicyInfo vs Policy. We already have a Policy (defined in policies.py), which is a different type (with more complexity too, not just a collection of simple values). Makes sense to follow the example of Arrangement (a Python class) vs ArrangementInfo (a mapping of a Solidity structure).
There was a problem hiding this comment.
Renamed Policy -> PolicyInfo and relocated ArrangementInfo to types.py for now.
There was a problem hiding this comment.
Do you have a preference for consistency? If it's the namepacing that matters most to you perhaps we make a typing module per contract?
I think some sort of more direct association between the types defined and the contracts would make them more intuitive. Having them live in nucypher/types.py just seems vague; it could be a typing module per contract or even keeping it general like nucypher.blockchain.eth.types...?
There was a problem hiding this comment.
^ BTW Doesn't have to be this PR - maybe the namespacing can be looked into as part of a separate PR.
|
|
||
| if writ_hrac in self.revoked_policies: | ||
| # Note: This is only an off-chain and in-memory check. | ||
| raise Policy.Unauthorized # Denied |
There was a problem hiding this comment.
Do we want to differentiate the reason for these raised Policy.Unauthorized exceptions (L1830, L1835, L1839) by contextualizing with a string message? Or is it unsafe to leak the reason?
There was a problem hiding this comment.
You're sensing my thoughts there. Do you think there's an added advantage to provide a bob with the reason why he's been denied access over obscurity?
There was a problem hiding this comment.
Feels like it - the one raised from the VerificationError (L1835) may be interesting to know ...
There was a problem hiding this comment.
Alternatively, we could do sub-classes of Policy.Unauthorized if differentiation is worth it.
|
Definitely going to be conflicts after this gets merged and I rebase Porter over it 😱 😅 . |
| return ursula | ||
|
|
||
|
|
||
| @pytest.mark.skip("David, send help!") |
| return treasure_map | ||
|
|
||
|
|
||
| # FIXME: a dirty hack to make the tests pass. Fix it ASAP. |
derekpierre
left a comment
There was a problem hiding this comment.
🎸 - approving in the interest of time/traction; my 2 outstanding comments can be addressed (or looked into and rejected) as part of follow-up issues/PRs.
#2687 (comment) (error messages)
#2687 (comment) (namespacing of types)
This replaces #2530 and incorporates subsequent work from @KPrasch (with touches from @jMyles).
Description modified from #2530:
Issues addressed:
Issues to address in a follow-up or related PR:
Use deterministic key derivation to produce Keyring keys #2225: Deterministic keyring derivationMergedNote: This PR underwent a conflict-fraught rebase from 64c8a78 to f098aac. Some of @fjarri's work, particularly 26076b1, may have been impacted.