Skip to content

refactor: remove asset value from callback outputs - #3513

Merged
bobbinth merged 8 commits into
nextfrom
ajl-remove-callback-value-return
Aug 7, 2026
Merged

refactor: remove asset value from callback outputs#3513
bobbinth merged 8 commits into
nextfrom
ajl-remove-callback-value-return

Conversation

@partylikeits1983

@partylikeits1983 partylikeits1983 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Closes #3505.

@partylikeits1983 partylikeits1983 added kernels Related to transaction, batch, or block kernels pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority labels Aug 6, 2026
@partylikeits1983
partylikeits1983 marked this pull request as ready for review August 6, 2026 15:47

@PhilippGackstatter PhilippGackstatter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few suggestions.

Comment thread crates/miden-protocol/asm/kernels/transaction-core/src/callbacks.masm Outdated
Comment thread crates/miden-testing/src/kernel_tests/tx/test_callbacks.rs Outdated
Comment thread crates/miden-testing/src/kernel_tests/tx/test_callbacks.rs Outdated
Comment thread docs/src/asset.md Outdated
PhilippGackstatter and others added 3 commits August 7, 2026 15:02
# Conflicts:
#	crates/miden-standards/asm/standards/faucets/policies/transfer/basic_allowlist.masm
#	crates/miden-standards/asm/standards/faucets/policies/transfer/basic_blocklist.masm

@zeapoz zeapoz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just two comments about the changelog

Comment thread CHANGELOG.md
Comment thread CHANGELOG.md Outdated

@PhilippGackstatter PhilippGackstatter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@partylikeits1983 partylikeits1983 self-assigned this Aug 7, 2026

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left one comment inline. The code technically works there, but should be improved because the pattern is used incorrectly. But we can fix it in a small follow-up.

Comment on lines +556 to +558
# No policy configured: consume the callback inputs.
dropw dropw dropw drop
# => [pad(16)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens to work because this procedure is always invoked as a last thing in a call-ed procedure, but I don't think we should be doing this - at least not without being super explicit about the invocation context in the comments.

Better yet, we should change the signature to something like:

#! Inputs:  [slot_id_suffix, slot_id_prefix, ASSET_ID, ASSET_VALUE, custom_data]
#! Outputs: []

And handle padding adjustments at the callsites.

@bobbinth
bobbinth added this pull request to the merge queue Aug 7, 2026
Merged via the queue into next with commit 0a69e33 Aug 7, 2026
20 checks passed
@bobbinth
bobbinth deleted the ajl-remove-callback-value-return branch August 7, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernels Related to transaction, batch, or block kernels pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the redundant asset-value return from asset callbacks

4 participants