Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CDK PP Docs Review Env: DO NOT MERGE!!! #2459
CDK PP Docs Review Env: DO NOT MERGE!!! #2459
Changes from 16 commits
49d7747
dc6b842
5e111f3
e0c1a09
2739fe6
08d3d30
9f88432
f0a1347
3f6f1f5
78983f0
c1dcfb4
95bc789
1e477ab
cffe05f
6cd106e
50fe59d
dab2f0d
fc11c3e
342775b
403945f
89dd66f
98b4573
c9ffa8d
a43098a
228e3d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what this sentence isa trying to cover, but I find the wording confusing... For starters,
Polygon zkEVM protocol
shoul be out of the question. Maybe useAggLayer
oruLxLy
instead.OTOH, I think that we need more clarity on what we mean by "sovereign chains". IMO, Sovereign chains should refer to chains that are governed by other means rather than the AL (aka existing chains such as base to give an example), and yet those chains can be integrated to the AL. Then we have the "AL native chains" or "CDK native" chains, which are chains that were built inside the protocol since it's genesis (the chains that this doc tries to explain)
Also on the naming, the protocol team has been using the term zkChains to refer to FEP and sovereign to refer to PP. But I think we also need to differentiate native/foreign chains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arnaubennassar this is a good point/question and will need further work in general, but I think for now we are going with sovereign when referring to the proverless config (PP chains) and we can update the terminology everywhere once we all agree on final naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing to me... The idea is that:
I wouldn't talk about prover stacks because:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going back to the first comment left on this file: we need to be extremely careful with naming. This line makes it feel that we're quite inconsistent with naming... Is it
CDK sovereign chains
orPP chains
? What will happen when we have chains that use PP and FEP?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, there is a weird level of detail... We list the CLI, but not the syncers? Most importantly, the aggSender looks like a standalone piece of software, when it's clearly just a functionality of the cdk client. I've been talking with some non Polygon folks and they're super confused with what's the CDK / AggLayer. We need to be very careful on how we articulate this things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AggSender is just a sub-component of the
cdk
client. I think that we should be talking aboutcdk client
orcdk node
more in the doc... Although it's ok to specify that theAggSender
is the specific sub-component that sends the certs to the AL. But note that theAggSender
works in conjunction with other sub-components of thecdk
such as the bridge syncer and others...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of
CDK PP
. No, there is the cdk. And the cdk happens to be used to give PP functionality, but there is no CDK PP... Again, once we've FEP+PP the same cdk will be used, and in this case we will have the aggsender as well as other componentsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit odd to have a sub section for AL, and not for CDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldnt it be better to talk about
AggLayer node
similar to the aggsender, the RPC is just a part of the AL node... but for sure avoidJSON-RPC
it's extremely confusing, people will think about the API of the Ethereum execution clients when they see this, and it has nothing to do with it