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

chore(protocol): cleanup autogen docs #14451

Closed
wants to merge 32 commits into from
Closed

Conversation

dionysuzx
Copy link
Collaborator

@dionysuzx dionysuzx commented Aug 11, 2023

@dantaik @adaki2004 I reviewed each file in the previous PR and applied a consistent standard. Could you please:

  1. Check for correctness (most important)
  2. Look at the three TODO(docs) that I left

I will merge this PR after review.

@dionysuzx dionysuzx changed the base branch from main to better_protocol_documentation August 11, 2023 01:19
@dionysuzx dionysuzx requested review from dantaik and adaki2004 August 11, 2023 01:19
@@ -431,7 +434,10 @@ contract ProverPool is EssentialContract, IProverPool {
return true;
}

// Calculates the user weight's when it stakes/unstakes/slashed
/// @dev See the documentation for the callers of this internal function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here in this doc, we used /** **/ and ///, shall we stick to one format across all files?

* system. It defines methods for sending and verifying signals with merkle
* proofs. The trust assumption is that the target chain has secure access
* to the merkle root (such as Taiko injects it in the anchor transaction).
* With this, verifying a signal is reduced to simply verifying a merkle proof.
Copy link
Contributor

@dantaik dantaik Aug 11, 2023

Choose a reason for hiding this comment

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

will put @title as the first line better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

overall, i agree with this. in fact, there are two changes i want to make:

  • @title before description
  • verbosely include "@notice" even though it is not required by solidity compiler
  • move multi-line comment blocks to /// instead of /** */ for simplicity

however, i would like to introduce that later, after we cleanup the bridge documentation as well and get consistency throughout the codebase. i already commented the bridge/signal service previously using the standards here: https://taiko.xyz/docs/manuals/contributing-manual#source-code-comments

so don't want to bikeshed over this. once we document to the spec, we can update the spec and use chatGPT to fixup all the files quickly.

@dantaik
Copy link
Contributor

dantaik commented Aug 11, 2023

@dantaik @adaki2004 I reviewed each file in the previous PR and applied a consistent standard. Could you please:

  1. Check for correctness (most important)
  2. Look at the three TODO(docs) that I left

I will merge this PR after review.

I removed the 3 TODOs

Base automatically changed from better_protocol_documentation to main August 11, 2023 03:44
@dionysuzx dionysuzx requested a review from dantaik August 11, 2023 16:14
@dionysuzx dionysuzx marked this pull request as draft August 11, 2023 20:35
@dionysuzx
Copy link
Collaborator Author

closing this PR in favor of #14462

@dionysuzx dionysuzx closed this Aug 11, 2023
@dionysuzx dionysuzx deleted the cleanup-better-proto-docs branch August 23, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants