Capture hpke reply key in session event#762
Conversation
| pub enum SessionEvent { | ||
| Created(SessionContext), | ||
| UncheckedProposal(v1::UncheckedProposal), | ||
| UncheckedProposal((v1::UncheckedProposal, Option<crate::HpkePublicKey>)), |
There was a problem hiding this comment.
This 2-state type is misuse of Option IMO. If this is really what the type is, it should be a variant with Publickey and without public key
see misuse resistance here: https://youtu.be/qfknfCsICUM?si=tEqhp50GxCiYwwuZ&t=2142
There was a problem hiding this comment.
Agreed. But doesnt this line of thinking apply to SessionContext as well?
There was a problem hiding this comment.
This 2-state type is misuse of Option IMO.
This is not 2 states. The tuple represents what information was learned by handling the state transition.
There was a problem hiding this comment.
But doesnt this line of thinking apply to SessionContext as well?
Yes, perhaps SessionContext should be 2 separate structs
There was a problem hiding this comment.
I actually had it as 2 structs in the sketch branch but removed to reduce the diff.
How would you prioritize this? Techdebt or needs to be fixed now
There was a problem hiding this comment.
can you point me to your fix?
There was a problem hiding this comment.
I just checked the branch. It looks like I revereted that change. Might be somewhere in the history.
It was essentially a wrapper around session context with the reply key. Deref trick can be employed to get rid of the wrapper.session_context.X boilerplate.
There was a problem hiding this comment.
git reflog is your friend :)
Pull Request Test Coverage Report for Build 15598397011Details
💛 - Coveralls |
The session reply key stays `None` until we parse it from an `UncheckedProposal`. We lose this information because we don’t store an updated `SessionContext` in the `UncheckedProposal` session event and we don’t want to store the full updated context either. Instead, we stick to storing only what the session learns at that point. In this case, that’s the proposal itself and the optional HPKE reply key.
arminsabouri
force-pushed
the
unchecked-proposal-hpke-pk
branch
from
7cd9382 to
320a9b8
Compare
|
Rebased against master |
|
The outstanding comment on Let's make a follow-up issue to maybe refactor |
4 participants
The session reply key stays
Noneuntil we parse it from anUncheckedProposal. We lose this information because we don’t store an updatedSessionContextin theUncheckedProposalsession event and we don’t want to store the full updated context either. Instead, we stick to storing only what the session learns at that point. In this case, that’s the proposal itself and the optional HPKE reply key.This change is cherry-picked off #750