refactor: improvements on rest v2 server #22292
Conversation
hieuvubk
requested review from
auricom,
julienrbrt and
tac0turtle
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Changes
Possibly related PRs
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
|
![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 (1)
scripts/init-simapp-v2.sh (1)
12-12: LGTM! Consider adding a comment for clarity.The change from
api.enabletorest-v2.enablecorrectly reflects the update to enable the REST v2 interface, which aligns with the PR objectives and the changes in theserverv2/api/restmodule.Consider adding a comment above this line to explain the purpose of enabling the REST v2 interface, for example:
+# Enable the REST v2 interface $SIMD_BIN config set app rest-v2.enable trueThis would improve the script's readability and make it easier for future maintainers to understand the purpose of this configuration.
scripts/init-simapp-v2.sh
Outdated
| @@ -9,7 +9,7 @@ if [ -d "$SIMD_HOME" ]; then rm -rv $SIMD_HOME; fi | |||
| $SIMD_BIN config set client chain-id simapp-v2-chain | |||
| $SIMD_BIN config set client keyring-backend test | |||
| $SIMD_BIN config set client keyring-default-keyname alice | |||
| $SIMD_BIN config set app api.enable true | |||
| $SIMD_BIN config set app rest-v2.enable true | |||
There was a problem hiding this comment.
can we as well rename the server to simply rest? I don't think we should have v2 in there.
github-actions
bot
added
C:server/v2
|
Could you add as well the app.toml content to confix (v2-app.toml)? |
scripts/init-simapp-v2.sh
Outdated
| @@ -1,6 +1,14 @@ | |||
| #!/usr/bin/env bash | |||
| set -o errexit | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/confix/data/v2-app.toml (1)
44-48: LGTM! Consider adding a comment about production deployment.The addition of the
[rest]section withenableandaddressparameters is appropriate and aligns with the PR objectives.Consider adding a comment above the
addressparameter to remind users to adjust this value for production deployments:[rest] # Enable defines if the REST server should be enabled. enable = true +# Address defines the REST server address to bind to. +# Note: For production deployments, consider changing this to a more appropriate address. address = 'localhost:8080'
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/init-simapp-v2.sh (1 hunks)
- tools/confix/data/v2-app.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/init-simapp-v2.sh
🧰 Additional context used
🔇 Additional comments (1)
tools/confix/data/v2-app.toml (1)
44-48: Clarification needed: Additionalapp.tomlcontentIn response to the PR comment requesting inclusion of
app.tomlcontent in this file:
- Could you please specify which additional content from
app.tomlneeds to be included here?- Are there any other REST-related configurations in
app.tomlthat should be transferred to this file?This information will help ensure that all necessary configurations are properly included and aligned between the two files.
To assist in identifying relevant content, you can run the following command to search for REST-related configurations in the
app.tomlfile:Once we have clarity on the required content, I can help draft the necessary additions to this file.
julienrbrt
changed the title
init-simapp-v2.sh
|
adding backport 0.52 label solely for confix changes |
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 20dd65b) # Conflicts: # server/v2/api/rest/server.go
Description
Closes: #XXXX
api key in app.toml change since
serverv2/api/restwas mergedAuthor 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
Bug Fixes