[TRA-433] Add MarketMapperRevShareDetails state and associated functions #1744
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 47 minutes and 33 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. WalkthroughThe changes involve renaming, reordering, and restructuring imports, introducing new functionalities and tests related to market mapper revenue share details, and updating protocols. New files facilitate querying and managing these market mapper revenue share details via gRPC and LCD clients, enhancing the revenue share module's capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LCDQueryClient
participant MarketMapperRevShareService
participant RevShareKeeper
User->>LCDQueryClient: marketMapperRevShareDetails(market_id)
LCDQueryClient->>MarketMapperRevShareService: MarketMapperRevShareDetails(QueryMarketMapperRevShareDetails)
MarketMapperRevShareService->>RevShareKeeper: GetMarketMapperRevShareDetails(market_id)
RevShareKeeper-->>MarketMapperRevShareService: MarketMapperRevShareDetailsResponse(details)
MarketMapperRevShareService-->>LCDQueryClient: MarketMapperRevShareDetailsResponse(details)
LCDQueryClient-->>User: Revenue Share Details
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/revshare/types/revshare.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/revshare.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/revshare.proto (1 hunks)
- protocol/x/revshare/keeper/revshare.go (2 hunks)
- protocol/x/revshare/keeper/revshare_test.go (1 hunks)
- protocol/x/revshare/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
- indexer/packages/v4-protos/src/codegen/google/bundle.ts
- protocol/x/revshare/types/keys.go
Additional comments not posted (5)
proto/dydxprotocol/revshare/revshare.proto (1)
6-11: LGTM! TheMarketMapperRevShareDetailsmessage is well-defined with clear documentation.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/revshare.ts (1)
8-64: LGTM! The TypeScript interfaces and functions forMarketMapperRevShareDetailsare correctly implemented with proper type handling.protocol/x/revshare/keeper/revshare_test.go (1)
13-69: LGTM! The unit tests for market rev share functions are thorough. However, please verify the deterministic nature of the block time used in the tests.Verification successful
To verify if the block time calls are deterministic in the tests, we need to understand how the block time is managed in the context (
ctx). The context is initialized usingtestapp.NewTestAppBuilder(t).Build().InitChain(). Let's inspect thetestapppackage to see howctx.BlockTime()is set.
The previous script failed to locate the
testapppackage, resulting in no output. Let's correct the approach to find thetestapppackage and inspect its contents to understand how the context (ctx) and block time are managed.
The previous attempt to locate the
testapppackage and inspect its contents was unsuccessful due to incorrect assumptions about the file naming conventions. To address this, we should search for theNewTestAppBuilderfunction definition directly, which will help us pinpoint the exact file and understand the context initialization.
We have located the
NewTestAppBuilderfunction definition inprotocol/testutil/app/app.go. Now, let's inspect this file to understand how the context (ctx) and block time are managed.
The
NewTestAppBuilderfunction inprotocol/testutil/app/app.goinitializes the context with a genesis time set to a specific value (time.Unix(0, 1)). This suggests that the block time can be deterministic if the tests are designed to use this initial context consistently.However, to ensure that the block time remains deterministic throughout the test execution, it is crucial to verify that no other parts of the code modify the block time in a non-deterministic manner.
Given this information, the block time used in the tests appears to be deterministic based on the initial genesis time configuration. Therefore, the review comment's concern about the deterministic nature of the block time can be considered addressed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if block time calls are deterministic in tests. # Test: Search for non-deterministic block time usage. Expect: Details on how block time is managed. grep -r "BlockTime" protocol/x/revshare/keeper/Length of output: 264
Script:
#!/bin/bash # Description: Inspect the testapp package to understand context initialization and block time management. # Locate the testapp package testapp_path=$(fd testapp | grep -m 1 '.go$' | xargs dirname) # Display the content of the testapp package to understand context initialization cat ${testapp_path}/*.goLength of output: 188
Script:
#!/bin/bash # Description: Inspect the testapp package to understand context initialization and block time management. # Locate the testapp package by searching for go files containing 'testapp' fd -e go -x grep -l 'testapp' | xargs dirname | sort -u | xargs -I {} sh -c 'echo {}; cat {}/testapp*.go'Length of output: 3795
Script:
#!/bin/bash # Description: Locate the definition of the NewTestAppBuilder function to understand context initialization and block time management. # Search for the definition of the NewTestAppBuilder function fd -e go -x grep -l 'NewTestAppBuilder' | xargs grep -H 'func NewTestAppBuilder'Length of output: 165
Script:
#!/bin/bash # Description: Inspect the content of the app.go file to understand context initialization and block time management. # Display the content of app.go to review the NewTestAppBuilder function and related context initialization cat ./protocol/testutil/app/app.goLength of output: 53096
protocol/x/revshare/keeper/revshare.go (1)
40-84: LGTM! The functions for managing market rev share details are correctly implemented. Please ensure that there are no data races due to concurrent access to the KV store.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
79-84: Renaming and addition of imports related to therevsharemodule appear correctly integrated into the namespace exports. Ensure that any dependent modules or services are updated to reflect these changes.Verification successful
The verification process has confirmed that the new import paths are correctly used in the codebase. Both
revshare/revshareandrevshare/txare present in the updated imports. Additionally, the old import pathrevshare/txis still present but in a different context (revshare/tx.rpc.msg), which is expected and correct.
revshare/revshareimport is correctly added.revshare/tximport is correctly added and used.No issues were found with the integration of the new import paths.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new and renamed imports are correctly used throughout the codebase. # Test: Search for old import usage. Expect: No occurrences of the old import path. rg --type typescript $'revshare/tx' --files-without-matchLength of output: 91
Script:
#!/bin/bash # Description: Verify that the new and renamed imports are correctly used throughout the codebase. # Test 1: Search for old import usage. Expect: No occurrences of the old import path. rg 'revshare/tx' --type ts # Test 2: Search for new import usage. Expect: Occurrences of the new import paths. rg 'revshare/revshare' --type ts rg 'revshare/tx' --type tsLength of output: 890
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/revshare/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (5)
- 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)
- proto/dydxprotocol/revshare/query.proto (1 hunks)
- protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details.go (1 hunks)
- protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go (1 hunks)
Additional comments not posted (7)
proto/dydxprotocol/revshare/query.proto (1)
16-21: The message definitions are appropriately structured for the query and response. Ensure that fields likemarket_idinQueryMarketMapperRevShareDetailsare adequately validated on the server side to handle potential invalid inputs.indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.ts (6)
1-1: Ensure all necessary imports are present and correctly referenced.
6-15: The interfaces for querying market mapper details are well-defined and follow consistent naming conventions.
18-25: The response type interfaces are correctly defined and include optional chaining for details, which is a good practice for potentially undefined data.
27-40: ThecreateBaseQueryMarketMapperRevShareDetailsfunction correctly initializes themarketIdto a default value of 0. This ensures that the structure is always in a valid state when instantiated.
72-115: The response handling functions, including base creation, encoding, decoding, and partial handling, are well-implemented. Ensure that thedetailsfield handling is robust, particularly in scenarios where it might be null or undefined.Verification successful
The handling of the
detailsfield in the response functions is robust, with checks forundefinedandnullvalues consistently applied across the project.
detailsfield handling in the provided code snippet is correct.- Multiple instances in the project show proper handling of
detailswith checks forundefinedandnull.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify robust handling of the `details` field in response functions. # Test: Search for uses of `details` field in the project to ensure it's handled properly everywhere. rg --type ts "details" --files-with-matches | xargs cat | rg "null|undefined"Length of output: 4356
33-69: The encoding and decoding functions are implemented correctly. However, ensure comprehensive testing, especially edge cases where unexpected data types might be passed to these functions.
protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go
Outdated
Show resolved
Hide resolved
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.rpc.Query.ts
Show resolved
Hide resolved
protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go
Show resolved
Hide resolved
| func (k Keeper) CreateNewMarketRevShare(ctx sdk.Context, marketId uint32) (err error) { | ||
| revShareParams := k.GetMarketMapperRevenueShareParams(ctx) | ||
|
|
||
| validDurationSeconds := int64(revShareParams.ValidDays * 24 * 60 * 60) |
There was a problem hiding this comment.
Unix time is int64 so this needs to be the same for the addition. The result later gets converted to uint64 which is fine because its not possible for these to be negative
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
There was a problem hiding this comment.
Actionable comments posted: 0
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/revshare.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/lcd.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/revshare.ts (1 hunks)
- proto/dydxprotocol/revshare/query.proto (1 hunks)
- proto/dydxprotocol/revshare/revshare.proto (1 hunks)
- protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (2 hunks)
- protocol/x/revshare/keeper/revshare_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/revshare.ts
- proto/dydxprotocol/revshare/query.proto
- proto/dydxprotocol/revshare/revshare.proto
- protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go
- protocol/x/revshare/keeper/revshare.go
- protocol/x/revshare/keeper/revshare_test.go
Additional comments not posted (1)
indexer/packages/v4-protos/src/codegen/dydxprotocol/lcd.ts (1)
42-44: Integration ofrevshareservice is correctly implemented.The addition follows the established pattern of dynamically importing and initializing
LCDQueryClientinstances for new services. This ensures consistency across service integrations within thecreateLCDClientfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/revshare/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.lcd.ts (1 hunks)
Additional comments not posted (3)
indexer/packages/v4-protos/src/codegen/dydxprotocol/revshare/query.lcd.ts (3)
1-3: Well-organized imports.The imports are clearly structured and directly relevant to the functionality of the
LCDQueryClient. Keeping imports minimal and relevant helps maintain clarity and reduce potential conflicts.
6-13: Constructor and method binding reviewed.The constructor is straightforward, taking a
requestClientand assigning it to thereqproperty. The binding ofmarketMapperRevShareDetailsis a good practice to ensure that the method retains the correctthiscontext when used as a callback.
17-20: Review ofmarketMapperRevShareDetailsmethod.The method
marketMapperRevShareDetailsis well-implemented. It dynamically constructs the endpoint URL using themarketIdparameter and uses thereqproperty to perform a GET request. This design is efficient and makes the function flexible to changes in the endpoint structure.
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
…ons (#1744) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
Changelist
Add MarketMapperRevShareDetails proto, state and associated functions
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
Release Notes
New Features
LCDQueryClientfor querying market mapper revenue share details via LCD client.Refactor
Tests