feat(aztec-nr)!: remove set_sender_for_tags oracle, use builder pattern for sender override

[nchamo]

Why we are doing this

The set_sender_for_tags oracle was a mutable, side-effectful call that contracts used to override which address is used for discovery tag derivation. The connection between the set_sender_for_tags call and the later .deliver() call was non-obvious.

Our fix

Replace the oracle with a .with_sender(address) builder method on OnchainDelivery, making the sender selection explicit at the delivery call site. MessageDelivery::onchain_constrained() and onchain_unconstrained() now return OnchainDelivery (which exposes with_sender), while offchain() returns MessageDelivery directly (no sender override, since offchain delivery does not use tags).

A MessageDeliveryBuilder trait lets deliver() accept both types generically.

Migration

The set_sender_for_tags oracle has been removed. Contracts that used it should pass the sender address via the .with_sender() builder on onchain delivery modes:

- use aztec::oracle::notes::set_sender_for_tags;

- unsafe { set_sender_for_tags(some_address) };
- note.deliver(MessageDelivery::onchain_constrained());
+ note.deliver(MessageDelivery::onchain_constrained().with_sender(some_address));

Contracts that did not call set_sender_for_tags require no changes.

[nventuro]

Thank you for trying to make the world a better place.

[nventuro]

This seems a bit verbose and ad-hoc. Two suggestions:

1. Have a second method that just takes the param, avoiding the object, similar to TestEnvironment.private_context_at, makes sense when some option is used often

note.deliver(MessageDelivery::onchain_constrained_from(some_address);

2. Have a more general options object and a variant that takes it, so that we can further extend these options in the future in backwards compatible ways (like TestEnvironment::private_context_opts:

note.deliver(MessageDelivery::onchain_constrained_opts(DeliveryOpts::new().sender(some_address)));

If you think we'll add more options in the future, I'd go with 2. If not 1 might be good enough.

[nventuro]

Actually in this case because the method is already deliver, what you could do instead is have deliver take the full options object and use a builder pattern. This would be more extensible and less verbose

struct DeliveryMethod {
  variant: u8,
  sender: Option<Address>, 
}

impl DeliveryMethod {
  fn onchain_constrained() -> Self {
     Self { variant: ONCHAIN_CONST, sender: none() }
  }

  fn onchain_unconstrained() -> Self { ... }

  fn with_sender(&mut self, sender: Addr) -> Self {
     self.sender = some(sender);
     self
  }
}

and you then do

msg.deliver(DeliveryMethod.onchain_constrained());
// or
msg.deliver(DeliveryMethod.onchain_unconstrained().with_sender(some_sender));

If you want to forbid certain combinations (e.g. not allow with_sender for offchain delivery) then you can either add runtime checks to that fn, or compile time checks by having each delivery fn return a different type, which then each have different methods of their own (the config for each deli mechanism).

Note how in this case we're not exposing the internal types, the members of the struct etc., we just commit to the method chain. So we can freely add more stuff or change the internal repr with no breaking changes.

[nchamo]

I really liked this idea, I implemented it so that .offchain() couldn't be chained with .with_sender()

[nchamo]

As requested

[nventuro]

Does this and MessageDelivery need to be pub?

[nchamo]

Yeah, sadly yes

[nventuro]

Eh it's not too bad I guess, as longs as the members are not pub (and so the thing cannot be constructed directly).

[AztecBot]

Flakey Tests

🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry.

\033FLAKED\033 (8;;http://ci.aztec-labs.com/969280e836194079�969280e8361940798;;�):  yarn-project/end-to-end/scripts/run_test.sh simple src/e2e_slashing/attested_invalid_proposal.test.ts (363s) (code: 0)