From 7ada75551b59df5cc561cb01d60134e3c10ea546 Mon Sep 17 00:00:00 2001 From: Jonathan Gimeno Date: Thu, 28 Dec 2023 23:03:27 +0100 Subject: [PATCH 1/5] add adr 1 proposal in the docs --- documentation/adrs/adr-1.md | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 documentation/adrs/adr-1.md diff --git a/documentation/adrs/adr-1.md b/documentation/adrs/adr-1.md new file mode 100644 index 000000000..ac78b8771 --- /dev/null +++ b/documentation/adrs/adr-1.md @@ -0,0 +1,45 @@ +# ADR1: Refactoring MsgServer and Keeper for Clear Separation of Concerns in Nibiru Chain + +## Introduction + +In developing Nibiru Chain, built on Cosmos SDK, we have identified design and development practices that require optimization. This document proposes methodologies to differentiate between `MsgServer` and `Keeper` in the code and to improve our action-based testing framework. + +## Cosmos SDK: `MsgServer` and `Keeper` + +### Issues with Combining `MsgServer` and `Keeper` + +Merging `MsgServer` and `Keeper` in the Cosmos SDK context goes against the fundamental principle of separation of responsibilities. This blending results in: + +- **Role Confusion:** It obscures the distinct functions that each component should ideally perform. `MsgServer` should focus on request validation, including Permissions, while `Keeper` should handle business logic. +- **Maintenance Challenges:** This conflation complicates code maintenance, as intertwined responsibilities make it difficult to isolate and address issues effectively. +- **Security Implications:** The lack of clear boundaries between validation and execution can lead to security vulnerabilities, as ensuring that each layer only handles its intended tasks becomes challenging. + +### Analogy with Web Applications + +Compared with a web application, the `MsgServer` would act as a controller to validate requests, while the `Keeper` would be equivalent to business logic or the model. + +## Specific Case in Nibiru Chain + +Some `MsgServer` methods in the Nibiru Chain are restricted to the `Sudo` group, underscoring the need for a clear separation in the validation and execution of requests. + +### Proposal for Functional Separation + +We propose that the `MsgServer` solely handles validation while the `Keeper` manages the business logic post-validation. + +## Benefits of the Refactor in the Action-Based Testing Framework + +With the proposed restructuring of `MsgServer` and `Keeper`, we gain significant benefits in our approach to testing, particularly within our action-based testing framework: + +### Simplification of Action-Based Tests + +The clear separation of responsibilities between `MsgServer` and `Keeper` allows our tests to focus on creating atomic actions without setting up complex scenarios. This eliminates the need for: + +- **Preparing Users and Permissions:** There's no longer a requirement to create a user and add it to the `Sudo` group for each test scenario, greatly simplifying the test setup process. + +- **Reducing Boilerplate in Tests:** We minimize additional code required to establish preconditions not directly related to the test's objective. + +- **Focus on Business Logic:** Tests can concentrate on assessing pure business logic, undistracted by security and permission configurations. + +## Conclusion + +Separating the `MsgServer` and `Keeper` in developing and testing the Nibiru Chain will significantly improve the code's clarity, maintenance, and security. These improvements reflect our commitment to efficient and robust development, aligned with the best industry practices. From 051e3f8f6c7a8d939207d73230de1f069a5ecb65 Mon Sep 17 00:00:00 2001 From: Jonathan Gimeno Date: Thu, 28 Dec 2023 23:34:57 +0100 Subject: [PATCH 2/5] append potential concerns --- documentation/adrs/adr-1.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/documentation/adrs/adr-1.md b/documentation/adrs/adr-1.md index ac78b8771..3d04e6343 100644 --- a/documentation/adrs/adr-1.md +++ b/documentation/adrs/adr-1.md @@ -40,6 +40,20 @@ The clear separation of responsibilities between `MsgServer` and `Keeper` allows - **Focus on Business Logic:** Tests can concentrate on assessing pure business logic, undistracted by security and permission configurations. +## Addressing Potential Concerns: Security and Accessibility of Keeper Methods + +### Concerns About Security and Access Control + +Some might argue that sharing Keeper's methods can lead to security risks, mainly if there are concerns about unauthorized access. This viewpoint stems from the belief that the `Keeper` should control access, which might lead to apprehensions about exposing specific methods. + +### Clarifying the Role of the Keeper + +However, this perspective needs to be revised in the fundamental role of the `Keeper`. The primary responsibility of the `Keeper` is to maintain a consistent state within the application rather than controlling access. Access control and validation of requests are the responsibilities of the `MsgServer`, which acts as the first line of defense. + +### Best Practices in Method Exposure + +Suppose there's a need to share the Keeper with other modules, and concerns arise about the safety of exposing specific methods. In that case, the preferred approach is to keep those sensitive methods private. Implementing access and permission layers within the `Keeper` goes against the principle of separation of responsibilities and can lead to a more cohesive and secure system. Instead, ensuring that only the appropriate methods are exposed and keeping others private aligns with the philosophy of keeping each component focused on its specific role. + ## Conclusion Separating the `MsgServer` and `Keeper` in developing and testing the Nibiru Chain will significantly improve the code's clarity, maintenance, and security. These improvements reflect our commitment to efficient and robust development, aligned with the best industry practices. From 3b6178fb38281a8ccab43a75fd130f8ccb25afe5 Mon Sep 17 00:00:00 2001 From: Unique-Divine Date: Thu, 28 Dec 2023 10:20:21 -0600 Subject: [PATCH 3/5] docs: file path for adrs in docs folder --- .gitignore | 2 +- .../adrs/adr-1.md => docs/adr/01-adr-msg-server-keeper.md | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename documentation/adrs/adr-1.md => docs/adr/01-adr-msg-server-keeper.md (100%) diff --git a/.gitignore b/.gitignore index 09a8df65f..03dff11ba 100644 --- a/.gitignore +++ b/.gitignore @@ -11,7 +11,7 @@ data build *_old mockdata/ -docs +docs/static dist coverage.txt temp diff --git a/documentation/adrs/adr-1.md b/docs/adr/01-adr-msg-server-keeper.md similarity index 100% rename from documentation/adrs/adr-1.md rename to docs/adr/01-adr-msg-server-keeper.md From 55fea2eec3e165e65f7cd2830626ccd9d8c1c816 Mon Sep 17 00:00:00 2001 From: Unique-Divine Date: Thu, 28 Dec 2023 10:53:19 -0600 Subject: [PATCH 4/5] quicksave wip! --- docs/adr/00-adr-template.md | 64 +++++++++++++++ docs/adr/01-adr-msg-server-keeper.md | 118 +++++++++++++++++++++++---- docs/adr/README.md | 50 ++++++++++++ 3 files changed, 214 insertions(+), 18 deletions(-) create mode 100644 docs/adr/00-adr-template.md create mode 100644 docs/adr/README.md diff --git a/docs/adr/00-adr-template.md b/docs/adr/00-adr-template.md new file mode 100644 index 000000000..a03bd4fa0 --- /dev/null +++ b/docs/adr/00-adr-template.md @@ -0,0 +1,64 @@ +# ADR-XXX: + +Brief abstract statement/paragraph. + +- [Changelog](#changelog) +- [Context](#context) +- [Decision](#decision) + - [Design](#design) + - [Implementation](#implementation) +- [Status](#status) +- [Consequences](#consequences) + - [Positive](#positive) + - [Negative](#negative) + - [Alternatives Considered](#alternatives-considered) +- [Related Documents](#related-documents) + +## Changelog + +- yyyy-mm-dd: Initial Draft +- yyyy-mm-dd: Accepted + +## Context + +- What is the background context for this decision? +- What problem does it solve? +- What external issues or forces influenced this decision? + +## Decision + +### Design + +- Outline the details of the solution and how it addresses the context +- Describe the proposed functionality and design + +``` +Code and pseudo-code snippets useful for explanation +``` + +### Implementation + +- Highlight any implementation details or considerations +- Changes to existing code, new modules, flow, etc + +## Status + +- Proposed, Approved, Deprecated, etc + +## Consequences + +### Positive + +- Benefits to the change + +### Negative + +- Costs or downsides to the change + +### Alternatives Considered + +- List any other approaches considered during discussion + +## Related Documents + +- Links to related docs, previous ADRs, etc diff --git a/docs/adr/01-adr-msg-server-keeper.md b/docs/adr/01-adr-msg-server-keeper.md index 3d04e6343..c3039be4f 100644 --- a/docs/adr/01-adr-msg-server-keeper.md +++ b/docs/adr/01-adr-msg-server-keeper.md @@ -1,59 +1,141 @@ -# ADR1: Refactoring MsgServer and Keeper for Clear Separation of Concerns in Nibiru Chain +# ADR-001: Separation of Concerns between MsgServer and Keeper ## Introduction -In developing Nibiru Chain, built on Cosmos SDK, we have identified design and development practices that require optimization. This document proposes methodologies to differentiate between `MsgServer` and `Keeper` in the code and to improve our action-based testing framework. +In developing Nibiru Chain, built on Cosmos SDK, we have identified design and +development practices that require optimization. This document proposes +methodologies to differentiate between `MsgServer` and `Keeper` in the code and +to improve our action-based testing framework. + +## Changelog + +- 2022-06-01: Proposed in [NibiruChain/nibiru + #524](https://github.com/NibiruChain/nibiru/issues/524) by @testinginprod. +- 2023-12-28: Formal ADR drafted and accepted. + +## Context + +Merging MsgServer and Keeper goes against separation of responsibilities +principles in Cosmos SDK: + +- Obscures distinct functions each component should perform +- Complicates code maintenance due to intertwined responsibilities +- Can lead to security vulnerabilities without clear boundaries + +The `MsgServer` should focus on request validation while Keeper handles business logic, like a web app's controller and model respectively. + +## Decision + +This ADR proposes that we separate MsgServer (validation) from Keeper (business +logic). + +For example, Some `MsgServer` methods are restricted to the `x/sudo` group, +showing the need for distinct validation and execution. + +## Benefits +Significantly simplifies and improves our action-based testing framework: + +Removes need to prepare complex permission schemes +Reduces boilerplate code in tests +Allows focusing purely on business logic +Concerns +Exposing Keeper methods could enable unauthorized access. However, access control is the MsgServer’s responsibility. Keeper maintains state consistency. + +Best practice is keeping sensitive methods private, not building permission schemes in Keeper. This aligns with separation of responsibilities. + +Conclusion +Separation significantly improves code clarity, maintenance and security - aligning with industry best practices. + + ## Cosmos SDK: `MsgServer` and `Keeper` ### Issues with Combining `MsgServer` and `Keeper` -Merging `MsgServer` and `Keeper` in the Cosmos SDK context goes against the fundamental principle of separation of responsibilities. This blending results in: +Merging `MsgServer` and `Keeper` in the Cosmos SDK context goes against the +fundamental principle of separation of responsibilities. This blending results +in: -- **Role Confusion:** It obscures the distinct functions that each component should ideally perform. `MsgServer` should focus on request validation, including Permissions, while `Keeper` should handle business logic. -- **Maintenance Challenges:** This conflation complicates code maintenance, as intertwined responsibilities make it difficult to isolate and address issues effectively. -- **Security Implications:** The lack of clear boundaries between validation and execution can lead to security vulnerabilities, as ensuring that each layer only handles its intended tasks becomes challenging. +- **Role Confusion:** It obscures the distinct functions that each component + should ideally perform. `MsgServer` should focus on request validation, + including Permissions, while `Keeper` should handle business logic. +- **Maintenance Challenges:** This conflation complicates code maintenance, as + intertwined responsibilities make it difficult to isolate and address issues + effectively. +- **Security Implications:** The lack of clear boundaries between validation and + execution can lead to security vulnerabilities, as ensuring that each layer + only handles its intended tasks becomes challenging. ### Analogy with Web Applications -Compared with a web application, the `MsgServer` would act as a controller to validate requests, while the `Keeper` would be equivalent to business logic or the model. +Compared with a web application, the `MsgServer` would act as a controller to +validate requests, while the `Keeper` would be equivalent to business logic or +the model. ## Specific Case in Nibiru Chain -Some `MsgServer` methods in the Nibiru Chain are restricted to the `Sudo` group, underscoring the need for a clear separation in the validation and execution of requests. +Some `MsgServer` methods in the Nibiru Chain are restricted to the `Sudo` group, +underscoring the need for a clear separation in the validation and execution of +requests. ### Proposal for Functional Separation -We propose that the `MsgServer` solely handles validation while the `Keeper` manages the business logic post-validation. +We propose that the `MsgServer` solely handles validation while the `Keeper` +manages the business logic post-validation. ## Benefits of the Refactor in the Action-Based Testing Framework -With the proposed restructuring of `MsgServer` and `Keeper`, we gain significant benefits in our approach to testing, particularly within our action-based testing framework: +With the proposed restructuring of `MsgServer` and `Keeper`, we gain significant +benefits in our approach to testing, particularly within our action-based testing +framework: ### Simplification of Action-Based Tests -The clear separation of responsibilities between `MsgServer` and `Keeper` allows our tests to focus on creating atomic actions without setting up complex scenarios. This eliminates the need for: +The clear separation of responsibilities between `MsgServer` and `Keeper` allows +our tests to focus on creating atomic actions without setting up complex +scenarios. This eliminates the need for: -- **Preparing Users and Permissions:** There's no longer a requirement to create a user and add it to the `Sudo` group for each test scenario, greatly simplifying the test setup process. +- **Preparing Users and Permissions:** There's no longer a requirement to create + a user and add it to the `Sudo` group for each test scenario, greatly + simplifying the test setup process. -- **Reducing Boilerplate in Tests:** We minimize additional code required to establish preconditions not directly related to the test's objective. +- **Reducing Boilerplate in Tests:** We minimize additional code required to + establish preconditions not directly related to the test's objective. -- **Focus on Business Logic:** Tests can concentrate on assessing pure business logic, undistracted by security and permission configurations. +- **Focus on Business Logic:** Tests can concentrate on assessing pure business + logic, undistracted by security and permission configurations. ## Addressing Potential Concerns: Security and Accessibility of Keeper Methods ### Concerns About Security and Access Control -Some might argue that sharing Keeper's methods can lead to security risks, mainly if there are concerns about unauthorized access. This viewpoint stems from the belief that the `Keeper` should control access, which might lead to apprehensions about exposing specific methods. +Some might argue that sharing Keeper's methods can lead to security risks, mainly +if there are concerns about unauthorized access. This viewpoint stems from the +belief that the `Keeper` should control access, which might lead to apprehensions +about exposing specific methods. ### Clarifying the Role of the Keeper -However, this perspective needs to be revised in the fundamental role of the `Keeper`. The primary responsibility of the `Keeper` is to maintain a consistent state within the application rather than controlling access. Access control and validation of requests are the responsibilities of the `MsgServer`, which acts as the first line of defense. +However, this perspective needs to be revised in the fundamental role of the +`Keeper`. The primary responsibility of the `Keeper` is to maintain a consistent +state within the application rather than controlling access. Access control and +validation of requests are the responsibilities of the `MsgServer`, which acts as +the first line of defense. ### Best Practices in Method Exposure -Suppose there's a need to share the Keeper with other modules, and concerns arise about the safety of exposing specific methods. In that case, the preferred approach is to keep those sensitive methods private. Implementing access and permission layers within the `Keeper` goes against the principle of separation of responsibilities and can lead to a more cohesive and secure system. Instead, ensuring that only the appropriate methods are exposed and keeping others private aligns with the philosophy of keeping each component focused on its specific role. +Suppose there's a need to share the Keeper with other modules, and concerns arise +about the safety of exposing specific methods. In that case, the preferred +approach is to keep those sensitive methods private. Implementing access and +permission layers within the `Keeper` goes against the principle of separation of +responsibilities and can lead to a more cohesive and secure system. Instead, +ensuring that only the appropriate methods are exposed and keeping others private +aligns with the philosophy of keeping each component focused on its specific +role. ## Conclusion -Separating the `MsgServer` and `Keeper` in developing and testing the Nibiru Chain will significantly improve the code's clarity, maintenance, and security. These improvements reflect our commitment to efficient and robust development, aligned with the best industry practices. +Separating the `MsgServer` and `Keeper` in developing and testing the Nibiru +Chain will significantly improve the code's clarity, maintenance, and security. +These improvements reflect our commitment to efficient and robust development, +aligned with the best industry practices. diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 000000000..6068db20c --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,50 @@ +# nibiru/docs/adr + +ADRs help us address and track significant software design choices. + +- [Key Terms](#key-terms) +- [Why ADRs Help](#why-adrs-help) +- [Creating ADRs](#creating-adrs) +- [ADR Table of Contents](#adr-table-of-contents) + +## Key Terms + +| Term | Definition | +| --- | --- | +| Architectural Decision Record (ADR) | Captures a single Architectural Decision (AD), often written informally like personal notes or meeting minutes. The collection of ADRs make up a project's decision log. | +| Architectural Knowledge Management (AKM) | The practice of formally documenting architectural decisions to manage critical project knowledge over time. ADRs are a common AKM technique. | +| Architectural Decision (AD) | A software design choice that addresses a functional or non-functional requirement. | +| Architecturally Significant Requirement (ASR) | A requirement that has a large, measurable effect on a system's architecture quality and design. | +| Functional Requirement | Describes functionality that a system must provide to fulfill user needs. These define specific behaviors and outcomes that the system should have. For example calculating taxes on a purchase or sending a message. | +| Non-functional Requirement | Describes quality attributes and constraints on a system. Non-functional requirements do not specify behaviors - instead they constrain functional requirements by setting performance metrics, reliability targets, etc. For example - the system should process 95% of transactions in under 1 second. | + +## Why ADRs Help + +- Docs stay up-to-date: By capturing decisions, not ephemeral state, records have + lasting relevance even as systems evolve. +- Discoverability: ADRs can be tagged and linked to facilitate discovery when + working in related code. +- Onboard new developers: ADRs capture context and rationale to quickly ramp up + new team members. + +To maximize these benefits, Nibiru Chain ADRs will: + +- Contain a tldr summary section +- Cover background and decision details +- ADRs help transform documentation from a chore to an asset! Diligently recording decisions will improve understanding and pass knowledge to future maintainers. +- Use Markdown formatting for README integration +- Link bidirectionally with related code comments + +## Creating ADRs + +1. Start Here: [Nibiru Chain ADR Template](./00-adr-template.md): This provides a starting + template to capture architectural decisions. +2. Read a few of the other ADRs to gain a feel for them. + +Reference Materials: + +- [Request for Comments (RFC) Best Practices](https://datatracker.ietf.org/doc/html/rfc2119) + +## ADR Table of Contents + +- [ADR-001: Separation of Concerns between MsgServer and Keeper ](./01-adr-msg-server-keeper.md) From 54033754ba76353a7972fe1bb83cf1611270b575 Mon Sep 17 00:00:00 2001 From: Unique-Divine <realuniquedivine@gmail.com> Date: Thu, 28 Dec 2023 11:02:05 -0600 Subject: [PATCH 5/5] format --- docs/adr/01-adr-msg-server-keeper.md | 104 +++++++++------------------ 1 file changed, 35 insertions(+), 69 deletions(-) diff --git a/docs/adr/01-adr-msg-server-keeper.md b/docs/adr/01-adr-msg-server-keeper.md index c3039be4f..c45f51273 100644 --- a/docs/adr/01-adr-msg-server-keeper.md +++ b/docs/adr/01-adr-msg-server-keeper.md @@ -32,80 +32,41 @@ logic). For example, Some `MsgServer` methods are restricted to the `x/sudo` group, showing the need for distinct validation and execution. -## Benefits -Significantly simplifies and improves our action-based testing framework: - -Removes need to prepare complex permission schemes -Reduces boilerplate code in tests -Allows focusing purely on business logic -Concerns -Exposing Keeper methods could enable unauthorized access. However, access control is the MsgServer’s responsibility. Keeper maintains state consistency. - -Best practice is keeping sensitive methods private, not building permission schemes in Keeper. This aligns with separation of responsibilities. - -Conclusion -Separation significantly improves code clarity, maintenance and security - aligning with industry best practices. - - - -## Cosmos SDK: `MsgServer` and `Keeper` - -### Issues with Combining `MsgServer` and `Keeper` - -Merging `MsgServer` and `Keeper` in the Cosmos SDK context goes against the -fundamental principle of separation of responsibilities. This blending results -in: +The format should be the following: -- **Role Confusion:** It obscures the distinct functions that each component - should ideally perform. `MsgServer` should focus on request validation, - including Permissions, while `Keeper` should handle business logic. -- **Maintenance Challenges:** This conflation complicates code maintenance, as - intertwined responsibilities make it difficult to isolate and address issues - effectively. -- **Security Implications:** The lack of clear boundaries between validation and - execution can lead to security vulnerabilities, as ensuring that each layer - only handles its intended tasks becomes challenging. +```go +func NewMsgServerImpl(k Keeper) types.MsgServer { return msgServer{k} } -### Analogy with Web Applications +type msgServer struct { + k Keeper // NOTE: NO EMBEDDING +} -Compared with a web application, the `MsgServer` would act as a controller to -validate requests, while the `Keeper` would be equivalent to business logic or -the model. +func NewQueryServerImpl(k Keeper) types.QueryServer { return queryServer{k} } -## Specific Case in Nibiru Chain +type queryServer struct { + k Keeper // NOTE: NO EMBEDDING +} +``` -Some `MsgServer` methods in the Nibiru Chain are restricted to the `Sudo` group, -underscoring the need for a clear separation in the validation and execution of -requests. +Rules to follow: -### Proposal for Functional Separation +- When possible the msg/query server should contain no business logic (not always + possible due to pagination sometimes) +- Focused only at stateless request validation, and conversion from request to + arguments required for the keeper function call. +- No embedding because it always ends up with name conflicts. -We propose that the `MsgServer` solely handles validation while the `Keeper` -manages the business logic post-validation. +Keepers: -## Benefits of the Refactor in the Action-Based Testing Framework +- Must not have references to request formats, API layer should be totally split + from business logic layer. -With the proposed restructuring of `MsgServer` and `Keeper`, we gain significant -benefits in our approach to testing, particularly within our action-based testing -framework: - -### Simplification of Action-Based Tests - -The clear separation of responsibilities between `MsgServer` and `Keeper` allows -our tests to focus on creating atomic actions without setting up complex -scenarios. This eliminates the need for: - -- **Preparing Users and Permissions:** There's no longer a requirement to create - a user and add it to the `Sudo` group for each test scenario, greatly - simplifying the test setup process. - -- **Reducing Boilerplate in Tests:** We minimize additional code required to - establish preconditions not directly related to the test's objective. - -- **Focus on Business Logic:** Tests can concentrate on assessing pure business - logic, undistracted by security and permission configurations. +## Benefits -## Addressing Potential Concerns: Security and Accessibility of Keeper Methods +- Simplifies and improves our action-based testing framework: +- Removes need to prepare complex permission schemes +- Reduces boilerplate code in tests when using the keeper as a dependency for + another module by not requiring explicit "module-name/types" imports. ### Concerns About Security and Access Control @@ -122,7 +83,7 @@ state within the application rather than controlling access. Access control and validation of requests are the responsibilities of the `MsgServer`, which acts as the first line of defense. -### Best Practices in Method Exposure +### On Function Privacy Suppose there's a need to share the Keeper with other modules, and concerns arise about the safety of exposing specific methods. In that case, the preferred @@ -133,9 +94,14 @@ ensuring that only the appropriate methods are exposed and keeping others privat aligns with the philosophy of keeping each component focused on its specific role. +## Concerns + +Exposing Keeper methods could enable unauthorized access. However, access control +is the MsgServer’s responsibility. Keeper maintains state consistency. + +Best practice is keeping sensitive methods private, not building permission +schemes in Keeper. This aligns with separation of responsibilities. + ## Conclusion -Separating the `MsgServer` and `Keeper` in developing and testing the Nibiru -Chain will significantly improve the code's clarity, maintenance, and security. -These improvements reflect our commitment to efficient and robust development, -aligned with the best industry practices. +Improves code clarity, maintainability, and security.