Comments on rollkit stack page

[musalbas]

General comments:

• [x] the page doesn't seem to mention about how to connect an application on top of rollkit in general (there is a section that is very specific to Cosmos SDK only): in particular, we should say that it accepts ABCI-compatible applications. Maybe add that to the start of "rollup application dependencies" section below. in the future we could also adopt the diagram in https://docs.celestia.org/developers/rollmint/ @tzdybal https://github.com/rollkit/docs/pull/90
• [x] "roll-up" should be changed to "rollup" 6a57587fbf2f7b33696e447517c484619c34c3b0
• [x] Would it be a good idea to link the respective Go packages for each component? eg the mempool section can link to https://github.com/rollkit/rollkit/tree/main/mempool b7cdaf8ddb5e0c332336a747be4479aeaa461abe
• [x] Remove hyphens in the headings. "P2P-Layer" should be "P2P layer", for example. d2bc8bd1f589ee157390d95a970405b5a045762a

The Rollkit Stack

• [x] Do light clients exist yet? If not, should be changed to "will". Also replace "fraud proofs" with "proofs", as you can't assume that all rollups will use fraud proofs, some may use zk proofs. d62048d742c1b2e874b09165e18589312f95ced3
• [x] Delete "A typical rollup has:"? seems abrupt d62048d742c1b2e874b09165e18589312f95ced3

Rollup Application Dependencies

• [x] I don't see how the content of this section matches the title. Or this is overly Cosmos SDK specific, we should add something about ABCI-compatibility in general. This section seems to describe how to make a Cosmos-SDK app work with rollkit, not the dependencies of rollkit in general. We should have both. Also related to comment 1 in "general comments" above. @tzdybal https://github.com/rollkit/docs/pull/90

State Fraud Proofs

• [x] Need to add something here about what the current state of this work is and clarify it's a work in progress, as it sounds like it's ready to be used, when it's a work in progress (the future plans section makes it sounds like those are just nice-to-haves). 27c2b1e97f535e17d33fa76d027ae11254601553
• [x] Add links to wip branch/ADRs/design doc? Is there a design doc on notion - if so perhaps that could be posted as an issue here or on the Celestia forum, and linked to? aa4e22a814247f23e6654a7d087b505de0651d1d

P2P-Layer

• [x] "advertises it's" -> "advertises its" d62048d742c1b2e874b09165e18589312f95ced3

Sequencer node

• [x] typo: respoonsible d62048d742c1b2e874b09165e18589312f95ced3
• [ ] We don't have censorship resistance for the centralized sequencer yet right, because there's no way to force transaction inclusion currently? @S1nus
• [x] Realistically none of this stuff is coming soon, so maybe we should change coming soon to "planned". d62048d742c1b2e874b09165e18589312f95ced3
• [x] What's "Pure Fork-Choice Rule" btw? @S1nus

Light node

• [x] Are light clients implemented yet? If not, that should be clarified. 049bb98e414ef8ee8a4ea800b240823bf19c3660

Block-Manager

• [x] typo: AggregatonLoop d62048d742c1b2e874b09165e18589312f95ced3
• [x] typo: Rollkit Notes d62048d742c1b2e874b09165e18589312f95ced3

RPC Layer

• [x] typo: committments d62048d742c1b2e874b09165e18589312f95ced3
• [x] for me, what is meant by "support other key APIs such as rollup state validation (optimistic)"?

[S1nus]

We don't have censorship resistance for the centralized sequencer yet right, because there's no way to force transaction inclusion currently?

We don't have it yet, but it's probably possible.

What's "Pure Fork-Choice Rule" btw?

anything without any privileged sequencers, e.g defer to Celestia ordering and apply "first-come-first-serve" fork-choice

[jcstein]

thanks @S1nus! It looks like the table has "Eventual ⏳" for the "censorship resistance" column on the "Centralized Sequencer" -- @musalbas what do you think we should put here instead (if something else)?

I addressed the second comment here: 9bbc8ba27b7bb2c22931331ffd1ee3b7bdf16a57 / a02105f6c25d16558b19cfe4e53b9d1309c897ad

[musalbas]

"Eventual" means that it has delayed censorship resistance (eg transactions will eventually be included).

If it doesn't currently have that implemented, then the current table is misleading because it suggests that's the current status quo (it says "Implemented"), and should therefore be clarified of fixed in some way.

[jcstein]

Understood, thank you. Is this implemented yet @S1nus, AFAIK centralized sequencer is? maybe no checkmark, or "Planned"?

[gupadhyaya]

@jcstein we can remove this statement: In future, we plan to support other key APIs such as rollup state validation (optimistic) and make the Rollkit RPCs generalizable beyond Tendermint. I was referring to exposing some of the rollup validation data like fraud proofs, but this information is not stored on rollup blocks and it is mostly at the node/client level. Until we have more clarity, we don't need to mention anything. Also, same for generalizing beyond tendermint rpcs. We don't to mention right now.

[jcstein]

thanks @gupadhyaya - done 186cfcf33ca3b086de7dcd216201f5d920fc83df

[S1nus]

If it doesn't currently have that implemented

it's not currently implemented, but it is possible to achieve with that configuration, so i think it should be included. Other unimplemented possibilities are documented, such as "deploying a new chain as easy as deploying a smart contract" and "deploy rollups on existing settlement and execution layers"

[musalbas]

it's not currently implemented, but it is possible to achieve with that configuration, so i think it should be included

It can be included but it should be clarified that it's not implemented/planned, because the table says "Implemented", so it's objectively not true:

such as "deploying a new chain as easy as deploying a smart contract"

This is a mission statement (as well as this being a subjective statement), and this statement is actually true using Ignite: https://docs.celestia.org/developers/gm-rollmint/

It can also be prefaced as "Our goal is to [make deploying a new chain as easy as deploying a smart contract]".

"deploy rollups on existing settlement and execution layers"

That should be fixed by clarifying that this is a future goal.

[jcstein]

It can be included but it should be clarified that it's not implemented/planned, because the table says "Implemented", so it's objectively not true:

what do you think of "In Progress" in this instance? (8ad47fb84ab3ee8a0f058b498bb151a9bc4cb626)

[musalbas]

Here's my concrete suggestion. I still think it's a good idea to show that at least one type of sequencer is implemented. So we can keep that row as Implemented.

However, we could add an asterisk next to Eventual, and then under that table put ( Implementation of this property is in progress), or similar.

That way, people know exactly what's implemented about centralized sequencers or not.

[jcstein]

Thank you. For the asterisk, you mean for Implemented* not for Eventual, right? As Eventual refers to delayed censorship resistance

[musalbas]

No I mean for Eventual, to indicate that this property (Eventual censorship-resistance) isn't implemented yet

[jcstein]

done in 1a40568f7a544a49005072ee1d9eb90e0980e1a8