Fix: Add node flag to WASM queries (build-address) #2194
Conversation
WalkthroughThe changes in this pull request focus on enhancing the command-line interface of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cmd/provenanced/cmd/root.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
cmd/provenanced/cmd/root.go (1)
225-225: Verify Proper Handling of the--nodeFlag in Query SubcommandsEnsure that all query subcommands correctly utilize the
--nodeflag to connect to the specified node endpoint. This includes verifying that the custom node address is properly passed to any underlying clients or services used by these subcommands.Run the following script to find usages of the
client.GetNodeContextfunction in query commands, which should respect the--nodeflag:
|
The |
|
Yes, it doesn't actually have any dependency on the node but it does tries to connect with the node. Thus, giving the following error: Error: post failed: Post "http://localhost:26657": dial tcp 127.0.0.1:26657: connect: connection refusedWe can also consider removing the validation for this. |
Sorry. You're right. I was looking at an earlier version of the command, but it looks like the current version does make a request of the node. |
|
This PR will need a changelog entry. That'll create a file in |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2194 +/- ##
==========================================
- Coverage 74.50% 74.24% -0.27%
==========================================
Files 318 373 +55
Lines 46237 45204 -1033
==========================================
- Hits 34449 33560 -889
+ Misses 10381 10187 -194
- Partials 1407 1457 +50
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/provenanced/cmd/root.go (1)
535-546: Add a changelog entry for this bug fix.As mentioned in the PR comments, please add a changelog entry using:
.changelog/add-change.sh 2194 bug-fixes fix-wasm-build-addr 'Add the query flags to the query wasm build-addr command.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cmd/provenanced/cmd/root.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/provenanced/cmd/root.go (1)
Learnt from: iramiller PR: provenance-io/provenance#2194 File: cmd/provenanced/cmd/root.go:225-225 Timestamp: 2024-10-21T17:12:02.352Z Learning: When adding flags in `cmd/provenanced/cmd/root.go`, use `flags.AddTxFlagsToCmd(cmd)` for each command to add the appropriate flags, instead of individually adding flags to the root command.
🔇 Additional comments (2)
cmd/provenanced/cmd/root.go (2)
136-136: LGTM! Correct placement of the fix function call.The
fixQueryWasmBuildAddressFlagsfunction is called right afterfixTxWasmInstantiate2Aliases, which is the appropriate location as suggested in the past review comments. This ensures that the fix is applied after all module commands have been added.
535-546: LGTM! Well-implemented fix for the node flag issue.The implementation correctly:
- Follows the same pattern as other fix functions in the file
- Uses
flags.AddQueryFlagsToCmd(cmd)as recommended- Handles the case where the command might not exist
- Includes clear documentation explaining the purpose
The function resolves the issue where the Wasm build-address query was attempting to connect to a node without having the proper flags configured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changelog/unreleased/bug-fixes/2194-fix-wasm-build-addr.md (1)
1-1: Enhance the changelog entry with more specific details.The current changelog entry is too vague. Consider providing more specific details about the
--nodeflag and the issue it resolves.Here's a suggested revision:
-* Add the query flags to the query wasm build-addr command [PR 2194](https://github.com/provenance-io/provenance/pull/2194). +* Add the `--node` flag to the `query wasm build-addr` command to allow specifying custom node endpoints, fixing connection errors when a local node is not available [PR 2194](https://github.com/provenance-io/provenance/pull/2194).
|
Hi @SpicyLemon @iramiller |
Looks good! Thanks for the fix! |
|
Oh darn. Sorry. We require all commits to be signed, so this can't be merged as it is. I've recreated it as #2199 (with signed commits), and I'll close this one. Here's more info on signing commits: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits Thanks again for the fix! |
Pull request was closed
Description
This PR adds a new
--nodeflag to support custom endpoints for node connections while making Wasm queries specificallybuild-address. This update ensures that theprovenanced q wasm build-addresscommand works correctly with custom node configurations.Issue:
While creating the predictable contract address I encountered the following error:
This occurred when running the following command:
It seems the flag is not being recognized properly. I suspect there might be an issue with either the flag registration in the command definition (root.go) or how the CLI parser processes the command.
The main file updated is:
cmd/provenanced/cmd/root.gocloses:
n/aI haven't created any particular issue for this change. However, this was discussed on Discord, where it was agreed that the node flag should function correctly.
Reference: Discord conversation.
Checklist
(I haven't created any issue for this minor fix.)
docs/) or specification (x/<module>/spec/)..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Summary by CodeRabbit
New Features
query wasm build-addrcommand with additional query flags for improved usability.Improvements
startcommand upon error, preventing misleading usage information.Miscellaneous Adjustments