Define style guidelines

[martriay]

We need to update, settle, and write down our coding conventions around naming, in-code documentation, parameter order, etc. for variables, storage structs, events, impls, traits, ABIs, and presets.

This includes Trait/ABI distinction, casing, internal/external/private functions (if still applies) and all related patterns.

Tasks

Preview Give feedback

1. Define project structure / directory guidelines
2. Define preset, trait, library module, events, and error naming guidelines
3. Define variable naming guidelines
4. Define library API guidelines (usage of underscore _upgrade, internal/external/private if it makes sense)
5. Define comments and in-code docs guidelines
6. Define casing guidelines
7. Define imports guidelines
8. Define testing guidelines (best practices)
9. Define ABI/Trait distinction guidelines
10. Update CONTRIBUTING.md

[martriay]

traits:

• we distinguish traits between "regular traits" and ABI traits, the latter we suffix with ABI as in trait ERC20ABI

imports:

• we sort imports alphabetically
• do we group imports or not?

testing:

• we should come up with an entire list/best practices
• we define a test for every revert in the codebase
• we test edge cases
• we test non-happy paths as much as possible
• we test interfaces and behavior, not implementation details (external methods and API of our library, but not utils/internals)

casing:

• see great interface migration

comments / in-code docs:

• being discussed

[andrew-fleming]

Here's a rough contract mod layout guide.

---

A. Outer contract elements

1. SPDX
2. OpenZeppelin Contracts for Cairo v0.0.0 (path/to/file.cairo)
3. Import statements for elements outside of the contract
4. Constant variables
5. Structs
6. Traits
   a. Note that actual interfaces should be defined in an interface.cairo module in the same directory as the contract mod

B. Inner contract elements

1. Import statements
2. Storage struct
3. Define events with:
   a. Event enum
   b. Event structs (which are the actual events)
4. Constructor
5. External implementation of interface
   a. The implementation itself removes the preceding i and adds the suffix Impl
   1. e.g. IERC20 is implemented as ERC20Impl of IERC20
6. Non-standard external functions e.g. functions that are not part of an external interface implementation
   a. e.g. increase_allowance in ERC20
7. Internal implementation
   a. The impl name should be InternalImpl e.g. InternalImpl of InternalTrait
   b. The first method should be initializer if it exists
8. Pure methods (outside of InternalImpl)
   a. These include the #[internal] macro for readability

[ericnordelo]

Regarding scoping:

• The [external] annotation is used to make a method a public entry point as defined in the protocol.
• We use the #[internal] annotation for functions that are not [external], but can be imported in Cairo for constructing other dependent modules.
• Every method inside the InternalImpl is internal.
• While there's no private scoping in Cairo, we annotate certain functions with #[private] to signal that they are not meant to be used outside the module. Note that the compiler does not enforce the restriction.

[ericnordelo]

Proposed rules for grouping imports:

• Group only children (avoid use mod_a::{mod_b::mod_c, mod_d})
• Group by logic units (related) avoiding more than three lines per group. Ex:

Instead of this:

use openzeppelin::account::{
    AccountCamelABIDispatcherTrait, AccountCamelABIDispatcher, AccountABIDispatcherTrait,
    AccountABIDispatcher
};

Use this:

use openzeppelin::account::{AccountABIDispatcherTrait, AccountABIDispatcher};
use openzeppelin::account::{AccountCamelABIDispatcherTrait, AccountCamelABIDispatcher};

Three lines is allowed:

use openzeppelin::tests::mocks::account_mocks::{
    CamelAccountPanicMock, CamelAccountMock, SnakeAccountMock, SnakeAccountPanicMock
};

NOTE: Sometimes could make sense to allow more than three lines, this is just a generic rule.

[martriay]

Traits representing external interfaces should begin with I, such as IAccount or IDeployer.

[andrew-fleming]

Contracts should order component impls, storage, and events in the same order that they're imported

#[starknet::contract]
mod MyContract {
    use openzeppelin::introspection::src5::SRC5Component;
    use openzeppelin::token::erc721::ERC721Component;
    use starknet::ContractAddress;

    component!(path: SRC5Component, storage: src5, event: SRC5Event);
    component!(path: ERC721Component, storage: erc721, event: ERC721Event);

    // SRC5
    #[abi(embed_v0)]
    impl SRC5Impl = SRC5Component::SRC5Impl<ContractState>;

    // ERC721
    #[abi(embed_v0)]
    impl ERC721Impl = ERC721Component::ERC721Impl<ContractState>;
    #[abi(embed_v0)]
    impl ERC721MetadataImpl = ERC721Component::ERC721MetadataImpl<ContractState>;
    #[abi(embed_v0)]
    impl ERC721CamelOnly = ERC721Component::ERC721CamelOnlyImpl<ContractState>;
    #[abi(embed_v0)]
    impl ERC721MetadataCamelOnly =
        ERC721Component::ERC721MetadataCamelOnlyImpl<ContractState>;
    impl ERC721InternalImpl = ERC721Component::InternalImpl<ContractState>;

    #[storage]
    struct Storage {
        #[substorage(v0)]
        src5: SRC5Component::Storage
        #[substorage(v0)]
        erc721: ERC721Component::Storage,
    }

    #[event]
    #[derive(Drop, starknet::Event)]
    enum Event {
        #[flat]
        SRC5Event: SRC5Component::Event
        #[flat]
        ERC721Event: ERC721Component::Event,
    }

    (...)

}

[ericnordelo]

Rules for test assertions:

• Use assertion macros instead of inline functions (assert! and assert_eq!). These allow us to pass ByteArrays instead of short strings.
• Use assert_eq! over assert! when applicable.
• Avoid adding messages to assertions as much as possible to avoid redundancy. There are cases where messages are helpful, but sometimes they are redundant. for example, assert!(big_point_1 != big_point_2); shouldn't need a message while assert_eq!(parity, true, "Parity should be odd"); add some clarity.
• When the message is helpful, I'm following these short rules by basically using "X should be Y" or "X should have Y" or just "Should be/have Y", etc...

[martriay]

• Avoid adding messages to assertions as much as possible to avoid redundancy.

Is this really wanted? Won't we lose the error message when the test fails? Because that's crucial to quickly spot the failing line, even if there's some redundancy in error strings (possible error lines get very reduced when it's test name + error string)

[ericnordelo]

We don't need the message to spot the line, by default we have test name + the assertion failed, and I think that's enough (even more if the assertion naming is explicit). For example:

From the test:

#[test]
fn test_dual_supportsInterface() {
    let (dispatcher, _) = setup_camel();
    let doesnt_supports_iaccesscontrol = !dispatcher.supports_interface(IACCESSCONTROL_ID);
    assert!(doesnt_support_iaccesscontrol);
}

$ scarb test -f test_dual_supportsInterface

failures:
   openzeppelin::tests::access::test_dual_accesscontrol::test_dual_supportsInterface - 
   Panicked with "assertion failed: `doesnt_support_iaccesscontrol`.".

Sometimes the message provides context and is helpful, but I don't think redundancy is giving us any real advantage.

[martriay]

In tests, we should perform checks/asserts right after state changes/calls/queries, instead of performing several of them an asserting later. For example:

let bar = contract.bar(args..);
assert!(bar);

let foo = contract.foo(args...);
assert!(foo);

let baz = contract.baz(args..);
assert!(baz);

instead of:

let bar = contract.bar(args..);
let foo = contract.foo(args...);
let baz = contract.baz(args..);

assert!(bar);
assert!(foo);
assert!(baz);

See discussion here and here.

[andrew-fleming]

Closing this issue as this is done internally.