docs: add adr-1 proposal for msg server and keeper consistency. #1751
Conversation
WalkthroughThe proposed changes aim to refactor the Changes
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- documentation/adrs/adr-1.md (1 hunks)
Additional comments: 1
documentation/adrs/adr-1.md (1)
- 1-45: The document provides a clear and structured explanation of the proposed changes, outlining the issues, analogies, specific cases, and benefits of the refactor. The markdown syntax appears to be correctly used, and the document is well-formatted. It is important to ensure that the document is accessible to all stakeholders and that the language is clear and concise.
There was a problem hiding this comment.
One thing to note here is that x/sudo checks are actually stateful validation, which would still go in the Keeper. They're not a msg validation for the MsgServer.
The ADR should probably propose a format for that or advocate for some convention like the original proposal from Frojdi
|
LGTM, just want everyone here to pinky promise that we will never do a binding from msg to keeper ever again. Any binding (like wasm for example) needs to go to msg server and not touch the keeper, otherwise we can lose the msg validation from the msg server. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- .gitignore (1 hunks)
- docs/adr/00-adr-template.md (1 hunks)
- docs/adr/01-adr-msg-server-keeper.md (1 hunks)
- docs/adr/README.md (1 hunks)
Files skipped from review due to trivial changes (2)
- docs/adr/00-adr-template.md
- docs/adr/README.md
Additional comments: 6
.gitignore (1)
- 14-14: The update to
.gitignoreto excludedocs/staticseems appropriate. Ensure that no necessary files withindocs/staticare currently being tracked by Git, as they will need to be removed manually.Verification successful
The script was intended to check if any files within
docs/staticare currently being tracked by Git. The absence of output suggests that there are no files being tracked indocs/static, or the command did not work as intended.To ensure accuracy, I will modify the script to list all files in
docs/staticthat are tracked by Git without using the--error-unmatchoption, which may have suppressed the output.
The absence of output from the script indicates that there are no files currently being tracked by Git within the
docs/staticdirectory. This aligns with the change made to the.gitignorefile, which now excludesdocs/staticfrom version control.Based on this information, it can be concluded that the update to
.gitignoreis consistent with the state of the repository, and no further action is required to remove files from tracking.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List files in docs/static that are currently tracked by Git git ls-files --error-unmatch docs/static/* > /dev/null 2>&1 && echo "Some files in docs/static are still being tracked by Git."Length of output: 46
Script:
#!/bin/bash # List all files in docs/static that are currently tracked by Git git ls-files docs/static/*Length of output: 30
docs/adr/01-adr-msg-server-keeper.md (5)
1-107: The ADR document is well-structured and follows the standard format for architectural decision records, providing clear context and rationale for the proposed changes.
12-13: Verify the existence and status of the referenced GitHub issue to ensure it is relevant to the ADR.
Verification successful
The shell script output confirms the existence and status of the GitHub issue #524 in the NibiruChain/nibiru repository. The issue is closed and was authored by
testinginprod, which matches the reference in the ADR document. The issue is labeled as a refactor and is part of the Testnet V1 milestone. The content of the issue also matches the changes proposed in the ADR, specifically the introduction ofNewMsgServerImplandNewQueryServerImplwithout embedding theKeeperstruct.Based on this information, the reference to the GitHub issue in the ADR document is accurate and relevant.
* 37-57: The proposed code changes and best practices for separating `MsgServer` and `Keeper` are technically sound and align with the principles of clean architecture.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the referenced GitHub issue gh issue view 524 --repo NibiruChain/nibiruLength of output: 1128
71-100: The clarification on the role of the
KeeperandMsgServerregarding security and access control is well-articulated and addresses potential concerns effectively.105-107: The conclusion effectively summarizes the benefits of the proposed architectural changes, highlighting improvements in code clarity, maintainability, and security.
Purpose / Abstract
Summary by CodeRabbit
Summary by CodeRabbit
MsgServerandKeeperin the Nibiru Chain.