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

docs: store spec template/guideline #14020

Merged
merged 9 commits into from
Dec 5, 2022
Merged

Conversation

angbrav
Copy link
Contributor

@angbrav angbrav commented Nov 25, 2022

Description

Contributes to: #12986

Proposes a spec template/guideline for store-related specification documents. There is overlapping between the format section of the document and the documentation writing guidelines. We could simply remove the former.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the documentation writing guidelines
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@angbrav angbrav requested a review from a team as a code owner November 25, 2022 11:58
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left 2 questions but overall looks good to me

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @angbrav for this work, I've added some suggestions, please take a look.Thank you Marko and Julien for the co-reviews!

store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
}
```

Pseudocode for algorithms should be written in simple Typescript, as functions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is going to be quite hard to translate

store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
@angbrav
Copy link
Contributor Author

angbrav commented Nov 28, 2022

Thanks all for the comments!


### System model and properties

This section should include an assumptions sub-section if any, and the mandatory properties sub-section. Note that both sub-sections are tightly coupled: how to enforce a property will depend directly on the assumptions made.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the important thing to capture here is the "rest of the world", that is,

  1. Which modules are used by the module under specification, and what do we expect from them
  2. If possible, which module use the module under specification and how.

For instance, let's assume module A uses module B. There might be the implicit assumption that module B panics in certain situations, and this is fine, as we want A to panic in this situations as well. This is an assumption we need to make explicit in the specification.

With this information, we obtain:
a. We understand what A is doing as a result of the behavior of B
b. If we ever go to redesign B, we need to understand that other modules (e.g. A) rely on maintaining this specific behavior (e.g., we still need to panic in this situation).

I guess Point 1. would go into the assumption paragraph and Point 2 in the property paragraph.

@angbrav
Copy link
Contributor Author

angbrav commented Nov 29, 2022

I have done a pass to generalize it and include @josef-widder comment. The only pending comment is whether to use agnostic-language pseudocode or simply go. Also, we should decide where to place this document (does not make sense to have it within the store folder).

store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
store/SPEC_STANDARD.md Outdated Show resolved Hide resolved
@@ -0,0 +1,116 @@
## What is a SDK standard?

A SDK standard is a design document describing a particular protocol, standard, or feature expected to be used by the Cosmos SDK. A SDK standard should list the desired properties of the standard, explain the design rationale, and provide a concise but comprehensive technical specification. The primary author is responsible for pushing the proposal through the standardization process, soliciting input and support from the community, and communicating with relevant stakeholders to ensure (social) consensus.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to me to be too broad. Do you mean what is an SDK Storage or State standard?

Copy link
Contributor Author

@angbrav angbrav Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by State standard?

This seems to me to be too broad.

This is intended. The first version was specific for store features, but reviewers requested to generalize it. I thought there was consensus, but we can discuss it and figure out what's best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why is it under the store package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to move it. Since the original was for store-related features, I put it here. After generalizing it, I am not sure where to put it so I haven't moved it. See #14020 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not a specification standard template specifically for storage/state, then it doesn't belong in store/. The docs/spec directory would be a perfectly adequate place to put it :)

@julienrbrt
Copy link
Member

julienrbrt commented Nov 30, 2022

I have done a pass to generalize it and include @josef-widder comment. The only pending comment is whether to use agnostic-language pseudocode or simply go. Also, we should decide where to place this document (does not make sense to have it within the store folder).

The document should be under docs/spec (https://github.com/cosmos/cosmos-sdk/tree/main/docs/spec). This way it will be rendered on docs.cosmos.network as well.

tac0turtle and others added 4 commits December 5, 2022 14:00
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

- history log, and
- copyright notice.

All top-level sections are required. References should be included inline as links, or tabulated at the bottom of the section if necessary. Included sub-sections should be listed in the order specified below.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A table a content is as well handy.

@tac0turtle tac0turtle enabled auto-merge (squash) December 5, 2022 22:38
@tac0turtle tac0turtle merged commit a31c4ac into main Dec 5, 2022
@tac0turtle tac0turtle deleted the manuel/store-spec-standard branch December 5, 2022 22:39
@angbrav angbrav mentioned this pull request Jan 6, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants