-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix(client/v2): improve message parsing by replacing proto.Merge with cloneMessage #24722
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
base: main
Are you sure you want to change the base?
Conversation
Ironbird - launch a networkTo use Ironbird, you can use the following commands:
Custom Load Test ConfigurationYou can provide a custom load test configuration using the `--load-test-config=` flag:
Use |
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Msg as Source Message
participant Clone as cloneMessage
participant Dst as Destination Message
CLI->>Clone: cloneMessage(dst, src)
loop For each field in src
Clone->>Clone: If field is message, recursively clone
Clone->>Dst: Set field value
end
Clone-->>CLI: Destination message cloned
Assessment against linked issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/v2/CHANGELOG.md (1)
37-39
: Fix grammatical error in changelog entryThere's a small grammatical error in the changelog entry.
-* [#24722](https://github.com/cosmos/cosmos-sdk/pull/24722) Fix msg parsing in when no pulsar file is present. +* [#24722](https://github.com/cosmos/cosmos-sdk/pull/24722) Fix msg parsing when no pulsar file is present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/v2/CHANGELOG.md
(1 hunks)client/v2/autocli/msg.go
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: main
- GitHub Check: Summary
🔇 Additional comments (3)
client/v2/autocli/msg.go (3)
163-163
: Appropriate replacement for proto.MergeThe switch from
proto.Merge
tocloneMessage
addresses the root issue with field descriptors from different repositories.
219-219
: Consistent application of message cloning approachGood consistency by applying the same cloning approach in the governance proposal flow.
227-282
: Well-implemented manual message cloning functionThe custom
cloneMessage
implementation thoroughly handles all potential field types (primitive fields, nested messages, lists, and maps) through explicit recursive copying.The panic on line 236 is appropriate as it will help catch development errors early by ensuring field compatibility between messages.
Let's verify that this implementation covers all scenarios by checking other usages of proto.Merge in the codebase:
#!/bin/bash # Check for other occurrences of proto.Merge in the codebase rg "proto\.Merge" --type go
|
||
// cloneMessage safely copies fields from src message to dst message. | ||
// this avoids the proto.Merge issue with field descriptors from different repositories. | ||
func cloneMessage(dst, src protoreflect.Message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to test this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll add a test 👍🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test: 6bdfb65.
It fails before with that error, passes after the fix.
@@ -1,13 +1,13 @@ | |||
syntax = "proto3"; | |||
|
|||
package testpb; | |||
package testpbpulsar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed, because test was failing (on ci only) for double registration (https://github.com/cosmos/cosmos-sdk/actions/runs/15034823230/job/42254518136 /
service Msg { |
@julienrbrt this is still failing |
fixed, was due because of the test proto renaming.. |
https://github.com/cosmos/cosmos-sdk/actions/runs/15051726043/job/42307904781?pr=24722 |
ffs, there was one more: 7ca9340. it passes now. |
LGTM with @technicallyty approval |
Awesome, a new client/v2 release with this fix is the last thing needed in order to be able to release Ignite CLI v29. |
Description
Closes: ignite/cli#4623
@Pantani found an issue when using autocli but no pulsar module was present: ignite/cli#4623
proto.Merge
fails when not present in modules (based on this SDK promise, Ignite v29 removed the pulsar file from its default template)We've checked the other usage of proto.Merge but they shouldn't matter.
cc @aaronc
Author 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. Your PR will not be merged unless you satisfy
all of these items.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Summary by CodeRabbit