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/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 new file mode 100644 index 000000000..c45f51273 --- /dev/null +++ b/docs/adr/01-adr-msg-server-keeper.md @@ -0,0 +1,107 @@ +# 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. + +## 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. + +The format should be the following: + +```go +func NewMsgServerImpl(k Keeper) types.MsgServer { return msgServer{k} } + +type msgServer struct { + k Keeper // NOTE: NO EMBEDDING +} + +func NewQueryServerImpl(k Keeper) types.QueryServer { return queryServer{k} } + +type queryServer struct { + k Keeper // NOTE: NO EMBEDDING +} +``` + +Rules to follow: + +- 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. + +Keepers: + +- Must not have references to request formats, API layer should be totally split + from business logic layer. + +## Benefits + +- 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 + +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. + +### 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 +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. + +## 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 + +Improves code clarity, maintainability, and security. 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)