fix(standards): register protocol callback slots for AllowAll transfer policy by onurinanc · Pull Request #2946 · 0xMiden/protocol

onurinanc · 2026-05-18T15:10:08Z

Closes: #2945

PhilippGackstatter

Looks good. I left a few comment/questions.

PhilippGackstatter · 2026-05-19T06:55:45Z

+        // Register the protocol-reserved asset-callback slots whenever any transfer policy is
+        // configured on this manager (active or reserved alternative, including `AllowAll`).
+        // With the flattened policy dispatch the kernel reads these slots directly and
+        // dispatches via `call`, so writing the active roots (including `AllowAll`, which is a
+        // no-op `dropw`) wires up the dispatch path uniformly.
+        //
+        // Registering the slots whenever transfer policies are present stamps
+        // `AssetCallbackFlag::Enabled` on every asset minted by this faucet (see
+        // `protocol::faucet::create_fungible_asset`, which gates the flag on the callback slots
+        // being populated). Without this, a faucet that ships with `AllowAll` for transfer
+        // would mint callback-less assets that are permanently exempt from any policy
+        // installed later via `set_send_policy` / `set_receive_policy` — fragmenting the
+        // circulating supply into enforceable and exempt subsets and defeating
+        // post-deployment compliance upgrades.


Nit: I think it would be better if this was more concise and included less details. Somehow the formatting is also a bit off (line wrapping at 100 characters).

Register the protocol-reserved asset-callback slots whenever any transfer policy is configured on this manager. Registering the slots whenever transfer policies are present stamps `AssetCallbackFlag::Enabled` on every asset minted by this faucet. Without this, a faucet that with `AllowAll` for transfer would mint callback-less assets that are permanently exempt from any transfer policy installed later. This would fragment the circulating supply into enforceable and exempt asset sets. When no transfer policy is set, the callback slots are not added meaning all minted assets have callbacks disabled.

Reading all of this though makes me think that adding the callback flag to minted assets based on the presence of the callback slot may be too subtle. Maybe there are cases, where faucets may want to always issue callback-enabled assets even if the faucet does not yet define any or even has the callback slots.

It may be worth another look how bad it would be to require this flag as an input instead of deriving it. It might be better to be more explicit and require the enable_callbacks flag as an input to the asset creation, even if it's a bit more annoying to carry this flag through in all the places.

Reading all of this though makes me think that adding the callback flag to minted assets based on the presence of the callback slot may be too subtle. Maybe there are cases, where faucets may want to always issue callback-enabled assets even if the faucet does not yet define any or even has the callback slots.

I think that's a really good point, in fact, I believe that faucets should always issue callback-enabled assets, but the policies uses callback such as send/receive policies needs to be either no-op (allow all) or enforce a policy. If we somehow design the changes in that way, we might even don't need for the enable_callbacks flag as an additional parameter to carry it in all the places.

Generally agree, but the one reason why we need the callback flag in the asset is to avoid having to load the foreign account for assets that don't need callbacks. Loading the foreign account can be quite time-consuming. Doing this only to find that no callbacks are set would add inefficiency to the callback process.

PhilippGackstatter · 2026-05-19T06:58:25Z

+    /// Regression test for "Enabling transfer callbacks after minting permanently exempts
+    /// earlier assets from send/receive policy enforcement".
+    ///
+    /// A faucet deployed with only `TransferAllowAll` (no reserved alternative) must still
+    /// register the protocol-reserved asset-callback slots, populated with `TransferAllowAll`'s
+    /// procedure root. The protocol stamps `AssetCallbackFlag::Enabled` on every minted asset
+    /// whenever those slots are populated — without this, a later policy switch could not
+    /// retroactively apply to assets minted under `AllowAll`.
+    ///
+    /// Pre-fix, `manager_storage_slots` omitted the callback slots in this configuration via
+    /// the `needs_callbacks` predicate (it considered `AllowAll` a "no enforcement, no
+    /// callbacks" case). This test pins the new contract: registering any transfer policy —
+    /// `AllowAll` included — populates the protocol callback slots.
+    #[test]
+    fn allow_all_transfer_policy_registers_protocol_callback_slots() {


Nit: I think this is a bit too much detail. Especially describing the situation before the fix makes sense in the context of the PR, but will probably not be helpful when read in a month. I'd rather link to the GH issue.

This is a repeating occurrence with AI-generated docs and we're trying to reduce this with a skill, but until that lands, might be worth manually checking this.

PhilippGackstatter · 2026-05-19T06:59:51Z

+        // Both slots must hold the AllowAll procedure root (not zero). The protocol stamps
+        // the callback flag on minted assets whenever these slots are non-empty.


Suggested change

// Both slots must hold the AllowAll procedure root (not zero). The protocol stamps

// the callback flag on minted assets whenever these slots are non-empty.

// Both slots must hold the AllowAll procedure root (not zero).

PhilippGackstatter · 2026-05-19T07:03:02Z

+    /// A manager configured without any send / receive policy (e.g. used purely for mint /
+    /// burn policy storage, with a custom callback component owning the protocol slots)
+    /// must NOT register the protocol callback slots — otherwise it would collide with the
+    /// custom component on the same slot names.
+    ///
+    /// This pairs with [`allow_all_transfer_policy_registers_protocol_callback_slots`]: the
+    /// guard "register iff a transfer policy is configured" is what keeps kernel-level
+    /// callback test helpers working after the fix.


I'm not sure the reasoning for why the component should not include the callback slots here makes sense to me. I think the primary motivation is to omit the callback slots such that minted assets have callbacks disabled and do not incur that additional check during transfers.

Suggested change

/// A manager configured without any send / receive policy (e.g. used purely for mint /

/// burn policy storage, with a custom callback component owning the protocol slots)

/// must NOT register the protocol callback slots — otherwise it would collide with the

/// custom component on the same slot names.

///

/// This pairs with [`allow_all_transfer_policy_registers_protocol_callback_slots`]: the

/// guard "register iff a transfer policy is configured" is what keeps kernel-level

/// callback test helpers working after the fix.

/// A manager configured without any send / receive policy must NOT register the

/// protocol callback slots — otherwise it would always needlessly mint assets with

/// callbacks enabled.

PhilippGackstatter · 2026-05-19T07:07:26Z

-    // Execute transaction to consume the output note with the target account
+    // Execute transaction to consume the output note with the target account. The faucet must
+    // be supplied as a foreign account so the kernel can dispatch the receive callback on it
+    // when the asset is added to the target account's vault.
+    let faucet_inputs = mock_chain.get_foreign_account_inputs(faucet.id())?;


Nit: I think the previous comment was sufficient.

PhilippGackstatter · 2026-05-19T07:09:36Z

+        .vault()
+        .get(expected_asset.vault_key())
+        .expect("vault should contain the minted asset");
+    assert_eq!(stored_asset.unwrap_fungible().amount(), expected_asset.amount());


This is fixed in #2939. We should use get_balance here, and so it would be good not to introduce this temporary change that we'll have to revert later (in #2939 or later). So for now, I'd revert all changes to tests that have this fix.

Edit: An alternative would be to introduce a get_balance_enabled API that we'll temporarily use, as it would be easier to revert/remove than the changes here.

PhilippGackstatter · 2026-05-19T07:12:21Z

-    let mint_asset: Asset =
-        FungibleAsset::new(faucet.id(), amount.as_canonical_u64()).unwrap().into();
+    let mint_asset: Asset = FungibleAsset::new(faucet.id(), amount.as_canonical_u64())
+        .unwrap()
+        .with_callbacks(AssetCallbackFlag::Enabled)
+        .into();
    let serial_num = Word::from([1, 2, 3, 4u32]);


What was the rationale for changing a couple of tests to use callback-enabled assets? For future PRs, could you include such details in the PR description as it speeds up understanding and reviews? Thanks!

PhilippGackstatter

Looks good to me!

PhilippGackstatter · 2026-05-20T07:53:19Z

+    /// Returns the balance of the callback-enabled fungible asset issued by the specified
+    /// faucet.
+    ///
+    /// # Errors
+    /// Returns an error if the specified ID is not an ID of a fungible asset faucet.
+    pub fn get_balance_enabled(
+        &self,
+        faucet_id: AccountId,
+    ) -> Result<AssetAmount, AssetVaultError> {
+        if !matches!(faucet_id.account_type(), AccountType::FungibleFaucet) {


Sorry for the back and forth on this, but now that #2939 was merged, we can remove this and use get_balance directly.

bobbinth

Looks good! Thank you! I mostly reviewed non-test code, and also tried to update the branch to the latest next - but seems like some tests are not passing now. I suspect this has something to do with AggLayer faucets always implying callbacks disabled (which should be fixed with #2963) - but also, I didn't look deep enough to know for sure.

cc @partylikeits1983 @mmagician in case you guys have some comments.

bobbinth · 2026-05-21T23:47:04Z

Seems like one test is still failing still. @onurinanc is there a path to fixing it?

mmagician · 2026-05-22T06:47:26Z

but seems like some tests are not passing now. I suspect this has something to do with AggLayer faucets always implying callbacks disabled (which should be fixed with #2963) - but also, I didn't look deep enough to know for sure.

No these are not AggLayer tests.
The issue is that test_public_note_creation_with_script_from_datastore creates a faucet with callbacks*, but the script hardcodes 0 as the callbacks_flag.

*the callbacks were not configured automatically until this PR

fixed in fix: tests should respect the faucets callback flag

…k-fix

mmagician · 2026-05-22T07:27:23Z

+    movdn.2 push.1
+    # => [1, faucet_id_suffix, faucet_id_prefix, native_amount, faucet_id_suffix, faucet_id_prefix]


@onurinanc we should have kept this TODO, regardless of which direction is the default. We need to be able to support the registration of both types of agglayer faucets, with & without callbacks

mmagician · 2026-05-22T07:36:40Z


    # prepare stack for asset::create_fungible_asset: [enable_callbacks, suffix, prefix, amount]
+    # TODO(#2963): callbacks bit is hardcoded to 1 to match AggLayer faucets configured via
+    # the M-01 fix (which register the protocol callback slots through their `AllowAll`


as per my previous comments, I'd avoid putting PR-specific changes in the doc comments. This won't make sense to anyone reading this comment outside of that PR context

onurinanc added 2 commits May 18, 2026 17:08

fix

820a045

changelog

5b4f918

onurinanc requested review from PhilippGackstatter and bobbinth May 18, 2026 15:26

PhilippGackstatter reviewed May 19, 2026

View reviewed changes

add get_balance_enabled

0e7f54c

onurinanc requested a review from PhilippGackstatter May 19, 2026 11:42

PhilippGackstatter approved these changes May 20, 2026

View reviewed changes

onurinanc added 2 commits May 20, 2026 11:24

merge next and use get_balance

5d48458

merge next

acbd43a

onurinanc requested a review from PhilippGackstatter May 20, 2026 10:35

PhilippGackstatter approved these changes May 20, 2026

View reviewed changes

onurinanc and others added 3 commits May 20, 2026 14:10

Merge branch 'next' into m1-callback-fix

520e2eb

Merge branch 'next' into m1-callback-fix

4cc22d3

chore: fix build

46bd3e6

bobbinth approved these changes May 21, 2026

View reviewed changes

onurinanc added 2 commits May 21, 2026 14:36

merge next

c672e97

merge origin/m1-callback-fix

f7b3738

mmagician approved these changes May 22, 2026

View reviewed changes

Comment thread crates/miden-testing/tests/scripts/faucet.rs Outdated

Comment thread crates/miden-testing/tests/scripts/faucet.rs Outdated

onurinanc and others added 2 commits May 22, 2026 08:37

Merge remote-tracking branch 'origin/next' into m1-callback-fix

e7b5a58

fix: tests should respect the faucets callback flag

eb4ee26

chore: make comments less change-verbose

8288d9c

mmagician enabled auto-merge May 22, 2026 06:50

onurinanc added 3 commits May 22, 2026 09:03

Merge branch 'next' into m1-callback-fix

1a633dd

align mint scripts with callback-enabled assets

f7c1bbc

Merge remote-tracking branch 'origin/m1-callback-fix' into m1-callbac…

23cde49

…k-fix

mmagician added this pull request to the merge queue May 22, 2026

mmagician removed this pull request from the merge queue due to a manual request May 22, 2026

onurinanc requested review from PhilippGackstatter, bobbinth and mmagician May 22, 2026 07:26

mmagician requested changes May 22, 2026

View reviewed changes

revert changes

5486a73

mmagician reviewed May 22, 2026

View reviewed changes

Merge branch 'next' into m1-callback-fix

8c5ad38

mmagician approved these changes May 22, 2026

View reviewed changes

Comment thread crates/miden-agglayer/asm/agglayer/bridge/bridge_in_output.masm Outdated

Update crates/miden-agglayer/asm/agglayer/bridge/bridge_in_output.masm

36af891

mmagician enabled auto-merge May 22, 2026 07:56

mmagician added this pull request to the merge queue May 22, 2026

Merged via the queue into next with commit 32f1a03 May 22, 2026
20 checks passed

mmagician deleted the m1-callback-fix branch May 22, 2026 08:18

		// Both slots must hold the AllowAll procedure root (not zero). The protocol stamps
		// the callback flag on minted assets whenever these slots are non-empty.

		movdn.2 push.1
		# => [1, faucet_id_suffix, faucet_id_prefix, native_amount, faucet_id_suffix, faucet_id_prefix]

Conversation

onurinanc commented May 18, 2026

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!

PhilippGackstatter May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

bobbinth commented May 21, 2026

Uh oh!

Uh oh!

Uh oh!

mmagician commented May 22, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PhilippGackstatter May 19, 2026 •

edited

Loading