docs(adr75): server v2 #21069
docs(adr75): server v2 #21069
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new document titled "ADR 75: V2 Upgrades," which outlines a comprehensive rewrite of the Cosmos SDK architecture. This version, V2, aims to enhance modularity, extensibility, and performance by breaking down the monolithic structure into smaller, more manageable components. The document details the migration path from V1 to V2 and includes high-level diagrams of the new architecture, including the Consensus and State Transition Function components. Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
tac0turtle
force-pushed
the
marko/serverv2-adr
branch
from
7aed8b2 to
7cba3f8
Compare
tac0turtle
force-pushed
the
marko/serverv2-adr
branch
from
7be1860 to
c3b4847
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
opening this r4r but happy to hear on things missing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
docs/architecture/adr-75-v2.md (12)
13-15: Minor grammatical improvement suggested in the abstract.The abstract provides a concise overview of the V2 upgrades. Consider the following minor grammatical improvement:
-V2 is a reset in the Cosmos SDK architecture. It is a complete rewrite of the SDK, with a focus on modularity, extensibility, and performance. The V2 SDK breaks apart the core SDK into smaller modular components allowing users to pick and choose the components they need for their specific use case. This document outlines the changes and migration path for users of the V1 SDK. +V2 is a reset in the Cosmos SDK architecture. It is a complete rewrite of the SDK, with a focus on modularity, extensibility, and performance. The V2 SDK breaks apart the core SDK into smaller modular components, allowing users to select the components they need for their specific use case. This document outlines the changes and migration path for users of the V1 SDK.This change improves readability and adheres to standard English grammar rules.
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
17-18: Fix typo and improve sentence structure in the context section.There's a typo and a missing space in the first sentence. Consider the following changes:
-The Cosmos SDK began in 2016, at this time the software was written with the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, with new features and improvements being added over time. +The Cosmos SDK began in 2016. At this time, the software was written with the direct use case of the Cosmos Hub. Since then, we have seen the SDK evolve and grow, with new features and improvements being added over time.These changes improve readability and correct the grammatical issues.
🧰 Tools
🪛 LanguageTool
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
50-51: Improve clarity in the description of Baseapp.The description of Baseapp could be more precise. Consider the following change:
-This is a high-level overview of Baseapp today. As we can see baseapp houses all the logic for the ABCI methods, state management, transaction processing, and query handling. This has led baseapp to be a very large monolith. +This is a high-level overview of Baseapp today. As we can see, Baseapp houses all the logic for the ABCI methods, state management, transaction processing, and query handling. This structure has led Baseapp to become a large monolithic component.This change improves clarity and avoids the use of subjective intensifiers like "very".
🧰 Tools
🪛 LanguageTool
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
52-56: Improve grammar in the alternatives section.Consider the following changes to improve grammar and clarity:
-The alternative to doing a rewrite is to spend more time cleaning up baseapp. This would not fix issues around forking the repository to make changes like we see today. Keeping the current codebase does not allow the project to progress and reduce the maintenance burden on the project. +The alternative to a rewrite is to spend more time cleaning up Baseapp. However, this would not address issues related to forking the repository for making changes, as we see today. Maintaining the current codebase would not allow the project to progress or reduce its maintenance burden.These changes improve the flow and clarity of the text while maintaining the original meaning.
🧰 Tools
🪛 LanguageTool
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
59-59: Fix typo in the decision statement.There's a typo in the word "Decision". Please correct it as follows:
-The Descision is to rewrite the core componenets (baseapp, server, store) of the SDK into smaller modules. +The Decision is to rewrite the core components (baseapp, server, store) of the SDK into smaller modules.This correction improves the professionalism of the document.
88-89: Correct typo and improve clarity in the runtime/v2 description.There's a typo and the sentence could be clearer. Consider the following change:
-In the above diagram we do not mention runtime/v2 because it is the componend that is responsible for combining all the other components into a single application. +In the above diagram, we do not mention runtime/v2 because it is the component responsible for combining all the other components into a single application.This change improves readability and corrects the spelling error.
92-93: Improve clarity in the Consensus component description.The description of the Consensus component could be clearer. Consider the following change:
-Consensus is the component that handles communication to the Consensus Engine (Networking & Consensus). The default implementation will be CometBFT, but other consensus engines can be used with v2. The goal of consensus is not to offer a consensus API to meet all needs, but a way to allow developers to swap out the server for a different consensus engine. An application developer should not assume that the cometbftserver will work with other consensus engines. +Consensus is the component that handles communication with the Consensus Engine (Networking & Consensus). The default implementation will be CometBFT, but other consensus engines can be used with v2. The goal of the Consensus component is not to offer a universal consensus API, but to provide a way for developers to swap out the server for a different consensus engine. Application developers should not assume that the CometBFT server will work with other consensus engines.These changes improve clarity and consistency in terminology.
164-196: Improve formatting and add comments to the Go interface.The Go interface for the State Transition Function could benefit from better formatting and comments. Consider the following changes:
type StateTransitionFunction[T transaction.Tx] interface { // DeliverBlock executes a block of transactions. DeliverBlock( ctx context.Context, block *server.BlockRequest[T], state store.ReaderMap, - ) (blockResult *server.BlockResponse, newState store.WriterMap, err error) + ) (*server.BlockResponse, store.WriterMap, error) // ValidateTx validates a transaction. ValidateTx( ctx context.Context, state store.ReaderMap, gasLimit uint64, tx T, ) server.TxResult // Simulate executes a transaction in simulation mode. Simulate( ctx context.Context, state store.ReaderMap, gasLimit uint64, tx T, - ) (server.TxResult, store.WriterMap) + ) (txResult server.TxResult, newState store.WriterMap) // Query executes a query on the application. Query( ctx context.Context, state store.ReaderMap, gasLimit uint64, req transaction.Msg, - ) (transaction.Msg, error) + ) (response transaction.Msg, err error) }These changes improve readability and follow Go best practices for interface definitions.
207-208: Improve clarity in the backwards compatibility explanation.The explanation of backwards compatibility could be clearer. Consider the following changes:
-The state machine was made to not affect modules that are not using the state transition function. If a user would like to migrate to v2 they will need to migrate to `appmodule.Environment` from `sdk.Context`. `sdk.Context` is a struct which is a global in the state machine, this desing limits the concurrency. +The state machine was designed to not affect modules that are not using the state transition function. Users migrating to v2 will need to transition from `sdk.Context` to `appmodule.Environment`. `sdk.Context` is a global struct in the state machine, which limits concurrency. The new design addresses this limitation.These changes improve the flow and clarity of the explanation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~207-~207: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
209-210: Clarify the breaking changes in CometBFT interaction.The description of breaking changes could be more precise. Consider the following changes:
-V2 will have a breaking changes in regards to how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK paniced and recovered in the case of out of gas, providing an error to CometBFT which we do not return in the new design. +V2 introduces breaking changes in how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK panicked and recovered in the case of out-of-gas errors, providing an error to CometBFT. In the new design, this error is not returned to CometBFT.These changes provide a clearer explanation of the breaking changes.
🧰 Tools
🪛 LanguageTool
[misspelling] ~209-~209: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~209-~209: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
227-230: Consider expanding the further discussions section.The further discussions section is quite brief and could benefit from more context. Consider expanding this section to provide more details on:
- The potential benefits of rewriting the core in Rust.
- What "crosslang" refers to and its significance.
- Any other potential future developments or areas of research.
This expansion would give readers a better understanding of the long-term vision for the Cosmos SDK beyond the V2 upgrades.
231-233: Add relevant references to the document.The references section currently contains only a placeholder. To enhance the document's credibility and provide readers with additional resources, consider adding relevant references. These could include:
- Links to related Cosmos SDK documentation
- Academic papers or articles that influenced the V2 design decisions
- Relevant discussions or proposals in the Cosmos community
Adding these references will make the document more comprehensive and valuable to readers.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/architecture/adr-75-v2.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/architecture/adr-75-v2.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
docs/architecture/adr-75-v2.md
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
[duplication] ~99-~99: Possible typo: you repeated a word
Context: ...the transactions in the same block. * Optimistic * Optimistic execution means different things to dif...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~100-~100: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...nsensus may not be made on every block. Instead consensus is made after execution. This...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...er execution. This design favors a fast chain as it will not slow down for execution ...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~101-~101: Possible typo: you repeated a word
Context: ...optimistic window may be exceeded. * Delayed * Delayed execution is the default execution mode...(ENGLISH_WORD_REPEAT_RULE)
[typographical] ~106-~106: Consider adding a comma.
Context: ... Since consensus servers can be swapped there are certain features features specific ...(IF_THERE_COMMA)
[duplication] ~106-~106: Possible typo: you repeated a word
Context: ...ervers can be swapped there are certain features features specific to consensus engines need to b...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~106-~106: Possible missing comma found.
Context: ...lemented in the server. In the CometBFT server we have implemented the following featu...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~113-~113: Possible missing comma found.
Context: ... server would like to utilize the above features they can be copied or implemented in th...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~131-~131: When ‘read-only’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...n function is stateless, it is handed a read only view of state and returns state changes...(READ_ONLY_HYPHEN)
[uncategorized] ~202-~202: Possible missing comma found.
Context: ...enance cost can be the same as existing Baseapp as handling many go.mods is a overhead....(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~202-~202: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ing Baseapp as handling many go.mods is a overhead. * Modularity means different ...(EN_A_VS_AN)
[uncategorized] ~207-~207: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~209-~209: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~209-~209: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
[typographical] ~211-~211: It seems that a comma is missing.
Context: ...e and Commitment. Once the migration is completed the state machine will query informatio...(IF_COMMA)
🔇 Additional comments (2)
docs/architecture/adr-75-v2.md (2)
1-11: LGTM: Header and changelog section are well-structured.The header section provides clear information about the ADR title, changelog, and current status. This follows good documentation practices for Architecture Decision Records.
1-233: Overall, a well-structured and informative ADR with room for minor improvements.This Architecture Decision Record (ADR) provides a comprehensive overview of the V2 upgrades for the Cosmos SDK. The document effectively communicates the context, decisions, and consequences of this significant architectural change.
Key strengths:
- Clear explanation of the motivation behind V2 upgrades
- Detailed description of the new architecture and components
- Balanced discussion of consequences and backwards compatibility
Areas for improvement:
- Minor grammatical and typographical errors throughout the document
- Some sections could benefit from improved clarity and precision
- The references section needs to be completed with relevant sources
Addressing the suggested improvements will enhance the document's professionalism and make it more valuable to the Cosmos SDK community. Great work on documenting this important architectural change!
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
[duplication] ~99-~99: Possible typo: you repeated a word
Context: ...the transactions in the same block. * Optimistic * Optimistic execution means different things to dif...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~100-~100: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...nsensus may not be made on every block. Instead consensus is made after execution. This...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...er execution. This design favors a fast chain as it will not slow down for execution ...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~101-~101: Possible typo: you repeated a word
Context: ...optimistic window may be exceeded. * Delayed * Delayed execution is the default execution mode...(ENGLISH_WORD_REPEAT_RULE)
[typographical] ~106-~106: Consider adding a comma.
Context: ... Since consensus servers can be swapped there are certain features features specific ...(IF_THERE_COMMA)
[duplication] ~106-~106: Possible typo: you repeated a word
Context: ...ervers can be swapped there are certain features features specific to consensus engines need to b...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~106-~106: Possible missing comma found.
Context: ...lemented in the server. In the CometBFT server we have implemented the following featu...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~113-~113: Possible missing comma found.
Context: ... server would like to utilize the above features they can be copied or implemented in th...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~131-~131: When ‘read-only’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...n function is stateless, it is handed a read only view of state and returns state changes...(READ_ONLY_HYPHEN)
[uncategorized] ~202-~202: Possible missing comma found.
Context: ...enance cost can be the same as existing Baseapp as handling many go.mods is a overhead....(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~202-~202: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ing Baseapp as handling many go.mods is a overhead. * Modularity means different ...(EN_A_VS_AN)
[uncategorized] ~207-~207: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~209-~209: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~209-~209: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
[typographical] ~211-~211: It seems that a comma is missing.
Context: ...e and Commitment. Once the migration is completed the state machine will query informatio...(IF_COMMA)
|
|
||
| ```mermaid | ||
| graph TD | ||
| subgraph Server |
There was a problem hiding this comment.
this is only server/v2/cometbft, I think we should show other servers and the server/v2 itself
There was a problem hiding this comment.
server/v2 is the cosensus server. Server/v2 the pkg is a set of apis, I can show different apis (grpc/gateway) if needed
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
docs/architecture/adr-75-v2.md (6)
13-13: Consider adding a comma for improved readabilityThe sentence structure could be improved by adding a comma after "components". This would enhance readability and clarify the relationship between the clauses.
Consider revising the sentence as follows:
-V2 is a reset in the Cosmos SDK architecture. It is a complete rewrite of the SDK, with a focus on modularity, extensibility, and performance. The V2 SDK breaks apart the core SDK into smaller modular components allowing users to pick and choose the components they need for their specific use case. +V2 is a reset in the Cosmos SDK architecture. It is a complete rewrite of the SDK, with a focus on modularity, extensibility, and performance. The V2 SDK breaks apart the core SDK into smaller modular components, allowing users to pick and choose the components they need for their specific use case.🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Possible missing comma found.
Context: ...apart the core SDK into smaller modular components allowing users to pick and choose the c...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
50-50: Improve sentence structure and word choiceThe sentence structure can be improved, and the use of "very" can be replaced with a more precise descriptor.
Consider revising the sentence as follows:
-This is a high-level overview of Baseapp today. As we can see baseapp houses all the logic for the ABCI methods, state management, transaction processing, and query handling. This has led baseapp to be a very large monolith. +This is a high-level overview of Baseapp today. As we can see, Baseapp houses all the logic for the ABCI methods, state management, transaction processing, and query handling. This has led Baseapp to become a substantial monolith.🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: Possible missing comma found.
Context: ...el overview of Baseapp today. As we can see baseapp houses all the logic for the AB...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
54-54: Improve wording and consider expanding the Alternatives sectionThe wording can be improved for clarity, and the section could benefit from more detailed alternatives.
Consider revising the sentence and expanding the section:
-The alternative to doing a rewrite is to spend more time cleaning up baseapp. This would not fix issues around forking the repository to make changes like we see today. Keeping the current codebase does not allow the project to progress and reduce the maintenance burden on the project. +The alternative to a complete rewrite is to invest more time in refactoring and improving the existing baseapp. However, this approach would not address issues related to repository forking for making changes, as we see today. Maintaining the current codebase structure would limit the project's ability to progress and reduce its maintenance burden.Additionally, consider expanding this section to include:
- More detailed exploration of potential alternatives
- Pros and cons of each alternative
- Reasons why these alternatives were ultimately rejected in favor of a complete rewrite
Would you like assistance in drafting an expanded Alternatives section?
🧰 Tools
🪛 LanguageTool
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
208-208: Improve sentence structure and clarityThe sentence structure can be improved for better clarity.
Consider revising the sentence as follows:
-The state machine was made to not affect modules that are not using the state transition function. If a user would like to migrate to v2 they will need to migrate to `appmodule.Environment` from `sdk.Context`. `sdk.Context` is a struct which is a global in the state machine, this desing limits the concurrency. +The state machine was designed to avoid affecting modules that are not using the state transition function. Users migrating to v2 will need to transition from `sdk.Context` to `appmodule.Environment`. `sdk.Context` is a struct that acts as a global in the state machine, a design that limits concurrency.🧰 Tools
🪛 LanguageTool
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
228-231: Consider expanding the Further Discussions sectionThe Further Discussions section is quite brief and could benefit from more detailed exploration of future directions.
Consider expanding this section to include:
- More detailed explanation of the potential Rust rewrite, including benefits and challenges
- Elaboration on what "crosslang" refers to and its significance
- Other potential future directions or areas of research for the Cosmos SDK
- Timeline considerations for these future discussions
Would you like assistance in drafting an expanded Further Discussions section?
1-234: Overall document review: Comprehensive but needs minor refinementsThis ADR provides a thorough overview of the V2 upgrades for Cosmos SDK. The document is well-structured and informative, covering key aspects such as context, decision rationale, architectural changes, and consequences.
However, there are several areas that could benefit from improvement:
- Minor grammatical and typographical errors throughout the document.
- Some sections, particularly Alternatives and Further Discussions, could be expanded for more comprehensive coverage.
- Consistency in terminology and capitalization (e.g., "baseapp" vs "Baseapp").
Recommendation:
- Conduct a thorough proofreading of the entire document to address grammatical and typographical issues.
- Consider expanding the Alternatives and Further Discussions sections as suggested in previous comments.
- Ensure consistency in terminology and capitalization throughout the document.
- Review the mermaid diagrams to ensure they accurately represent the latest architectural decisions.
Would you like assistance in proofreading or expanding any specific sections of the document?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Possible missing comma found.
Context: ...apart the core SDK into smaller modular components allowing users to pick and choose the c...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
[uncategorized] ~50-~50: Possible missing comma found.
Context: ...el overview of Baseapp today. As we can see baseapp houses all the logic for the AB...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
[duplication] ~100-~100: Possible typo: you repeated a word
Context: ...the transactions in the same block. * Optimistic * Optimistic execution means different things to dif...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~101-~101: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...nsensus may not be made on every block. Instead consensus is made after execution. This...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~101-~101: Possible missing comma found.
Context: ...er execution. This design favors a fast chain as it will not slow down for execution ...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~102-~102: Possible typo: you repeated a word
Context: ...optimistic window may be exceeded. * Delayed * Delayed execution is the default execution mode...(ENGLISH_WORD_REPEAT_RULE)
[typographical] ~107-~107: Consider adding a comma.
Context: ... Since consensus servers can be swapped there are certain features features specific ...(IF_THERE_COMMA)
[duplication] ~107-~107: Possible typo: you repeated a word
Context: ...ervers can be swapped there are certain features features specific to consensus engines need to b...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...lemented in the server. In the CometBFT server we have implemented the following featu...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~114-~114: Possible missing comma found.
Context: ... server would like to utilize the above features they can be copied or implemented in th...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~132-~132: When ‘read-only’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...n function is stateless, it is handed a read only view of state and returns state changes...(READ_ONLY_HYPHEN)
[uncategorized] ~203-~203: Possible missing comma found.
Context: ...enance cost can be the same as existing Baseapp as handling many go.mods is a overhead....(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~203-~203: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ing Baseapp as handling many go.mods is a overhead. * Modularity means different ...(EN_A_VS_AN)
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~210-~210: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~210-~210: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
[typographical] ~212-~212: It seems that a comma is missing.
Context: ...e and Commitment. Once the migration is completed the state machine will query informatio...(IF_COMMA)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/architecture/adr-75-v2.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/architecture/adr-75-v2.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
docs/architecture/adr-75-v2.md
[uncategorized] ~13-~13: Possible missing comma found.
Context: ...apart the core SDK into smaller modular components allowing users to pick and choose the c...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~13-~13: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...er modular components allowing users to pick and choose the components they need for their spec...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
[uncategorized] ~50-~50: Possible missing comma found.
Context: ...el overview of Baseapp today. As we can see baseapp houses all the logic for the AB...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~50-~50: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ry handling. This has led baseapp to be a very large monolith. ## Alternatives The alterna...(EN_WEAK_ADJECTIVE)
[style] ~54-~54: Consider using a different verb for a more formal wording.
Context: ...ime cleaning up baseapp. This would not fix issues around forking the repository to...(FIX_RESOLVE)
[duplication] ~100-~100: Possible typo: you repeated a word
Context: ...the transactions in the same block. * Optimistic * Optimistic execution means different things to dif...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~101-~101: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...nsensus may not be made on every block. Instead consensus is made after execution. This...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~101-~101: Possible missing comma found.
Context: ...er execution. This design favors a fast chain as it will not slow down for execution ...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~102-~102: Possible typo: you repeated a word
Context: ...optimistic window may be exceeded. * Delayed * Delayed execution is the default execution mode...(ENGLISH_WORD_REPEAT_RULE)
[typographical] ~107-~107: Consider adding a comma.
Context: ... Since consensus servers can be swapped there are certain features features specific ...(IF_THERE_COMMA)
[duplication] ~107-~107: Possible typo: you repeated a word
Context: ...ervers can be swapped there are certain features features specific to consensus engines need to b...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...lemented in the server. In the CometBFT server we have implemented the following featu...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~114-~114: Possible missing comma found.
Context: ... server would like to utilize the above features they can be copied or implemented in th...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~132-~132: When ‘read-only’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...n function is stateless, it is handed a read only view of state and returns state changes...(READ_ONLY_HYPHEN)
[uncategorized] ~203-~203: Possible missing comma found.
Context: ...enance cost can be the same as existing Baseapp as handling many go.mods is a overhead....(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~203-~203: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ing Baseapp as handling many go.mods is a overhead. * Modularity means different ...(EN_A_VS_AN)
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...ion. If a user would like to migrate to v2 they will need to migrate to `appmodule...(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~210-~210: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~210-~210: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
[typographical] ~212-~212: It seems that a comma is missing.
Context: ...e and Commitment. Once the migration is completed the state machine will query informatio...(IF_COMMA)
|
|
||
| ## Context | ||
|
|
||
| The Cosmos SDK began in 2016, at this time the software was written with the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, with new features and improvements being added over time. The SDK today is used by over 100 different projects, with more users joining the ecosystem every day. This has led to a number of challenges, including: |
There was a problem hiding this comment.
Fix typographical error and improve sentence structure
There's a typographical error and the sentence structure could be improved for better readability.
Please revise the sentence as follows:
-The Cosmos SDK began in 2016, at this time the software was written with the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, with new features and improvements being added over time.
+The Cosmos SDK began in 2016. At this time, the software was written with the direct use case of the Cosmos Hub in mind. Since then, we have seen the SDK evolve and grow, with new features and improvements being added over time.📝 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.
| The Cosmos SDK began in 2016, at this time the software was written with the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, with new features and improvements being added over time. The SDK today is used by over 100 different projects, with more users joining the ecosystem every day. This has led to a number of challenges, including: | |
| The Cosmos SDK began in 2016. At this time, the software was written with the direct use case of the Cosmos Hub in mind. Since then, we have seen the SDK evolve and grow, with new features and improvements being added over time. The SDK today is used by over 100 different projects, with more users joining the ecosystem every day. This has led to a number of challenges, including: |
🧰 Tools
🪛 LanguageTool
[typographical] ~17-~17: It seems that a comma is missing after this introductory phrase.
Context: ...h the direct use case of the Cosmos Hub.Since then we have seen the SDK evolve and grow, w...(SINCE_THEN_COMMA)
|
|
||
| The consensus server is responsible for handling the execution model of the state machine. The state machine executes transactions when it is told, it is unaware of the execution model. | ||
|
|
||
| Since consensus servers can be swapped there are certain features features specific to consensus engines need to be implemented in the server. In the CometBFT server we have implemented the following features: |
There was a problem hiding this comment.
Fix grammatical issues and remove duplicate word
There are some grammatical issues in this sentence, including a duplicate word "features".
Please revise the sentence as follows:
-Since consensus servers can be swapped there are certain features features specific to consensus engines need to be implemented in the server.
+Since consensus servers can be swapped, certain features specific to consensus engines need to be implemented in the server.📝 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.
| Since consensus servers can be swapped there are certain features features specific to consensus engines need to be implemented in the server. In the CometBFT server we have implemented the following features: | |
| Since consensus servers can be swapped, certain features specific to consensus engines need to be implemented in the server. In the CometBFT server we have implemented the following features: |
🧰 Tools
🪛 LanguageTool
[typographical] ~107-~107: Consider adding a comma.
Context: ... Since consensus servers can be swapped there are certain features features specific ...(IF_THERE_COMMA)
[duplication] ~107-~107: Possible typo: you repeated a word
Context: ...ervers can be swapped there are certain features features specific to consensus engines need to b...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...lemented in the server. In the CometBFT server we have implemented the following featu...(AI_HYDRA_LEO_MISSING_COMMA)
| H --> J[State Commitment] | ||
| ``` | ||
|
|
||
| In the above diagram we do not mention runtime/v2 because it is the componend that is responsible for combining all the other components into a single application. |
There was a problem hiding this comment.
Correct typo and improve sentence structure
There's a typo in "component" and the sentence structure can be improved.
Please revise the sentence as follows:
-In the above diagram we do not mention runtime/v2 because it is the componend that is responsible for combining all the other components into a single application.
+In the above diagram, we do not mention runtime/v2 because it is the component responsible for combining all the other components into a single application.📝 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.
| In the above diagram we do not mention runtime/v2 because it is the componend that is responsible for combining all the other components into a single application. | |
| In the above diagram, we do not mention runtime/v2 because it is the component responsible for combining all the other components into a single application. |
|
|
||
| ## Decision | ||
|
|
||
| The Descision is to rewrite the core componenets (baseapp, server, store) of the SDK into smaller modules. |
There was a problem hiding this comment.
Fix typo in "Decision"
There's a typo in the word "Decision".
Please correct the spelling:
-The Descision is to rewrite the core componenets (baseapp, server, store) of the SDK into smaller modules.
+The Decision is to rewrite the core components (baseapp, server, store) of the SDK into smaller modules. 📝 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.
| The Descision is to rewrite the core componenets (baseapp, server, store) of the SDK into smaller modules. | |
| The Decision is to rewrite the core components (baseapp, server, store) of the SDK into smaller modules. |
|
|
||
| The state machine was made to not affect modules that are not using the state transition function. If a user would like to migrate to v2 they will need to migrate to `appmodule.Environment` from `sdk.Context`. `sdk.Context` is a struct which is a global in the state machine, this desing limits the concurrency. | ||
|
|
||
| V2 will have a breaking changes in regards to how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK paniced and recovered in the case of out of gas, providing an error to CometBFT which we do not return in the new design. |
There was a problem hiding this comment.
Correct grammatical issues and improve clarity
There are several grammatical issues in this paragraph that need to be addressed.
Please revise the paragraph as follows:
-V2 will have a breaking changes in regards to how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK paniced and recovered in the case of out of gas, providing an error to CometBFT which we do not return in the new design.
+V2 will have breaking changes regarding how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK panicked and recovered in the case of out-of-gas errors, providing an error to CometBFT. In the new design, we do not return this error.📝 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.
| V2 will have a breaking changes in regards to how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK paniced and recovered in the case of out of gas, providing an error to CometBFT which we do not return in the new design. | |
| V2 will have breaking changes regarding how CometBFT handles certain fields in ABCI. Previously, the Cosmos SDK panicked and recovered in the case of out-of-gas errors, providing an error to CometBFT. In the new design, we do not return this error. |
🧰 Tools
🪛 LanguageTool
[misspelling] ~210-~210: Although “in regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...ency. V2 will have a breaking changes in regards to how CometBFT handles certain fields in ...(IN_OR_WITH_REGARDS_TO_OF)
[grammar] ~210-~210: The usual collocation for “return” is “to”, not “in”.
Context: ...ng an error to CometBFT which we do not return in the new design. V2 only works with `S...(RETURN_IN_THE)
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
Description
ADR for server v2 with design diagrams
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Documentation