Fix: Add node flag to WASM queries (build-address) #2199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
8 tasks
nullpointer0x00
approved these changes
Oct 24, 2024
![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)
.changelog/unreleased/bug-fixes/2199-fix-wasm-build-addr.md (1)
1-1: Enhance the changelog entry to be more specific and informative.While the current entry correctly documents the change, it could be more helpful by specifically mentioning the
--nodeflag and the problem it solves. Consider expanding it to:-* Add the query flags to the query wasm build-addr command [PR 2199](https://github.com/provenance-io/provenance/pull/2199). +* Fix the `query wasm build-addr` command to properly support the `--node` flag, allowing users to specify custom node endpoints when executing Wasm queries [PR 2199](https://github.com/provenance-io/provenance/pull/2199).This provides more context about:
- The specific flag that was fixed (
--node)- The practical benefit (ability to specify custom node endpoints)
- The command's use case (executing Wasm queries)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .changelog/unreleased/bug-fixes/2199-fix-wasm-build-addr.md (1 hunks)
- 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 (3)
cmd/provenanced/cmd/root.go (3)
135-135: LGTM!The placement of the
fixQueryWasmBuildAddressFlagscall is appropriate, grouped with other command fixes and executed after all commands are initialized.
540-551: LGTM! Clean and focused implementation.The implementation correctly adds query flags to the
build-addresscommand using the SDK'sflags.AddQueryFlagsToCmd. The error handling is appropriate, and the code follows the established patterns in the codebase.
540-551: Verify that this resolves the node flag issue.Let's verify that this change resolves the specific issue mentioned in the PR objectives where the following command was failing:
provenanced query wasm build-address <code-hash> <creator-address> <salt> --node https://rpc.test.provenance.io:443
SpicyLemon
added a commit
that referenced
this pull request
Oct 24, 2024
scirner22
approved these changes
Oct 24, 2024
SpicyLemon
added a commit
that referenced
this pull request
Oct 24, 2024
…tion events), #2196 (timeout_commit), #2197 (recordspec cmd), #2198 (ParameterChangeProposal) #2199 (wasm build-address cmd). (#2200) * Suppress scope value owner migration events. (#2195) * Create a no-op event manager and use that during the metadata module migration. * Do not suppress the events for a testnet upgrade since they were emitted when the migration ran on testnet. * Add changelog entry. * Update all the spec proto links to reference v1.20.0 (#2192) * Update all the spec proto links to reference v1.20.0 (instead of 1.19.0). * Add changelog entry. * When prepping a release, combine the dependency bump changelog entries. (#2181) * Add a note to get-dep-changes to alert folks that changing those formats might break other things. * Create an awk script that will combine dependency changelog entries. Update prep-release to use it. Also apply a couple fixes that are alread in the release branch (and will be in main shortly). Also tweak the step 4 and 5 names to provide more context, and fix the verbose output header when recombining the sections. * Add changelog entry. * Clarify the new comment in get-dep-changes.sh. * Update stuff that uses or talks about RELEASE_NOTES.md because it should actually be RELEASE_CHANGELOG.md. The SDK uses _NOTES but only puts a blurb in there, so it's not a changelog. But we include a changelog, so it makes sense to keep it named that way. * Fix the `query metadata recordspec` command when given a rec-spec-id. (#2197) * [2148]: Fix the query metadata recordspec command to correctly use the RecordSpecification query (instead of RecordSpecificationsForContractSpecification) when provided a record specification id. * [2148]: Add changelog entry. * Fix decoding of gov props with a ParameterChangeProposal in them. (#2198) * Write a unit test that fails to parse a gov proposal with a ParameterChangeProposal in it because that type isn't being registered anymore. * Register the params module stuff with the codecs since there's some gov props with a ParameterChangeProposal in them. * Add changelog entry. * Hard-code consensus.timeout_commit to 3.5s for mainnet. (#2196) * [2121]: Change the default consensus timeout value to 3.5 seconds. * [2121]: Hard-code the consensus.timeout_commit value. * [2121]: Fix TestIsTestnetFlagSet to not be affected by existing env vars. * [2121]: Fix a couple unit tests that broke when I changed the default commit timout. * [2121]: Only hard-code the timeout commit on non-testnets. * [2121]: Change the default back to 1.5s for faster default testnets. * [2121]: Fix the TestPreUpgradeCmd that broke because of the hard-coded timeout commit. * [2121]: Add some unit tests that make sure the consensus timeout commit value is behaving as expected. * [2121]: Add changelog entry. * [2121]: When forcing the timeout_commit to be 3.5 seconds, also force the skip flag to be false. * [2121]: Update warnAboutSettings: Evaluate the timeout commit and skip-timeout-commit fields separately. Issue a warning if skip-timeout-commit is true. Issue a warning if the timeout commit is not exactly what we want it to be. * Fix: Add node flag to WASM queries (build-address) (#2199)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a recreation of #2194 by GunanshuJoshi, but with signed commits.
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.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
query wasm build-addrcommand, enhancing its functionality and output customization.Bug Fixes