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: add adr-1 proposal for msg server and keeper consistency. #1751

Merged
merged 9 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ data
build
*_old
mockdata/
docs
docs/static
dist
coverage.txt
temp
64 changes: 64 additions & 0 deletions docs/adr/00-adr-template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# ADR-XXX: <Title>

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
107 changes: 107 additions & 0 deletions docs/adr/01-adr-msg-server-keeper.md
Original file line number Diff line number Diff line change
@@ -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.
50 changes: 50 additions & 0 deletions docs/adr/README.md
Original file line number Diff line number Diff line change
@@ -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)
Loading