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: copy across cat pool spec from celestia-core #2207

Merged
merged 2 commits into from
Aug 8, 2023
Merged

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Aug 2, 2023

Overview

Given that it is only the specification of celestia-app that is published it makes more sense to include it there rather than in the directory of the cat pool itself

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@MSevey MSevey requested a review from a team August 2, 2023 14:16
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-2207/
on branch gh-pages at 2023-08-07 09:12 UTC

@codecov-commenter
Copy link

Codecov Report

Merging #2207 (7937bc1) into main (772d0b8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2207   +/-   ##
=======================================
  Coverage   24.89%   24.89%           
=======================================
  Files         127      127           
  Lines       14335    14335           
=======================================
  Hits         3568     3568           
  Misses      10400    10400           
  Partials      367      367           

rootulp
rootulp previously approved these changes Aug 2, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM. In the past there was a push for specs to live in the same repo as the code they are relevant to. If we still want that, we can alternatively consider publishing celestia-core specs to their own website so that they don't need to be copied to celestia-app. I'm curious what @adlerjohn thinks.

@@ -7,6 +7,7 @@
- [Namespace](./specs/namespace.md)
- [Shares](./specs/shares.md)
- [Consensus](./specs/consensus.md)
- [CAT Pool](./specs/cat_pool.md)
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 we may want to add this to the README here:

- [Consensus](./specs/consensus.md)

It's a bit ridiculous to maintain 3 locations for the same table of contents so we may consider deleting that README alltogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I added it to the SUMMARY.md which I think is the main one in terms of the rendered static site

@rootulp rootulp added the specs directly relevant to the specs label Aug 2, 2023
evan-forbes
evan-forbes previously approved these changes Aug 2, 2023
@evan-forbes
Copy link
Member

evan-forbes commented Aug 2, 2023

In the past there was a push for specs to live in the same repo

alternatively we could simply link to the catpool spec in core. all that matters imo is that we have a single starting place that at least links to everything.

@cmwaters
Copy link
Contributor Author

cmwaters commented Aug 2, 2023

I tend to agree that they should live in their own repo but its a bit strange when it comes to forks. I would also prefer that from the outside Celestia is presented as a single cohesive spec

@cmwaters cmwaters dismissed stale reviews from evan-forbes and rootulp via 359cb83 August 7, 2023 09:12
@MSevey MSevey requested a review from a team August 7, 2023 09:12
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Looks good! I'd recommend removing the original spec from the cat pool directory and adding a link to this one to avoid having multiple copies.

Update: I see that the cat pool lives in the core repo, so, it may as well make sense to have a separate website dedicated to the core related specs. Though no strong opinion.

@cmwaters
Copy link
Contributor Author

cmwaters commented Aug 8, 2023

it may as well make sense to have a separate website dedicated to the core related specs

Ideally I'd like to have a single site that contains all the specification of the celestia network (including celestia-node as well). For now, to make it more visible, I think it's better here and once we develop a better system we can remove it and just have a spec directory in core

@cmwaters cmwaters merged commit f921fbe into main Aug 8, 2023
27 checks passed
@cmwaters cmwaters deleted the cal/copy-cat-spec branch August 8, 2023 09:45
@evan-forbes evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Sep 5, 2023
mergify bot pushed a commit that referenced this pull request Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging specs directly relevant to the specs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants