-
Notifications
You must be signed in to change notification settings - Fork 119
Description
Issue #1717 previously brought up the suggestion to avoid returning output parameters that are equivalent to the input parameters. For the compiler it results in unnecessary overhead and for MASM users it would be easy to duplicate the inputs if they are needed. In many cases, they aren't needed and so we should consider removing those.
This applies to:
{active_note, input_note, output_note}::get_assets takes and returns dest_ptr and note_index. In almost all cases where this procedure is called, the returned note_index is either incorrectly not documented and/or unused or could be easily duplicated.
active_note::get_storage takes and returns dest_ptr. A typical pattern we have in standard note scripts is to use constants for the storage ptrs, and so we actually have to deliberately drop the returned pointer to produce more readable code using the constants, e.g.:
protocol/crates/miden-standards/asm/standards/notes/p2id.masm
Lines 44 to 57 in 24ca144
| pub proc main | |
| # store the note storage to memory starting at address 0 | |
| push.STORAGE_PTR exec.active_note::get_storage | |
| # => [num_storage_items, storage_ptr] | |
| # make sure the number of storage items is 2 | |
| eq.2 assert.err=ERR_P2ID_UNEXPECTED_NUMBER_OF_STORAGE_ITEMS | |
| # => [storage_ptr] | |
| # read the target account ID from the note storage | |
| drop | |
| mem_load.TARGET_ACCOUNT_ID_PREFIX_PTR | |
| mem_load.TARGET_ACCOUNT_ID_SUFFIX_PTR | |
| # => [target_account_id_suffix, target_account_id_prefix] |
note::write_assets_to_memory takes and returns num_assets and dest_ptr. Since it is used internally in the above-mentioned procedures, the need for returning these params goes away when we refactor the above procedures.
After looking more closely at faucet::mint I realized that it returns the asset to be minted after merging it into the transaction's input vault. So, assuming the same asset issuer, if we have an input note with amount 40 and we call mint(50) it would return 90 and calling mint(60) again would return 150. Because this value is affected by input notes and by the calling account's initial balance plus the previous mint (or burn) calls, I'm not sure it is actually a meaningful value to return. So, I think we could remove the return value from faucet::mint and since faucet::burn has the same "problem", also remove the output value from that.
Another offender is native_account::remove_asset, though instead of removing it, we should refactor it. See #2524.
After these tiny refactors we should be consistent in terms of this rule.
fyi @greenhat