fix(txe): align tagging strategy oracle with PXE#24561
Conversation
Review cleanup for the unconstrained delivery coverage: - TestEnvironmentOptions now exposes a keyed with_tag_secret_strategy(mode, strategy) setter plus with_tag_secret_strategy_all_modes; a mode left unset falls back to the PXE default (non-interactive handshake), matching a hook that hardcodes the default for modes it does not customize. - Share DEFAULT_TAGGING_SECRET_STRATEGY between PXE and the TXE hook fallback. - Add compute_directional_app_secret so tests derive expected secrets instead of inlining the poseidon2 derivation. - Merge the per-mode next-index test wrappers into next_index_for_secret(secret, mode). - Drop dead locals, reuse toSingle in the oracle unit test, reword the test contract module doc.
Replaces the per-mode aztec_txe_setTaggingSecretStrategy oracle (called twice per TestEnvironment creation) with a single aztec_txe_setTaggingSecretStrategies call carrying an option per delivery mode, halving the setup round-trips every TXE test pays. Also updates the stale execution_hooks doc example to the current TestEnvironmentOptions API, refreshes the onchain delivery test module header to cover the unconstrained tests, and tidies minor conventions in the TXE tagging strategy code (JSDoc, stale comment, compound test assertion).
vezenovm
changed the title
nchamo
left a comment
nchamo
left a comment
There was a problem hiding this comment.
Great work!
This is much more helpful than the original oracle way I had done it. Just left a few comments/questions
| /// and the recipient is folded in for direction. Mirrors `AppTaggingSecret.computeDirectional` on the PXE side; tests | ||
| /// use it to derive the secret they expect an [`arbitrary_secret`](TaggingSecretStrategy::arbitrary_secret) strategy | ||
| /// to produce. | ||
| pub fn compute_directional_app_secret(secret: EmbeddedCurvePoint, app: AztecAddress, recipient: AztecAddress) -> Field { |
There was a problem hiding this comment.
I like that we have this, but I have two questions:
- Should it be here? Maybe it should be next to
compute_app_siloed_shared_secret? - Does it need to be
pub, or can it bepub(crate)?
There was a problem hiding this comment.
- I thought about moving it there too, but this is only used by tests so it felt better to leave in the test module.
- Yeah it can be
pub(crate).
There was a problem hiding this comment.
Actually we used this method in the new test in noir-projects/noir-contracts/contracts/test/onchain_delivery_test_contract/src/test.nr. I kept it as pub for now (I think another reason to leave it under the test module).
| } | ||
|
|
||
| #[test] | ||
| unconstrained fn constrained_mode_arbitrary_secret_resolves_without_rejecting() { |
There was a problem hiding this comment.
I'm not sure what we are trying to test here. I mean, I know that we don't validate it in resolve_tagging_strategy because we do that later, in constrained world. But do we need to test it? What do we win from it?
There was a problem hiding this comment.
I just wanted something to pin the boundary. TXE resolves the strategy, while constrained delivery rejects
unsound secrets later. I could rename to something like defers_constrained_arbitrary_secret_validation_to_delivery. Or do you think it is still unnecessary?
There was a problem hiding this comment.
I personally think it's unnecessary. But if you want to keep it, that name helps
There was a problem hiding this comment.
I decided to merge this into app_silos_an_arbitrary_secret_point where we just test both delivery modes and I left a comment about the deferred validation. There were no other tests for the constrained * arbitrary secret combo.
3 participants
Fixes F-700
Fixes F-765