[OTE-839] add query for unconditional revshare #2380
Conversation
WalkthroughThe pull request introduces a new method for querying unconditional revenue share configurations across multiple files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
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 (
|
affanv14
force-pushed
the
affan/unconditional-query
branch
from
7bcfe05 to
2f84006
Compare
affanv14
force-pushed
the
affan/unconditional-query
branch
from
2f84006 to
7c64ff5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
protocol/x/revshare/keeper/grpc_query_unconditional_revshare.go (1)
14-21: LGTM with a minor suggestion: Consider wrapping the error for better context.The implementation is concise and follows best practices:
- Correctly uses
sdk.UnwrapSDKContextto access the underlying SDK context.- Properly handles errors by returning early if
GetUnconditionalRevShareConfigParamsfails.- Returns the expected response type with the retrieved configuration.
Consider wrapping the error returned by
GetUnconditionalRevShareConfigParamsto provide more context. This can be achieved usingfmt.Errorf:config, err := k.GetUnconditionalRevShareConfigParams(sdk.UnwrapSDKContext(ctx)) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get unconditional rev share config params: %w", err) }Don't forget to import the
fmtpackage if you make this change.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.lcd.ts (1)
32-33: Consider removing unnecessary blank linesThere are two consecutive blank lines (32-33) that appear to be unnecessary. Consider removing them to maintain consistent spacing throughout the file.
/* Queries unconditional revenue share config */ - - async unconditionalRevShareConfig(_params: QueryUnconditionalRevShareConfig = {}): Promise<QueryUnconditionalRevShareConfigResponseSDKType> {proto/dydxprotocol/revshare/query.proto (2)
29-34: LGTM! Consider enhancing the comment.The new RPC method
UnconditionalRevShareConfigis well-defined and consistent with the PR objective. The HTTP mapping is correctly set up.Consider enhancing the comment to provide more context:
- // Queries unconditional revenue share config + // Queries the unconditional revenue share configuration
56-59: LGTM! Consider enhancing the comment.The
QueryUnconditionalRevShareConfigResponsemessage is well-defined with an appropriate structure for returning the unconditional revshare configuration. The use ofgogoproto.nullable = falsefor theconfigfield ensures it will always be present in the response.Consider enhancing the comment to provide more context:
-// Response type for QueryUnconditionalRevShareConfig +// Response type for QueryUnconditionalRevShareConfig, containing the unconditional revenue share configurationprotocol/x/revshare/client/cli/query_params.go (1)
70-70: Consider adding a comment to describe the command's purpose.To improve code readability and maintainability, it would be helpful to add a brief comment describing the purpose of this command and what unconditional revenue share config means in this context.
Here's a suggested comment to add above the function:
// CmdQueryUnconditionalRevShareConfig creates a Cobra command to query the unconditional revenue share configuration. // This configuration determines how revenue is shared without specific conditions attached.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts (1)
45-58: Consider adding unit tests for the new query interfacesTo ensure the reliability of the new
QueryUnconditionalRevShareConfigquery and its responses, consider adding unit tests to cover this functionality.🧰 Tools
🪛 Biome
[error] 45-45: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 48-48: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
protocol/x/revshare/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/revshare/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts (3 hunks)
- proto/dydxprotocol/revshare/query.proto (2 hunks)
- protocol/x/revshare/client/cli/query.go (1 hunks)
- protocol/x/revshare/client/cli/query_params.go (1 hunks)
- protocol/x/revshare/keeper/grpc_query_unconditional_revshare.go (1 hunks)
- protocol/x/revshare/keeper/grpc_query_unconditional_revshare_test.go (1 hunks)
🧰 Additional context used
🪛 Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts
[error] 45-45: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 48-48: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (22)
protocol/x/revshare/keeper/grpc_query_unconditional_revshare.go (3)
3-8: LGTM: Imports are appropriate for the context.The imports are well-structured and include the necessary packages for implementing a gRPC query handler in a Cosmos SDK module. The custom import from
dydxprotocolaligns with the specific protocol implementation.
10-13: LGTM: Function signature follows gRPC query handler conventions.The
UnconditionalRevShareConfigmethod signature is well-structured and follows the conventions for gRPC query handlers in Cosmos SDK:
- It's a method of the
Keeperstruct.- It takes a
context.Contextand a pointer to the request type.- It returns a pointer to the response type and an error.
The naming is clear and descriptive, accurately representing the query's purpose.
1-21: Summary: Implementation aligns with PR objectives and follows best practices.This new file successfully implements the gRPC query handler for unconditional revenue sharing configuration, as outlined in the PR objectives. The code is well-structured, follows Cosmos SDK conventions, and implements proper error handling.
A minor suggestion was made to improve error context, but this is not a blocking issue. The implementation should work as intended and can be safely merged.
To ensure this change is properly integrated, please run the following verification script:
This script will help confirm that the new query is properly registered in the gRPC query server, ensuring it can be accessed as intended.
protocol/x/revshare/client/cli/query.go (2)
Line range hint
1-28: Overall, the change is well-implemented and consistent with existing code.The addition of the new command
CmdQueryUnconditionalRevShareConfig()is minimal and follows the existing pattern in the file. It doesn't introduce any apparent issues or inconsistencies. The change aligns well with the PR objective of adding a query for unconditional revshare data.
25-25: LGTM! New command successfully added.The new command
CmdQueryUnconditionalRevShareConfig()has been correctly added to the command set. This addition aligns with the PR objective of introducing a new query for unconditional revshare data.To ensure the new command is properly implemented, let's verify its existence:
✅ Verification successful
Verified:
CmdQueryUnconditionalRevShareConfigis correctly implemented and integrated.The function
CmdQueryUnconditionalRevShareConfig()is properly defined inquery_params.goand successfully added to the CLI commands inquery.go. No issues were found during the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of CmdQueryUnconditionalRevShareConfig # Test: Search for the function definition rg --type go -A 5 'func CmdQueryUnconditionalRevShareConfig\(\)'Length of output: 630
Script:
#!/bin/bash # Description: Extract the RunE function implementation of CmdQueryUnconditionalRevShareConfig # Search for the RunE function block within CmdQueryUnconditionalRevShareConfig rg --type go 'func CmdQueryUnconditionalRevShareConfig\(\)' -A 20Length of output: 1648
protocol/x/revshare/keeper/grpc_query_unconditional_revshare_test.go (3)
3-10: LGTM: Imports are appropriate for the test file.The import statements include all necessary packages for testing, including custom test utilities, constants, and the required types from the revshare module.
12-43: LGTM: Well-structured table-driven tests.The test function
TestQueryUnconditionalRevShareis well-organized using table-driven tests. This approach allows for easy addition of new test cases and provides a clear overview of different scenarios being tested.
1-58: Overall, well-structured and effective test file with room for enhancement.This test file for
UnconditionalRevShareConfigis well-organized and follows good testing practices, particularly with its use of table-driven tests. It effectively covers the main scenarios for unconditional revshare configuration.To further improve the robustness and effectiveness of these tests, consider:
- Adding more edge cases and invalid configurations as suggested earlier.
- Enhancing error checking and adding more specific assertions in the test execution logic.
- Ensuring comprehensive coverage of the
UnconditionalRevShareConfigmethod's behavior.These enhancements will contribute to a more thorough and maintainable test suite for the revshare module.
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.lcd.ts (4)
2-2: LGTM: Import statement updated correctlyThe import statement has been appropriately updated to include the new types
QueryUnconditionalRevShareConfigandQueryUnconditionalRevShareConfigResponseSDKType. This change is consistent with the addition of the new method and follows the existing naming conventions.
14-14: LGTM: Method binding added correctlyThe new method
unconditionalRevShareConfigis properly bound to the class instance in the constructor. This follows the established pattern in the class and ensures the correctthiscontext when the method is called.
31-37: LGTM: New method implemented correctlyThe
unconditionalRevShareConfigmethod is well-implemented and follows the established patterns in the class. It correctly uses an async function, returns a Promise with the expected type, and uses the appropriate endpoint for the GET request. The optional parameter allows for flexibility in the query.
Line range hint
1-37: Summary: Implementation looks good overallThe changes in this file successfully implement the new query for unconditional revenue sharing as described in the PR objectives. The code is well-structured, follows established patterns, and is consistent with the existing codebase. Only minor formatting suggestions were made. The new functionality appears to be correctly implemented and integrated into the
LCDQueryClientclass.proto/dydxprotocol/revshare/query.proto (2)
53-54: LGTM! Empty message is appropriate.The
QueryUnconditionalRevShareConfigmessage is correctly defined as an empty message, which is appropriate for a query that doesn't require any parameters. The naming convention is consistent with other query messages in the file.
Line range hint
28-59: Summary: Changes align well with PR objectivesThe additions to the
Queryservice and the new message types (QueryUnconditionalRevShareConfigandQueryUnconditionalRevShareConfigResponse) are well-implemented and align perfectly with the PR objective of adding a query for unconditional revshare configuration. The changes follow existing naming conventions and structure, maintaining consistency within the file.To ensure that the
UnconditionalRevShareConfigtype is properly defined, let's verify its existence:This will help confirm that the
UnconditionalRevShareConfigtype, which is used in the response, is properly defined in the project.✅ Verification successful
Verification Successful:
UnconditionalRevShareConfigis DefinedThe
UnconditionalRevShareConfigtype is properly defined inproto/dydxprotocol/revshare/revshare.proto, ensuring that the response type in the query is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of UnconditionalRevShareConfig type # Test: Search for the UnconditionalRevShareConfig type definition rg --type proto "message UnconditionalRevShareConfig"Length of output: 85
Script:
#!/bin/bash # Description: Verify the existence of UnconditionalRevShareConfig type by searching all .proto files # Test: Search for the UnconditionalRevShareConfig type definition in .proto files rg 'message UnconditionalRevShareConfig' --glob '*.proto'Length of output: 140
protocol/x/revshare/client/cli/query_params.go (1)
70-91: LGTM! The implementation looks good.The new
CmdQueryUnconditionalRevShareConfigfunction is well-structured and consistent with other query commands in the file. It correctly sets up the Cobra command, handles the query execution, and manages potential errors.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts (5)
4-4: LGTM: Import statement correctly updatedThe import statement has been appropriately updated to include the new types
QueryUnconditionalRevShareConfigandQueryUnconditionalRevShareConfigResponse. This change is necessary for the new functionality being added.
16-18: LGTM: New query method added to interfaceThe
unconditionalRevShareConfigmethod has been correctly added to theQueryinterface. The method signature is consistent with other methods in the interface, using an optional request parameter and returning a Promise. The method name accurately describes its purpose of querying unconditional revenue share configuration.
27-27: LGTM: QueryClientImpl class correctly updatedThe
QueryClientImplclass has been properly updated to include the newunconditionalRevShareConfigmethod:
- The method is correctly bound in the constructor (line 27).
- The method implementation (lines 42-46) follows the established pattern:
- It encodes the request
- Sends it via RPC with the correct service and method names
- Decodes the response using the appropriate type
These changes are consistent with the existing code structure and implement the new functionality correctly.
Also applies to: 42-46
59-62: LGTM: createRpcQueryExtension function updated correctlyThe
createRpcQueryExtensionfunction has been properly updated to include the newunconditionalRevShareConfigmethod. The implementation is consistent with other methods in the extension and correctly calls the corresponding method on thequeryService. This change ensures that the new query method is available through the extended query service.
Line range hint
1-65: Overall assessment: Changes implement the new query successfullyThe changes in this file successfully implement the new query for unconditional revenue share configuration as described in the PR objectives. The modifications are consistent with the existing code structure and patterns, and all necessary components (interface, implementation, and extension) have been updated correctly.
Key points:
- New imports added correctly
Queryinterface updated with the new methodQueryClientImplclass implementation addedcreateRpcQueryExtensionfunction updated to include the new queryThe implementation appears to be complete and ready for integration.
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts (2)
2-2: New imports for UnconditionalRevShareConfigThe import statement correctly adds
UnconditionalRevShareConfigandUnconditionalRevShareConfigSDKTypefrom"./revshare".
229-305: Implementation of encoding and decoding functionsThe encoding and decoding functions for
QueryUnconditionalRevShareConfigandQueryUnconditionalRevShareConfigResponseare correctly implemented and follow existing patterns in the codebase.
| testCases := map[string]struct { | ||
| config types.UnconditionalRevShareConfig | ||
| }{ | ||
| "Single recipient": { | ||
| config: types.UnconditionalRevShareConfig{ | ||
| Configs: []types.UnconditionalRevShareConfig_RecipientConfig{ | ||
| { | ||
| Address: constants.AliceAccAddress.String(), | ||
| SharePpm: 100_000, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "Multiple recipients": { | ||
| config: types.UnconditionalRevShareConfig{ | ||
| Configs: []types.UnconditionalRevShareConfig_RecipientConfig{ | ||
| { | ||
| Address: constants.AliceAccAddress.String(), | ||
| SharePpm: 50_000, | ||
| }, | ||
| { | ||
| Address: constants.BobAccAddress.String(), | ||
| SharePpm: 30_000, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "Empty config": { | ||
| config: types.UnconditionalRevShareConfig{}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding edge cases and invalid configurations.
The current test cases cover the main scenarios well. To enhance the robustness of the tests, consider adding the following cases:
- A configuration with the maximum allowed number of recipients.
- A configuration where the total
SharePpmexceeds 1,000,000 (100%). - A configuration with an invalid address.
- A configuration with a negative
SharePpm.
These additional cases would help ensure proper error handling and validation in the UnconditionalRevShareConfig method.
| for name, tc := range testCases { | ||
| t.Run(name, func(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.RevShareKeeper | ||
|
|
||
| k.SetUnconditionalRevShareConfigParams(ctx, tc.config) | ||
|
|
||
| resp, err := k.UnconditionalRevShareConfig(ctx, &types.QueryUnconditionalRevShareConfig{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, tc.config, resp.Config) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error checking and add more assertions.
The test execution logic is generally correct, but consider the following improvements:
-
Add an assertion to check that
SetUnconditionalRevShareConfigParamssucceeds:err := k.SetUnconditionalRevShareConfigParams(ctx, tc.config) require.NoError(t, err)
-
For the query result, add more specific assertions:
require.Equal(t, len(tc.config.Configs), len(resp.Config.Configs)) for i, config := range tc.config.Configs { require.Equal(t, config.Address, resp.Config.Configs[i].Address) require.Equal(t, config.SharePpm, resp.Config.Configs[i].SharePpm) }
These changes will provide more detailed error messages if a test fails, making it easier to identify the exact point of failure.
| @@ -40,6 +40,22 @@ export interface QueryMarketMapperRevShareDetailsResponse { | |||
| export interface QueryMarketMapperRevShareDetailsResponseSDKType { | |||
| details?: MarketMapperRevShareDetailsSDKType; | |||
| } | |||
| /** Queries unconditional revenue share details */ | |||
|
|
|||
| export interface QueryUnconditionalRevShareConfig {} | |||
There was a problem hiding this comment.
Replace empty interfaces with type aliases
The interfaces QueryUnconditionalRevShareConfig and QueryUnconditionalRevShareConfigSDKType are empty. Since empty interfaces are equivalent to {}, consider using type aliases instead for clarity.
Apply this diff to fix the issue:
-export interface QueryUnconditionalRevShareConfig {}
+type QueryUnconditionalRevShareConfig = {};
-export interface QueryUnconditionalRevShareConfigSDKType {}
+type QueryUnconditionalRevShareConfigSDKType = {};Also applies to: 48-48
🧰 Tools
🪛 Biome
[error] 45-45: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
|
https://github.com/Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 60cc7aa)
Changelist
Add query to get unconditional revshare
Test Plan
Tested locally
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests