feat(aztec-nr): Get tagging index for constrained delivery #23359
Conversation
…eNote, Context>, Context>, Context> and additional test
…hake_registry_contract/src/main.nr Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
… dom sep
The new DOM_SEP__CONSTRAINED_MSG_LOG_TAG domain separator added in this PR
adds one assert_dom_sep_matches_derived call, which pushes to both seen_u32
and seen_values. constants_tests::hashed_values_match_derived sized the
tester at <71, 64>, so the extra push overflowed the BoundedVec
("push out of bounds" at bounded_vec.nr:186). Bump to <72, 65> to match the
actual 72 value / 65 u32 hashed entries.
…next_app_tag_as_sender-for-constrained' into maxim/f-668-aztec-nr-extend-get_next_app_tag_as_sender-for-constrained
nchamo
left a comment
There was a problem hiding this comment.
I haven't done an in-depth review, but I have some question that we should address before that
| * Both shapes expose `{ secret, app }` and a `kind` literal discriminator, plus a self-describing `toString()` so | ||
| * they can share storage that keys on `secret.toString()`. | ||
| */ | ||
| export type AppTaggingSecret = ExtendedDirectionalAppTaggingSecret | ConstrainedAppTaggingSecret; |
There was a problem hiding this comment.
I'm curious about this. What if only had:
type AppTaggingSecret = {
secret: Fr,
app: AztecAddress,
kind: 'constrained' | 'unconstrained'
}It could be a class if we need it. And then, when converting to string, we could to:
${kind}:${secret}"${app}
And when converting from string, we could check if `.split(":") has length 2 or 3. If it's 2, then we know it's unconstrained (and we leave a comment explaining that we are keeping it that way for backwards compatibility purposes)
Would that be possible? I think that it makes sense to generalize the concept of AppTaggingSecret and just have one, instead of having multiple
Do you see a reason why this wouldn't work?
There was a problem hiding this comment.
There shouldn't be anything preventing that design. I considered generalizing in that direction too, which is why both secret shapes now share the same sender-side storage path.
However, I decided to maintain the separate types internally as the ExtendedDirectionalAppTaggingSecret is used in various places such as recipient syncing and I wanted to preserve existing APIs. We do still have one "app tagging secret" abstraction here, the internal representation is just a union of types rather than a unified single concrete type.
Ultimately I was trying to reduce unrelated changes in this PR (e.g., unrelated call-sites of ExtendedDirectionalAppTaggingSecret). If you feel strongly about this structure I can migrate fully to a single AppTaggingSecret type here, but I think this is a reasonable follow-up refactor. I could also stack the PR on this one to split out unrelated changes a bit more.
There was a problem hiding this comment.
Noise reduced with #23602 and switched to a single AppTaggingSecret
There was a problem hiding this comment.
Switching to a single AppTaggingSecret let us get rid of siloedTagFor and we could discriminate on the kind inside of SiloedTag.compute now, which was a nice benefit.
| * @param appSiloedSecret - The app-siloed shared secret retrieved from the handshake registry by the caller. | ||
| * @returns The next index to use for this secret. | ||
| */ | ||
| public async getNextConstrainedIndex(appSiloedSecret: Fr): Promise<number> { |
There was a problem hiding this comment.
Does this detect already if another PXE with the same recipient-sender pair used an index? For example, PXE A uses 1, so PXE B detects it and moves on to index 2
Maybe the unconstrained path already did it, what why I'm asking. It would be a nice e2e test to have if I'm being honest
There was a problem hiding this comment.
I realized I edited the tag computation in yarn-project/pxe/src/tagging/sender_sync/sync_sender_tagging_indexes.ts but didn't update any tests in yarn-project/pxe/src/tagging/sender_sync/sync_sender_tagging_indexes.test.ts. Do you think that would be sufficient for testing here? Looking at "step 1: highest finalized index is updated" this looks like basically what we want to test.
There was a problem hiding this comment.
You are right, that is exactly what we needed. I love it when the code already works as expected 🎉
No need to do anything else
There was a problem hiding this comment.
The siloing only checks unconstrained tags still though. So I will extend that one test to check both tag kinds.
There was a problem hiding this comment.
Added a single constrained variant test. Didn't feel the need to replicate the entire test suite but still exercises the constrained secret path.
Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar> Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
…f-668-aztec-nr-extend-get_next_app_tag_as_sender-for-constrained # Conflicts: # yarn-project/pxe/src/contract_function_simulator/oracle/oracle.ts # yarn-project/pxe/src/oracle_version.ts
## Summary Renames `ExtendedDirectionalAppTaggingSecret` to `AppTaggingSecret`, including the source file, exports, call sites, tests, and generated API references. This prepares for #23359 to make `AppTaggingSecret` the single concrete tagging secret type with a `kind` discriminator. Review context: #23359 (comment) --------- Co-authored-by: AztecBot <tech@aztec-labs.com>
…f-668-aztec-nr-extend-get_next_app_tag_as_sender-for-constrained # Conflicts: # yarn-project/pxe/src/contract_function_simulator/execution_tagging_index_cache.ts # yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts # yarn-project/pxe/src/storage/tagging_store/sender_tagging_store.test.ts # yarn-project/pxe/src/storage/tagging_store/sender_tagging_store.ts # yarn-project/pxe/src/tagging/reconcile_tagging_index_ranges.ts # yarn-project/pxe/src/tagging/sender_sync/utils/load_and_store_new_tagging_indexes.ts # yarn-project/stdlib/src/logs/app_tagging_secret.test.ts # yarn-project/stdlib/src/logs/app_tagging_secret.ts # yarn-project/stdlib/src/logs/index.ts # yarn-project/stdlib/src/logs/tagging_index_range.ts # yarn-project/stdlib/src/tests/factories.ts
| // Keep the existing two-part unconstrained key so stored tagging entries remain addressable. | ||
| if (this.kind === AppTaggingSecretKind.UNCONSTRAINED) { | ||
| return `${this.secret.toString()}:${this.app.toString()}`; | ||
| } |
There was a problem hiding this comment.
I don't quite follow this. Why do we need to keep doing it this way?
There was a problem hiding this comment.
I improved the comments to note that this is to remain aligned with the legacy key. Most accesses of the SenderTaggingStore will supply secret.toString() when wanting to fetch a value. To remove this logic we would have to support reading both variants from the store or migrate the storage fully to the new format. Should I write a follow-up to do a full storage migration? In general a strategy for migrating storage keys sounds like it would be useful but out of scope here.
There was a problem hiding this comment.
I see. Let's create a linear issue for it then (for next cycle maybe?) and reference it in a comment
There was a problem hiding this comment.
Issue here https://linear.app/aztec-labs/issue/F-680/migrate-pxe-tagging-stores-to-prefixed-apptaggingsecret-keys. Linked in toString, fromString, and the relevant test.
| return Promise.all( | ||
| deduplicatedSenders.map(async sender => { | ||
| const secret = await AppTaggingSecret.compute( | ||
| const secret = await AppTaggingSecret.computeUnconstrained( |
There was a problem hiding this comment.
Changed the name here to be more accurate
Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
…next_app_tag_as_sender-for-constrained' into maxim/f-668-aztec-nr-extend-get_next_app_tag_as_sender-for-constrained
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
BEGIN_COMMIT_OVERRIDE refactor(aztec-nr): use constructor methods for MessageDelivery variants (AztecProtocol#23596) docs: update testing_contracts.md for two-crate aztec new layout (AztecProtocol#23617) fix: drop usage of include and indexof on types that support equals (AztecProtocol#23595) fix: unused ts expressions in tests (AztecProtocol#23621) feat(aztec-nr): Get tagging index for constrained delivery (AztecProtocol#23359) feat!: demote auth registry to non-protocol contract (AztecProtocol#23106) feat(aztec-nr)!: embed BoundedVec max length in validation requests (AztecProtocol#23622) fix: regenerate standard contract addresses after auth registry demotion (AztecProtocol#23640) feat(aztec-nr): encrypt handshake log for indistinguishability (AztecProtocol#23638) feat!: demote public_checks to non-protocol contract (AztecProtocol#23217) fix: noir precommit re-staging inside worktrees (AztecProtocol#23628) END_COMMIT_OVERRIDE
Fixes F-668
get_next_constrained_index_oraclethat accepts an app siloed secret (expected to be returned from the handshake registry utility contract added in feat(aztec-nr): V2 handshake registry for non interactive constrained delivery #23278