Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

chore: improve topos-tce-storage documentation #353

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

Freyskeyd
Copy link
Member

Description

This PR is adding documentation (crate level) for topos-tce-storage.
More to come soon.

Preview of the doc on README.md

PR Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@Freyskeyd Freyskeyd added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 30, 2023
@Freyskeyd Freyskeyd requested a review from a team as a code owner October 30, 2023 09:23
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6a7bc6) 61.35% compared to head (27a675b) 62.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   61.35%   62.64%   +1.29%     
==========================================
  Files         219      218       -1     
  Lines       12109    11874     -235     
==========================================
+ Hits         7429     7438       +9     
+ Misses       4680     4436     -244     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Freyskeyd Freyskeyd force-pushed the chore/improve-doc-tce-storage branch 3 times, most recently from ab38c77 to 4685219 Compare October 30, 2023 10:21
Copy link
Contributor

@gruberb gruberb left a comment

Choose a reason for hiding this comment

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

A few comments:

  • Discussing different options as a database layer is I think not needed here. It's not a discussion but a "matter of fact". Mentioning RocksDB is good, but comparing it doesn't make sense here.
  • "Each store represents a different kind of capabilities": I think it's clear that they serve different purposes, or they wouldn't exist. I would still give a high level overview which store is responsbile for what exactly, and why we split them in these different types of stores.
  • No reason to mention how a new store is instantiated. It's not a library. And the reader will probably never instantiate a new store. If they do, it's easy to point that out in the store documentation itself.
  • Never mention "fairly simple" in a documentation :)
  • I am also not so sure that we have to mention Arc and async-trait.

What I would like to see is something along these lines:

"The topos-tce-storage is serves as the storage layer solely for the tce, and is split into:

  • ValidatorStore
  • FullNodeStore
  • ValidatorPerEpochStore
  • EpochValidatorsStore
  • IndexStore

The relationship between them is as follows (adding the diagram here).

The responsibilties for this storage layer are:

  • Storing and retrieving certificates
  • Managing pending certificates
  • Managing the certificate status
  • Metadata around the protocol and the state of the TCE

The certificates are managed in a pool (explaining here how exactly we think about this and what a pool is, when a certificate is pending and why, the different possible states of a certificate).

Also mentioning here: What is the metadata for us, and why do we store it?

After this is cleared, give a brief overview of each store and what is does exactly, its responsibilities.

@Freyskeyd
Copy link
Member Author

Freyskeyd commented Oct 30, 2023

A few comments:

  • Discussing different options as a database layer is I think not needed here. It's not a discussion but a "matter of fact". Mentioning RocksDB is good, but comparing it doesn't make sense here.
  • "Each store represents a different kind of capabilities": I think it's clear that they serve different purposes, or they wouldn't exist. I would still give a high level overview which store is responsbile for what exactly, and why we split them in these different types of stores.
  • No reason to mention how a new store is instantiated. It's not a library. And the reader will probably never instantiate a new store. If they do, it's easy to point that out in the store documentation itself.
  • Never mention "fairly simple" in a documentation :)
  • I am also not so sure that we have to mention Arc and async-trait.

What I would like to see is something along these lines:

"The topos-tce-storage is serves as the storage layer solely for the tce, and is split into:

  • ValidatorStore
  • FullNodeStore
  • ValidatorPerEpochStore
  • EpochValidatorsStore
  • IndexStore

The relationship between them is as follows (adding the diagram here).

The responsibilties for this storage layer are:

  • Storing and retrieving certificates
  • Managing pending certificates
  • Managing the certificate status
  • Metadata around the protocol and the state of the TCE

The certificates are managed in a pool (explaining here how exactly we think about this and what a pool is, when a certificate is pending and why, the different possible states of a certificate).

Also mentioning here: What is the metadata for us, and why do we store it?

After this is cleared, give a brief overview of each store and what is does exactly, its responsibilities.

Some point are interesting but the part on what specific store are doing will be directly defined on the store itself, not on the documentation of the library.

@gruberb
Copy link
Contributor

gruberb commented Oct 30, 2023

Some point are interesting but the part on what specific store are doing will be directly defined on the store itself, not on the documentation of the library.

Sure, not in detail, but a high level overview. If we mention something, we should explain it. Like in 2-3 sentences what it does. I also don't think we document a library here but more a inner workings of the application.

@Freyskeyd
Copy link
Member Author

@gruberb

Discussing different options as a database layer is I think not needed here. It's not a discussion but a "matter of fact". Mentioning RocksDB is good, but comparing it doesn't make sense here.

The point is to define that this storage implementation doesn't aim to be used in every place. It also offers suggestion and explain why and how it is built like that.
It also mentions that for some usage, a dedicated (and relational) storage could be better. This kind of information can help people understand why some methods are missing or not implemented, it is also a reminder for us that we can't do everything with this single tool.

I am also not so sure that we have to mention Arc and async-trait.
The goal was to explain that there is no multiple instances of one store but a unique one. I'll rephrase.

@Freyskeyd Freyskeyd force-pushed the chore/improve-doc-tce-storage branch 2 times, most recently from d1e7303 to 0367f17 Compare October 31, 2023 09:27
@Freyskeyd Freyskeyd force-pushed the chore/improve-doc-tce-storage branch from 0367f17 to a8c4688 Compare October 31, 2023 14:29
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

This is a HUGE improvement, thank you @Freyskeyd ! ❤️

crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/mod.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/mod.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/mod.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
crates/topos-tce-storage/src/validator/tables.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gruberb gruberb left a comment

Choose a reason for hiding this comment

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

I still think we should not mention other possible solutions or going too much into detail into other technology advantages. This can just lead to opinionated comments or outdated arguments.

Otherwise I think it's a great first step which we can iterate and improve on.

crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
crates/topos-tce-storage/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gruberb gruberb left a comment

Choose a reason for hiding this comment

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

👍

@Freyskeyd Freyskeyd merged commit 9c66d8c into main Nov 17, 2023
16 checks passed
@Freyskeyd Freyskeyd deleted the chore/improve-doc-tce-storage branch November 17, 2023 09:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants