[TRA-417] Allocate market mapper revshare for appropriate markets #1773
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
|
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 29 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 introduce a new 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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (23)
- protocol/app/app.go (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/prices.go (2 hunks)
- protocol/testutil/keeper/sending.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (7 hunks)
- protocol/x/clob/keeper/process_single_match.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/prices/keeper/keeper.go (2 hunks)
- protocol/x/prices/keeper/market.go (1 hunks)
- protocol/x/prices/types/expected_keepers.go (2 hunks)
- protocol/x/revshare/keeper/revshare.go (1 hunks)
- protocol/x/subaccounts/genesis_test.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_collateral_pool_test.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_subaccount_test.go (2 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (1 hunks)
- protocol/x/subaccounts/keeper/keeper.go (3 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (2 hunks)
- protocol/x/subaccounts/keeper/safety_heap_state_test.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (10 hunks)
- protocol/x/subaccounts/keeper/transfer.go (1 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (18 hunks)
- protocol/x/subaccounts/module_test.go (1 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
Additional comments not posted (36)
protocol/x/prices/types/expected_keepers.go (1)
24-28: New method added toRevShareKeeperinterface.The method
SetMarketMapperRevShareDetailshas been added to handle the setting of market mapper revenue share details. Ensure that all implementations of this interface are updated to include this new method.protocol/x/subaccounts/keeper/keeper.go (1)
22-22: Integration ofRevShareKeeperinKeeperstruct.The
revShareKeeperfield has been added to theKeeperstruct and is properly initialized in theNewKeeperfunction. This change aligns with the system's expansion to include revenue sharing capabilities.Also applies to: 34-34, 44-44
protocol/x/subaccounts/genesis_test.go (1)
37-37: Updated test function to include new parameter.The
SubaccountsKeepersfunction now correctly includes an additional parameter, reflecting the integration of theRevShareKeeper. Ensure that the test logic accounts for the new keeper's behavior.protocol/x/subaccounts/keeper/grpc_query_collateral_pool_test.go (1)
55-55: Updated test function to accommodate new parameter.The
SubaccountsKeepersfunction call inTestQueryCollateralPoolAddresscorrectly includes an additional parameter for theRevShareKeeper. Ensure that the test cases are adjusted to account for any new logic introduced by this keeper.protocol/x/subaccounts/types/expected_keepers.go (1)
100-109: NewRevShareKeeperinterface added.The
RevShareKeeperinterface has been introduced with the methodGetMarketMapperRevenueShareForMarket. This method is essential for retrieving revenue share details for a market, aligning with the system's new functionalities.protocol/x/prices/keeper/keeper.go (2)
30-30: Field naming consistency improvedRenaming
revShareKeepertoRevShareKeeperin theKeeperstruct improves consistency with Go naming conventions, where exported fields should start with an uppercase letter.
55-55: Constructor updated to accommodate new dependencyThe addition of
RevShareKeeperas a parameter in theNewKeeperconstructor is correctly implemented. This change ensures that theRevShareKeeperis properly initialized when a newKeeperinstance is created.protocol/x/clob/types/expected_keepers.go (1)
76-81: New method added toSubaccountsKeeperinterfaceThe addition of the
DistributeFeesmethod to theSubaccountsKeeperinterface is appropriate for the new functionality related to fee distribution. This change aligns with the overall feature enhancement described in the PR.protocol/x/subaccounts/keeper/grpc_query_subaccount_test.go (1)
22-22: Updated test setup to accommodate new parameterThe test setup has been correctly updated to include the new parameter in the
SubaccountsKeepersfunction. This ensures that the tests remain valid and are able to test the new functionality.Also applies to: 93-93
protocol/testutil/keeper/sending.go (1)
95-95: Proper initialization ofSubaccountsKeeperwith new parameterThe conditional initialization of
SubaccountsKeepernow includes therevShareKeeperwhensaKeeperisnil. This ensures that all dependencies are correctly set up during testing, reflecting the changes in the main codebase.protocol/x/subaccounts/keeper/safety_heap_state_test.go (3)
14-14: Code Review: Updated function call to includeRevShareKeeper.The function call has been correctly updated to include the new
RevShareKeeperparameter. This change aligns with the modifications made across the repository to integrateRevShareKeeper.
63-63: Code Review: Consistent update in function calls.Similar to previous changes, the function call here is updated to pass the
RevShareKeeper. This maintains consistency across test setups.
112-112: Code Review: Updated function signature in test setup.The test setup function
SubaccountsKeepershas been updated consistently to include theRevShareKeeperacross all tests in this file.protocol/x/prices/keeper/market.go (1)
93-93: Code Review: Updated method call toRevShareKeeper.The method call
CreateNewMarketRevSharehas been updated to use the capitalized field nameRevShareKeeper, ensuring it adheres to Go's convention for exported fields. This change is part of a larger refactoring to improve code quality and ensure consistency.protocol/testutil/keeper/subaccounts.go (2)
Line range hint
35-44: Code Review: Comprehensive update to accommodateRevShareKeeper.The changes in this file correctly integrate
RevShareKeeperinto the test setup functions for subaccounts keeper. This includes passing theRevShareKeeperto other related keepers and ensuring it is initialized correctly. These changes are crucial for maintaining the integrity of the test environment and ensuring that new functionality related to revenue sharing is correctly tested.Also applies to: 56-80, 85-91
Line range hint
102-121: Code Review: Correct initialization ofRevShareKeeperin helper function.The helper function
createSubaccountsKeeperhas been correctly updated to includeRevShareKeeper, ensuring that all dependencies are properly initialized. This change is vital for the correct operation of the subaccounts keeper in the context of revenue sharing.protocol/x/subaccounts/module_test.go (1)
42-42: Code Review: Updated function call in module test setup.The function call in
createAppModuleWithKeeperhas been updated to include theRevShareKeeper. This change is consistent with the updates across other test files and is necessary for ensuring that the module tests have access to the correctly initialized keepers.protocol/testutil/keeper/clob.go (1)
141-141: Code Review: Correct integration ofRevShareKeeperin CLOB test setup.The function
createSubaccountsKeeperwithin the CLOB test setup has been updated to includeRevShareKeeper. This change ensures that the subaccounts keeper is correctly initialized with all necessary dependencies, including the new revenue sharing functionality, which is critical for the correct operation of the CLOB module in tests.protocol/testutil/keeper/prices.go (2)
7-7: Import Addition:revsharetypesThe import of
revsharetypesis appropriate given the usage ofRevShareKeepermethods in this file.
121-126: Set Market Mapper Revenue Share DetailsThis block of code correctly sets the revenue share details for each market to have no revenue share. This is consistent with the PR's objective to manage revenue shares more dynamically.
protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (1)
270-273: Ensure Proper Initialization of Test EnvironmentThe test setup includes initializing subaccounts, prices, perpetuals, and blocktime keepers, which is essential for the comprehensive testing of withdrawal and transfer block queries. This change reflects an increase in the number of return parameters from
SubaccountsKeepers, likely due to the inclusion ofRevShareKeeper.protocol/x/clob/keeper/process_single_match.go (2)
429-431: Consider optimizing repeated data retrieval:The TODO comment suggests optimizing the retrieval of perpetual objects, which is a valid point given the potential performance impact in high-frequency trading environments.
[REFACTOR_SUGGESTS]
439-455: Refactor suggested for fee distribution logic:The commented-out block for fee transfer and the new
DistributeFeesmethod call indicate a change in how fees are handled. It's crucial to ensure that this new method correctly implements all intended fee logic, especially in a financial application where accurate fee handling is critical.- //if err := k.subaccountsKeeper.TransferFeesToFeeCollectorModule( - // ctx, - // assettypes.AssetUsdc.Id, - // bigTotalFeeQuoteQuantums, - // perpetualId, - //); err != nil { - // return takerUpdateResult, makerUpdateResult, errorsmod.Wrapf( - // types.ErrSubaccountFeeTransferFailed, - // "persistMatchedOrders: subaccounts (%v, %v) updated, but fee transfer (bigFeeQuoteQuantums: %v)"+ - // " to fee-collector failed. Err: %v", - // matchWithOrders.MakerOrder.GetSubaccountId(), - // matchWithOrders.TakerOrder.GetSubaccountId(), - // bigTotalFeeQuoteQuantums, - // err, - // ) - //} + if err := k.subaccountsKeeper.DistributeFees( + ctx, + assettypes.AssetUsdc.Id, + bigTotalFeeQuoteQuantums, + perpetualId, +); err != nil { + return takerUpdateResult, makerUpdateResult, errorsmod.Wrapf( + types.ErrSubaccountFeeTransferFailed, + "persistMatchedOrders: subaccounts (%v, %v) updated, but fee transfer (bigFeeQuoteQuantums: %v)"+ + " to fee-collector failed. Err: %v", + matchWithOrders.MakerOrder.GetSubaccountId(), + matchWithOrders.TakerOrder.GetSubaccountId(), + bigTotalFeeQuoteQuantums, + err, + ) + }protocol/x/subaccounts/keeper/transfer_test.go (2)
23-23: Include package import documentation.It's a good practice to include comments describing why certain packages are imported, especially for less obvious dependencies.
1598-1598: Ensure comprehensive testing of new functionality.The function
TestTransferInsuranceFundPaymentsintroduces significant new logic. Ensure that all edge cases are covered, especially those involving error states and boundary conditions.Verification successful
Ensure comprehensive testing of new functionality.
The
TestTransferInsuranceFundPaymentsfunction in theprotocol/x/subaccounts/keeper/transfer_test.gofile appears to cover a variety of scenarios, including successful transfers, failures due to insufficient funds, and panics when assets do not exist. These tests encompass a broad range of edge cases and error states, indicating a thorough approach to testing.
- Success cases: Sending to and from the insurance fund, including zero payments and isolated insurance fund accounts.
- Failure cases: Insufficient funds in subaccounts and insurance fund, and non-existent assets leading to panics.
The current test cases seem to comprehensively cover the new logic introduced by the
TestTransferInsuranceFundPaymentsfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all edge cases for insurance fund payments are covered. # Test: Search for test cases in the function. Expect: Comprehensive coverage including edge cases. rg --type python $'TestTransferInsuranceFundPayments'Length of output: 83
Script:
#!/bin/bash # Locate the TestTransferInsuranceFundPayments function in the transfer_test.go file and extract its content. ast-grep --lang go --pattern 'func TestTransferInsuranceFundPayments($_) $_' --json | jq -r '.[].text'Length of output: 8396
protocol/app/app.go (1)
1033-1033: Ensure proper initialization ofRevShareKeeper.The
RevShareKeeperis added as a parameter to theNewfunction and subsequently used to initialize theAppstruct. This is a critical integration point for the revenue sharing functionality.Ensure that all dependencies and configurations required by
RevShareKeeperare correctly set up before this point.protocol/x/subaccounts/keeper/subaccount_test.go (10)
156-156: Ensure consistency in the number of parameters forSubaccountsKeepersacross all tests.The addition of extra parameters in the
SubaccountsKeepersfunction call is consistent with the changes described in the PR and AI-generated summary. Ensure that all tests correctly handle these new parameters.
191-191: Verify that the new parameters are integrated correctly inTestSubaccountGet.The test function
TestSubaccountGethas been updated to include additional parameters. Verify that these parameters are used appropriately within the test to maintain its integrity.
205-205: Check the integration of new parameters inTestSubaccountSet_Empty.This change reflects the updated function signature. Confirm that the additional parameters do not alter the expected behavior of the test.
223-223: Confirm correct parameter handling inTestSubaccountGetNonExistent.The test has been adapted to include more parameters, which should be carefully managed to ensure the test's purpose remains clear and effective.
237-237: Ensure the additional parameters are properly utilized inTestGetAllSubaccount.The integration of additional parameters in the test setup should be checked to ensure they are relevant and correctly used within the test's context.
278-278: Review parameter expansion inTestGetAllSubaccountfor specific scenarios.The addition of extra parameters should support the test's requirements without introducing inconsistencies or errors in the expected outcomes.
2790-2790: Assess the expanded parameter list in complex test scenarios.The test has been modified to include a more extensive parameter list, which must be correctly handled to preserve the integrity and purpose of the test.
[APROVED]
4234-4234: Ensure the extended parameters are correctly utilized in varied test environments.This modification introduces additional complexity in parameter management, which should be carefully reviewed to ensure the test remains effective.
5345-5348: Check for correct handling of parameters in dynamic test setups.This test function's modification involves more parameters, which need to be correctly integrated to maintain the test's functionality and reliability.
5784-5787: Confirm the effective integration of additional parameters in complex test scenarios.The test setup now includes more parameters, which must be managed effectively to ensure the test achieves its intended outcomes without introducing errors.
| func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) ( | ||
| address sdk.AccAddress, | ||
| revenueSharePpm uint32, | ||
| err error, | ||
| ) { | ||
| // get the revenue share details for the market | ||
| revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| // check if the rev share details are expired | ||
| if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) { | ||
| return nil, 0, nil | ||
| } | ||
|
|
||
| // Get revenue share params | ||
| revShareParams := k.GetMarketMapperRevenueShareParams(ctx) | ||
|
|
||
| revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| return revShareAddr, revShareParams.RevenueSharePpm, nil | ||
| } |
There was a problem hiding this comment.
New method to retrieve market mapper revenue share
The method GetMarketMapperRevenueShareForMarket is well implemented. It fetches the revenue share details and checks for expiration. However, it returns nil instead of an empty sdk.AccAddress when an error occurs, which could lead to potential issues if not handled properly by the caller.
- return nil, 0, err
+ return sdk.AccAddress{}, 0, errCommittable 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.
| func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) ( | |
| address sdk.AccAddress, | |
| revenueSharePpm uint32, | |
| err error, | |
| ) { | |
| // get the revenue share details for the market | |
| revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId) | |
| if err != nil { | |
| return nil, 0, err | |
| } | |
| // check if the rev share details are expired | |
| if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) { | |
| return nil, 0, nil | |
| } | |
| // Get revenue share params | |
| revShareParams := k.GetMarketMapperRevenueShareParams(ctx) | |
| revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address) | |
| if err != nil { | |
| return nil, 0, err | |
| } | |
| return revShareAddr, revShareParams.RevenueSharePpm, nil | |
| } | |
| func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) ( | |
| address sdk.AccAddress, | |
| revenueSharePpm uint32, | |
| err error, | |
| ) { | |
| // get the revenue share details for the market | |
| revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId) | |
| if err != nil { | |
| return sdk.AccAddress{}, 0, err | |
| } | |
| // check if the rev share details are expired | |
| if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) { | |
| return sdk.AccAddress{}, 0, nil | |
| } | |
| // Get revenue share params | |
| revShareParams := k.GetMarketMapperRevenueShareParams(ctx) | |
| revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address) | |
| if err != nil { | |
| return sdk.AccAddress{}, 0, err | |
| } | |
| return revShareAddr, revShareParams.RevenueSharePpm, nil | |
| } |
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/testutil/keeper/subaccounts.go (7 hunks)
- protocol/x/clob/keeper/process_single_match.go (1 hunks)
- protocol/x/subaccounts/keeper/transfer.go (2 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (17 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/testutil/keeper/subaccounts.go
- protocol/x/clob/keeper/process_single_match.go
- protocol/x/subaccounts/keeper/transfer.go
- protocol/x/subaccounts/keeper/transfer_test.go
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/clob/types/expected_keepers.go (2 hunks)
- protocol/x/subaccounts/keeper/transfer.go (2 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/transfer.go
- protocol/x/subaccounts/keeper/transfer_test.go
Additional comments not posted (1)
protocol/x/clob/types/expected_keepers.go (1)
75-80: New MethodDistributeFeesAdded toSubaccountsKeeperThe addition of the
DistributeFeesmethod in theSubaccountsKeeperinterface aligns with the PR's objective to enhance fee distribution mechanisms. This method appears to be well-defined with appropriate parameters for asset ID, quantum amounts, and perpetual ID. Ensure that all implementations of this interface are updated to include this new method to avoid runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- protocol/app/app.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (7 hunks)
- protocol/x/clob/keeper/process_single_match.go (1 hunks)
- protocol/x/subaccounts/genesis_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (12 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (18 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- protocol/app/app.go
- protocol/testutil/keeper/subaccounts.go
- protocol/x/clob/keeper/process_single_match.go
- protocol/x/subaccounts/genesis_test.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/types/expected_keepers.go
Additional comments not posted (4)
protocol/x/subaccounts/keeper/transfer_test.go (4)
24-24: Approved the addition of therevsharetypesimport.This is necessary for the new revenue sharing functionality as indicated by the PR objectives.
197-197: Ensure test setup is properly abstracted.The repetitive setup code across different test cases (like creating test markets, liquidity tiers, etc.) should ideally be abstracted into helper functions to enhance readability and maintainability.
458-458: Consider abstracting repetitive test setup code.Similar to the previous test function, consider abstracting repetitive setup code into helper functions to reduce code duplication and improve maintainability.
731-731: Abstract repetitive test setup into helper functions.The setup code for creating test markets and perpetuals is repeated across tests. Abstracting these into helper functions can reduce code duplication and improve readability.
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/revshare/keeper/revshare.go (3 hunks)
- protocol/x/revshare/keeper/revshare_test.go (2 hunks)
- protocol/x/revshare/types/errors.go (1 hunks)
- protocol/x/subaccounts/keeper/transfer.go (3 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/revshare/keeper/revshare.go
- protocol/x/subaccounts/keeper/transfer.go
Additional comments not posted (8)
protocol/x/revshare/types/errors.go (1)
18-22: Error Definition ApprovedThe error
ErrMarketMapperRevShareDetailsNotFoundis clearly defined and follows the existing pattern of error definitions in this module. The error code3seems appropriately sequenced following the existing error codes1and2.protocol/x/revshare/keeper/revshare_test.go (1)
82-169: Comprehensive Testing of Revenue Share RetrievalThe test function
TestGetMarketMapperRevenueShareForMarketeffectively tests both valid and invalid scenarios for retrieving revenue share details. The use of a map to define test cases is a good practice for clarity and extensibility. However, the commented-out test case for expired market revenue shares ("expired market rev share") should either be implemented or removed to avoid confusion.- // TODO: investigate why tApp blocktime doesn't translate to ctx.BlockTime() - //"expired market rev share": { - // revShareParams: types.MarketMapperRevenueShareParams{ - // Address: constants.AliceAccAddress.String(), - // RevenueSharePpm: 100_000, // 10% - // ValidDays: 0, - // }, - // marketId: 42, - // expirationDelta: -10, - // setRevShareDetails: true, - // - // expectedMarketMapperAddr: nil, - // expectedRevenueSharePpm: 0, - //}, + // Implement or remove the commented-out test case for clarity.Additionally, consider verifying the block time synchronization issue noted in the TODO comment, as it may affect the reliability of the test outcomes.
protocol/x/subaccounts/keeper/transfer_test.go (6)
197-197: Ensure test setup is properly abstracted.The repetitive setup code across different test cases (like creating test markets, liquidity tiers, etc.) should ideally be abstracted into helper functions to enhance readability and maintainability.
458-458: Consider abstracting repetitive test setup code.Similar to the previous test function, consider abstracting repetitive setup code into helper functions to reduce code duplication and improve maintainability.
731-731: Abstract repetitive test setup into helper functions.The setup code for creating test markets and perpetuals is repeated across tests. Abstracting these into helper functions can reduce code duplication and improve readability.
1057-1057: Refactor repetitive setup code into helper functions.The setup code for creating test accounts, markets, and liquidity tiers is being repeated. Consider creating helper functions to handle these operations to improve code readability and reduce duplication.
1179-1179: Clarify the purpose of new fields and methods in documentation.The introduction of new fields such as
marketMapperAccBalanceand methods likeTestTransferFeesToFeeCollectorModuleshould be accompanied by comments explaining their purpose and usage, especially when they involve complex financial calculations or business logic.Would you like me to help draft these explanations?
1415-1415: Document new test scenarios thoroughly.This test function introduces complex scenarios involving fee distribution and market mappers. It would be beneficial to include detailed comments describing each test case, the expected behaviors, and the setup required.
If you need assistance in documenting these scenarios, please let me know.
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go (1 hunks)
- protocol/x/revshare/keeper/revshare_test.go (3 hunks)
- protocol/x/subaccounts/keeper/transfer.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/revshare/keeper/revshare_test.go
- protocol/x/subaccounts/keeper/transfer.go
Additional comments not posted (1)
protocol/x/revshare/keeper/grpc_query_marketmapper_revshare_details_test.go (1)
43-43: Improved Error Handling in Test CaseThe change from
ErrorContainstoErrorIsis a significant improvement in testing practices. It ensures that the exact error typeErrMarketMapperRevShareDetailsNotFoundis returned, which is crucial for accurate and reliable unit tests.
Signed-off-by: Shrenuj Bansal <[email protected]>
Changelist
This change calculates the market mapper revenue share for a trade in a specific market and transfers the share amount to the market mapper address
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
Bug Fixes
Tests
Refactor