feat(protocol): tie the asset callback flag to the callback slots by onurinanc · Pull Request #3658 · 0xMiden/protocol

onurinanc · 2026-08-21T08:07:24Z

Summary

Derive an account's AssetCallbackFlag from the protocol-reserved asset callback slots installed by its components, replacing AccountBuilder::with_asset_callbacks with enable_asset_callbacks.
Reject new accounts in the transaction kernel prologue when their storage contains an asset callback slot while their account ID has callbacks disabled.
Apply the same rule in Account::new so accounts built or deserialized outside a transaction, including genesis accounts, cannot violate it either.

We have discussed here (https://github.com/0xMiden/protocol/pull/3547/changes#r3804351573) to apply "Callback slots are present <-> callbacks are enabled" in the protocol level.

However, this PR only implements one direction: if a callback slot is present, callbacks must be enabled. We left the other direction out because a faucet is allowed to enable callbacks now and add the policy slots later, and since the flag is immutable and inserted into the account ID, forbidding that would take the option away permanently.

…k slots

…k slot is present

…rade TODO

…AssetCallbackFlag

PhilippGackstatter

Looks good!

I think the main suggestion I have is adding the callback slots in TokenPolicyManager unconditionally.

PhilippGackstatter · 2026-08-21T10:10:00Z

+#! flag encoded in its account ID, and that flag is immutable once the ID is ground. A callback slot
+#! installed on an account whose flag is disabled would therefore look correctly configured while
+#! never being invoked, silently and permanently disabling whatever the callback enforces.
+#!


I think not requiring slots when the account ID flag is enabled actually makes sense. This retains the ability to add new types of callback slots that are introduced in a later protocol version.

I'd add something like this here:

`has_callback_slot` must imply `has_callbacks`, but not vice versa. That is, the callback flag can be enabled without callback slots present. This is allowed so that an account retains the ability to add a callback slot via an account upgrade later, which is particularly useful if new types of callbacks are introduced.

PhilippGackstatter · 2026-08-21T10:14:55Z

+    # The same applies to the asset callback rule validated in
+    # prologue::validate_asset_callbacks: an upgrade must not add an asset callback slot to an
+    # account whose asset callback flag is disabled, since the flag is immutable and the callback
+    # could then never be invoked.


Thanks for adding this 👌

PhilippGackstatter · 2026-08-21T10:16:51Z

+    /// The flag determines whether assets issued by the account (if any) trigger callbacks and is
+    /// encoded into the resulting [`AccountId`] at creation. It is normally derived from the
+    /// account's storage: it is [`AssetCallbackFlag::Enabled`] if any component installs one of the
+    /// protocol-reserved asset callback slots (see [`AssetCallbacks::is_installed`]) and
+    /// [`AssetCallbackFlag::Disabled`] otherwise. There is deliberately no way to disable the flag
+    /// for an account that does install such a slot, since the kernel gates callback invocation on
+    /// the flag alone and the flag cannot be changed after the ID is ground.
+    pub fn enable_asset_callbacks(mut self) -> Self {


This seems like general account builder information, so I would move this to the AccountBuilder type-level docs. These function docs can keep just the first sentence.

PhilippGackstatter · 2026-08-21T10:19:40Z

+        let account = Account::builder([7; 32])
+            .with_component(NoopAuthComponent)
+            .with_component(CustomComponent1 { slot0: 25 })
+            .build()
+            .unwrap();
+
+        assert_eq!(account.id().asset_callback_flag(), AssetCallbackFlag::Disabled);
+
+        let account = Account::builder([7; 32])
+            .enable_asset_callbacks()
+            .with_component(NoopAuthComponent)
+            .with_component(CustomComponent1 { slot0: 25 })
+            .build()
+            .unwrap();
+
+        assert_eq!(account.id().asset_callback_flag(), AssetCallbackFlag::Enabled);


let builder = Account::builder([7; 32]) .with_component(NoopAuthComponent) .with_component(CustomComponent1 { slot0: 25 }); let account = builder.clone().build().unwrap(); assert_eq!(account.id().asset_callback_flag(), AssetCallbackFlag::Disabled); let account = builder.enable_asset_callbacks().build().unwrap(); assert_eq!(account.id().asset_callback_flag(), AssetCallbackFlag::Enabled);

nit: conciseness

PhilippGackstatter · 2026-08-21T10:21:25Z

+    /// Accounts constructed outside of the builder are rejected if they install a callback slot
+    /// without having callbacks enabled.
+    #[test]
+    fn account_new_rejects_callback_slot_with_disabled_flag() {


nit: This tests the behavior of Account::new so the test module in crates/miden-protocol/src/account/mod.rs seems like a better place.

PhilippGackstatter · 2026-08-21T10:22:23Z

+///
+/// The transaction kernel decides whether to invoke an account's asset callbacks solely from the
+/// [`AssetCallbackFlag`] encoded in its [`AccountId`], and that flag is immutable once the ID is
+/// ground. A callback slot installed on an account whose flag is disabled would therefore look
+/// correctly configured while never being invoked, silently and permanently disabling whatever the
+/// callback enforces. The transaction kernel rejects such accounts when they are created; this
+/// mirrors that rule for accounts that are constructed or deserialized outside of a transaction.
+///


I'd remove this and point to AccountBuilder docs for details, so we don't duplicate this info.

PhilippGackstatter · 2026-08-21T10:24:22Z

+    for slot_name in AssetCallbacks::slot_names() {
+        if storage.get(slot_name).is_some() {
+            return Err(AccountError::AssetCallbackSlotWithDisabledFlag {
+                account_id: id,
+                slot_name: slot_name.clone(),
+            });
+        }
+    }


I think this could be written more concisely as:

if AssetCallbacks::is_installed(storage) { return Err(...); }

But I think this would read more nicely if it was storage.has_callbacks().

PhilippGackstatter · 2026-08-21T10:25:50Z

+    /// Returns `true` if `storage` contains at least one of the protocol-reserved asset callback
+    /// slots, `false` otherwise.
+    pub fn is_installed(storage: &AccountStorage) -> bool {


As mentioned in another comment, I'd make this a method on AccountStorage.

PhilippGackstatter · 2026-08-21T10:29:23Z

+/// switch. The slots are omitted only when no send or receive policy of any kind is registered, in
+/// which case the faucet's account ID is created with
 /// [`AssetCallbackFlag::Disabled`][miden_protocol::account::AssetCallbackFlag::Disabled].


Independent of the ability to add callback slots later, I think it makes sense to let the TokenPolicyManager always add the callback slots, because otherwise the callback flag will be disabled for the lifetime of the account and the transfer policies of the policy manager become unusable permanently.

Technically users can of course enable the flag by themselves to override this, but I find this a bit too subtle.

PhilippGackstatter · 2026-08-21T10:38:09Z

+    let code = "
+      use miden::tx_kernel_core::prologue
+
+      begin
+          exec.prologue::prepare_transaction
+      end
+      ";
+
+    let result = mock_tx.execute_code(code).await;
+
+    assert_execution_error!(


Suggested change

let code = "

use miden::tx_kernel_core::prologue

begin

exec.prologue::prepare_transaction

end

";

let result = mock_tx.execute_code(code).await;

assert_execution_error!(

let result = mock_tx.execute().await;

assert_transaction_executor_error!(

nit: bit more concise

…ariant

…ots conditionally

…execution

…he manager docs

…faucets

mmagician

LGTM ✅

PhilippGackstatter

Looks good to me!

Having TokenPolicyManager still only conditionally add the callback slots makes sense. I missed that this would mean that all faucets that add it, which is all of them, would always have to be FPI-ed into to check if callbacks are defined, and that would defeat the purpose of the callback flag. Good call 👍

PhilippGackstatter · 2026-08-26T12:23:03Z

+    pub fn has_callbacks(&self) -> bool {
+        AssetCallbacks::slot_names()
+            .iter()
+            .any(|slot_name| self.get(slot_name).is_some())


nit: I'd name this has_callback_slots because has_callbacks makes it sound like the storage could define a callback (function), so would be nice to disambiguate a bit.

…ack_slots

onurinanc added 11 commits August 21, 2026 10:57

feat(protocol): derive the asset callback flag from installed callbac…

671eeea

…k slots

feat(protocol): require an enabled asset callback flag when a callbac…

172424b

…k slot is present

docs(protocol): record the callback slot invariant in the account upg…

ebca364

…rade TODO

refactor: drop manual asset callback flag wiring from account factories

c78a467

chore: add changelog entry for the asset callback slot invariant

a184449

changelog

396da34

remove comments

6ed08a6

refactor(protocol): keep the builder's asset callback field typed as …

4806c4e

…AssetCallbackFlag

fix comments

53e5f10

fix comments

e4fa0b6

Merge remote-tracking branch 'origin' into refactor-callback-fix

4ae9bbd

onurinanc requested review from PhilippGackstatter and bobbinth August 21, 2026 09:44

PhilippGackstatter reviewed Aug 21, 2026

View reviewed changes

onurinanc added 4 commits August 24, 2026 13:51

merge

708a1b2

refactor(protocol): address review comments on the asset callback inv…

bcff8f8

…ariant

docs(standards): document why TokenPolicyManager installs callback sl…

912ef1d

…ots conditionally

test: simplify the callback flag prologue test to a full transaction …

434685a

…execution

mmagician reviewed Aug 24, 2026

View reviewed changes

Comment thread crates/miden-protocol/asm/kernels/transaction/lib/api.masm Outdated

Comment thread crates/miden-protocol/src/account/builder/mod.rs Outdated

Comment thread crates/miden-standards/src/account/policies/manager.rs Outdated

Comment thread crates/miden-agglayer/src/lib.rs Outdated

onurinanc added 3 commits August 24, 2026 16:51

docs(protocol): separate asset callback dispatch from callback presence

83a7f69

docs(standards): drop the AccountBuilder implementation detail from t…

e5db4dd

…he manager docs

fix(agglayer): require the reserved asset callback slots on AggLayer …

baf1ffc

…faucets

mmagician approved these changes Aug 25, 2026

View reviewed changes

onurinanc mentioned this pull request Aug 25, 2026

[H-02]: Multisig And Guarded Fungible Faucet Factories Disable Transfer Policy Enforcement OpenZeppelin/miden-confidential-contracts#147

Closed

mmagician requested a review from PhilippGackstatter August 26, 2026 11:16

PhilippGackstatter approved these changes Aug 26, 2026

View reviewed changes

onurinanc added 2 commits August 26, 2026 17:36

Merge remote-tracking branch 'origin' into refactor-callback-fix

78a1ce1

refactor(protocol): rename AccountStorage::has_callbacks to has_callb…

10e46b2

…ack_slots

Uh oh!

Conversation

onurinanc commented Aug 21, 2026

Summary

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants