Function naming convention

[martriay]

In the light of recent discussions on #244 and #240, a spinoff discussion of #34 requires to be reopen. I'll attempt to clarify existing naming rules for the library and then point out open questions around them.

Current status

Given four kind of variables in library modules:

• A: internal to a library, not meant to be used outside the module
• B: part of the public API of a library
• C: subset of B where the function is meant to be exported as-is by contracts
• D: an even smaller subset of C that aims to comply with EIP standards
• S: storage variables

Today:

• A functions should be prefixed with an underscore (e.g. _foo), and never exposed/suggested/documented as API.
• Anything that’s meant to be used outside a library (aka the library's API), should be prefixed with the library name (e.g. ERC20_approve) to avoid clashes with other modules
• S variables must always have a prefix, since collisions are possible even without being exported.
• we distinguish C from D depending on whether they use camelCase or not (everything but D is snake_case (*)

(*) this brings ambiguity for single-word names like ERC20_approve, where the current -and weak- assumption is to consider it camelCase/part of group D.

Questions:

1. how should we name B/"internal" functions? (in the Solidity sense, where they're part of the module's API)
2. how can we distinguish between B, C, and D?
3. should libraries expose storage variables as-is or encapsulate them with getters/setters?

My own attempt to answer:

1. e.g. ERC20_unguarded_approve for Solidity's _approve, and so on
2. I think we should accept departing from Solidity and use more straightforward names, while maintaining feature parity. This is, let's try to export the same building blocks but with different names, since users cannot rely on overloading nor visibility modifiers to distinguish between functions like Solidity's approve and _approve.
   I would even drop the camelCase in favor of more explicit names to distinguish between C and D because it's too soft and may lead to confusions. Maybe we should leave camelCase for contracts only, never for libraries. Users should study the library API to understand what functions should it expose as-is.
3. No strong opinions, I can see it as overkill sometimes but I'm fine with either.

[Amxx]

Distinguishing between C and D, using only the ERC as a reference point, is IMO not the right approach.

• Some contract, like OZ's Ownable have a bit of both. owner() and transferOwnership(address) are part of ERC173, but renounceOwnership() is not. In The Cairo implementation, should the first two be camelCase and the last one be snake_case? I think this would add a lot of confusion.

• Some interfaces are turned into ERC after the fact. I'm thinking of ERC20Permit (ERC2612). In that case, we would change the case when the ERC is submitted? When the ERC is final?

• Some interfaces are not (proper) ERC but are commonly accepted. I'm thinking of "ERC" 666 or OZ's access control. Tooling is built on top of these interfaces, and I believe using the same naming conventions would help these tools a lot.

IMO distinguishing C & D is to sensitive, and both should be camelCase.

---

B contains both "internal" functions and "modifiers" replacement. IMO these have two very different concerns. To avoid confusion, I would go for something like

• ERC20_internal_approve
• ERC20_guard_only_owner

---

IMO storage variables should never be exposed. If a user wants to interact with them, they should use B functions that bundle the state changes with events:

• ERC20_internal_burn
• ERC20_internal_mint
• ERC20_internal_transfer

[milancermak]

Have you guys tried using namespaces? I've noticed the discussion in #244 mentions:

If only the language allowed import openzeppelin.token.erc20.library as ERC20

Well, this works for me (cairo 0.8.1), at least in local testing:

# tkn.cairo
%lang starknet

from starkware.cairo.common.cairo_builtins import HashBuiltin

from contracts.token.library import ERC20

@view
func getBalance{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
    owner : felt
) -> (balance : felt):
    let (balance) = ERC20.getBalance(owner)
    return (balance)
end

@external
func setBalance{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
    owner : felt, balance : felt
):
    ERC20.setBalance(owner, balance)
    return ()
end

# library.cairo
%lang starknet

from starkware.cairo.common.cairo_builtins import HashBuiltin

@storage_var
func balanceOf(address : felt) -> (balance : felt):
end

namespace ERC20:
    func getBalance{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        addr : felt
    ) -> (balance : felt):
        let (balance) = balanceOf.read(addr)
        return (balance)
    end

    func setBalance{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        addr : felt, balance : felt
    ):
        balanceOf.write(addr, balance)
        return ()
    end
end

I didn't give it much deeper though, especially with regards to the discussion above (I'm sorry, too much work), so I'm not quite sure if it's helpful at all. Only wanted to bring it up just in case it's of any use.

[koloz193]

i tested this out further and functions (including storage vars) not included in the name space can be defined in the importing contract without a collision which is great

[Amxx]

from contracts.token.library import ERC20

This requires a change in the file hierarchy, right?

I'm trying from openzeppelin.token.erc20 import library as ERC20
But it gives me openzeppelin/token/erc20/ERC20.cairo:9:6: Could not find module 'openzeppelin.token.erc20'. Searched in the following paths:

[milancermak]

Not a change in the hierarchy but in the internal structure. I don't fully know what makes sense in this repo, but maybe something like still having a file token/erc20/library.cairo in which you'd declare namespace ERC20 and have all the internal functions inside renamed, e.g. ERC20_initializer would become initializer accessible as ERC20.initializer when using from openzeppelin.token.erc20.library import ERC20 in the presets.

from openzeppelin.token.erc20 import library as ERC20 is looking for a namespace library in erc20.cairo file.

[Amxx]

❤️

clearly, if its possible to import a file under a symbol, I'd much rather not have any conflict-preventing prefix in the function names, and use that instead.

[koloz193]

I like what @Amxx has proposed and i think that we can make it more generic where we have Module_type_name so ERC20_storage_allowances, Ownable_guard_only_owner, ERC20_internal_set_approval, etc. Using namespace would be a great option too but I don't know too much about how they work/if name collisions could still occur.

+1 to storage variables being encapsulated by getters/setters with events

other than that there's really no strong reason IMO to have camel case. it's not like these functions are being called directly from ethereum contracts. As long as the cairo ecosystem knows that camel case is transformed into snake case i dont see any reason to keep it.

the only other naming stuff (not that it has an impact really) are function parameters and return names. seeing as they're not enforced AFAIK i think we should adopt a snake case standard

[Amxx]

other than that there's really no strong reason IMO to have camel case. it's not like these functions are being called directly from ethereum contracts. As long as the cairo ecosystem knows that camel case is transformed into snake case i dont see any reason to keep it.

It's not just about other contracts calling these functions its also about external tooling (etherscan, thegraph, ...) being able to call view functions in addition to listening to events.

[koloz193]

good point, i do like the consistency argument but there's already going to be the need for changes to these though since stuff like Uint256 are a struct with 2 fields unlike in evm land

[andrew-fleming]

To preface, much of the discussion regarding function naming transpired before separating libraries from contracts (the Extensibility Pattern). Since methods need to be exposed in contracts, I sort of see the lib methods as internal methods of the contract. So here's the question: should the libraries themselves be fully compliant with the (naming) standards set in ERCs even though they must be exposed?

• If yes, with @milancermak's recommendation of namespace, I believe we should blanket all methods with camelCase to avoid confusing exceptions
• If no, I believe we should we should blanket all methods with snake_case and +1 to @martriay's "Maybe we should leave camelCase for contracts only, never for libraries. Users should study the library API to understand what functions should it expose as-is."

With that said, here are my answers:

1. The B methods can be snake_cased and prefixed with an underscore i.e. _set_token_uri and within a contract's exposed method using namespace: ERC721._set_token_uri
2. In agreement with @Amxx's point, there are many gray areas regarding when an ERC is an ERC, convention, and the like which makes differentiating between C and D a little murky and perhaps unnecessary. I vote for no difference between C and D.
   • B : _set_token_uri — ERC721._set_token_uri
   • C : renounce_ownership — Ownable.renounce_ownership
   • D : transfer_ownership — Ownable.transfer_ownership
     So long as a contract's exposed methods (external and view) are in camelCase, contracts will be ERC compliant (in the naming respect). Further, I believe this ameliorates concerns brought up with tooling.

Since Cairo uses snake_case by convention, IMO it makes sense to treat camelCasing as the exception and to generally favor snake_case. The more the casings are mixed, the more it turns into sna_melCase.

3. It probably is safer to use getters and setters for storage var interactions, but that also creates more code that may be unnecessary. If there's a strong argument either way, I'd like to hear it.

[martriay]

Agree with most of this. I still believe it's a bit confusing but much less than before.