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

Standardise conventions for comments and naming in Kernel #858

Closed
phklive opened this issue Sep 7, 2024 · 5 comments
Closed

Standardise conventions for comments and naming in Kernel #858

phklive opened this issue Sep 7, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@phklive
Copy link
Contributor

phklive commented Sep 7, 2024

Feature description

Throughout the kernel different conventions for comments and naming have been used, e.g. in account.masm:

#! Panics:
#! - slot index is out of bounds
#! - computed index is out of bounds

#! Gets an item from the account storage. Panics if the index is out of bounds.

#! Panics if
#! - index is out of bounds

#! Sets an item in the account storage. Panics if
#! - the index is out of bounds
#! - the slot type is not value

This is an isolated example with panics but it can be seen in documentation comments, naming of return values, etc...

Why is this feature needed?

We need to make sure that the code is clean, readable and understandable.

Furthermore setting the right conventions early will enable the project to scale easier.

@phklive phklive added the enhancement New feature or request label Sep 7, 2024
@bobbinth
Copy link
Contributor

bobbinth commented Sep 7, 2024

Agreed!

In terms of naming, a few things that I noticed:

  • get_output_notes_hash should probably be renamed into get_output_notes_commitment.
  • We should probably use the same convention for all methods in the kernel. For example, we have set_account_item but `account_vault_add_asset. I think it should be either:
    • set_account_item and add_account_asset, or
    • account_set_item and account_add_asset.

@bobbinth
Copy link
Contributor

@Fumuran - this is largely done already, right? Or is there something remaining?

@Fumuran
Copy link
Contributor

Fumuran commented Jan 21, 2025

The ABI PR mostly addressed the format of the comments and procedures, but I think there are still some ambiguity in naming, I'm not sure that we updated the usage of hash/commitment terms.

@bobbinth
Copy link
Contributor

@Fumuran - could you create a more targeted issues for naming? Then we can close this one. Re hash vs. commitment - I think we should try to move away from hash toward commitment.

@Fumuran
Copy link
Contributor

Fumuran commented Jan 21, 2025

Created an issue: #1092

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

No branches or pull requests

3 participants