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

Rethink current naming convention in contracts #386

Open
1 task done
bidzyyys opened this issue Oct 30, 2024 · 6 comments · May be fixed by #500
Open
1 task done

Rethink current naming convention in contracts #386

bidzyyys opened this issue Oct 30, 2024 · 6 comments · May be fixed by #500
Assignees
Labels
needs triage Needs to be assigned the appropriate labels priority: 0 Nice-to-have. Willing to ship without this. type: ref A code update that doesn't meaningfully change functionality.

Comments

@bidzyyys
Copy link
Collaborator

bidzyyys commented Oct 30, 2024

What is the feature you would like to see?

Right now we use in some cases Solidity like naming convention (_do_sth()). It is not recommended attitude in Rust and causes cargo clippy issues (right now disabled). We should rethink this topic and make sure our library is consistent in terms of naming.

Related #491
Related #499

TODO: Remove _ prefix from state variables (for functions see #386 (comment))

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines
@bidzyyys bidzyyys added priority: 0 Nice-to-have. Willing to ship without this. needs triage Needs to be assigned the appropriate labels type: ref A code update that doesn't meaningfully change functionality. labels Oct 30, 2024
@qalisander qalisander mentioned this issue Oct 31, 2024
2 tasks
@bidzyyys
Copy link
Collaborator Author

The same applies for struct attributes, like _pub _owner: StorageAddress,

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 13, 2025

Do we include test name standardization in this issue as well?

  • function_name_description for standard tests.
  • function_name_revert_when_condition for tests expecting a revert

@bidzyyys
Copy link
Collaborator Author

Do we include test name standardization in this issue as well?

  • function_name_description for standard tests.
  • function_name_revert_when_condition for tests expecting a revert

@0xNeshi not in this issue, for test name standardisation I suggest having another one. Easier to solve & merge.

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 13, 2025

Makes sense, created a separate issue #491

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 16, 2025

@bidzyyys @qalisander

In Solidity, internal functions use the _fn_name prefix pattern, making their visibility immediately clear. In our Rust contracts, we have a few scenarios to consider:

  1. When a public function has an internal implementation (like transfer_ownership), we can use the fn_name_internal suffix pattern. This maintains clarity while following Rust conventions. We could also use do_fn_name. Which one do you like more (I prefer the latter, as it's shorter?

  2. However, we also have internal functions meant for inheritance that don't have public counterparts (like _mint in ERC20). Simply removing the underscore could be misleading - developers might assume these functions are public since they don't follow the _internal/do_ pattern they see elsewhere in the codebase. Having a function like mint could also pose problems with inheritance, as the name might conflict with the public function version's name 🤔
    On the other hand, we don't want all internal functions to follow this convention - those functions that act like modifiers in Solidity (e.g. Ownable::only_owner) should be probably be left as-is, as they're clear enough (I like neither do_only_owner nor only_owner_internal).

Here's my proposal: Let's consistently use the same pattern for all internal functions that are likely to have a public counterpart. This approach would:

  • Maintain clear visibility indicators like Solidity
  • Follow Rust naming conventions
  • Provide consistent patterns across our codebase
  • Help developers quickly understand function visibility

What do you think about adopting this consistent pattern? It seems like a good balance between maintaining familiarity for Solidity developers while embracing Rust's conventions.

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 16, 2025

We agreed to keep _ prefix for internal functions, until the moment stylus sdk with advanced inheritance is released.
Then we will reconsider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Needs to be assigned the appropriate labels priority: 0 Nice-to-have. Willing to ship without this. type: ref A code update that doesn't meaningfully change functionality.
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants