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

docs: introduce ADR for permissionless ICS #2001

Merged
merged 50 commits into from
Jul 26, 2024
Merged

Conversation

insumity
Copy link
Contributor

@insumity insumity commented Jul 1, 2024

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct docs: prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR only changes documentation
  • Reviewed content for consistency
  • Reviewed content for spelling and grammar
  • Tested instructions (if applicable)
  • Checked that the documentation website can be built and deployed successfully (run make build-docs)

Summary by CodeRabbit

  • New Features

    • Introduced "Permissionless Interchain Security" (ICS) through ADR 18, enabling Opt In consumer chains to join ICS via transactions without governance proposals.
    • Added a visual representation of consumer chain proposals and transactions, enhancing understanding of operational flows.
    • Introduced new messages for managing consumer chains: MsgRegisterConsumerChain, MsgInitializeConsumerChain, MsgUpdateConsumerChain, and MsgStopConsumerChain.
  • Documentation

    • Detailed documentation on "Permissionless Interchain Security" (ICS) and its related processes, including phases and state management.

@github-actions github-actions bot added C:Testing Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler C:ADR Assigned automatically by the PR labeler labels Jul 1, 2024
@insumity insumity force-pushed the insumity/permissionless-ADR branch 2 times, most recently from 117e938 to 815ca88 Compare July 1, 2024 15:31
@insumity insumity marked this pull request as ready for review July 2, 2024 13:05
@insumity insumity requested a review from a team as a code owner July 2, 2024 13:05
coderabbitai[bot]

This comment was marked as spam.

@insumity insumity self-assigned this Jul 2, 2024
coderabbitai[bot]

This comment was marked as spam.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 20

p-offtermatt

This comment was marked as resolved.

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
Copy link
Contributor

@p-offtermatt p-offtermatt left a comment

Choose a reason for hiding this comment

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

Great work. I left some comments

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 29

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
The information needed to set up a `ConsumerChainRecord` are stored in the corresponding `ConsumerAdditionProposal` (see `MsgLaunchConsumerChain` below).

### New Messages
In what follows, we describe the new messages that Permissionless ICS introduces and on how those can be used.
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 3, 2024

Choose a reason for hiding this comment

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

Add a comma after 'introduces'.

Possible missing comma found.

- In what follows, we describe the new messages that Permissionless ICS introduces and on how those can be used.
+ In what follows, we describe the new messages that Permissionless ICS introduces, and on how those can be used.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
In what follows, we describe the new messages that Permissionless ICS introduces and on how those can be used.
In what follows, we describe the new messages that Permissionless ICS introduces, and on how those can be used.
Tools
LanguageTool

[uncategorized] ~153-~153: Possible missing comma found.
Context: ...ntroduces and on how those can be used. We then, describe how we can utilize those...

(AI_HYDRA_LEO_MISSING_COMMA)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In what follows, we describe the new messages that Permissionless ICS introduces and on how those can be used.
In this section, we describe the new messages that Permissionless ICS introduces and how they can be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mpoke, your suggested rephrasing improves the clarity of the sentence. I agree with your suggestion:

In this section, we describe the new messages that Permissionless ICS introduces and how they can be used.

I'll update the documentation accordingly. Thank you for the suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this still needs to be addressed

Copy link
Contributor

Choose a reason for hiding this comment

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

@mpoke, thank you for pointing that out. I'll ensure the documentation is updated with the rephrased sentence:

In this section, we describe the new messages that Permissionless ICS introduces and how they can be used.

This change will enhance the clarity of the documentation. Thank you for your attention to detail!


Learnings used
Learnt from: p-offtermatt
PR: cosmos/interchain-security#2001
File: docs/docs/adrs/adr-018-permissionless-ics.md:195-195
Timestamp: 2024-07-03T10:36:06.506Z
Learning: When reviewing documentation for the Interchain Security project, remember that the `ConsumerAdditionProposal`s should use the `consumerID` instead of using both `chainID` and `consumerID`.

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 15

Outside diff range and nitpick comments (10)
docs/docs/adrs/adr-018-permissionless-ics.md (10)

14-14: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ## Context
+ ## Context
Tools
Markdownlint

14-14: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


26-26: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ### The Phases of a Consumer Chain
+ ### The Phases of a Consumer Chain
Tools
Markdownlint

26-26: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


53-53: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ### From `chainID` to `consumerID`
+ ### From `chainID` to `consumerID`
Tools
Markdownlint

53-53: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


62-62: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- #### State
+ #### State
Tools
Markdownlint

62-62: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


152-152: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ### New Messages
+ ### New Messages
Tools
Markdownlint

152-152: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


156-156: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- #### Launch a Consumer Chain
+ #### Launch a Consumer Chain
Tools
Markdownlint

156-156: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


199-199: Consider a more expressive alternative.

Consider a more expressive alternative for the phrase "To do this."

- to avoid having consumer chains lingering for too long before they get added. To do this, we introduce a `maxSpawnTime` limit in [`ConsumerAdditionProposal`](https://github.com/cosmos/interchain-security/blob/v4.3.0/proto/interchain_security/ccv/provider/v1/provider.proto#L29).
+ to avoid having consumer chains lingering for too long before they get added. Therefore, we introduce a `maxSpawnTime` limit in [`ConsumerAdditionProposal`](https://github.com/cosmos/interchain-security/blob/v4.3.0/proto/interchain_security/ccv/provider/v1/provider.proto#L29).
Tools
LanguageTool

[style] ~199-~199: Consider a more expressive alternative.
Context: ... for too long before they get added. To do this, we introduce a maxSpawnTime lim...

(DO_ACHIEVE)


253-253: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ### Additional Modifications
+ ### Additional Modifications
Tools
Markdownlint

253-253: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


256-256: Replace 'at the moment' with 'currently'.

For conciseness, consider replacing this expression with an adverb.

- Because we only have two consumer chains at the moment, this is not going to be an expensive migration even if we have some
+ Because we currently only have two consumer chains, this is not going to be an expensive migration even if we have some

Use 'will' instead of 'going to'.

Use ‘will’ instead of ‘going to’ if the following action is certain.

- this is not going to be an expensive migration even if we have some consumer chains that are being voted upon.
+ this will not be an expensive migration even if we have some consumer chains that are being voted upon.
Tools
LanguageTool

[style] ~256-~256: For conciseness, consider replacing this expression with an adverb.
Context: ...ecause we only have two consumer chains at the moment, this is not going to be an expensive m...

(AT_THE_MOMENT)


[style] ~256-~256: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...two consumer chains at the moment, this is not going to be an expensive migration even if we ha...

(GOING_TO_WILL)


[uncategorized] ~256-~256: Possible missing comma found.
Context: ...t, this is not going to be an expensive migration even if we have some consumer chains th...

(AI_HYDRA_LEO_MISSING_COMMA)


262-262: Add a blank line below the heading.

Headings should be surrounded by blank lines for better readability.

- ## Consequences
+ ## Consequences
Tools
Markdownlint

262-262: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
@p-offtermatt
Copy link
Contributor

@coderabbitai pause
I thought I turned the bot off on markdown files :(

This comment was marked as spam.

@cosmos cosmos deleted a comment from coderabbitai bot Jul 8, 2024
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Partial review.

docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-018-permissionless-ics.md Outdated Show resolved Hide resolved
@cosmos cosmos deleted a comment from coderabbitai bot Jul 8, 2024
@cosmos cosmos deleted a comment from coderabbitai bot Jul 8, 2024
@cosmos cosmos deleted a comment from coderabbitai bot Jul 8, 2024
@insumity
Copy link
Contributor Author

@coderabbitai resolve
@codereabbitai stop
@codereabbitai pause

Copy link
Contributor

coderabbitai bot commented Jul 25, 2024

@insumity: I have resolved all comments, stopped, and paused the bot as requested.


Learnings used
Learnt from: p-offtermatt
PR: cosmos/interchain-security#2001
File: docs/docs/adrs/adr-018-permissionless-ics.md:195-195
Timestamp: 2024-07-03T10:36:06.506Z
Learning: When reviewing documentation for the Interchain Security project, remember that the `ConsumerAdditionProposal`s should use the `consumerID` instead of using both `chainID` and `consumerID`.
Actions performed

Comments resolved.


Reviews paused.

@insumity insumity force-pushed the insumity/permissionless-ADR branch from 7134fe8 to 50233f9 Compare July 25, 2024 09:07
@insumity insumity force-pushed the insumity/permissionless-ADR branch from 0c66486 to ac34a98 Compare July 25, 2024 09:58
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Great work @insumity.

@insumity insumity added this pull request to the merge queue Jul 26, 2024
Merged via the queue into main with commit 76fd678 Jul 26, 2024
9 of 12 checks passed
@insumity insumity deleted the insumity/permissionless-ADR branch July 26, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:ADR Assigned automatically by the PR labeler C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler C:Testing Assigned automatically by the PR labeler C:x/consumer Assigned automatically by the PR labeler C:x/democracy Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:x/types Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants