[TRA-415] implement market mapper rev share gov msg #1733
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
WalkthroughThe changes introduce a new revenue-sharing feature to the dydxprotocol, specifically focusing on the market mapper. It includes updates to protobuf definitions, Go code, and TypeScript code for RPC messages. New functionalities include setting market mapper revenue share parameters and handling related messages, along with necessary test cases for validation. Changes
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 (
|
Signed-off-by: Shrenuj Bansal <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
protocol/x/revshare/types/errors.go (1)
3-16: The error definitions look good. Consider adding documentation comments for each error to explain when they should be used, enhancing maintainability.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/params.ts (1)
5-20: Clarify documentation for better understanding.
The comments explaining the fields in theMarketMapperRevenueShareParamsinterface could be more descriptive to provide better clarity on how these parameters are used and their impact.protocol/scripts/genesis/sample_pregenesis.json (1)
Line range hint
538-538: Sensitive data exposure detected: Generic API Key. This should be removed or securely managed.- "exchange_config_json": "{\"exchanges\":[{\"exchangeName\":\"Binance\",\"ticker\":\"BTCUSDT\",\"adjustByMarket\":\"USDT-USD\"},{\"exchangeName\":\"Bybit\",\"ticker\":\"BTCUSDT\",\"adjustByMarket\":\"USDT-USD\"},{\"exchangeName\":\"CoinbasePro\",\"ticker\":\"BTC-USD\"},{\"exchangeName\":\"Huobi\",\"ticker\":\"btcusdt\",\"adjustByMarket\":\"USDT-USD\"},{\"exchangeName\":\"Kraken\",\"ticker\":\"XXBTZUSD\"},{\"exchangeName\":\"Kucoin\",\"ticker\":\"BTC-USDT\",\"adjustByMarket\":\"USDT-USD\"},{\"exchangeName\":\"Okx\",\"ticker\":\"BTC-USDT\",\"adjustByMarket\":\"USDT-USD\"}]}" + "exchange_config_json": "<REDACTED_FOR_SECURITY>"
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/params.pb.gois excluded by!**/*.pb.goprotocol/x/revshare/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (26)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/genesis.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/params.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.rpc.msg.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.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/params.proto (1 hunks)
- proto/dydxprotocol/revshare/tx.proto (1 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (2 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/lib/ante/internal_msg.go (2 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/revshare/genesis.go (1 hunks)
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go (1 hunks)
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share_test.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (1 hunks)
- protocol/x/revshare/types/errors.go (1 hunks)
- protocol/x/revshare/types/genesis.go (1 hunks)
- protocol/x/revshare/types/keys.go (1 hunks)
- protocol/x/revshare/types/params.go (1 hunks)
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts
[error] 22-22: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
[error] 25-25: 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)
Additional comments not posted (25)
indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1)
1-2: Ensure the updated import_112correctly points to the intended module version, and verify that all dependent code correctly handles the new import.protocol/x/revshare/types/keys.go (1)
11-16: The new constantMarketMapperRevenueShareParamsKeyis appropriately defined and well-placed within the module's constants. This should facilitate easy reference and maintenance.proto/dydxprotocol/revshare/genesis.proto (2)
4-5: The import statements are correctly updated to include necessary dependencies.
11-11: The addition of theparamsfield with non-nullability enforced is appropriate for ensuring robust configuration management.indexer/packages/v4-protos/src/codegen/google/bundle.ts (2)
1-6: The new imports are correctly added and appropriately named, ensuring no namespace conflicts and enabling new functionalities.
8-14: The restructuring of theapiandprotobufnamespaces using spread syntax is modern and efficient, facilitating easier extension and maintenance.protocol/x/revshare/genesis.go (2)
11-15: Proper initialization and error handling inInitGenesis. Usingpanicensures that initialization errors are not silently ignored, which is crucial for genesis configurations.
20-21: Correct implementation ofExportGenesisto ensure consistent genesis states across exports.proto/dydxprotocol/revshare/params.proto (1)
8-21: The definition ofMarketMapperRevenueShareParamsis clear and comprehensive. The use of specific annotations likecosmos_proto.scalarfor address validation and parts-per-million for revenue sharing ensures precision and correctness in parameter handling.protocol/x/revshare/types/params.go (1)
18-31: The validation logic inValidateis robust, ensuring that the address is correctly formatted and the revenue share is within a valid range. Good implementation of error handling.proto/dydxprotocol/revshare/tx.proto (1)
12-31: The service and message definitions intx.protoare clear and well-defined. The use of options likecosmos.msg.v1.signerand attributes likecosmos.AddressStringensure that the messages conform to expected standards and types.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.rpc.msg.ts (2)
6-8: Clear and concise documentation for thesetMarketMapperRevenueSharefunction.
18-21: Ensure that the encoding and RPC request logic is robust and handles errors appropriately.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/genesis.ts (2)
6-8: Ensure the optional nature ofparamsis consistently handled across interfaces and default creation.Also applies to: 11-13, 16-18
22-26: The encoding, decoding, and partial construction forGenesisStateare implemented correctly, ensuring robust type handling.Also applies to: 39-41, 52-54
protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share_test.go (1)
15-86: The test cases are well-structured and cover a variety of scenarios, including success and error cases.protocol/lib/ante/internal_msg.go (1)
28-28: Ensure consistent formatting and imports.
The import for therevsharepackage and the addition ofMsgSetMarketMapperRevenueShareto theIsInternalMsgfunction are correctly implemented. Ensure that the formatting and alignment with other case statements in the switch block are consistent.Also applies to: 115-116
protocol/app/msgs/internal_msgs.go (2)
29-29: Ensure that the newly added import forrevshareis used appropriately throughout the module.
171-172: The addition ofMsgSetMarketMapperRevenueShareand its response toInternalMsgSamplesDydxCustomaligns with the PR's objective to manage market mapper revenue shares. Ensure that these messages are integrated correctly in the message handling logic and tested thoroughly.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2)
77-83: The addition of imports forrevsharecomponents (genesis,params,query,tx) is crucial for supporting the new functionality introduced in this PR. Verify that these imports are correctly mapped in the corresponding namespace exports.
300-302: The namespace exports forrevsharehave been correctly updated to include the new components. This ensures that the new functionality is accessible as intended.protocol/app/testdata/default_genesis_state.json (1)
405-410: The initialization ofrevshareparameters in the genesis state is consistent with the new functionality. Ensure that these parameters (address,revenue_share_ppm,valid_days) are validated during the genesis state initialization to prevent configuration errors.protocol/app/msgs/all_msgs.go (1)
253-255: Ensure that the new message types are correctly integrated into the system.This addition aligns with the PR objectives to implement market mapper revenue share functionality. Please ensure these messages are appropriately handled wherever necessary in the application logic.
protocol/testutil/constants/genesis.go (1)
1353-1359: Initialization values forrevshareparameters are all set to zero. Please confirm if these are intended defaults or if they should be populated with initial values.protocol/scripts/genesis/sample_pregenesis.json (1)
1756-1762: The newrevshareconfiguration has been added as expected according to the PR summary.
| } | ||
|
|
||
| // Revenue share ppm must be less than 1000000 (100%) | ||
| if p.RevenueSharePpm >= 1000000 { |
There was a problem hiding this comment.
nit: Why can't it be 100%?
There was a problem hiding this comment.
Don't have strong opinion on this since it's never gonna be close to 100% but I was thinking of the implementation to always have something sent to the fee collector
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/revshare/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (5)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.rpc.msg.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts (1 hunks)
- proto/dydxprotocol/revshare/tx.proto (1 hunks)
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share_test.go (1 hunks)
- protocol/x/revshare/types/params.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.rpc.msg.ts
- proto/dydxprotocol/revshare/tx.proto
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share_test.go
- protocol/x/revshare/types/params.go
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts
[error] 22-22: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
[error] 25-25: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
Additional comments not posted (7)
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/tx.ts (7)
6-11: Interface Definition for MsgSetMarketMapperRevenueShare
The interface is well-defined with clear documentation and optional parameters. This flexibility allows for partial message construction which is useful in scenarios where not all data might be available at the time of message creation.
14-19: Interface Definition for MsgSetMarketMapperRevenueShareSDKType
Similar to the previous interface, this SDK-specific interface maintains consistency and clarity in its structure.
27-32: Base Message Creation Function
The functioncreateBaseMsgSetMarketMapperRevenueShareis effectively setting default values for the message creation, which is a common pattern in TypeScript to handle optional or partial data. Good implementation.
34-45: Message Encoding Function
The functionencodeinMsgSetMarketMapperRevenueShareis well-implemented with checks for non-default values before encoding, which is efficient as it avoids unnecessary data processing. This is a good practice in protocol buffer handling.
47-70: Message Decoding Function
This function handles decoding efficiently by checking the type of input and using a switch statement to handle different fields based on the protobuf tag. This is robust and ensures that each field is correctly decoded based on its tag.
73-78: Partial Message Construction Function
The methodfromPartialallows for the construction of a message from an incomplete data object. This is particularly useful in scenarios where messages are being constructed in stages or from various sources of input.
[APROVED]
82-112: Response Message Handling
The functions related toMsgSetMarketMapperRevenueShareResponseare minimal due to the simplicity of the response structure. This is appropriate given the context, as the response does not need to carry additional data.
| /** Response to a MsgSetMarketMapperRevenueShare */ | ||
|
|
||
| export interface MsgSetMarketMapperRevenueShareResponse {} |
There was a problem hiding this comment.
Replace empty interfaces with type aliases.
Per the static analysis tool's suggestion and previous comments, these interfaces should be converted to type aliases to simplify the code and adhere to TypeScript best practices.
- export interface MsgSetMarketMapperRevenueShareResponse {}
+ export type MsgSetMarketMapperRevenueShareResponse = {};
- export interface MsgSetMarketMapperRevenueShareResponseSDKType {}
+ export type MsgSetMarketMapperRevenueShareResponseSDKType = {};Also applies to: 25-25
Tools
Biome
[error] 22-22: An empty interface is equivalent to {}. (lint/suspicious/noEmptyInterface)
Safe fix: Use a type alias instead.
Signed-off-by: Shrenuj Bansal <[email protected]>
Changelist
Implement a governance msg to set the market mapper revenue share
Test Plan
Added tests
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
SetMarketMapperRevenueShare.Bug Fixes
Tests
Chores