celestia-app icon indicating copy to clipboard operation
celestia-app copied to clipboard

Spec migration

Open adlerjohn opened this issue 3 years ago • 2 comments

Ref: #650

Staging branch for specs migration. Draft PR so it can be tracked.

adlerjohn avatar Sep 27 '22 13:09 adlerjohn

hmm I can't merge main into this branch

Screen Shot 2022-09-28 at 2 55 15 PM

I wanted to merge main to pick up the new CODEOWNERS file

rootulp avatar Sep 28 '22 18:09 rootulp

What's the status of this?

musalbas avatar Oct 19 '22 14:10 musalbas

@musalbas apologies for delayed response. @adlerjohn and I triaged issues in https://github.com/celestiaorg/celestia-app/issues/650 today.

I think that the specs-staging branch is already a more accurate representation of the specs than https://celestiaorg.github.io/celestia-specs/latest/index.html so I'd like to merge specs-staging to main ASAP. But if we want to block this merge on updating specs to match the existing implementation, we can block on all the issues in the "Pure specs" section.

rootulp avatar Nov 01 '22 20:11 rootulp

What's the latest with this and generally adding specs to celestia-app/core now?

musalbas avatar Jan 20 '23 14:01 musalbas

This branch is the most up to date version of the specs but it's not publicly visible yet. IMO we should merge this branch to main, make it publicly visible, and take down the existing specs site at https://celestiaorg.github.io/celestia-specs/latest/index.html

The remaining specs issues are tracked under https://github.com/celestiaorg/celestia-app/issues/650 . We intend on prioritizing specs and documentation work after the core/app feature freeze for incentivized testnet.

rootulp avatar Jan 20 '23 14:01 rootulp

I have some general comments that can possible be followed up at a later point:

  • Can we break up data structures into smaller more self-explanatory files so that parts can be easily accessed in the table of contents. I also think it's less intuitive to explain a spec by just dumping all the data structures that are included in it. Would prefer more of a hierarchical function-orientated tree.
  • Can we add a section explaining how the "celestia app" fits in with the rest of the system and include links so people can be easily redirected if they're interested with something in "celestia node" for example. Furthermore, I think calling it "celestia app" can be better worded. As this isn't the specification of the entire system, rather a component of it, we should look to distinguish it functionally. Call it "celestia data publication specification" (to complement "celestia data availability specfication") if we want to be deliberate about having two networks.
  • I don't think Rationale should be a separate section but should be embedded right next to the part in the spec it refers to. I should read what a certain protocol is and directly after why it is so.
  • It's evident that the specification includes not just novel work specific to Celestia but the components that it is built on top of. There is a lot of work on Tendermint's consensus algorithm (such as block validity, leader selection) and how the validator set is controlled. What about other components like governance, IBC, vesting, upgrades that still seem to be missing. It seems that not all state transition messages have been documented.
  • Is QGB spec somewhere else?
  • Can we have some preamble/context in the top level pages to each of the subpages.
  • Are we sure we want a copy of many of the proto files in the spec as well? These are not the canonical ones that get used in generation and will constantly become outdated.

cmwaters avatar Mar 14 '23 10:03 cmwaters

Overall I'm on-board with basically all of the suggestions you proposed. I think the feedback could be spun out into individual issues that we add to the epic rather than blocking feedback on this PR.

Can we break up data structures into smaller more self-explanatory files

I'm on-board. I agree that the data structures page is long and difficult to read top to bottom so I think multiple logically contained data structures pages would be helpful for readability.

I don't think Rationale should be a separate section but should be embedded right next to the part in the spec it refers to.

Agreed. I think we should move the rationale/message_block_layout section to block_proposer#laying-out-transactions-and-messages

Is QGB spec somewhere else?

Perhaps https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/README.md

Are we sure we want a copy of many of the proto files in the spec as well?

Good question. I think we should not duplicate the proto definitions in the specs unless we have an automated way to update them if the canonical ones change.

rootulp avatar Mar 14 '23 15:03 rootulp

are the markdown linter complaints valid?

They are valid but they don't appear when make lint is run locally. They appear resolved after the most recent commit.

Docker / hadolint fails but that occurs on other branches too (potentially b/c Github outage).

rootulp avatar Mar 15 '23 14:03 rootulp

Will merge this by EOD tomorrow if no more feedback.

rootulp avatar Mar 15 '23 21:03 rootulp

its weird that the docker builds are still failing here despite passing in other PRs, we didn't change anything related here afaict

edit: Rootul fixed it

evan-forbes avatar Mar 16 '23 14:03 evan-forbes