[TRA-414] Add x/revshare module skeleton #1719
Conversation
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
|
Warning Rate limit exceeded@shrenujb has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 5 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update introduces a new module for revenue sharing ( Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI
participant RPCServer
participant RevShareKeeper
User->>CLI: Execute Query Command
CLI->>RPCServer: Send GRPC Query
RPCServer->>RevShareKeeper: Handle Query
RevShareKeeper->>RPCServer: Return Result
RPCServer->>CLI: Send Response
CLI->>User: Display Query Result
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Outside diff range and nitpick comments (4)
protocol/x/revshare/types/errors.go (1)
1-5: The error variable block is currently empty. Consider adding a TODO or FIXME to ensure that specific errors related to therevsharemodule are defined in the future.protocol/testutil/constants/genesis.go (1)
Line range hint
342-342: Detected a potential security issue with a generic API key. Please verify if this is a real API key or a placeholder, and if real, consider removing it from the codebase or using more secure handling methods like environment variables or secure vaults.protocol/scripts/genesis/sample_pregenesis.json (1)
Line range hint
538-538: Potential security issue detected: Generic API Key exposure. Please verify if this is a legitimate key and ensure it's not hardcoded in the codebase. If this is a valid key, consider using environment variables or secure vault solutions to manage it securely.protocol/testing/containertest/preupgrade_genesis.json (1)
Line range hint
675-675: Potential security risk: Detected a Generic API Key.This line appears to contain a generic API key, which could expose sensitive operations if leaked or mishandled. Consider removing or rotating this key, and ensure that sensitive credentials are managed securely, possibly using environment variables or a secure vault solution.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
protocol/x/revshare/types/genesis.pb.gois excluded by!**/*.pb.goprotocol/x/revshare/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/revshare/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (29)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.query.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
- proto/dydxprotocol/revshare/genesis.proto (1 hunks)
- proto/dydxprotocol/revshare/query.proto (1 hunks)
- proto/dydxprotocol/revshare/tx.proto (1 hunks)
- protocol/app/app.go (9 hunks)
- protocol/app/app_test.go (2 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/revshare/client/cli/query.go (1 hunks)
- protocol/x/revshare/client/cli/tx.go (1 hunks)
- protocol/x/revshare/genesis.go (1 hunks)
- protocol/x/revshare/keeper/grpc_query.go (1 hunks)
- protocol/x/revshare/keeper/keeper.go (1 hunks)
- protocol/x/revshare/keeper/keeper_test.go (1 hunks)
- protocol/x/revshare/keeper/msg_server.go (1 hunks)
- protocol/x/revshare/keeper/msg_server_test.go (1 hunks)
- protocol/x/revshare/module.go (1 hunks)
- protocol/x/revshare/types/codec.go (1 hunks)
- protocol/x/revshare/types/errors.go (1 hunks)
- protocol/x/revshare/types/expected_keepers.go (1 hunks)
- protocol/x/revshare/types/genesis.go (1 hunks)
- protocol/x/revshare/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.query.ts
- proto/dydxprotocol/revshare/genesis.proto
- proto/dydxprotocol/revshare/query.proto
- protocol/x/revshare/keeper/msg_server_test.go
- protocol/x/revshare/types/expected_keepers.go
- protocol/x/revshare/types/keys.go
Additional context used
Gitleaks
protocol/testutil/constants/genesis.go
342-342: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/scripts/genesis/sample_pregenesis.json
538-538: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/testing/containertest/preupgrade_genesis.json
675-675: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (31)
indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (2)
1-1: The import statement appears correctly updated to match the new file structure.
2-2: The export statement correctly re-exports all bindings from the imported module.proto/dydxprotocol/revshare/tx.proto (1)
1-7: The Protocol Buffers definition for therevsharemodule is correctly set up with appropriate syntax, package declaration, and an empty service definition for initial setup.protocol/x/revshare/types/genesis.go (1)
3-6: TheDefaultGenesisfunction correctly initializes an empty genesis state.protocol/x/revshare/keeper/keeper_test.go (1)
10-16: TheTestLoggerfunction is well-implemented and correctly verifies the non-nullity of the logger.protocol/x/revshare/keeper/msg_server.go (2)
7-9: ThemsgServerstruct is correctly defined as a simple wrapper aroundKeeper.
11-15: TheNewMsgServerImplfunction is correctly implemented to set up a new message server.indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
1-14: The imports and exports are correctly structured for the Google-related protobuf modules. The use of spread syntax in namespaces is well-applied.protocol/app/upgrades/v6.0.0/constants.go (1)
7-7: The addition ofrevsharetypes.StoreKeyto theStoreUpgradesstruct is correctly implemented.Also applies to: 19-19
protocol/x/revshare/types/codec.go (1)
12-14: Proper implementation of codec registration and interface registration for therevsharemodule.Also applies to: 18-18
protocol/x/revshare/genesis.go (1)
9-11: Structural setup of genesis functions is correct, but ensure to implement the actual logic in future iterations.Also applies to: 14-16
protocol/x/revshare/client/cli/tx.go (1)
13-22: Correct implementation of CLI transaction commands for therevsharemodule. Good use of dynamic naming withtypes.ModuleName.protocol/x/revshare/client/cli/query.go (1)
12-23: The CLI setup for therevsharemodule is correctly implemented.protocol/x/revshare/keeper/keeper.go (1)
14-32: TheKeeperstruct and its constructor are correctly implemented, ensuring proper initialization and management of authorities.protocol/app/basic_manager/basic_manager.go (1)
Line range hint
35-97: The integration of therevsharemodule into the ModuleBasics manager is correctly executed.protocol/x/revshare/module.go (1)
32-143: BothAppModuleBasicandAppModulestructures along with their methods are correctly implemented, covering all necessary module functionalities and integrations.protocol/app/app_test.go (1)
221-221: LGTM! Ensure comprehensive testing for therevsharemodulein the application context.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
76-82: LGTM! Ensure all references and dependencies for therevsharemodule are correctly updated across the project.Also applies to: 293-296
protocol/app/testdata/default_genesis_state.json (1)
412-412: LGTM! Ensure that therevshareobject is correctly initialized during the genesis and does not conflict with other modules.Verification successful
The search results indicate that the
revsharemodule is referenced in multiple files, including genesis initialization files (protocol/x/revshare/genesis.go), test files, and the main application files. This suggests that therevsharemodule is integrated into the genesis state and is being initialized.Key files where
revshareis referenced:
protocol/x/revshare/genesis.goprotocol/app/app.goprotocol/app/testdata/default_genesis_state.json- Various test files
The presence of
revsharein these files indicates that it is being correctly initialized during genesis and is part of the application module setup.
protocol/x/revshare/genesis.go: Handles the genesis state for therevsharemodule.protocol/app/app.go: Integrates therevsharemodule into the main application.protocol/app/testdata/default_genesis_state.json: Contains the genesis state with therevshareentry.Given this information, it appears that the
revsharemodule is correctly initialized and integrated into the genesis state.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `revshare` in the genesis state. # Test: Search for the `revshare` initialization in the genesis tests. Expect: Correct initialization without conflicts. rg --type json 'revshare' protocol/app/testdata/Length of output: 115
Script:
#!/bin/bash # Description: Verify the initialization of `revshare` in the genesis state. # Search for `revshare` in the genesis initialization code. ast-grep --lang go --pattern '$_ = $_ { "revshare": $_, $$$ }' # Search for tests that validate `revshare` initialization. rg 'revshare' --type go --type jsonLength of output: 4022
protocol/testutil/constants/genesis.go (1)
1360-1360: The addition of the emptyrevshareobject in the genesis state is appropriate as it prepares the ground for future enhancements specific to the revshare module.protocol/scripts/genesis/sample_pregenesis.json (1)
1763-1763: The addition of the emptyrevshareobject is noted. Please ensure that this module is correctly initialized in subsequent updates to start functioning as expected.protocol/app/app.go (9)
172-174: The imports for therevsharemodule,revsharemodulekeeper, andrevsharemoduletypesare correctly added to integrate the new module.
316-317: TheRevShareKeeperis appropriately declared within theAppstruct. This is necessary for the new module's keeper to be accessible throughout the application.
439-439: Therevsharemoduletypes.StoreKeyis correctly added to the list of KV store keys. This is crucial for therevsharemodule to have its own dedicated storage space within the app's multistore.
1122-1129: The instantiation ofRevShareKeeperusingrevsharemodulekeeper.NewKeeperis correctly implemented. The parameters passed includingappCodec,keys[revsharemoduletypes.StoreKey], and the authority list are as expected for initializing the keeper.
1200-1200: TherevShareModuleis correctly added to theModuleManager. This ensures that therevsharemodule is included in the app's lifecycle, including genesis initialization and transaction processing.
1249-1249: Therevsharemoduletypes.ModuleNameis added to theSetOrderBeginBlockersmethod. This ensures that therevsharemodule's begin blocker logic is executed at the correct time in the block lifecycle.
1291-1291: Therevsharemoduletypes.ModuleNameis added to theSetOrderEndBlockersmethod. This is crucial for ensuring that therevsharemodule's end blocker logic is executed, which might include important cleanup or final state updates at the end of each block.
1337-1337: Therevsharemoduletypes.ModuleNameis correctly included in theSetOrderInitGenesismethod. This inclusion is essential for therevsharemodule to participate in the app's initial chain setup and genesis state configuration.
1379-1379: The inclusion ofrevsharemoduletypes.ModuleNamein theSetOrderMigrationsmethod is appropriate. This setup is necessary for handling state migrations, which are critical during upgrades or when the blockchain's state format changes.protocol/testing/containertest/preupgrade_genesis.json (1)
2190-2190: Added an emptyrevshareobject to the genesis state.This change aligns with the PR's objective to integrate the
revsharemodule into the system. Ensure that this module is correctly initialized elsewhere in the codebase to handle its expected functionality.
| @@ -44,6 +44,7 @@ import ( | |||
| perpetualsmodule "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals" | |||
| pricesmodule "github.com/dydxprotocol/v4-chain/protocol/x/prices" | |||
| ratelimitmodule "github.com/dydxprotocol/v4-chain/protocol/x/ratelimit" | |||
| revsharemodule "github.com/dydxprotocol/v4-chain/protocol/x/revshare" | |||
There was a problem hiding this comment.
LGTM! Ensure proper integration and testing of the new revsharemodule.
Would you like me to assist in creating integration tests for the revsharemodule?
| // Validate performs basic genesis state validation returning an error upon any | ||
| // failure. | ||
| func (gs GenesisState) Validate() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The Validate function currently does not perform any validation. Consider implementing actual validation logic based on expected constraints in future updates.
| import ( | ||
| "github.com/dydxprotocol/v4-chain/protocol/x/listing/types" | ||
| ) | ||
|
|
||
| var _ types.QueryServer = Keeper{} |
There was a problem hiding this comment.
The import path appears incorrect. It should likely import from protocol/x/revshare/types instead of protocol/x/listing/types to match the module context.
- import "github.com/dydxprotocol/v4-chain/protocol/x/listing/types"
+ import "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"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.
| import ( | |
| "github.com/dydxprotocol/v4-chain/protocol/x/listing/types" | |
| ) | |
| var _ types.QueryServer = Keeper{} | |
| import ( | |
| "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types" | |
| ) | |
| var _ types.QueryServer = Keeper{} |
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
| "github.com/dydxprotocol/v4-chain/protocol/x/listing/keeper" | ||
| "github.com/dydxprotocol/v4-chain/protocol/x/listing/types" |
There was a problem hiding this comment.
nit: Part of a separate PR perhaps?
| @@ -8,7 +8,7 @@ import ( | |||
| "github.com/cosmos/cosmos-sdk/codec" | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| "github.com/dydxprotocol/v4-chain/protocol/lib" | |||
| "github.com/dydxprotocol/v4-chain/protocol/x/vault/types" | |||
| "github.com/dydxprotocol/v4-chain/protocol/x/listing/types" | |||
There was a problem hiding this comment.
nit: Part of a separate PR?
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
protocol/testutil/constants/genesis.go (1)
Line range hint
342-342: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.Please ensure that this API key is either removed or securely managed if it's necessary for the operation. Consider using secrets management solutions or environment variables.
protocol/scripts/genesis/sample_pregenesis.json (1)
Line range hint
538-538: Potential security risk detected: Generic API Key.Please verify if the detected 'Generic API Key' is a placeholder or test data. If it's sensitive, consider removing or securing it.
protocol/testing/containertest/preupgrade_genesis.json (1)
Line range hint
675-675: Potential exposure of a Generic API Key.It appears that an API key might be embedded within the configuration or data fields. Exposing API keys in public repositories or configurations can lead to unauthorized access to services. It is recommended to remove the key from the codebase and rotate it if it has been committed previously. Consider using environment variables or secure vault solutions to manage sensitive keys.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/genesis.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts (1 hunks)
- proto/dydxprotocol/revshare/genesis.proto (1 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/listing/keeper/keeper.go (1 hunks)
- protocol/x/listing/keeper/msg_server_test.go (1 hunks)
- protocol/x/revshare/client/cli/query.go (1 hunks)
- protocol/x/revshare/keeper/grpc_query.go (1 hunks)
- protocol/x/revshare/keeper/msg_server_test.go (1 hunks)
- protocol/x/revshare/types/errors.go (1 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts
- protocol/x/listing/keeper/keeper.go
Files skipped from review as they are similar to previous changes (6)
- proto/dydxprotocol/revshare/genesis.proto
- protocol/app/testdata/default_genesis_state.json
- protocol/x/revshare/client/cli/query.go
- protocol/x/revshare/keeper/grpc_query.go
- protocol/x/revshare/keeper/msg_server_test.go
- protocol/x/revshare/types/errors.go
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts
[error] 5-5: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/genesis.ts
[error] 5-5: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
[error] 8-8: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
Gitleaks
protocol/testutil/constants/genesis.go
342-342: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/scripts/genesis/sample_pregenesis.json
538-538: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/testing/containertest/preupgrade_genesis.json
675-675: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (6)
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts (1)
14-18: Ensure that thecreateRpcQueryExtensionfunction returns the expected value. Currently, it returns an empty object, which might not be the intended functionality.Please verify if the empty return object is intentional and aligns with the expected behavior.
protocol/x/listing/keeper/msg_server_test.go (2)
8-9: The import changes reflect the updated module structure and are correctly adjusted.
16-16: Initialization ofListingKeeperis correctly done in the test setup function.protocol/testutil/constants/genesis.go (1)
1351-1351: Added an emptyrevshareobject toGenesisState.This change aligns with the PR's objective to initialize the
revsharemodule. It's crucial to ensure that this module is properly integrated and tested in all relevant environments.protocol/scripts/genesis/sample_pregenesis.json (1)
1754-1754: Initialization of therevsharemodule with an empty object is correct as per the PR objectives.protocol/testing/containertest/preupgrade_genesis.json (1)
2181-2181: Initialization ofrevsharemodule in the genesis state.The addition of an empty
revshareobject ensures that the new module is recognized and initialized properly during the genesis state setup. This change aligns with the PR objectives to integrate therevsharemodule into the system. Please ensure that the corresponding module logic and handlers are implemented to handle this new state.
| import { QueryClient, createProtobufRpcClient } from "@cosmjs/stargate"; | ||
| /** Query defines the gRPC querier service. */ | ||
|
|
||
| export interface Query {} |
There was a problem hiding this comment.
Replace the empty interface with a type alias for better clarity and to adhere to best practices.
- export interface Query {}
+ export type Query = any; // Consider specifying a more appropriate type if available.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.
| export interface Query {} | |
| export type Query = any; // Consider specifying a more appropriate type if available. |
Tools
Biome
[error] 5-5: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
| import { DeepPartial } from "../../helpers"; | ||
| /** GenesisState defines `x/revshare`'s genesis state. */ | ||
|
|
||
| export interface GenesisState {} |
There was a problem hiding this comment.
Replace the empty interfaces with type aliases to adhere to best practices and improve code clarity.
- export interface GenesisState {}
- export interface GenesisStateSDKType {}
+ export type GenesisState = any; // Consider specifying a more appropriate type if available.
+ export type GenesisStateSDKType = any; // Consider specifying a more appropriate type if available.Also applies to: 8-8
Tools
Biome
[error] 5-5: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
Changelist
Add the skeleton for the x/revshare module which will hold all logic and state related to trading fee revenue sharing
Test Plan
None
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
New Features
revsharemodule to support revenue sharing.revsharemodule.revsharemodule.Chores
revshareobjects to various genesis and test data JSON files.revsharemodule.Tests
MsgServerimplementation in therevsharemodule.