feat(client/v2): broadcast logic #22282
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to 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
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
client/v2/internal/broadcast/broadcaster_test.go (2)
10-26: LGTM: Well-structured table-driven test.The test function and test case structure are well-designed. Consider adding a comment describing the purpose of the
Test_newBroadcasterfunction for better documentation.You could add a comment like this before the function:
// Test_newBroadcaster verifies that the newBroadcaster function correctly creates // and configures broadcasters based on different consensus types and options. func Test_newBroadcaster(t *testing.T) { // ... (existing code) }
27-39: LGTM: Well-implemented test execution loop.The test execution loop is well-structured and covers both error and success cases. Consider adding a check for the broadcasting mode in non-error cases to ensure it's set correctly.
You could add a check like this after line 35:
if cometBroadcaster, ok := got.(*CometBftBroadcaster); ok { require.Equal(t, tt.opts[0].(func(*options)), cometBroadcaster.mode) }This assumes that the first option is always the mode setter. Adjust as necessary based on your actual implementation.
client/v2/internal/broadcast/comet_test.go (3)
21-51: Consider expanding test cases forTestNewCometBftBroadcaster.While the current test case is a good start, consider adding more test cases to cover:
- Error scenarios (e.g., invalid options)
- Different combinations of options
- Edge cases (e.g., nil codec)
This will ensure more comprehensive testing of the
NewCometBftBroadcasterfunction.
53-90: Enhance test coverage forTestCometBftBroadcaster_Broadcast.To improve the test coverage:
- Add test cases for other broadcast modes (e.g., async, commit).
- Include error scenarios (e.g., RPC client returning an error).
- Test with different input payloads.
- Verify the returned
TxResponsefields more thoroughly.These additions will provide a more comprehensive test suite for the
Broadcastmethod.
92-126: LGTM: Comprehensive error checking inTest_checkCometError.The test function effectively covers various error scenarios and their corresponding response codes. The use of table-driven tests is commendable.
Minor suggestion: Consider adding a test case for an unknown error to ensure the function handles unexpected errors gracefully.
tests/go.mod (1)
Line range hint
232-268: Carefully manage replace directives for local developmentThe
go.modfile contains numerous replace directives, many of which point to local paths. While this is common for local development in a monorepo setup, please consider the following:
- Ensure that these replace directives are necessary for the test environment and don't accidentally override important versions in production builds.
- Document the purpose of these replace directives, especially for newcomers to the project who might not be familiar with the local development setup.
- Consider implementing a mechanism to automatically update or remove these replace directives when creating release builds to ensure reproducibility.
- Regularly review and clean up any outdated or unnecessary replace directives to keep the
go.modfile maintainable.Consider adding a comment at the beginning of the replace section explaining its purpose and any necessary steps for contributors working with this setup.
client/v2/internal/broadcast/broadcaster.go (1)
46-46: Consider utilizing the 'context' parameter or removing it if unnecessaryThe
createmethod currently includes acontext.Contextparameter named_, which is unused. If the context isn't needed, consider removing it to simplify the function signature. If you plan to use it later, you might want to rename it from_toctxfor clarity.If removing the context parameter:
-func (f broadcasterFactory) create(_ context.Context, consensus, url string, opts ...Option) (Broadcaster, error) { +func (f broadcasterFactory) create(consensus, url string, opts ...Option) (Broadcaster, error) {If keeping it for future use:
-func (f broadcasterFactory) create(_ context.Context, consensus, url string, opts ...Option) (Broadcaster, error) { +func (f broadcasterFactory) create(ctx context.Context, consensus, url string, opts ...Option) (Broadcaster, error) {client/v2/internal/broadcast/comet.go (2)
76-76: Return nil slices when returning errorsWhen returning an error, it's idiomatic in Go to return
nilslices instead of empty slices. This clearly indicates that no valid data is being returned.Update the return statements:
- return []byte{}, fmt.Errorf("unknown broadcast mode: %s", c.mode) + return nil, fmt.Errorf("unknown broadcast mode: %s", c.mode) - return []byte{}, err + return nil, errAlso applies to: 81-81
156-159: Avoid unnecessary named return valuesThe function
parseABCILogsuses named return values, which are unnecessary in this context and can reduce code clarity.Simplify the function signature:
- func parseABCILogs(logs string) (res []*apiacbci.ABCIMessageLog, err error) { + func parseABCILogs(logs string) ([]*apiacbci.ABCIMessageLog, error) {Adjust the return statement accordingly:
return res, err
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
client/v2/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
- client/v2/go.mod (4 hunks)
- client/v2/internal/broadcast/broadcaster.go (1 hunks)
- client/v2/internal/broadcast/broadcaster_test.go (1 hunks)
- client/v2/internal/broadcast/comet.go (1 hunks)
- client/v2/internal/broadcast/comet_test.go (1 hunks)
- client/v2/internal/broadcast/testutil/comet_mock.go (1 hunks)
- simapp/go.mod (1 hunks)
- simapp/v2/go.mod (1 hunks)
- tests/go.mod (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
client/v2/internal/broadcast/broadcaster.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/internal/broadcast/broadcaster_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/v2/internal/broadcast/comet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/internal/broadcast/comet_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/v2/internal/broadcast/testutil/comet_mock.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 GitHub Check: CodeQL
client/v2/internal/broadcast/testutil/comet_mock.go
[notice] 14-14: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
🔇 Additional comments (12)
client/v2/internal/broadcast/broadcaster_test.go (1)
1-8: LGTM: Package declaration and imports are correct.The package name is appropriate, and the imports are relevant and correctly ordered.
client/v2/internal/broadcast/comet_test.go (2)
3-17: LGTM: Import statements are well-organized.The import statements are correctly grouped and organized, including both standard library and third-party packages. The use of aliasing for some imports (e.g.,
apiacbci,mockrpc) improves readability.
1-126: Overall assessment: Good foundation with room for improvement.The
comet_test.gofile provides a solid foundation for testing theCometBftBroadcaster. The tests are well-structured and follow Go testing conventions. However, there's room for improvement in test coverage, particularly for edge cases and error scenarios.Consider implementing the suggested enhancements to create a more robust and comprehensive test suite. This will increase confidence in the correctness and reliability of the
CometBftBroadcasterimplementation.client/v2/go.mod (4)
32-32: Approve the update ofcosmossdk.io/schemaand monitor for stable releases.The
cosmossdk.io/schemadependency has been updated to a more recent pre-release version. This update likely includes bug fixes or new features that benefit the project.To stay informed about new releases and potentially upgrade to a stable version in the future, you can periodically check for updates:
#!/bin/bash go list -m -versions cosmossdk.io/schemaThis will show all available versions, helping to identify when a stable release becomes available.
52-52: Verify the direct usage ofgithub.com/cometbft/cometbftand its implications.The
github.com/cometbft/cometbftdependency is now directly used in this module, as indicated by the removal of the// indirectcomment. This change suggests a more direct integration with CometBFT functionality.To understand the extent of this change and its implications, you can run:
#!/bin/bash # Search for direct usage of cometbft package grep -R "github.com/cometbft/cometbft" --include="*.go" .This will help identify where and how CometBFT is being used directly in the codebase. Consider reviewing these occurrences to ensure they align with the module's architecture and design goals.
180-185: Approve the addition of replace directives and verify local paths.The new replace directives for
cosmossdk.io/server/v2,cosmossdk.io/server/v2/cometbft,cosmossdk.io/store/v2, andcosmossdk.io/x/consensusare consistent with a monorepo structure or local development setup. This approach facilitates easier development and testing of interdependent modules.To ensure these local paths are correct and the modules exist, you can run:
#!/bin/bash # Check if the local paths exist for path in ./../../server/v2 ./../../server/v2/cometbft ./../../store/v2 ./../../x/consensus; do if [ -d "$path" ]; then echo "$path exists" else echo "$path does not exist" fi doneThis script will verify the existence of the local paths specified in the replace directives. If any path is missing, it may indicate a potential issue with the project structure or an outdated replace directive.
17-17: Approve the addition ofgo.uber.org/mockand verify its usage.The addition of
go.uber.org/mock v0.4.0is a positive step towards improving the testing infrastructure. This mocking framework can enhance the quality and reliability of unit tests.To ensure this dependency is being utilized effectively, you can run:
This will help verify that the mocking framework is being used in the codebase and identify areas where it could be further leveraged.
tests/go.mod (3)
Line range hint
1-268: Overall assessment of go.mod changesThe changes to the
tests/go.modfile align with the PR objectives of introducing new broadcast logic and expanding the test suite. The updates include:
- Updating existing dependencies, particularly
cosmossdk.io/schema.- Adding new dependencies related to testing and Cosmos SDK modules.
- Maintaining a set of replace directives for local development.
These changes suggest a significant expansion or improvement of the test suite, which is generally positive. However, to ensure the stability and maintainability of the project, please:
- Verify that all new dependencies are necessary and don't introduce conflicts.
- Ensure that the updated
cosmossdk.io/schemadependency is compatible with the existing codebase.- Carefully manage the replace directives to maintain reproducibility between development and production environments.
- Consider adding documentation about the purpose and management of the replace directives.
After addressing these points, the changes to the
go.modfile appear to be appropriate for the PR objectives.
68-68: Verify compatibility with updated cosmossdk.io/schema dependencyThe
cosmossdk.io/schemadependency has been updated to versionv0.3.1-0.20241010135032-192601639cac. Please ensure that this update doesn't introduce any breaking changes that could affect the tests. It's recommended to review the changelog of this dependency and run the test suite to confirm compatibility.#!/bin/bash # Description: Check for any breaking changes in the updated cosmossdk.io/schema dependency # Test: Search for any usage of cosmossdk.io/schema in the test files rg --type go 'cosmossdk\.io/schema' tests/
Line range hint
28-41: Verify necessity and impact of new dependenciesSeveral new dependencies have been added to the
go.modfile, including testing libraries and Cosmos SDK modules. While expanding the test suite is generally positive, please ensure that:
- Each new dependency is necessary and serves a specific purpose in the tests.
- There are no version conflicts between the new and existing dependencies.
- The addition of these dependencies doesn't significantly increase the build time or complexity of the test suite.
It's recommended to document the purpose of each new dependency, especially for the newly added Cosmos SDK modules, to maintain clarity for future contributors.
✅ Verification successful
Dependencies Verified Successfully
All newly added dependencies in
tests/go.modare actively used in the test suite, ensuring their necessity and proper integration. No conflicts or unnecessary complexities were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of newly added dependencies in test files # Test: Search for usage of new dependencies in test files for dep in "cosmossdk.io/core/testing" "cosmossdk.io/x/accounts" "cosmossdk.io/x/bank" "cosmossdk.io/x/gov" "github.com/cometbft/cometbft/api" "github.com/google/go-cmp" "github.com/google/gofuzz" "github.com/jhump/protoreflect" "github.com/rs/zerolog" "github.com/spf13/viper"; do echo "Checking usage of $dep" rg --type go "$dep" tests/ doneLength of output: 138
Script:
#!/bin/bash # Description: Check for usage of newly added dependencies in test files # Test: Search for usage of new dependencies in test files for dep in "cosmossdk.io/core/testing" "cosmossdk.io/x/accounts" "cosmossdk.io/x/bank" "cosmossdk.io/x/gov" "github.com/cometbft/cometbft/api" "github.com/google/go-cmp" "github.com/google/gofuzz" "github.com/jhump/protoreflect" "github.com/rs/zerolog" "github.com/spf13/viper"; do echo "Checking usage of $dep" rg --type go "$dep" tests/ doneLength of output: 20189
simapp/go.mod (1)
65-65: Verify compatibility with updatedcosmossdk.io/schemadependencyThe
cosmossdk.io/schemadependency has been updated to a newer version. While this update likely includes improvements or bug fixes, it's important to ensure compatibility with the rest of the module and its dependencies.
- Please verify that this update doesn't introduce any breaking changes that could affect the simapp module.
- Review the changelog or commit history of the
cosmossdk.io/schemarepository for any significant changes between September 30, 2024, and October 10, 2024.- Consider running the test suite to ensure that the update doesn't cause any regressions.
To help verify the impact of this change, you can run the following script:
simapp/v2/go.mod (1)
14-14: Dependency update looks good, verify compatibility.The update of
cosmossdk.io/server/v2/cometbftto versionv0.0.0-20241015140036-ee3d320eaa55appears to be in line with the PR objectives. This change is likely related to the new broadcast logic being introduced.To ensure compatibility, please run the following commands:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
client/v2/internal/broadcast/broadcaster_test.go (1)
57-83: LGTM: Good test coverage forFactory.Register.The
TestFactory_Registerfunction provides a good test case for registering and creating a custom broadcaster. The implementation is correct and follows Go testing conventions.Consider adding an additional test case to verify the behavior when attempting to register a broadcaster with an already existing consensus type. This would ensure that the
Registermethod handles potential conflicts correctly.client/v2/CHANGELOG.md (1)
47-47: LGTM! Consider adding more detail to the feature description.The new changelog entry for custom broadcast logic is correctly formatted and placed in the appropriate section. Well done!
To improve clarity, consider expanding the description slightly. For example:
* [#22282](https://github.com/cosmos/cosmos-sdk/pull/22282) Added custom broadcast logic to enhance transaction broadcasting flexibility.client/v2/internal/broadcast/comet.go (2)
55-55: Rename struct fieldcdcto improve clarityThe field
cdcin theCometBFTBroadcasterstruct is not immediately descriptive. Renaming it tojsonCodecenhances readability and aligns with Go naming conventions.Apply this diff to rename the field:
type CometBFTBroadcaster struct { rpcClient CometRPC mode string - cdc codec.JSONCodec + jsonCodec codec.JSONCodec }Ensure all references to
c.cdcare updated toc.jsonCodecthroughout the codebase.
60-64: Avoid unnecessary abbreviations in variable namesIn the functions
withModeandwithJSONCodec, the variablecbcis used to represent aCometBFTBroadcaster. Using a more descriptive name likebroadcasterimproves code readability.Update the variable names as shown:
For
withMode:func withMode(mode string) func(broadcaster Broadcaster) { return func(b Broadcaster) { - cbc, ok := b.(*CometBFTBroadcaster) + broadcaster, ok := b.(*CometBFTBroadcaster) if !ok { return } - cbc.mode = mode + broadcaster.mode = mode } }For
withJSONCodec:func withJSONCodec(codec codec.JSONCodec) func(broadcaster Broadcaster) { return func(b Broadcaster) { - cbc, ok := b.(*CometBFTBroadcaster) + broadcaster, ok := b.(*CometBFTBroadcaster) if !ok { return } - cbc.jsonCodec = codec + broadcaster.jsonCodec = codec } }Also applies to: 70-74
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
- client/v2/CHANGELOG.md (1 hunks)
- client/v2/go.mod (3 hunks)
- client/v2/internal/broadcast/broadcaster.go (1 hunks)
- client/v2/internal/broadcast/broadcaster_test.go (1 hunks)
- client/v2/internal/broadcast/comet.go (1 hunks)
- client/v2/internal/broadcast/comet_test.go (1 hunks)
- client/v2/internal/broadcast/testutil/comet_mock.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/v2/internal/broadcast/testutil/comet_mock.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/v2/go.mod
- client/v2/internal/broadcast/comet_test.go
🧰 Additional context used
📓 Path-based instructions (4)
client/v2/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"client/v2/internal/broadcast/broadcaster.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/internal/broadcast/broadcaster_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/v2/internal/broadcast/comet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
client/v2/internal/broadcast/broadcaster_test.go (3)
10-18: LGTM:testBFTstruct implementation is correct.The
testBFTstruct correctly implements theBroadcasterinterface withBroadcastandConsensusmethods. The implementation is suitable for testing purposes.
20-55: LGTM: Comprehensive test cases fornewBroadcaster.The
Test_newBroadcasterfunction provides good coverage with test cases for both supported ("comet") and unsupported consensus types. This addresses the previous review comment about adding more test cases.
1-9: LGTM: File structure and imports are correct.The file structure follows Go conventions, and the necessary imports are included without any unnecessary ones.
client/v2/internal/broadcast/broadcaster.go (7)
1-7: LGTM: Package declaration and imports are appropriate.The package name "broadcast" is concise and descriptive, adhering to the Uber Golang style guide. The imports are standard and necessary for the functionality implemented in this file.
8-18: LGTM: Constants are well-defined and past review comment addressed.The constants for broadcast modes and consensus engine are clearly defined and documented. The naming convention is consistent, and the previously suggested renaming of 'cometBftConsensus' to 'cometBFTConsensus' has been implemented, addressing the past review comment.
20-35: LGTM: Broadcaster interface and NewBroadcasterFn type are well-defined.The Broadcaster interface and NewBroadcasterFn type are clearly defined and well-documented. The method names are descriptive and follow Go conventions. The structure provides a flexible way to create and use different broadcaster implementations.
36-47: LGTM: BroadcasterFactory interface is well-defined and addresses past concerns.The BroadcasterFactory interface is clearly defined with Register and Create methods, following Go conventions. This design addresses the previous concern about extendability, allowing for easy registration of new broadcaster types.
49-68: LGTM: Factory struct and methods are well-implemented.The Factory struct correctly implements the BroadcasterFactory interface. The Create method includes appropriate error handling for invalid consensus types, and the Register method allows for easy extension of supported consensus engines.
70-79: LGTM: NewFactory function is well-implemented and past review comment addressed.The NewFactory function correctly initializes a Factory instance with a default CometBFT broadcaster. The previously suggested renaming of 'NewCometBftBroadcaster' to 'NewCometBFTBroadcaster' has been implemented, addressing the past review comment.
1-79: Overall: Excellent implementation of the broadcasting mechanism.This file introduces a well-structured and extensible broadcasting mechanism for transactions. The code follows the Uber Golang style guide, with clear interfaces, appropriate documentation, and consistent naming conventions. All past review comments have been addressed, resulting in a robust and maintainable implementation.
client/v2/CHANGELOG.md (1)
Line range hint
1-148: Excellent adherence to changelog best practices!The overall structure and formatting of the changelog are exemplary. It follows the guiding principles, maintains consistent formatting, and presents information in a clear, chronological order. Great job in maintaining a user-friendly and informative changelog!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- client/v2/broadcast/factory.go (1 hunks)
- client/v2/broadcast/factory_test.go (1 hunks)
- client/v2/broadcast/types/broadcaster.go (1 hunks)
- client/v2/tx/tx.go (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/v2/broadcast/factory.go
- client/v2/broadcast/factory_test.go
- client/v2/broadcast/types/broadcaster.go
🧰 Additional context used
📓 Path-based instructions (1)
client/v2/tx/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
client/v2/tx/tx.go (7)
5-5: Importing the 'context' Package is AppropriateThe addition of the
"context"import is necessary for handling context in function calls, specifically used in thegetBroadcasterfunction.
14-15: Adding 'broadcast' Dependencies CorrectlyThe imports of
"cosmossdk.io/client/v2/broadcast"andbroadcasttypes "cosmossdk.io/client/v2/broadcast/types"are appropriate and required for the new broadcasting logic introduced in the code.
17-17: Including the 'comet' Internal PackageImporting
"cosmossdk.io/client/v2/internal/comet"is necessary for creating the broadcaster with CometBFT-specific options, which aligns with the new implementation.
47-52: Proper Error Handling When Obtaining the BroadcasterThe code correctly initializes the broadcaster using
getBroadcaster, checks for errors, and handles them appropriately before proceeding. This ensures robustness in the broadcasting process.
148-148: Updating 'BroadcastTx' Function Signature Enhances ModularityThe
BroadcastTxfunction now accepts abroadcasterparameter, decoupling the broadcasting mechanism from the client context. This improves modularity and allows for greater flexibility in broadcasting strategies.
202-202: Utilizing the Broadcaster for Transaction BroadcastingThe change to use
broadcaster.Broadcastensures that the transaction broadcasting follows the new abstraction, making it consistent with the updated broadcasting logic.
207-207: 🛠️ Refactor suggestionEnhance Broadcast Response Handling for Better User Feedback
The current implementation returns the broadcast response as a plain string, which might not provide sufficient information to the user, especially in the case of errors or detailed transaction results.
Consider parsing the response into a structured format or enriching the output to include more detailed information:
- return clientCtx.PrintString(string(res)) + // Assuming 'res' is of a specific type that can be marshaled into JSON + output, err := ctx.Codec.MarshalJSON(res) + if err != nil { + return fmt.Errorf("failed to marshal broadcast response: %w", err) + } + return clientCtx.PrintString(string(output))This approach provides a JSON-formatted response, improving readability and usefulness for the end-user.
Likely invalid or redundant comment.
client/v2/broadcast/factory.go
Outdated
There was a problem hiding this comment.
Thinking about it, maybe we don't need a factory at all. When would you require multiple broadcasters? This adds another complexity layer
client/v2/tx/tx.go
Outdated
| return BroadcastTx(ctx, txf, broadcaster, msgs...) | ||
| } | ||
|
|
||
| func getBroadcaster(ctx client.Context, flagSet *pflag.FlagSet) (broadcasttypes.Broadcaster, error) { |
There was a problem hiding this comment.
That works, but let's precise in the godoc that this uses by default a cometbft broadcaster.
Let's add GenerateOrBroadcastTxCLIWithBroadcaster as well so people don't need to re-implement this themselves.
client/v2/internal/comet/comet.go
Outdated
| } | ||
|
|
||
| // NewCometBFTBroadcaster creates a new CometBftBroadcaster. | ||
| func NewCometBFTBroadcaster(rpcURL string, opts ...broadcasttypes.Option) (*CometBFTBroadcaster, error) { |
There was a problem hiding this comment.
let's create a broadcaster package and make this public
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
client/v2/broadcast/broadcaster.go (1)
5-15: Consider enhancing the interface documentation.While the current documentation provides basic information, it could be more comprehensive to help implementers better understand the contract.
Consider expanding the documentation as follows:
-// Broadcaster defines an interface for broadcasting transactions to the consensus engine. +// Broadcaster defines an interface for broadcasting transactions to the consensus engine. +// Implementations of this interface are responsible for handling the specifics of +// transaction broadcasting for different consensus engines (e.g., CometBFT). +// +// Example usage: +// +// broadcaster := GetBroadcaster() +// result, err := broadcaster.Broadcast(ctx, txBytes) +// if err != nil { +// // Handle error +// } +// // Process resultAlso consider documenting common error conditions in the
Broadcastmethod documentation:// Broadcast sends a transaction to the network and returns the result. // // It returns a byte slice containing the formatted result that will be - // passed to the output writer, and an error if the broadcast failed. + // passed to the output writer, and an error if the broadcast failed. + // Common error conditions include: + // - Network connectivity issues + // - Invalid transaction format + // - Consensus engine-specific validation failuresclient/v2/broadcast/comet/comet_test.go (2)
20-20: Consider adding a descriptive comment for the global codec variable.While using a global codec variable is common in tests, adding a comment would help explain its purpose and configuration.
+// cdc is a codec instance used across all tests, configured with standard test options var cdc = testutil.CodecOptions{}.NewCodec()
22-58: Add more test cases for comprehensive constructor testing.While the current test cases cover basic scenarios, consider adding tests for:
- Invalid broadcast mode
- Empty endpoint URL
- Malformed endpoint URL
tests := []struct { name string cdc codec.JSONCodec mode string want *CometBFTBroadcaster wantErr bool }{ + { + name: "invalid mode", + mode: "invalid", + cdc: cdc, + wantErr: true, + }, + { + name: "empty endpoint", + mode: BroadcastSync, + cdc: cdc, + wantErr: true, + },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- client/v2/broadcast/broadcaster.go (1 hunks)
- client/v2/broadcast/comet/comet.go (1 hunks)
- client/v2/broadcast/comet/comet_test.go (1 hunks)
- client/v2/broadcast/comet/testutil/comet_mock.go (1 hunks)
- client/v2/tx/tx.go (5 hunks)
- scripts/mockgen.sh (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/v2/broadcast/comet/testutil/comet_mock.go
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/mockgen.sh
🧰 Additional context used
📓 Path-based instructions (4)
client/v2/broadcast/broadcaster.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/broadcast/comet/comet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/broadcast/comet/comet_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/v2/tx/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
client/v2/broadcast/broadcaster.go (1)
1-3: LGTM: Package declaration and imports are well-organized.The package name is concise and follows Go conventions, and imports are properly organized.
client/v2/broadcast/comet/comet_test.go (1)
1-18: LGTM! Package and imports are well-organized.The package name follows Go conventions, and imports are properly organized and necessary for the test implementation.
client/v2/broadcast/comet/comet.go (2)
100-101: 🛠️ Refactor suggestionImprove the method comment to follow Go documentation conventions.
The comment for the
Broadcastmethod should be more descriptive and start with the method name.Apply this diff to enhance the comment:
-// Broadcast sends a transaction to the network and returns the result. -// returns a byte slice containing the JSON-encoded result and an error if the broadcast failed. +// Broadcast sends a transaction to the network and returns a JSON-encoded result. +// It returns an error if the broadcast fails.Likely invalid or redundant comment.
94-96: 🛠️ Refactor suggestionUse pointer receiver for the
Consensusmethod to avoid copying the struct.Currently, the
Consensusmethod has a value receiver. It's recommended to use a pointer receiver to avoid unnecessary copying of theCometBFTBroadcasterstruct.Apply this diff to change the method receiver:
-func (c CometBFTBroadcaster) Consensus() string { +func (c *CometBFTBroadcaster) Consensus() string { return cometBFTConsensus }Likely invalid or redundant comment.
client/v2/tx/tx.go (1)
63-64: Handle errors returned byflagSet.GetStringThe errors from
flagSet.GetStringfor"node"and"broadcast-mode"are ignored. Consider handling these errors to prevent potential issues.Apply this diff to handle the errors:
func getCometBroadcaster(ctx client.Context, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) { - url, _ := flagSet.GetString("node") - mode, _ := flagSet.GetString("broadcast-mode") + url, err := flagSet.GetString("node") + if err != nil { + return nil, fmt.Errorf("failed to retrieve 'node' flag: %w", err) + } + mode, err := flagSet.GetString("broadcast-mode") + if err != nil { + return nil, fmt.Errorf("failed to retrieve 'broadcast-mode' flag: %w", err) + } return comet.NewCometBFTBroadcaster(url, mode, ctx.Codec) }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
client/v2/broadcast/comet/comet_test.go (4)
20-20: Consider moving the codec variable into test functions.Package-level variables in tests can lead to shared state and potential test interdependence. Consider moving
cdcinto the test functions where it's needed for better test isolation.
23-45: Enhance test coverage for NewCometBftBroadcaster.Consider adding these test cases:
- Invalid broadcast mode
- Validation of rpcClient initialization
tests := []struct { name string cdc codec.JSONCodec mode string want *CometBFTBroadcaster wantErr bool }{ + { + name: "invalid broadcast mode", + mode: "invalid_mode", + cdc: cdc, + wantErr: true, + }, + { + name: "empty endpoint", + mode: BroadcastSync, + cdc: cdc, + wantErr: true, + },
105-110: Enhance broadcast response validation.The test only checks if the response is not nil. Consider validating specific fields in the response:
- Code
- Data
- Log
- Codespace
- Hash
- require.NotNil(t, got) + require.Equal(t, uint32(0), got.Code) + require.NotEmpty(t, got.TxHash) + require.Empty(t, got.Codespace)
116-142: Enhance error checking test coverage.Consider adding these test cases:
- Nil error (should return success response)
- Unknown error type (should return appropriate error code)
Also, validate additional response fields beyond just the Code:
- Data
- Log
- Codespace
tests := []struct { name string err error want *apiacbci.TxResponse }{ + { + name: "nil error", + err: nil, + want: &apiacbci.TxResponse{ + Code: 0, + }, + }, + { + name: "unknown error", + err: errors.New("unknown error"), + want: &apiacbci.TxResponse{ + Code: 1, + Log: "unknown error", + Codespace: "sdk", + }, + },client/v2/tx/tx.go (1)
207-212: Consider documenting the response format.The function now converts the broadcast response to a string before printing. It would be helpful to document the expected format of this response to help users parse or handle it appropriately.
// BroadcastTx attempts to generate, sign and broadcast a transaction with the // given set of messages. It will also simulate gas requirements if necessary. -// It will return an error upon failure. +// It will return an error upon failure. The broadcast response is printed as a string +// and follows the format: <describe expected format here> func BroadcastTx(clientCtx client.Context, txf Factory, broadcaster broadcast.Broadcaster, msgs ...transaction.Msg) error {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- client/v2/broadcast/comet/comet.go (1 hunks)
- client/v2/broadcast/comet/comet_test.go (1 hunks)
- client/v2/tx/tx.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/v2/broadcast/comet/comet.go
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/broadcast/comet/comet_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/v2/tx/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
client/v2/tx/tx.go (2)
Line range hint
24-47: LGTM! Well-structured function with clear separation of concerns.The function properly handles different transaction modes and cleanly integrates the new broadcaster parameter.
60-64: Improve error handling in getCometBroadcaster.The function currently ignores errors from
flagSet.GetString. While these errors are unlikely in practice, proper error handling would make the code more robust.This issue was previously identified in a past review. Please implement the suggested error handling:
func getCometBroadcaster(ctx client.Context, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) { - url, _ := flagSet.GetString("node") - mode, _ := flagSet.GetString("broadcast-mode") + url, err := flagSet.GetString("node") + if err != nil { + return nil, fmt.Errorf("failed to retrieve 'node' flag: %w", err) + } + mode, err := flagSet.GetString("broadcast-mode") + if err != nil { + return nil, fmt.Errorf("failed to retrieve 'broadcast-mode' flag: %w", err) + } return comet.NewCometBFTBroadcaster(url, mode, ctx.Codec) }
|
|
client/v2/broadcast/comet/comet.go
Outdated
| fn func(ctx context.Context, tx cmttypes.Tx) (*coretypes.ResultBroadcastTx, error), | ||
| ) (*apiacbci.TxResponse, error) { | ||
| bResult, err := fn(ctx, txBytes) | ||
| if errRes := checkCometError(err, txBytes); err != nil { |
There was a problem hiding this comment.
is there any chance, checkCometError switch check return the default value which is nil here ? Just want to make sure the error is properly return
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
client/v2/broadcast/comet/comet.go (2)
31-32: Consider renaming cometBFTConsensus to CometBFTConsensus for consistency.
The constant naming should follow the same casing convention as other exported constants in the file (BroadcastSync and BroadcastAsync).
- cometBFTConsensus = "comet"
+ CometBFTConsensus = "comet"82-85: Enhance error message with RPC URL details.
Include the RPC URL in the error message to help with debugging connection issues.
- return nil, fmt.Errorf("failed to create CometBft RPC client: %w", err)
+ return nil, fmt.Errorf("failed to create CometBFT RPC client for URL %s: %w", rpcURL, err)📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
client/v2/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- client/v2/broadcast/comet/comet.go (1 hunks)
- client/v2/go.mod (3 hunks)
- simapp/go.mod (1 hunks)
- simapp/v2/go.mod (1 hunks)
- tests/go.mod (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/v2/go.mod
- simapp/go.mod
- simapp/v2/go.mod
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
client/v2/broadcast/comet/comet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
client/v2/broadcast/comet/comet.go (1)
35-61: Well-structured interface definition!
The CometRPC interface is well-documented and provides a comprehensive set of methods for interacting with the CometBFT RPC client.
| bResult, err := fn(ctx, txBytes) | ||
| if errRes := checkCometError(err, txBytes); errRes != nil { | ||
| return errRes, nil | ||
| } | ||
|
|
||
| return newResponseFormatBroadcastTx(bResult), err | ||
| } |
There was a problem hiding this comment.
Fix inconsistent error handling in broadcast method.
The method returns both the formatted response and the error on line 134, but the error was already handled in the previous if statement. This could lead to returning a successful response with an error.
func (c *CometBFTBroadcaster) broadcast(ctx context.Context, txBytes []byte,
fn func(ctx context.Context, tx cmttypes.Tx) (*coretypes.ResultBroadcastTx, error),
) (*apiacbci.TxResponse, error) {
bResult, err := fn(ctx, txBytes)
if errRes := checkCometError(err, txBytes); errRes != nil {
return errRes, nil
}
+ if err != nil {
+ return nil, err
+ }
- return newResponseFormatBroadcastTx(bResult), err
+ return newResponseFormatBroadcastTx(bResult), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bResult, err := fn(ctx, txBytes) | |
| if errRes := checkCometError(err, txBytes); errRes != nil { | |
| return errRes, nil | |
| } | |
| return newResponseFormatBroadcastTx(bResult), err | |
| } | |
| bResult, err := fn(ctx, txBytes) | |
| if errRes := checkCometError(err, txBytes); errRes != nil { | |
| return errRes, nil | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| return newResponseFormatBroadcastTx(bResult), nil | |
| } |
| parsedLogs, _ := parseABCILogs(res.Log) | ||
|
|
There was a problem hiding this comment.
Consider handling log parsing errors.
The unmarshal error from parseABCILogs is silently ignored. Consider either logging the error or including it in the response for better debugging.
- parsedLogs, _ := parseABCILogs(res.Log)
+ parsedLogs, err := parseABCILogs(res.Log)
+ if err != nil {
+ // Either log the error or include it in the response
+ parsedLogs = []*apiacbci.ABCIMessageLog{{
+ Log: fmt.Sprintf("failed to parse logs: %v", err),
+ }}
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parsedLogs, _ := parseABCILogs(res.Log) | |
| parsedLogs, err := parseABCILogs(res.Log) | |
| if err != nil { | |
| // Either log the error or include it in the response | |
| parsedLogs = []*apiacbci.ABCIMessageLog{{ | |
| Log: fmt.Sprintf("failed to parse logs: %v", err), | |
| }} | |
| } |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
Co-authored-by: Julián Toledano <[email protected]> Co-authored-by: Julien Robert <[email protected]>
* main: (24 commits) build(deps): upgrade to [email protected] (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Closes:
#21851
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.
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
Improvements
Bug Fixes
Documentation
Tests