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

SRC-2; Inline Documentation Standard #5

Closed
bitzoic opened this issue May 10, 2023 · 7 comments · Fixed by #11
Closed

SRC-2; Inline Documentation Standard #5

bitzoic opened this issue May 10, 2023 · 7 comments · Fixed by #11
Assignees
Labels
SRC 2 Label used to filter for the standard issue

Comments

@bitzoic
Copy link
Member

bitzoic commented May 10, 2023

Abstract

The following standard intends to define the structure and organization of inline documentation for functions within the Sway Language. This will be a living standard.

Motivation

The standard seeks to provide a better developer experience using Fuel's tooling.

Prior Art

A number of pre-existing functions in the sway standard library, sway-applications, and sway-libs repositories have inline documentation. The inline documentation for these is already compatible with Fuel's VS Code extension. These however do not all follow the same structure and outline.

Specification

Functions

The following describes the structure and order of inline documentation for functions. Some sections may not apply to each function. When a section is not relevant it shall be omitted.

- Description

This section has no header.
A simple explanation of the function's intent or functionality.
Example:

/// This function computes the hash of two numbers.

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the function's intent or functionality.
Example:

/// ### Additional Information
///
/// This function also has some complex behaviors.

- Arguments

This section has a h3 header.
Lists the arguments of the function's definition with the * symbol and describes each one. The list shall provide the name, type, and description. The argument shall be encapsulated between two backticks: argument. The type shall be encapsulated between two square brackets: [type].
Example:

/// ### Arguments
///
/// * `argument_1`: [Identity] - This argument is a user to be hashed.

- Returns

This section has a h3 header.
Lists the return values of the function with the * symbol and describes each one. This list shall be in the order of return. index and provide the type and description. The type shall be encapsulated between two square brackets: [type].
Example:

/// ### Returns
///
/// * [u64] - The number of hashes performed.

- Reverts

This section has a h3 header.
Lists the cases in which the function will revert starting with the * symbol. The list SHALL be in the order of occurrence within the function.
Example:

/// ### Reverts
///
/// * When `argument_1` or `argument_2` are a zero [b256].

- Number of Storage Accesses

This section has a h3 header.
Provides information on how many storage reads, writes, and clears occur within the function.
Example:

/// ### Number of Storage Accesses
///
/// * Reads: `1`
/// * Clears: `2`

- Examples

This section has a h3 header.
This section provides an example of the use of the function. This section is not required to follow the SRC-2 standard however encouraged for auxiliary and library functions.
Example:

/// ### Examples
///
/// ```sway
/// fn foo(argument_1: b256, argument_2: b256) {
///     let result = my_function(argument_1, argument_2);
/// }

Structs

The following describes the structure and order of inline documentation for structs. Some sections may not apply to each struct. When a section is not relevant it shall be omitted.

- Description

This section has no header.
A simple explanation of the struct's purpose or functionality.
Example:

/// This struct contains information on an NFT.

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the struct's purpose or functionality.
Example:

/// ### Additional Information
///
/// This struct also has some complex behaviors.

- Fields

The following describes the structure and order of inline documentation for fields within structs. Some sections may not apply to each field. When a section is not relevant it shall be omitted.

Description

This section has no header.
Each field shall have its own description with a simple explanation of the field's purpose or functionality.
Example:

/// This field represents an owner.
field_1: Identity,

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the field's purpose or functionality.
Example:

/// ### Additional Information
///
/// This field also has some complex behaviors.

Enums

The following describes the structure and order of inline documentation for enums. Some sections may not apply to each enum. When a section is not relevant it shall be omitted.

- Description

This section has no header.
A simple explanation of the enum's purpose or functionality.
Example:

/// This enum holds the state of a contract.

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the enum's purpose or functionality.
Example:

/// ### Additional Information
///
/// This enum also has some complex behaviors.

- Variant

The following describes the structure and order of inline documentation for fields within enums. Some sections may not apply to each field. When a section is not relevant it shall be omitted.

Description

This section has no header.
Each variant shall have its own description with a simple explanation of the variant's purpose or functionality.
Example:

/// This variant represents the uninitialized state of a contract.
variant_1: (),
/// This variant represents the initialized state of a contract.
variant_2: Identity,

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the variant's purpose or functionality.
Example:

/// ### Additional Information
///
/// This variant also has some complex behaviors.

Errors

In Sway, errors are recommended to be enums. They shall follow the same structure and order for inline documentation as described above for enums. Some sections may not apply to each error. When a section is not relevant it shall be omitted.

Logs

In Sway, logs are recommended to be structs. They shall follow the same structure and order for inline documentation as described above for structs. Some sections may not apply to each log. When a section is not relevant it shall be omitted.

Storage

The following describes the structure and order of inline documentation for variables within the storage block. Some sections may not apply to each storage variable. When a section is not relevant it shall be omitted.

- Description

This section has no header.
A simple explanation of the storage variable's purpose or functionality.
Example:

/// This storage variable is used for state.

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the storage variable's purpose or functionality.
Example:

/// ### Additional Information
///
/// This storage variable maps a user to a state.

Configurable

The following describes the structure and order of inline documentation for variables in the configurable block. Some sections may not apply to each storage variable. When a section is not relevant it shall be omitted.

- Description

This section has no header.
A simple explanation of the configurable variable's purpose or functionality.
Example:

/// This configurable variable is used for an address.

- Additional Information

This section has a h3 header.
This section is directly below the description and can provide additional information beyond the configurable variable's purpose or functionality.
Example:

/// ### Additional Information
///
/// This configurable variable makes security assumptions.

Rationale

The SRC-2 standard should help provide developers with an easy way to both quickly write inline documentation and get up to speed on other developers' code. This standard in combination with Fuel's VS Code extension provides for readily accessible information on functions, structs, and enums

Screenshot 2023-05-10 125656

Backwards Compatibility

There are no standards that the SRC-2 standard requires to be backward compatible with.

Security Considerations

This standard will improve security by providing developers with relevant information such as revert cases.

Examples

/// Ensures that the sender is the owner.
///
/// ### Arguments
///
/// * `number`: [u64] - A value that is checked to be 5.
///
/// ### Returns
///
/// * [bool] - Determines whether `number` is or is not 5.
///
/// ### Reverts
///
/// * When the sender is not the owner.
///
/// ### Number of Storage Accesses
///
/// * Reads: `1`
///
/// ### Examples
///
/// ```sway
/// use ownable::Ownership;
///
/// storage {
///     owner: Ownership = Ownership::initalized(Identity::Address(Address::from(ZERO_B256))),
/// }
///
/// fn foo() {
///     storage.owner.only_owner();
///     // Do stuff here
/// }
#[storage(read)]
pub fn only_owner(self, number: u64) -> bool {
    require(self.owner() == State::Initialized(msg_sender().unwrap()), AccessError::NotOwner);
    number == 5
}
/// Metadata that is tied to a token.
pub struct NFTMetadata {
    /// Represents the token ID of this NFT.
    value: u64,
}
/// Determines the state of ownership.
pub enum State {
    /// The ownership has not been set.
    Uninitialized: (),
    /// The user which has been given ownership.
    Initialized: Identity,
    /// The ownership has been given up and can never be set again.
    Revoked: (),
}
/// Error log for when access is denied.
pub enum AccessError {
    /// Emitted when the caller is not the owner of the contract.
    NotOwner: (),
}
/// Log of a bid.
pub struct Bid {
    /// The number of tokens that were bid.
    amount: u64,
    /// The user which placed this bid.
    bidder: Identity,
}
storage {
    /// The contract of the tokens which is to be distributed.
    asset: Option<ContractId> = Option::None,
    /// Stores the ClaimState of users that have interacted with the Airdrop Distrubutor contract.
    ///
    /// ### Additional Information
    ///
    /// Maps (user => claim)
    claims: StorageMap<Identity, ClaimState> = StorageMap {},
}
configurable {
    /// The threshold required for activation.
    THRESHOLD: u64 = 5,
}
@bitzoic bitzoic added Draft This standard is currently in draft. Experimentation and feedback is encouraged. SRC 2 Label used to filter for the standard issue labels May 10, 2023
@K1-R1
Copy link
Member

K1-R1 commented May 15, 2023

Looks great!
Do you think we need a specification for storage and configurable blocks too?
Will this standard have any opinion on whether, for a contract, the function docs should be in the interface or implementation?

@K1-R1
Copy link
Member

K1-R1 commented May 15, 2023

Might also be worth adding logs and returns sections to the function docs.
Also worth considering that contract interfaces require function docs as they are imported, but contract interfaces can't guarantee reverts and storage accesses, which would perhaps be better placed in contract implementations.

@sdankel
Copy link
Member

sdankel commented May 18, 2023

This is awesome, thanks for formalizing this. We can make the LSP support generating doc comments via code action per this standard.

Looks great! Do you think we need a specification for storage and configurable blocks too? Will this standard have any opinion on whether, for a contract, the function docs should be in the interface or implementation?

Agreed it'd be good to call this out explicitly in the Standard. IMO, we should prefer adding doc comments to interfaces, and add doc comments to implementations only when they differ or have something interesting to note. Thoughts?

@bitzoic
Copy link
Member Author

bitzoic commented Jun 2, 2023

Might also be worth adding logs and returns sections to the function docs. Also worth considering that contract interfaces require function docs as they are imported, but contract interfaces can't guarantee reverts and storage accesses, which would perhaps be better placed in contract implementations.

I've gone ahead and added the following inline docs standards:

  • Logs
  • Return Values for functions
  • Errors
  • Storage block variables
  • Configurable block variables

@bitzoic bitzoic mentioned this issue Jun 7, 2023
sdankel added a commit to FuelLabs/sway that referenced this issue Jun 12, 2023
## Description

Closes #4571

Following the directory structure for code actions that was there
before, I've added new modules for:
- function decls
- enum variants
- storage fields
- configurable constants

Following FuelLabs/sway-standards#5, the doc
comment template is the same for all of these except for functions. For
functions, the template includes the argument names & types, return
value, and a generated example of usage which is just meant to be a
starting point for the comment author.

For the basic template that most of these use, I created
`BasicDocCommentCodeAction`. I also refactored some of the "generate
impl" code actions that were there before into a trait called
`GenerateImplCodeAction` to keep their common functionality separate
from the "generate doc comment" code actions.

I updated the existing code action tests for enums & structs, and added
a new one for functions. I didn't think it was necessary to test enum
variants, storage fields, etc. since the template and underlying code is
the same.

### Example generating docs for a function

![Jun-07-2023
17-01-17](https://github.com/FuelLabs/sway/assets/47993817/b645afd9-a474-450d-bb12-e69ed2ccaacf)

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
@bitzoic bitzoic linked a pull request Jun 13, 2023 that will close this issue
@bitzoic bitzoic self-assigned this Jun 13, 2023
@Braqzen
Copy link
Contributor

Braqzen commented Jun 13, 2023

Function

For the description and additional info I would consider elaborating to state that they outline what the function does instead of how the function may do it. Sometimes developers mix the two. The function should not outline how it does something as that content should be encapsulated within the implementation. As an interface to functionality the user should be informed of what happens.

For the returned arguments I'll pose a proposition. The description states that the arguments should be listed in order but perhaps we should also number the arguments to explicitly state which argument (positionally) has each type and what it is.

/// ### Returns
///
/// * 1st: [u64] - The number of hashes performed.
/// * 2nd: [u64] - The number of ... .

Thoughts?

For reverting I think we should also state that we will only list the reverts from the conditions in that function rather than additional reverts that may occur if we call another function.
Potentially also mention that the reverts should be listed in the order that they may occur from the top of the function to the bottom?

Misc

Note: the h3 examples sections does not close the code block for the function in the Function and the Examples sections.

I recommend moving the code blocks from the bottom of the issue into their relevant sections so that the read can instantly see the example rather than think there isn't one until they scroll to the bottom of the issue.

There's also duplicated content regarding sections not being applicable and thus being omitted. I would remove that wording where it's not relevant and only state what applies - and only present an example with that content. For example, at a car dealership you do not expect the salesperson to tell you about flowers because that's irrelevant information. In the same way, if the content is omitted then it's irrelevant and only what is presented should be considered.

I would also consider the documentation of private structures / modules / functions etc. Should they be documented in the code and should that code then be interactable via the extension?

@bitzoic
Copy link
Member Author

bitzoic commented Jun 13, 2023

Function

For the description and additional info I would consider elaborating to state that they outline what the function does instead of how the function may do it. Sometimes developers mix the two. The function should not outline how it does something as that content should be encapsulated within the implementation. As an interface to functionality the user should be informed of what happens.

The description of a function is an explanation of the function's intent or functionality. which covers the what and additional information explains beyond the function's intent or functionality. which covers the how.

For the returned arguments I'll pose a proposition. The description states that the arguments should be listed in order but perhaps we should also number the arguments to explicitly state which argument (positionally) has each type and what it is.

/// ### Returns
///
/// * 1st: [u64] - The number of hashes performed.
/// * 2nd: [u64] - The number of ... .

Thoughts?

I've updated the wording on how it should be ordered but I'm not sure how I feel about numbering within the docs themselves. I feel that this would add an additional barrier/take more time to document and devs simply won't do it. We need to strike a balance between ease of implementation and thorough documentation in order for the standard to become widely adopted.

For reverting I think we should also state that we will only list the reverts from the conditions in that function rather than additional reverts that may occur if we call another function. Potentially also mention that the reverts should be listed in the order that they may occur from the top of the function to the bottom?

I've added the requirement of order but I disagree with disallowing reverts that may occur in another function. This would mean I could not document a function that reverts because I've implemented SRC-5 and called only_owner().

There's also duplicated content regarding sections not being applicable and thus being omitted. I would remove that wording where it's not relevant and only state what applies - and only present an example with that content. For example, at a car dealership you do not expect the salesperson to tell you about flowers because that's irrelevant information. In the same way, if the content is omitted then it's irrelevant and only what is presented should be considered.

These are deliberately verbose and on each section. It is required so that sections like "Additional Information" can be excluded if need be.

I would also consider the documentation of private structures / modules / functions etc. Should they be documented in the code and should that code then be interactable via the extension?

These fall under existing categories already.

@bitzoic bitzoic removed the Draft This standard is currently in draft. Experimentation and feedback is encouraged. label Jun 13, 2023
@MaksymOrshovskyi
Copy link

gj guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SRC 2 Label used to filter for the standard issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants