Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ordering and naming of kernel procedure offsets #1037

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Dec 26, 2024

This PR updates the naming and ordering of the kernel procedure offsets, making them more consistent. It also adds some kernel procedures which were missing, but could be potentially useful in future.

@Fumuran
Copy link
Contributor Author

Fumuran commented Dec 26, 2024

I chose the naming and ordering strategy which looks like so:

  1. Leave the high-level partitions (account, faucet, note, tx), but update the ordering inside them.
  2. Change the low-lever ordering to go from the procedures which corresponds to the entire unit to its smaller parts (for example, for note it is creation -> i/o commitments -> asset management -> internal parameters).
  3. For the account the rule in point 2 is slightly changed to match the order in which account parts are stored in memory (`entire hash -> [id and nonce -> procedures -> storage -> vault]).
  4. For the names I chose this pattern:
    HIGH_LEVEL_UNIT->MAYBE_LOW_LEVEL_UNIT->MAYBE_GET/SET->PURPOSE_DESCRIPTION->"OFFSET"_KEYWORD.
    It works fine almost everywhere, except for the NOTE_GET_INPUT/OUTPUT_NOTES_COMMITMENT_OFFSET constants,
    since the note keyword is used in the purpose description part, so it looks a bit redundant.

I deliberately changed only constant names for now (so the project doesn't even build) because first I need to make sure that my approach is fine and the naming is acceptable.

I also didn't add the missing kernel procedures, since I want to make sure that we will need them in future. For now it looks like we could possibly need these procedures:

  1. account_procedures_get_code
  2. account_vault_set_commitment
  3. faucet_non_fungible_is_issued

@Fumuran Fumuran requested a review from bobbinth December 26, 2024 14:17
Copy link
Contributor

@bobbinth bobbinth left a comment

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 some comments inline.

Comment on lines +6 to +7
const.ACCOUNT_GET_CURRENT_HASH_OFFSET=0
const.ACCOUNT_GET_INITIAL_HASH_OFFSET=1
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move ACCOUNT_GET_INITIAL_HASH_OFFSET up to be the first one in the list.

Comment on lines +17 to +18
## get account code is missing
const.ACCOUNT_PROCEDURES_SET_CODE_OFFSET=5
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this ACCOUNT_SET_CODE_COMMITMENT_OFFSET? More generally though, I wonder if we should just remove this procedure and all associated code as I don't think we currently support updating account code and the way it'll eventually work will probably be different from now (e.g., we'll also need to update storage slot data).

But, I'd add ACCOUNT_GET_CODE_COMMITMENT_OFFSET and create a procedure for it. The procedure would be pretty simple (i.e., just return the code commitment of the current account).

Comment on lines +20 to +24
# Storage
const.ACCOUNT_STORAGE_GET_ITEM_OFFSET=6
const.ACCOUNT_STORAGE_SET_ITEM_OFFSET=7
const.ACCOUNT_STORAGE_GET_MAP_ITEM_OFFSET=8
const.ACCOUNT_STORAGE_SET_MAP_ITEM_OFFSET=9
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add ACCOUNT_GET_STORAGE_COMMITMENT_OFFSET and a corresponding procedure (this procedure would just return the storage commitment word).

I'm also thinking that maybe we could skip the STORAGE part? So, this could be:

const.ACCOUNT_GET_STORAGE_COMMITMENT_OFFSET = 6
const.ACCOUNT_GET_ITEM_OFFSET=7
const.ACCOUNT_SET_ITEM_OFFSET=8
const.ACCOUNT_GET_MAP_ITEM_OFFSET=9
const.ACCOUNT_SET_MAP_ITEM_OFFSET=10

Comment on lines +27 to +34
const.ACCOUNT_VAULT_ADD_ASSET_OFFSET=10
const.ACCOUNT_VAULT_REMOVE_ASSET_OFFSET=11
const.ACCOUNT_VAULT_GET_BALANCE_OFFSET=12
const.ACCOUNT_VAULT_HAS_NON_FUNGIBLE_ASSET_OFFSET=13

## set commitment is missing, but corresponding memory procedure is used in the
## start_foreign_context kernel procedure
const.ACCOUNT_VAULT_GET_COMMITMENT_OFFSET=14
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the above comment, I'd probably change this to:

const.ACCOUNT_GET_VAULT_COMMITMENT_OFFSET=11
const.ACCOUNT_ADD_ASSET_OFFSET=12
const.ACCOUNT_REMOVE_ASSET_OFFSET=13
const.ACCOUNT_GET_BALANCE_OFFSET=14
const.ACCOUNT_HAS_NON_FUNGIBLE_ASSET_OFFSET=15

Comment on lines +38 to +43
const.FAUCET_BURN_ASSET_OFFSET=15
const.FAUCET_FUNGIBLE_GET_TOTAL_ISSUANCE_OFFSET=16

# add a new procedure to the kernel
# const.FAUCET_NON_FUNGIBLE_IS_ISSUED_OFFSET=17
const.FAUCET_MINT_ASSET_OFFSET=18
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the order to be:

const.FAUCET_MINT_ASSET_OFFSET=16
const.FAUCET_BURN_ASSET_OFFSET=17
const.FAUCET_GET_TOTAL_FUNGIBLE_ASSET_ISSUANCE_OFFSET=18
const.FAUCET_IS_NON_FUNGIBLE_ASSET_ISSUED_OFFSET=19

And let's add FAUCET_IS_NON_FUNGIBLE_ASSET_ISSUED_OFFSET (can stab it out for now and add the real implementation in a separate PR).

Comment on lines +47 to +52
# creation
const.NOTE_CREATION_OFFSET=19

# io commitments
const.NOTE_GET_INPUT_NOTES_COMMITMENT_OFFSET=20
const.NOTE_GET_OUTPUT_NOTES_COMMITMENT_OFFSET=21
Copy link
Contributor

Choose a reason for hiding this comment

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

I think of these as transaction-wide procedures - so, I'd rename them into:

const.TX_CREATE_NOTE_OFFSET=19

const.TX_GET_INPUT_NOTES_COMMITMENT_OFFSET=20
const.TX_GET_OUTPUT_NOTES_COMMITMENT_OFFSET=21

Comment on lines +55 to +56
const.NOTE_ADD_ASSET_OFFSET=22 # mutator
const.NOTE_GET_ASSETS_INFO_OFFSET=23 # accessor
Copy link
Contributor

Choose a reason for hiding this comment

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

I would swap these (don't we usually put accessor before mutator?)

Comment on lines +75 to +76
const.TX_UPDATE_EXPIRATION_BLOCK_NUM_OFFSET=32 # mutator
const.TX_GET_EXPIRATION_DELTA_OFFSET=33 # accessor
Copy link
Contributor

Choose a reason for hiding this comment

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

I would swap these (don't we usually put accessor before mutator?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants