feat(aztec-nr): Add a delivery mode to handshake notes#23783
Conversation
nchamo
left a comment
nchamo
left a comment
There was a problem hiding this comment.
Left some question, maybe we need to re-think how we store the mode. Let me know if I missed something though
| handshake_type: u8, | ||
| /// The delivery mode of messages tagged with this handshake. | ||
| /// This is used by the recipient to know how to scan for tags for a given onchain handshake. | ||
| mode: u8, |
There was a problem hiding this comment.
Look at my other comments below. Maybe, instead of storing this here, we should add a new Map to storage:
Map<AztecAddress, Map<Mode, Owned<PrivateMutable<HandshakeNote, Context>, Context>, Context>, Context>,
That way, we can have both handshakes per (sender, recipient) pair
There was a problem hiding this comment.
Switching from one handshake per (sender, recipient) to one handshake per (sender, recipient, mode) pair feels like a bigger change in the semantics of the contract and general delivery flow. I prefer keeping the invariant that if a (sender, recipient) pair wants to switch modes, they can just re-handshake. Maintaining handshakes per mode feels like it would add unnecessary complexity at this stage when I am not sure of a concrete use case why an app would want two handshake modes simultaneously. I expect apps to either control delivery/discovery fully e2e, where unconstrained is fine, or need the additional guarantees from constrained delivery.
There was a problem hiding this comment.
But remember that handshakes are not per app, they are per (sender, recipient). So an app could want to use unconstrained handshakes, while another might need constrained handshakes. Forcing users to perform new handshakes each time they switch apps sounds too expensive when they can simply co-exist.
There was a problem hiding this comment.
Ah yes, good point. Given that, keying by mode is reasonable. I will switch over.
There was a problem hiding this comment.
Done. With this we also were able to simplify HandshakeNote and remove the AppSiloedSecret return type as it is unnecessary to return the mode as well.
…tore-a-delivery-mode-on-handshakenote
nchamo
left a comment
nchamo
left a comment
There was a problem hiding this comment.
Great work!
Really liked the tests 🚀
…hake_registry_contract/src/main.nr Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
vezenovm
deleted the
mv/f-694-handshake-registry-store-a-delivery-mode-on-handshakenote
branch
BEGIN_COMMIT_OVERRIDE feat(aztec-nr): Add a delivery mode to handshake notes (AztecProtocol#23783) fix(pxe): enforce full field consumption at oracle boundaries (AztecProtocol#23802) chore(ci): Static oracle version check (AztecProtocol#23805) END_COMMIT_OVERRIDE
2 participants
Fixes F-694
mode: u8field onHandshakeNoteu8and constant identifiers to represent the valid modes. We could switch to a new type, this just will require some extra serialization and type scaffolding. Happy to do that in this PR or file a follow-up. Made F-706 for now as a follow-up.