[OTE-777][OTE-690][OTE-688] Update clob code to add revshare logic #2251
Conversation
WalkthroughThe changes across multiple files enhance the functionality related to revenue sharing within the application. Key modifications include the addition of parameters and return values for affiliate revenue sharing in various functions, updates to event structures, and adjustments to method signatures in keeper interfaces. These changes collectively aim to improve the handling of revenue share calculations and facilitate more detailed tracking and reporting. Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Additional comments not posted (5)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (1)
270-270: Update test to match the new function signature.The change in the number of underscore placeholders suggests that the
keepertest.SubaccountsKeepersfunction signature has been updated to return an additional value.Consider using named variables instead of underscore placeholders for better readability:
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, blocktimeKeeper, _, _, _ := keepertest.SubaccountsKeepers( t, true, )can be updated to:
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, blocktimeKeeper, _, _, newVar, _ := keepertest.SubaccountsKeepers( t, true, ) _ = newVarprotocol/x/clob/keeper/process_operations_test.go (2)
2638-2639: Consider adding a comment to clarify the purpose ofbig.NewInt(0).It's not immediately clear what this zero value represents. Adding a comment would improve readability and maintainability.
2658-2659: Consider adding a comment to clarify the purpose ofbig.NewInt(0).Similar to the previous comment, it would be helpful to explain what this zero value represents in the context of the
NewOrderFillEventfunction.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (81)
- protocol/app/app.go (1 hunks)
- protocol/indexer/events/order_fill.go (5 hunks)
- protocol/indexer/events/order_fill_test.go (5 hunks)
- protocol/mocks/AnteDecorator.go (1 hunks)
- protocol/mocks/AppOptions.go (1 hunks)
- protocol/mocks/BankKeeper.go (1 hunks)
- protocol/mocks/BridgeKeeper.go (1 hunks)
- protocol/mocks/BridgeQueryClient.go (1 hunks)
- protocol/mocks/BridgeServiceClient.go (1 hunks)
- protocol/mocks/CacheMultiStore.go (1 hunks)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/mocks/Configurator.go (1 hunks)
- protocol/mocks/DelayMsgKeeper.go (1 hunks)
- protocol/mocks/EthClient.go (1 hunks)
- protocol/mocks/ExchangeConfigUpdater.go (1 hunks)
- protocol/mocks/ExchangeQueryHandler.go (1 hunks)
- protocol/mocks/ExchangeToMarketPrices.go (1 hunks)
- protocol/mocks/ExtendVoteHandler.go (1 hunks)
- protocol/mocks/FileHandler.go (1 hunks)
- protocol/mocks/GrpcClient.go (1 hunks)
- protocol/mocks/GrpcServer.go (1 hunks)
- protocol/mocks/HealthCheckable.go (1 hunks)
- protocol/mocks/IndexerEventManager.go (1 hunks)
- protocol/mocks/IndexerMessageSender.go (1 hunks)
- protocol/mocks/Logger.go (1 hunks)
- protocol/mocks/MarketPairFetcher.go (1 hunks)
- protocol/mocks/MemClob.go (1 hunks)
- protocol/mocks/MemClobKeeper.go (4 hunks)
- protocol/mocks/MsgRouter.go (1 hunks)
- protocol/mocks/MultiStore.go (1 hunks)
- protocol/mocks/OracleClient.go (1 hunks)
- protocol/mocks/PerpetualsClobKeeper.go (1 hunks)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/mocks/PrepareBridgeKeeper.go (1 hunks)
- protocol/mocks/PrepareClobKeeper.go (1 hunks)
- protocol/mocks/PreparePerpetualsKeeper.go (1 hunks)
- protocol/mocks/PriceFeedMutableMarketConfigs.go (1 hunks)
- protocol/mocks/PriceFetcher.go (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (1 hunks)
- protocol/mocks/ProcessBridgeKeeper.go (1 hunks)
- protocol/mocks/ProcessClobKeeper.go (1 hunks)
- protocol/mocks/ProcessPerpetualKeeper.go (1 hunks)
- protocol/mocks/ProcessStakingKeeper.go (1 hunks)
- protocol/mocks/QueryClient.go (1 hunks)
- protocol/mocks/QueryServer.go (1 hunks)
- protocol/mocks/RequestHandler.go (1 hunks)
- protocol/mocks/SendingKeeper.go (1 hunks)
- protocol/mocks/Server.go (1 hunks)
- protocol/mocks/SubaccountsKeeper.go (3 hunks)
- protocol/mocks/TimeProvider.go (1 hunks)
- protocol/mocks/TxBuilder.go (1 hunks)
- protocol/mocks/TxConfig.go (1 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
- protocol/mocks/VaultKeeper.go (1 hunks)
- protocol/mocks/mocks/CacheMultiStore.go (13 hunks)
- protocol/mocks/mocks/MultiStore.go (13 hunks)
- protocol/testutil/keeper/clob.go (3 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (5 hunks)
- protocol/testutil/memclob/keeper.go (3 hunks)
- protocol/x/clob/e2e/long_term_orders_test.go (4 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (4 hunks)
- protocol/x/clob/keeper/process_operations_test.go (2 hunks)
- protocol/x/clob/keeper/process_single_match.go (17 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (4 hunks)
- protocol/x/clob/types/mem_clob_keeper.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (3 hunks)
- protocol/x/revshare/keeper/revshare_test.go (5 hunks)
- protocol/x/revshare/types/types.go (1 hunks)
- protocol/x/rewards/keeper/keeper.go (3 hunks)
- protocol/x/rewards/keeper/keeper_test.go (8 hunks)
- protocol/x/sending/client/cli/sending_cli_test.go (0 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)
Files not processed due to max files limit (7)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
- protocol/x/subaccounts/keeper/safety_heap_state_test.go
- protocol/x/subaccounts/keeper/safety_heap_test.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/keeper/transfer.go
- protocol/x/subaccounts/keeper/transfer_test.go
- protocol/x/subaccounts/module_test.go
Files not reviewed due to no reviewable changes (1)
- protocol/x/sending/client/cli/sending_cli_test.go
Files skipped from review due to trivial changes (53)
- protocol/mocks/AnteDecorator.go
- protocol/mocks/AppOptions.go
- protocol/mocks/BankKeeper.go
- protocol/mocks/BridgeKeeper.go
- protocol/mocks/BridgeQueryClient.go
- protocol/mocks/BridgeServiceClient.go
- protocol/mocks/CacheMultiStore.go
- protocol/mocks/Configurator.go
- protocol/mocks/DelayMsgKeeper.go
- protocol/mocks/EthClient.go
- protocol/mocks/ExchangeConfigUpdater.go
- protocol/mocks/ExchangeQueryHandler.go
- protocol/mocks/ExchangeToMarketPrices.go
- protocol/mocks/ExtendVoteHandler.go
- protocol/mocks/FileHandler.go
- protocol/mocks/GrpcClient.go
- protocol/mocks/GrpcServer.go
- protocol/mocks/HealthCheckable.go
- protocol/mocks/IndexerEventManager.go
- protocol/mocks/IndexerMessageSender.go
- protocol/mocks/Logger.go
- protocol/mocks/MarketPairFetcher.go
- protocol/mocks/MemClob.go
- protocol/mocks/MsgRouter.go
- protocol/mocks/MultiStore.go
- protocol/mocks/OracleClient.go
- protocol/mocks/PerpetualsClobKeeper.go
- protocol/mocks/PerpetualsKeeper.go
- protocol/mocks/PrepareBridgeKeeper.go
- protocol/mocks/PrepareClobKeeper.go
- protocol/mocks/PreparePerpetualsKeeper.go
- protocol/mocks/PriceFeedMutableMarketConfigs.go
- protocol/mocks/PriceFetcher.go
- protocol/mocks/PriceUpdateGenerator.go
- protocol/mocks/PricesKeeper.go
- protocol/mocks/ProcessBridgeKeeper.go
- protocol/mocks/ProcessClobKeeper.go
- protocol/mocks/ProcessPerpetualKeeper.go
- protocol/mocks/ProcessStakingKeeper.go
- protocol/mocks/QueryClient.go
- protocol/mocks/QueryServer.go
- protocol/mocks/RequestHandler.go
- protocol/mocks/SendingKeeper.go
- protocol/mocks/Server.go
- protocol/mocks/TimeProvider.go
- protocol/mocks/TxBuilder.go
- protocol/mocks/TxConfig.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
- protocol/mocks/VaultKeeper.go
- protocol/x/clob/memclob/memclob.go
- protocol/x/subaccounts/genesis_test.go
- protocol/x/subaccounts/keeper/grpc_query_collateral_pool_test.go
- protocol/x/subaccounts/keeper/grpc_query_subaccount_test.go
Additional comments not posted (76)
protocol/x/revshare/types/types.go (1)
37-37: LGTM!The addition of the
RevSharePpmfield to theRevSharestruct is a valid enhancement to store more granular revenue sharing metrics. The field name clearly conveys its purpose, and theuint32type is appropriate for representing PPM values. This change aligns with the PR objective of implementing revenue sharing logic.protocol/x/clob/types/mem_clob_keeper.go (1)
28-28: LGTM!The addition of the
affiliateRevSharesQuoteQuantumsparameter to theProcessSingleMatchmethod in theMemClobKeeperinterface is a well-designed change that enhances the interface's functionality to handle affiliate revenue share calculations.The use of a pointer to
big.Intis appropriate for handling large numbers involved in financial calculations, and the naming of the parameter is clear and descriptive.This change aligns with the overall purpose of the
MemClobKeeperinterface and suggests a shift towards more complex financial interactions in the context of clob operations.protocol/indexer/events/order_fill_test.go (6)
4-4: LGTM!The import of the
math/bigpackage is necessary for using thebig.Inttype to represent theaffiliateRevSharevariable.
24-24: LGTM!The initialization of the
affiliateRevSharevariable as abig.Intwith a value of zero is correct and necessary for passing it as a parameter to theNewOrderFillEventandNewLiquidationOrderFillEventfunctions.
36-36: LGTM!Passing the
affiliateRevSharevariable as an additional parameter to theNewOrderFillEventfunction is correct and aligns with the addition of theAffiliateRevSharefield in theOrderFillEventV1struct.
44-49: LGTM!Updating the
expectedOrderFillEventPrototo include theAffiliateRevSharefield and its expected value is correct and ensures that the test case validates the newly added field in theOrderFillEventV1struct.
63-63: LGTM!Passing the
affiliateRevSharevariable as an additional parameter to theNewLiquidationOrderFillEventfunction is correct and aligns with the addition of theAffiliateRevSharefield in theOrderFillEventV1struct.
79-84: LGTM!Updating the
expectedOrderFillEventPrototo include theAffiliateRevSharefield and its expected value is correct and ensures that the test case validates the newly added field in theOrderFillEventV1struct.protocol/indexer/events/order_fill.go (2)
Line range hint
23-37: LGTM!The addition of the
affiliateRevShareQuoteQuantumsparameter and its usage in setting theAffiliateRevSharefield is correct and aligns with the purpose of capturing the affiliate revenue share associated with order fills. The comment about the revenue share being less than the taker fee is a good clarification to ensure that the conversion toUint64()will not result in an overflow. The changes are well-integrated into the existing function and do not introduce any issues.
Line range hint
51-73: LGTM!The addition of the
affiliateRevShareQuoteQuantumsparameter and its usage in setting theAffiliateRevSharefield is correct and aligns with the purpose of capturing the affiliate revenue share associated with liquidation order fills. The comment about the revenue share being less than the taker fee is a good clarification to ensure that the conversion toUint64()will not result in an overflow. The changes are well-integrated into the existing function and do not introduce any issues.protocol/x/clob/types/clob_keeper.go (1)
88-88: LGTM!The addition of the
affiliateRevSharesQuoteQuantumsparameter to theProcessSingleMatchmethod signature aligns with the PR objective of implementing revenue sharing logic. The use of*big.Inttype is appropriate for handling large integer values.This change sets the foundation for returning affiliate revenue shares from the
ProcessSingleMatchmethod, enabling further implementation of the revenue sharing functionality.protocol/x/clob/types/expected_keepers.go (4)
87-88: LGTM!The changes to the
DistributeFeesmethod signature are consistent with the PR objectives and the AI-generated summary. The changes are localized to the interface and do not introduce any breaking changes.
152-152: LGTM!The addition of the
GetUserStatsmethod is consistent with the PR objectives and the AI-generated summary. The changes are localized to the interface and do not introduce any breaking changes.
169-170: LGTM!The changes to the
AddRewardSharesForFillmethod signature are consistent with the PR objectives and the AI-generated summary. The changes are localized to the interface and do not introduce any breaking changes.
174-179: LGTM!The addition of the
RevShareKeeperinterface is consistent with the PR objectives and the AI-generated summary. The changes are localized to the interface and do not introduce any breaking changes.protocol/testutil/keeper/listing.go (1)
153-154: LGTM!The addition of
revShareKeeperto theListingKeeperstest utility function aligns with the PR objective of implementing revenue sharing logic. The changes are limited to the test setup and do not directly impact the production code.protocol/testutil/keeper/subaccounts.go (4)
8-8: LGTM!The import statement for
affiliateskeeperis correct and necessary for using it in this file.
45-45: LGTM!The addition of
affiliatesKeeperto the return values of theSubaccountsKeepersfunction is consistent with the provided summary and indicates that it will be initialized and returned for use in the subaccounts context.
79-81: LGTM!The initialization of
affiliatesKeeperandrevShareKeeper, along with setting therevShareKeeperin theaffiliatesKeeper, establishes a relationship between the affiliate and revenue sharing systems. This allows for better management of affiliate-related operations within the subaccounts context.
108-108: LGTM!The addition of
affiliatesKeeperto the returned slice ofGenesisInitializerindicates that it will now be part of the initial setup process. ReturningaffiliatesKeeperfrom theSubaccountsKeepersfunction is consistent with the addition at line 45.Also applies to: 123-123
protocol/mocks/mocks/MultiStore.go (13)
21-24: LGTM!The added error handling ensures that a return value is always provided when mocking the
CacheMultiStoremethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
41-44: LGTM!The added error handling ensures that a return value is always provided when mocking the
CacheMultiStoreWithVersionmethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
71-74: LGTM!The added error handling ensures that a return value is always provided when mocking the
CacheWrapmethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
91-94: LGTM!The added error handling ensures that a return value is always provided when mocking the
CacheWrapWithTracemethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
111-114: LGTM!The added error handling ensures that a return value is always provided when mocking the
GetKVStoremethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
131-134: LGTM!The added error handling ensures that a return value is always provided when mocking the
GetStoremethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
151-154: LGTM!The added error handling ensures that a return value is always provided when mocking the
GetStoreTypemethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
169-172: LGTM!The added error handling ensures that a return value is always provided when mocking the
LatestVersionmethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
187-190: LGTM!The added error handling ensures that a return value is always provided when mocking the
SetTracermethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
207-210: LGTM!The added error handling ensures that a return value is always provided when mocking the
SetTracingContextmethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
227-230: LGTM!The added error handling ensures that a return value is always provided when mocking the
TracingEnabledmethod. This is a good practice to catch potential issues during testing where a mock is not set up correctly.
241-243: LGTM!The updated comments provide clearer documentation for the
NewMultiStorefunction. The comments help developers understand the purpose and usage of the function.
246-246: LGTM!The change clarifies the expected type of the first argument of the
NewMultiStorefunction. This helps developers understand how to use the function correctly.protocol/mocks/mocks/CacheMultiStore.go (1)
Line range hint
1-258: LGTM!The changes to the auto-generated mock code for
CacheMultiStorelook good. The addition of error handling logic to check for specified return values and panic if missing, enhances the robustness of the mock. The changes are consistent across all mocked methods and do not introduce any logical issues.Since these changes are auto-generated by an upgraded version of the mockery tool, they can be safely merged.
protocol/x/revshare/keeper/revshare.go (3)
214-214: LGTM!The addition of the
RevSharePpmfield to the returnedRevSharestruct in thegetAffiliateRevSharesfunction is a good change. It provides more information about the affiliate revenue share percentage.
235-235: Looks good!Adding the
RevSharePpmfield to theRevSharestruct in thegetUnconditionalRevSharesfunction is a nice addition. It provides more details about the unconditional revenue share percentage.
262-262: Approved.The inclusion of the
RevSharePpmfield in the returnedRevSharestruct of thegetMarketMapperRevSharefunction is a good change. It provides additional information about the market mapper revenue share percentage.protocol/testutil/keeper/clob.go (2)
188-188: LGTM!The addition of
revShareKeeperas an argument to thecreateClobKeeperfunction call is consistent with the updated function signature.
227-227: LGTM!The addition of the
revShareKeeperparameter to thecreateClobKeeperfunction is consistent with the changes made to the caller function. The type of the parameter is also correctly specified.protocol/x/clob/keeper/keeper.go (2)
45-45: LGTM!The addition of the
revshareKeeperfield to theKeeperstruct is a valid change that aligns with the PR objective of implementing revenue sharing logic in the clob code. The field is correctly typed astypes.RevShareKeeper.
Line range hint
95-127: LGTM!The updates to the
NewKeeperconstructor function to accept and assign therevshareKeeperparameter are valid and necessary changes. TherevshareKeeperparameter is correctly typed astypes.RevShareKeeperand assigned to the corresponding field in theKeeperstruct, ensuring proper initialization of the revenue sharing functionality in the clob keeper.protocol/mocks/SubaccountsKeeper.go (2)
189-205: LGTM!The new mock function
GetStreamSubaccountUpdateis implemented correctly and follows the established pattern for mock functions in this file. The panic mechanism is a good practice for catching potential issues during testing.
253-256: LGTM!The new mock function
SendFinalizedSubaccountUpdatesis implemented correctly and follows the established pattern for mock functions in this file. The absence of a panic mechanism for handling missing return values is appropriate, as the function does not return any value.protocol/x/rewards/keeper/keeper.go (1)
131-164: LGTM!The changes to the
AddRewardSharesForFillfunction look good. The updates to the function signature and the reward share calculation logic align well with the new data structures and enhance the clarity and efficiency of the process.To ensure the function is being called with the correct parameters, run the following script:
Verification successful
Function calls verified:
AddRewardSharesForFillis consistently used with correct parametersThe verification process confirms that all occurrences of
AddRewardSharesForFillin the codebase use the updated function signature withfillandrevsharesparameters. This includes:
- Test files
- Interface definitions
- Actual function calls in the codebase
No instances of the old function signature or incorrect parameter usage were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `AddRewardSharesForFill` have the correct parameters. # Test: Search for the function usage. Expect: Only occurrences with the new parameters. rg --type go -A 5 $'AddRewardSharesForFill'Length of output: 1903
protocol/mocks/MemClobKeeper.go (1)
Line range hint
324-371: LGTM!The mock function has been correctly updated to match the real implementation. The additional return value allows the mock to return more information, which is crucial for the logic that depends on this function. The handling of return values has been adjusted correctly to accommodate the new return type.
protocol/x/clob/keeper/process_single_match.go (5)
17-17: LGTM!The addition of the
revsharetypesimport statement is necessary for the code changes related to revenue sharing.
49-49: LGTM!The addition of the
affiliateRevSharesQuoteQuantumsreturn parameter to theProcessSingleMatchfunction signature is consistent with the changes made to track affiliate revenue shares. The parameter type*big.Intis appropriate for representing quote quantums.
77-77: LGTM!The error return statements in the
ProcessSingleMatchfunction are correctly updated to include theaffiliateRevSharesQuoteQuantumsparameter, maintaining consistency with the updated function signature. The parameter is passed without any modifications, which is appropriate.Also applies to: 92-92, 100-100, 134-134, 160-160, 193-193, 215-215, 230-230
219-219: LGTM!The
persistMatchedOrdersfunction signature and its return statements are correctly updated to include theaffiliateRevSharesQuoteQuantumsparameter. The parameter is passed and returned appropriately, maintaining consistency with the changes made to track affiliate revenue shares.Also applies to: 293-293, 313-313, 407-407, 419-419, 439-445, 525-525
Line range hint
447-497: LGTM!The added code block correctly retrieves affiliate revenue shares using the
revshareKeeper.GetAllRevSharesfunction and stores the affiliate revenue share quote quantums inaffiliateRevSharesQuoteQuantums. The error handling forrevshareKeeper.GetAllRevSharesis appropriate, returning the error along with other return values.The
subaccountsKeeper.DistributeFeesandrewardsKeeper.AddRewardSharesForFillfunction calls are updated with therevsharesparameter, ensuring that fees are distributed and reward shares are added considering the affiliate revenue shares.protocol/x/revshare/keeper/revshare_test.go (5)
286-286: LGTM!The addition of the
RevSharePpmfield enhances the test case by providing the revenue share metric. The assigned value of150_000(15%) for the affiliate revenue share looks reasonable.
293-293: LGTM!The addition of the
RevSharePpmfield enhances the test case by providing the revenue share metrics for unconditional revenue shares. The assigned values of200_000(20%) and300_000(30%) match the setup and look reasonable.Also applies to: 300-300
307-307: LGTM!The addition of the
RevSharePpmfield enhances the test cases by providing the revenue share metric for the market mapper revenue share. The assigned value of100_000(10%) matches the setup and looks reasonable.Also applies to: 376-376
355-355: LGTM!The addition of the
RevSharePpmfield enhances the test case by providing the revenue share metrics for affiliate, unconditional, and market mapper revenue shares. The assigned values of150_000(15%),200_000(20%), and300_000(30%) match the setup and look reasonable.Also applies to: 362-362, 369-369
433-433: LGTM!The addition of the
RevSharePpmfield enhances the test cases by providing the revenue share metrics for various scenarios, including cases with no unconditional or market mapper revenue shares. The assigned values match the setup in each test case and look reasonable.Also applies to: 440-440, 447-447, 485-485, 492-492, 527-527, 534-534
protocol/x/rewards/keeper/keeper_test.go (2)
Line range hint
147-421: LGTM!The changes to the
TestAddRewardSharesForFillfunction significantly improve the test coverage and clarity:
- The updated function signature with
clobtypes.FillForProcessand[]revsharetypes.RevShareparameters allows for more comprehensive testing of theAddRewardSharesForFillfunction.- The new test cases cover important scenarios involving revenue sharing, using realistic numbers and a good range of scenarios. This is crucial for ensuring the correctness of the reward distribution logic.
- The test case structure has been improved for better readability and maintainability.
Great job on enhancing the test suite! 👍
Line range hint
459-800: Excellent test coverage!The
TestProcessRewardsForBlockfunction has a comprehensive set of test cases that cover various scenarios and edge cases:
- The test cases use different combinations of reward shares, token prices, treasury account balances, and fee multipliers to ensure the correctness of the
ProcessRewardsForBlockfunction under different conditions.- The use of realistic numbers and coverage of edge cases such as insufficient treasury balance and rounding helps catch potential issues and ensures the robustness of the function.
- The test setup is well-structured, using helper functions to create test markets and set up the keeper state, which improves readability and maintainability.
The thorough test coverage gives confidence in the correctness of the
ProcessRewardsForBlockfunction. Great work! 🚀protocol/x/clob/keeper/process_operations.go (2)
509-509: LGTM!The changes introduce a new return value
affiliateRevSharesQuoteQuantumsfrom theProcessSingleMatchfunction and pass it to theNewOrderFillEventfunction. This looks good as it likely includes affiliate revenue share information in the indexer event for order fills.Also applies to: 546-546
618-618: LGTM!The changes introduce a new return value
affiliateRevSharesQuoteQuantumsfrom theProcessSingleMatchfunction and pass it to theNewLiquidationOrderFillEventfunction. This looks good as it likely includes affiliate revenue share information in the indexer event for liquidation order fills.Also applies to: 649-649
protocol/mocks/ClobKeeper.go (2)
1036-1050: LGTM!The internal logic changes to handle the new
*big.Intreturn value are implemented correctly.
Line range hint
1003-1015: Verify the impact of the function signature change on the codebase.The function signature of
ProcessSingleMatchhas been modified to return an additional*big.Intvalue. This is a breaking change that could affect the callers of this function.Run the following script to verify the usage of
ProcessSingleMatchin the codebase:Verification successful
Function signature change has been consistently applied
The modification to the
ProcessSingleMatchfunction signature, which now includes an additional*big.Intreturn value, has been consistently implemented across the entire codebase. This includes:
- Interface definitions
- Concrete implementations
- Mock objects
- Function calls
The change has been properly propagated, and no discrepancies or mismatches were found in the function signature across different files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `ProcessSingleMatch` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'ProcessSingleMatch'Length of output: 8011
protocol/x/clob/e2e/short_term_orders_test.go (4)
218-218: LGTM!The test cases comprehensively cover various scenarios for placing orders, including single order placement, full and partial order matching. The assertions for expected offchain/onchain messages and order state ensure the correctness of the clob and indexer behavior.
The addition of
big.NewInt(0)argument in theNewOrderFillEventcalls seems to be a valid change based on the context.Also applies to: 398-398, 578-578
Line range hint
580-780: Looks good, no changes!The test cases comprehensively cover various scenarios for replacing short term orders, including:
- Replacing in the same block and next block
- Replacing on the same side and opposite side
- Replacing with higher and lower GTB
- Replacing after partial fills
- Replacing with decreased order size
- Failure scenarios for invalid replacements
The test assertions for the expected order state ensure the correctness of the order replacement behavior.
Line range hint
782-912: Looks good, no changes!The test cases comprehensively cover various scenarios for canceling short term orders, including:
- Canceling unfilled orders
- Canceling partially filled orders in the same block and next block
- Canceling fully filled orders
- Behavior when cancel GTB is lower or higher than existing order or cancel
The test assertions for the expected order state, cancel expirations, and fill amounts ensure the correctness of the order cancellation behavior.
Line range hint
914-1160: Looks good, no changes!The test cases comprehensively cover various scenarios for advanced short term orders like IOC and post-only, including:
For IOC orders:
- Full match
- Partial match and not placed on the book
- Failure when already filled
For post-only orders:
- Not crossing and placed on the book
- Crossing and not placed on the book
The test assertions for the expected order state and fill amounts ensure the correctness of the IOC and post-only order behavior.
protocol/x/clob/e2e/long_term_orders_test.go (7)
Line range hint
821-1165: Comprehensive test coverage for placing long-term orders.The
TestPlaceLongTermOrderfunction provides thorough test coverage for placing long-term orders under various scenarios. It verifies the expected behavior by checking the orderbook state, fill amounts, and subaccount balances after each block. The test cases cover important scenarios such as placing an order, fully matching an order as a taker, placing a post-only order on the book, and partially matching an order as a taker and then fully as a maker.However, the test function is quite long and complex, which could make it difficult to understand and maintain. Consider the following improvements:
- Extract the test cases into separate test functions to improve readability and maintainability.
- Use helper functions to create orders and expected values instead of hardcoding them.
Line range hint
1336-1735: Comprehensive test coverage for matching an order against an invalid time in force.The
TestRegression_InvalidTimeInForcefunction provides thorough test coverage for matching an order fully as a taker against a pre-existing order with an invalid time in force. It verifies the expected behavior by checking the orderbook state, fill amounts, and subaccount balances after each block. The test case covers an important scenario of handling orders with invalid time in force values.However, the test function is quite long and complex, which could make it difficult to understand and maintain. Consider the following improvements:
- Extract the test case into a separate test function to improve readability and maintainability.
- Use helper functions to create orders and expected values instead of hardcoding them.
Line range hint
1165-1188: LGTM!The
TestLongTermOrderExpiresfunction provides a clear and concise test case for the expiration of a long-term order based on the block time. It verifies the expected behavior by checking the presence of the order in the state at different block times.
Line range hint
1188-1222: LGTM!The
TestImmediateExecutionLongTermOrdersfunction provides a clear and concise test case for rejecting long-term orders with immediate execution requirements (IOC and FOK). It verifies the expected behavior by checking the error messages returned when attempting to place the orders in both CheckTx and DeliverTx.
Line range hint
1222-1288: LGTM!The
TestCancelStatefulOrderfunction provides comprehensive test coverage for canceling stateful orders under various scenarios. It uses table-driven tests with clear test case definitions and assertions. The test cases cover important scenarios such as canceling an order in the same block it was placed, canceling an order in a future block, canceling a partially matched order, and canceling a non-existent order. The test function thoroughly verifies the expected behavior by checking the presence of the order in the state and the fill amount after each block.
Line range hint
1288-1336: LGTM!The
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledfunction provides a clear and concise test case for attempting to cancel a fully filled stateful order in the same block it is filled. It verifies the expected behavior by checking the error returned when attempting to cancel the order in DeliverTx. The test function is well-structured and readable, with clear setup, execution, and assertions.
Line range hint
1735-1774: LGTM!The
TestPlaceOrder_StatefulCancelFollowedByPlaceInSameBlockErrorsInCheckTxfunction provides a clear and concise test case for attempting to place a stateful order after canceling it in the same block. It verifies the expected behavior by checking the error returned when attempting to place the order again in CheckTx. The test function is well-structured and readable, with clear setup, execution, and assertions.protocol/app/app.go (1)
1130-1130: LGTM! TheRevShareKeeperis now a dependency of theClobKeeper.Adding
app.RevShareKeeperas an argument toNewKeeperwhen initializing theClobKeeperinjects theRevShareKeeperdependency into the clob module. This allows the clob module to integrate with the revenue sharing functionality.protocol/x/clob/keeper/process_operations_test.go (3)
Line range hint
2583-2678: LGTM!The
setupNewMockEventManagerfunction is implemented correctly and covers all the necessary cases for setting up the mock indexer event manager expectations. The code is well-structured and readable.
Line range hint
2708-2723: LGTM!The
assertSubaccountStatefunction is implemented correctly. It properly validates the quote balances and perpetual positions of subaccounts against the expected values provided in the test case.
Line range hint
1-2723: Excellent test coverage and structure!The
process_operations_test.gofile contains a comprehensive set of test cases for theProcessProposerOperationsfunction. The test cases cover various scenarios, including order matching, liquidations, deleveraging, and error cases. The test setup and assertions are thorough and well-organized.The code follows good practices for writing unit tests in Go, such as using table-driven tests, mock objects, and clear assertions. The test file demonstrates a high level of test coverage and ensures the robustness of the
ProcessProposerOperationsfunction.Overall, the test file is well-structured, readable, and maintainable. Great job!
| @@ -259,6 +261,7 @@ func createClobKeeper( | |||
| flags.GetDefaultClobFlags(), | |||
| rate_limit.NewNoOpRateLimiter[sdk.Msg](), | |||
| liquidationtypes.NewDaemonLiquidationInfo(), | |||
| revShareKeeper, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The order of arguments in the NewKeeper function call is correct, but some arguments are missing.
The order of the arguments passed to the NewKeeper function in protocol/testutil/keeper/clob.go matches the order specified in the function signature in protocol/x/clob/keeper/keeper.go for the first 17 arguments. However, there are some discrepancies:
- The
transientStoreKeyin the function call corresponds to theliquidationsStoreKeyin the function signature. - The last three arguments are missing from the function call:
txDecoderclobFlagsplaceCancelOrderRateLimiter
Additionally, the revShareKeeper mentioned in the original review comment is not present in either the function signature or the function call.
Please update the NewKeeper function call to include the missing arguments and ensure that all required dependencies are properly injected.
Analysis chain
Verify the order of the arguments passed to the NewKeeper function.
The addition of revShareKeeper as an argument to the NewKeeper function call is consistent with the changes made to the function signature. However, please ensure that the order of the arguments matches the order specified in the NewKeeper function signature in the protocol/x/clob/keeper/keeper.go file.
To properly verify the order of arguments passed to the NewKeeper function, we need to retrieve more specific information. Let's generate shell scripts to get the required details.
To ensure we have the complete picture of the NewKeeper function signature, let's retrieve more lines from the function definition. This will help us verify if the additional arguments we see in the function call (feeTiersKeeper, perpKeeper, and potentially revShareKeeper) are part of the updated function signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the order of the arguments passed to the `NewKeeper` function.
# Test: Search for the `NewKeeper` function definition.
# Expect: The order of the parameters in the function signature should match the order of the arguments passed in this file.
rg --type go $'func NewKeeper\('
Length of output: 1189
Script:
#!/bin/bash
# Description: Retrieve the NewKeeper function signature and function call
# Get the NewKeeper function signature from the CLOB keeper
echo "CLOB Keeper NewKeeper function signature:"
rg --type go "func NewKeeper\(" protocol/x/clob/keeper/keeper.go -A 10
echo "\nCLOB testutil NewKeeper function call:"
# Get the NewKeeper function call from the testutil
rg --type go "NewKeeper\(" protocol/testutil/keeper/clob.go -A 15
Length of output: 933
Script:
#!/bin/bash
# Description: Retrieve the complete NewKeeper function signature
echo "Complete CLOB Keeper NewKeeper function signature:"
rg --type go "func NewKeeper\(" protocol/x/clob/keeper/keeper.go -A 20
echo "\nComplete CLOB testutil NewKeeper function call:"
rg --type go "NewKeeper\(" protocol/testutil/keeper/clob.go -A 20
Length of output: 1498
| @@ -331,6 +331,7 @@ func (f *FakeMemClobKeeper) ProcessSingleMatch( | |||
| success bool, | |||
| takerUpdateResult satypes.UpdateResult, | |||
| makerUpdateResult satypes.UpdateResult, | |||
| affiliateRevSharesQuoteQuantums *big.Int, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The affiliateRevSharesQuoteQuantums parameter is unused in the function body
The new affiliateRevSharesQuoteQuantums parameter has been added to the ProcessSingleMatch function signature, but it's not being used within the function body. This could lead to incorrect calculations or missed functionality related to affiliate revenue sharing.
To address this issue:
- Implement the logic to use the
affiliateRevSharesQuoteQuantumsparameter within theProcessSingleMatchfunction. - Ensure that the calculated affiliate revenue share is returned instead of the hardcoded
big.NewInt(0).
The callers of this function are already handling the new return value appropriately, so no changes are needed there.
Analysis chain
Verify the usage of the new affiliateRevSharesQuoteQuantums parameter and return value.
The changes to the function signature and return statement look good. The addition of the affiliateRevSharesQuoteQuantums parameter and the corresponding big.NewInt(0) return value suggest that the function will now handle affiliate revenue share calculations during the processing of a match.
To ensure correctness, please verify that:
- The
affiliateRevSharesQuoteQuantumsparameter is being used correctly within the function to calculate the affiliate revenue share. - The calculated affiliate revenue share is being returned as the fourth return value.
- The callers of this function are handling the new parameter and return value appropriately.
Run the following script to verify the usage:
Also applies to: 367-367, 414-414
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new `affiliateRevSharesQuoteQuantums` parameter and return value.
# Test 1: Search for the usage of `affiliateRevSharesQuoteQuantums` within the function.
# Expect: At least one occurrence where it's being used to calculate the affiliate revenue share.
rg --type go -A 5 $'affiliateRevSharesQuoteQuantums' protocol/testutil/memclob/keeper.go
# Test 2: Search for the return statements of the function.
# Expect: All return statements to return `big.NewInt(0)` as the fourth return value.
rg --type go -A 5 $'return' protocol/testutil/memclob/keeper.go
# Test 3: Search for the callers of the `ProcessSingleMatch` function.
# Expect: All callers to pass a `*big.Int` argument for `affiliateRevSharesQuoteQuantums` and handle the fourth return value.
rg --type go -A 5 $'ProcessSingleMatch' protocol/
Length of output: 11653
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
protocol/testutil/memclob/keeper.go (1)
334-334: Update function to calculate actual affiliate revenue share.The function signature has been updated to include a new
affiliateRevSharesQuoteQuantumsparameter and return value, which is a step towards handling affiliate revenue share calculations.However, the function body only returns a zero value for this new return value, indicating that the actual affiliate revenue share calculation logic is not yet implemented.
Consider implementing the logic to calculate the actual affiliate revenue share and return the calculated value instead of the current placeholder zero value.
Also applies to: 367-367, 414-414
protocol/x/clob/e2e/short_term_orders_test.go (2)
218-218: Consider extracting the hard-codedbig.NewInt(0)into a named constant.To improve readability and maintainability, consider extracting the hard-coded
big.NewInt(0)value into a named constant. This will make the code more expressive and easier to understand.For example, you could define a constant like:
const zeroRewardShares = big.NewInt(0)Then replace the hard-coded values with this constant:
- big.NewInt(0), + zeroRewardShares,Also applies to: 398-398, 578-578
Line range hint
32-578: Comprehensive test coverage. Consider adding a brief description to each test case.The test function covers various important scenarios for placing orders, such as placing an order, matching orders fully and partially. The assertions check the expected orders filled, offchain and onchain messages, which is comprehensive.
To further enhance the readability and maintainability of the test, consider adding a brief description to each test case in the
testsmap. This will make it easier for readers to understand the scenario being tested without having to analyze the test case details.For example:
tests := map[string]struct { // ... }{ "Test placing an order": { // Test case details... }, "Test matching an order fully": { // Test case details... }, // ... }protocol/x/clob/keeper/process_operations_test.go (1)
Line range hint
2638-2658: Consider refactoring thesetupNewMockEventManagerfunction for better readability and maintainability.The function is quite complex, with nested conditionals and loops. While it correctly sets up expectations for various events, it could benefit from some refactoring to improve readability and maintainability.
Some suggestions:
- Extract the logic for setting up expectations for liquidation order fills and regular order fills into separate functions.
- Use more descriptive variable names instead of
callandmatchOrderCallMap.- Add comments explaining the purpose of each block of code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (81)
- protocol/app/app.go (1 hunks)
- protocol/indexer/events/order_fill.go (5 hunks)
- protocol/indexer/events/order_fill_test.go (5 hunks)
- protocol/mocks/AnteDecorator.go (1 hunks)
- protocol/mocks/AppOptions.go (1 hunks)
- protocol/mocks/BankKeeper.go (1 hunks)
- protocol/mocks/BridgeKeeper.go (1 hunks)
- protocol/mocks/BridgeQueryClient.go (1 hunks)
- protocol/mocks/BridgeServiceClient.go (1 hunks)
- protocol/mocks/CacheMultiStore.go (1 hunks)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/mocks/Configurator.go (1 hunks)
- protocol/mocks/DelayMsgKeeper.go (1 hunks)
- protocol/mocks/EthClient.go (1 hunks)
- protocol/mocks/ExchangeConfigUpdater.go (1 hunks)
- protocol/mocks/ExchangeQueryHandler.go (1 hunks)
- protocol/mocks/ExchangeToMarketPrices.go (1 hunks)
- protocol/mocks/ExtendVoteHandler.go (1 hunks)
- protocol/mocks/FileHandler.go (1 hunks)
- protocol/mocks/GrpcClient.go (1 hunks)
- protocol/mocks/GrpcServer.go (1 hunks)
- protocol/mocks/HealthCheckable.go (1 hunks)
- protocol/mocks/IndexerEventManager.go (1 hunks)
- protocol/mocks/IndexerMessageSender.go (1 hunks)
- protocol/mocks/Logger.go (1 hunks)
- protocol/mocks/MarketPairFetcher.go (1 hunks)
- protocol/mocks/MemClob.go (1 hunks)
- protocol/mocks/MemClobKeeper.go (4 hunks)
- protocol/mocks/MsgRouter.go (1 hunks)
- protocol/mocks/MultiStore.go (1 hunks)
- protocol/mocks/OracleClient.go (1 hunks)
- protocol/mocks/PerpetualsClobKeeper.go (1 hunks)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/mocks/PrepareBridgeKeeper.go (1 hunks)
- protocol/mocks/PrepareClobKeeper.go (1 hunks)
- protocol/mocks/PreparePerpetualsKeeper.go (1 hunks)
- protocol/mocks/PriceFeedMutableMarketConfigs.go (1 hunks)
- protocol/mocks/PriceFetcher.go (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (1 hunks)
- protocol/mocks/ProcessBridgeKeeper.go (1 hunks)
- protocol/mocks/ProcessClobKeeper.go (1 hunks)
- protocol/mocks/ProcessPerpetualKeeper.go (1 hunks)
- protocol/mocks/ProcessStakingKeeper.go (1 hunks)
- protocol/mocks/QueryClient.go (1 hunks)
- protocol/mocks/QueryServer.go (1 hunks)
- protocol/mocks/RequestHandler.go (1 hunks)
- protocol/mocks/SendingKeeper.go (1 hunks)
- protocol/mocks/Server.go (1 hunks)
- protocol/mocks/SubaccountsKeeper.go (3 hunks)
- protocol/mocks/TimeProvider.go (1 hunks)
- protocol/mocks/TxBuilder.go (1 hunks)
- protocol/mocks/TxConfig.go (1 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
- protocol/mocks/VaultKeeper.go (1 hunks)
- protocol/mocks/mocks/CacheMultiStore.go (13 hunks)
- protocol/mocks/mocks/MultiStore.go (13 hunks)
- protocol/testutil/keeper/clob.go (3 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (5 hunks)
- protocol/testutil/memclob/keeper.go (3 hunks)
- protocol/x/clob/e2e/long_term_orders_test.go (4 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (4 hunks)
- protocol/x/clob/keeper/process_operations_test.go (2 hunks)
- protocol/x/clob/keeper/process_single_match.go (17 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (4 hunks)
- protocol/x/clob/types/mem_clob_keeper.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (3 hunks)
- protocol/x/revshare/keeper/revshare_test.go (5 hunks)
- protocol/x/revshare/types/types.go (1 hunks)
- protocol/x/rewards/keeper/keeper.go (3 hunks)
- protocol/x/rewards/keeper/keeper_test.go (8 hunks)
- protocol/x/sending/client/cli/sending_cli_test.go (0 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)
Files not processed due to max files limit (7)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
- protocol/x/subaccounts/keeper/safety_heap_state_test.go
- protocol/x/subaccounts/keeper/safety_heap_test.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/keeper/transfer.go
- protocol/x/subaccounts/keeper/transfer_test.go
- protocol/x/subaccounts/module_test.go
Files not reviewed due to no reviewable changes (1)
- protocol/x/sending/client/cli/sending_cli_test.go
Files skipped from review due to trivial changes (53)
- protocol/mocks/AnteDecorator.go
- protocol/mocks/AppOptions.go
- protocol/mocks/BankKeeper.go
- protocol/mocks/BridgeKeeper.go
- protocol/mocks/BridgeQueryClient.go
- protocol/mocks/BridgeServiceClient.go
- protocol/mocks/CacheMultiStore.go
- protocol/mocks/Configurator.go
- protocol/mocks/DelayMsgKeeper.go
- protocol/mocks/EthClient.go
- protocol/mocks/ExchangeConfigUpdater.go
- protocol/mocks/ExchangeQueryHandler.go
- protocol/mocks/ExchangeToMarketPrices.go
- protocol/mocks/ExtendVoteHandler.go
- protocol/mocks/FileHandler.go
- protocol/mocks/GrpcClient.go
- protocol/mocks/GrpcServer.go
- protocol/mocks/HealthCheckable.go
- protocol/mocks/IndexerEventManager.go
- protocol/mocks/IndexerMessageSender.go
- protocol/mocks/Logger.go
- protocol/mocks/MarketPairFetcher.go
- protocol/mocks/MemClob.go
- protocol/mocks/MsgRouter.go
- protocol/mocks/MultiStore.go
- protocol/mocks/OracleClient.go
- protocol/mocks/PerpetualsClobKeeper.go
- protocol/mocks/PerpetualsKeeper.go
- protocol/mocks/PrepareBridgeKeeper.go
- protocol/mocks/PrepareClobKeeper.go
- protocol/mocks/PreparePerpetualsKeeper.go
- protocol/mocks/PriceFeedMutableMarketConfigs.go
- protocol/mocks/PriceFetcher.go
- protocol/mocks/PriceUpdateGenerator.go
- protocol/mocks/PricesKeeper.go
- protocol/mocks/ProcessBridgeKeeper.go
- protocol/mocks/ProcessClobKeeper.go
- protocol/mocks/ProcessPerpetualKeeper.go
- protocol/mocks/ProcessStakingKeeper.go
- protocol/mocks/QueryClient.go
- protocol/mocks/QueryServer.go
- protocol/mocks/RequestHandler.go
- protocol/mocks/SendingKeeper.go
- protocol/mocks/Server.go
- protocol/mocks/TimeProvider.go
- protocol/mocks/TxBuilder.go
- protocol/mocks/TxConfig.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
- protocol/x/clob/memclob/memclob.go
- protocol/x/subaccounts/genesis_test.go
- protocol/x/subaccounts/keeper/grpc_query_collateral_pool_test.go
- protocol/x/subaccounts/keeper/grpc_query_subaccount_test.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go
Additional comments not posted (63)
protocol/x/revshare/types/types.go (1)
37-37: LGTM!The addition of the
RevSharePpmfield to theRevSharestruct is a valid enhancement that enables more granular control and representation of revenue share calculations. Theuint32data type is appropriate for storing PPM values, and the field name clearly conveys its purpose. This change aligns with the PR objective of incorporating revenue sharing logic.protocol/x/clob/types/mem_clob_keeper.go (1)
28-28: LGTM! The addition of theaffiliateRevSharesQuoteQuantumsparameter enhances the functionality.The inclusion of the
affiliateRevSharesQuoteQuantumsparameter in theProcessSingleMatchmethod aligns with the PR objective of incorporating revenue sharing logic within the clob functionality. This change allows the method to calculate and return the affiliate revenue shares for a single match.The use of
*big.Inttype for theaffiliateRevSharesQuoteQuantumsparameter is suitable for handling large numeric values related to financial calculations.Please ensure that the corresponding implementations of the
MemClobKeeperinterface are updated to accommodate this change in the method signature.To verify the compatibility of this change, run the following script:
Verification successful
To complete our verification, we need to check the actual method signatures in the main implementation and test utility files. Let's run the following script:
Verification successful: All key implementations of
ProcessSingleMatchhave been updated correctly.The main implementation in
protocol/x/clob/keeper/process_single_match.goand the test utility implementation inprotocol/testutil/memclob/keeper.goboth include the newaffiliateRevSharesQuoteQuantums *big.Intparameter in their method signatures. These implementations are consistent with the interface definition we reviewed earlier.This confirms that the change has been properly implemented across the codebase, ensuring compatibility with the new method signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all implementations of the `MemClobKeeper` interface have been updated to match the new method signature. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'ProcessSingleMatch'Length of output: 8011
Script:
#!/bin/bash # Description: View the full method signatures for ProcessSingleMatch in the main implementation and test utility files # Check the main implementation echo "Main implementation (protocol/x/clob/keeper/process_single_match.go):" rg --type go -A 10 'func \(k Keeper\) ProcessSingleMatch' protocol/x/clob/keeper/process_single_match.go echo "\nTest utility implementation (protocol/testutil/memclob/keeper.go):" rg --type go -A 10 'func \(f \*FakeMemClobKeeper\) ProcessSingleMatch' protocol/testutil/memclob/keeper.goLength of output: 1124
protocol/indexer/events/order_fill_test.go (5)
24-24: LGTM!The
affiliateRevSharevariable is correctly declared and initialized. The variable name is clear and follows the naming convention.
36-36: LGTM!The
TestNewOrderFillEvent_Successtest function is correctly updated to include the newaffiliateRevShareargument in theNewOrderFillEventfunction call. This change is consistent with the updates made to theNewOrderFillEventfunction.
44-49: LGTM!The
expectedOrderFillEventProtostruct is correctly updated to include the newAffiliateRevSharefield. The field is assigned the value ofaffiliateRevShare.Uint64(), which is the correct way to convert the*big.Intvalue touint64.The updates to other fields to use the
ToUint64()method instead of direct type conversion are consistent and improve code readability.
63-63: LGTM!The
TestNewLiquidationOrderFillEvent_Successtest function is correctly updated to include the newaffiliateRevShareargument in theNewLiquidationOrderFillEventfunction call. This change is consistent with the updates made to theNewLiquidationOrderFillEventfunction.
79-84: LGTM!The
expectedOrderFillEventProtostruct is correctly updated to include the newAffiliateRevSharefield. The field is assigned the value ofaffiliateRevShare.Uint64(), which is the correct way to convert the*big.Intvalue touint64.The updates to other fields to use the
ToUint64()method instead of direct type conversion are consistent and improve code readability.protocol/indexer/events/order_fill.go (2)
Line range hint
23-37: LGTM!The addition of the
affiliateRevShareQuoteQuantumsparameter and the corresponding update to set theAffiliateRevSharefield in theOrderFillEventV1struct enhance the functionality by allowing for the tracking of affiliate revenue sharing in the order fill events.The comment provides clarity that the revenue share is always less than the taker fee, ensuring that the conversion to
Uint64()will not result in an overflow.
Line range hint
51-73: LGTM!The addition of the
affiliateRevShareQuoteQuantumsparameter and the corresponding update to set theAffiliateRevSharefield in theOrderFillEventV1struct enhance the functionality by allowing for the tracking of affiliate revenue sharing in the liquidation order fill events.Similar to the
NewOrderFillEventfunction, the comment provides clarity that the revenue share is always less than the taker fee, ensuring that the conversion toUint64()will not result in an overflow.protocol/x/clob/types/clob_keeper.go (1)
88-88: LGTM!The addition of the
affiliateRevSharesQuoteQuantumsreturn value to theProcessSingleMatchmethod is a valid enhancement to theClobKeeperinterface. This change aligns with the goal of improving the system's handling of affiliate revenue share calculations and distribution.The use of
*big.Inttype for the return value is appropriate for handling large financial values with precision.No issues or potential bugs are evident in the provided code change.
protocol/x/clob/types/expected_keepers.go (4)
87-88: LGTM!The changes to the
DistributeFeesmethod signature align with the PR objectives and the AI-generated summary. The new parameters ([]revsharetypes.RevShareandFillForProcess) enable a more structured approach for handling fee distribution and allow for more complex fee-sharing logic.
152-152: LGTM!The addition of the
GetUserStatsmethod aligns with the PR objectives and the AI-generated summary. It enhances the ability to track and analyze user-specific data within the system.
169-170: LGTM!The changes to the
AddRewardSharesForFillmethod signature align with the PR objectives and the AI-generated summary. The new parameters (FillForProcessand[]revsharetypes.RevShare) simplify the method signature and align it with the new data structures introduced.
174-179: LGTM!The addition of the
RevShareKeeperinterface and theGetAllRevSharesmethod aligns with the PR objectives and the AI-generated summary. It introduces a new capability for managing and querying revenue shares within the system.protocol/testutil/keeper/listing.go (1)
153-155: LGTM!The changes to the
ListingKeeperstest utility function are consistent with the PR objective of enhancing the clob functionality by incorporating revenue sharing logic. TherevShareKeeperis created and passed to thecreateClobKeeperfunction, which is expected to manage revenue sharing within the clob.The changes are limited to the test utility function and do not directly impact the production code. However, ensure that the corresponding unit tests have been updated to cover the new functionality.
protocol/testutil/keeper/subaccounts.go (1)
8-8: LGTM!The changes in the
SubaccountsKeeperstest utility function properly integrate theaffiliatesKeeperinto the keeper setup. TheaffiliatesKeeperis instantiated, returned as part of theGenesisInitializerarray, and linked to therevShareKeeper. These modifications ensure that theaffiliatesKeeperis available for testing and establish the necessary relationship with therevShareKeeper.The changes are well-contained within the test utility function and do not introduce any issues.
Also applies to: 45-45, 79-81, 108-108, 123-123
protocol/mocks/mocks/MultiStore.go (12)
21-24: LGTM!The added check for return values in the
CacheMultiStoremock function is a good practice to prevent silent failures during testing.
41-44: LGTM!The added check for return values in the
CacheMultiStoreWithVersionmock function is a good practice to prevent silent failures during testing.
71-74: LGTM!The added check for return values in the
CacheWrapmock function is a good practice to prevent silent failures during testing.
91-94: LGTM!The added check for return values in the
CacheWrapWithTracemock function is a good practice to prevent silent failures during testing.
111-114: LGTM!The added check for return values in the
GetKVStoremock function is a good practice to prevent silent failures during testing.
131-134: LGTM!The added check for return values in the
GetStoremock function is a good practice to prevent silent failures during testing.
151-154: LGTM!The added check for return values in the
GetStoreTypemock function is a good practice to prevent silent failures during testing.
169-172: LGTM!The added check for return values in the
LatestVersionmock function is a good practice to prevent silent failures during testing.
187-190: LGTM!The added check for return values in the
SetTracermock function is a good practice to prevent silent failures during testing.
207-210: LGTM!The added check for return values in the
SetTracingContextmock function is a good practice to prevent silent failures during testing.
227-230: LGTM!The added check for return values in the
TracingEnabledmock function is a good practice to prevent silent failures during testing.
241-246: LGTM!The modification to the
NewMultiStoreconstructor function improves clarity by explicitly specifying the expected parameter type, ensuring that it implements bothmock.TestingTand provides a cleanup function.protocol/mocks/mocks/CacheMultiStore.go (1)
Line range hint
1-258: LGTM!The changes to the auto-generated
CacheMultiStoremock are consistent and enhance its robustness by adding error handling for cases where no return values are specified. This ensures that developers are alerted to potential misconfigurations in their tests.The changes follow a uniform pattern across all mocked methods and do not alter the core functionality or behavior of the mock. As these changes are auto-generated and do not introduce any manual modifications or potential errors, they can be safely approved.
protocol/mocks/VaultKeeper.go (2)
1-1: Skip reviewing the mock generation tool version update.The mock generation tool version update is a minor change and does not require any action.
1-1: Verify the removal of theGetVaultClobOrderClientIdfunction from the actualVaultKeeperinterface.The removal of the
GetVaultClobOrderClientIdfunction from the mock suggests that it was removed from the actualVaultKeeperinterface. However, this cannot be verified without reviewing the interface changes.To verify, run the following script:
Verification successful
Removal of
GetVaultClobOrderClientIdfromVaultKeeperinterface verifiedThe
GetVaultClobOrderClientIdfunction has indeed been removed from theVaultKeeperinterface. However, it's important to note that the function still exists in the codebase as a standalone utility function inprotocol/x/vault/types/orders.go. This change suggests a refactoring to improve code organization or accessibility.
- The function is no longer part of any interface
- It's still used in the codebase, particularly in the keeper implementation and tests
- No TODO comments or explanations were found regarding this change
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of the `GetVaultClobOrderClientId` function from the `VaultKeeper` interface. # Test: Search for the function declaration in the `VaultKeeper` interface. Expect: No results. rg --type go $'func \(VaultKeeper\) GetVaultClobOrderClientId\(ctx sdk\.Context, side clobtypes\.Order_Side, layer uint8\) uint32'Length of output: 131
Script:
#!/bin/bash # Find the file containing the VaultKeeper interface echo "Searching for VaultKeeper interface file:" fd -e go | xargs grep -l "type VaultKeeper interface" # Display the contents of the VaultKeeper interface file echo -e "\nContents of VaultKeeper interface file:" fd -e go | xargs grep -l "type VaultKeeper interface" | xargs cat # Perform a broader search for GetVaultClobOrderClientId echo -e "\nBroader search for GetVaultClobOrderClientId:" rg --type go "GetVaultClobOrderClientId"Length of output: 2758
Script:
#!/bin/bash # Check if GetVaultClobOrderClientId is part of any interface echo "Searching for GetVaultClobOrderClientId in interfaces:" rg --type go "type.*interface.*GetVaultClobOrderClientId" # Look for TODO comments or explanations near GetVaultClobOrderClientId echo -e "\nSearching for TODO comments or explanations:" rg --type go -C 5 "GetVaultClobOrderClientId" | grep -E "(TODO|FIXME|NOTE)" # Display the GetVaultClobOrderClientId function definition echo -e "\nGetVaultClobOrderClientId function definition:" rg --type go -A 10 "func GetVaultClobOrderClientId"Length of output: 1002
protocol/x/revshare/keeper/revshare.go (3)
214-214: LGTM!The addition of the
RevSharePpmfield to the returnedRevSharestruct enhances the level of detail provided for affiliate revenue shares. The value is correctly populated from thefeeSharePpmvariable fetched from the affiliates module.
235-235: Looks good!The addition of the
RevSharePpmfield to theRevSharestruct created within this function is in line with the changes made ingetAffiliateRevShares. The value is correctly populated fromrevShare.SharePpmfetched from the unconditional revenue share configuration.
262-262: Looks great!The addition of the
RevSharePpmfield to theRevSharestruct appended to therevSharesslice is consistent with the changes made in the other functions. The value is correctly populated from therevenueSharePpmvariable fetched usingGetMarketMapperRevenueShareForMarket.protocol/testutil/keeper/clob.go (2)
188-188: LGTM!The addition of the
revShareKeeperparameter aligns with the objective of integrating revenue sharing capabilities within the Clob Keeper's operations. The parameter is correctly passed to thecreateClobKeeperfunction for initialization.
Line range hint
227-264: LGTM!The addition of the
revShareKeeperparameter aligns with the objective of integrating revenue sharing capabilities within the Clob Keeper's operations. The parameter is correctly passed to thekeeper.NewKeeperfunction for utilization within the Clob Keeper.protocol/x/clob/keeper/keeper.go (2)
45-45: LGTM!The addition of the
revshareKeeperfield in theKeeperstruct aligns with the PR objective of implementing revenue sharing logic within the clob module. This change suggests that theKeepernow interacts with a revenue sharing module to manage or facilitate revenue sharing processes.
Line range hint
95-127: Looks good!The modification to the
NewKeeperconstructor function to accept and initialize therevshareKeeperis consistent with the addition of the corresponding field in theKeeperstruct. This change ensures that therevshareKeeperis properly set up when creating a new instance of theKeeper, enabling it to interact with the revenue sharing module.protocol/mocks/SubaccountsKeeper.go (2)
189-205: LGTM!The
GetStreamSubaccountUpdatemock function is implemented correctly, following the conventions of the mock framework. The function signature, return value handling, and panic message are all appropriate.
253-256: LGTM!The
SendFinalizedSubaccountUpdatesmock function is implemented correctly, following the conventions of the mock framework. The function signature is appropriate for handling the sending of finalized subaccount updates, and the function correctly registers the call with the mock framework using theCalledmethod.protocol/x/rewards/keeper/keeper.go (1)
Line range hint
131-196: The changes toAddRewardSharesForFillalign with the objective of incorporating revenue sharing logic and enhance the clarity and efficiency of the reward share calculation process.Key improvements:
- The function signature change aligns with the objective of incorporating revenue sharing logic by accepting a
clobtypes.FillForProcessobject and a slice ofrevsharetypes.RevShareobjects.- The updated logic for calculating reward shares is more dynamic and accounts for the revenue shares associated with the fill, processing the slice of
revsharetypes.RevShareobjects to compute the total net fee revenue share and affiliate revenue share.- The calculation of
takerWeightandmakerWeighthas been adjusted to reflect the changes in the fill data and the computed revenue shares, ensuring that the weights are derived from the updated fill data and the computed revenue shares.Overall, the changes enhance the clarity and efficiency of the reward share calculation process, aligning it more closely with the underlying data structures used in the application.
protocol/mocks/MemClobKeeper.go (1)
Line range hint
324-371: LGTM!The changes to the mock
ProcessSingleMatchfunction correctly reflect the updated signature of the actual function. The additional*big.Intreturn value is properly handled within the mock implementation.Verify that all calls to
ProcessSingleMatchin the codebase have been updated to handle the new*big.Intreturn value.Run the following script to find all occurrences of
ProcessSingleMatchand check if they are handling the new return value correctly:Verification successful
All calls to
ProcessSingleMatchcorrectly handle the new*big.Intreturn valueThe verification process has confirmed that all occurrences of
ProcessSingleMatchin the codebase have been updated to handle the new*big.Intreturn value correctly. This includes interface definitions, implementations, mocks, and usage sites. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `ProcessSingleMatch` handle the new `*big.Int` return value. # Test: Search for calls to `ProcessSingleMatch`. Expect: All occurrences handle 5 return values. rg --type go -A 5 $'ProcessSingleMatch'Length of output: 8011
protocol/x/clob/keeper/process_single_match.go (2)
49-49: LGTM!The changes to
ProcessSingleMatchlook good. The newaffiliateRevSharesQuoteQuantumsparameter is passed through the function and returned appropriately, ensuring it is consistently updated and returned alongside existing results.Also applies to: 77-77, 92-92, 100-100, 134-134, 160-160, 193-193, 215-215, 230-230, 241-241, 293-293
313-313: Looks good!The changes to
persistMatchedOrdershandle the newaffiliateRevSharesQuoteQuantumsparameter and revenue share logic appropriately:
- All revenue shares associated with a fill are retrieved using
k.revshareKeeper.GetAllRevShares.- The affiliate revenue share is specifically extracted and assigned to
affiliateRevSharesQuoteQuantums.- The
k.subaccountsKeeper.DistributeFeescall is updated to accept the list of revenue shares instead of the total fee amount, reflecting the more complex fee structure.- Error handling for the new
GetAllRevSharescall is in place.- The
affiliateRevSharesQuoteQuantumsparameter is passed through return statements correctly.Overall, the changes align with the goal of incorporating affiliate revenue sharing into the order processing workflow.
Also applies to: 407-407, 419-419, 439-445, 447-457, 462-472, 476-477, 479-479, 525-525
protocol/x/revshare/keeper/revshare_test.go (2)
286-307: LGTM!The addition of the
RevSharePpmfield to theRevSharestruct instances in the test cases enhances the level of detail in the revenue share information being tested. The changes provide more comprehensive test coverage without altering the overall logic or flow of the tests.Also applies to: 355-376, 433-447, 485-492, 527-534
Line range hint
600-700:protocol/x/rewards/keeper/keeper_test.go (1)
Line range hint
148-420: LGTM! The changes significantly improve the test coverage and maintainability.The refactoring of the
TestAddRewardSharesForFillfunction enhances the structure and clarity of the test cases:
- The usage of the
clobtypes.FillForProcessstruct consolidates the fill-related data into a single entity, improving readability and maintainability.- The addition of
revsharesas a parameter allows for more comprehensive testing of reward share calculations, particularly in scenarios involving net fee revenue sharing.- The new test cases cover a wide range of scenarios, including various conditions regarding maker and taker fees, multiple revenue shares, and different fee sources. This ensures the robustness of the
AddRewardSharesForFillfunction.Great job on improving the test coverage and maintainability!
protocol/x/clob/keeper/process_operations.go (2)
Line range hint
509-546: LGTM!The change correctly captures the additional
affiliateRevSharesQuoteQuantumsreturn value fromProcessSingleMatchand includes it when emitting theOrderFillEvent. This enhances the indexer event by incorporating affiliate revenue share quantums for order fills.
Line range hint
618-649: LGTM!The change correctly captures the additional
affiliateRevSharesQuoteQuantumsreturn value fromProcessSingleMatchand includes it when emitting theLiquidationOrderFillEvent. This enhances the indexer event by incorporating affiliate revenue share quantums for liquidation order fills.protocol/mocks/ClobKeeper.go (1)
Line range hint
1003-1050: LGTM!The changes made to the
ProcessSingleMatchmock function look good:
- The function signature has been correctly updated to return an additional
*big.Intvalue.- The internal logic properly handles the new return value
r3and shifts the error return value tor4.- The conditional checks for the return values have been appropriately modified to process the new
*big.Intreturn type.These changes are consistent with the description provided in the AI-generated summary.
protocol/x/clob/e2e/short_term_orders_test.go (3)
Line range hint
580-778: Comprehensive and well-structured test for short term order replacements.The test function
TestShortTermOrderReplacementscomprehensively covers various scenarios for replacing short term orders, such as:
- Replacing orders in the same block and next block
- Handling partial fills
- Increasing and decreasing order sizes
- Handling IOC (Immediate-Or-Cancel) orders
The test uses a clean table-driven approach, making it easy to understand and extend with additional test cases. The assertions check the expected order existence in memclob and fill amounts, which is sufficient for verifying the behavior.
Overall, the test is well-structured, readable, and provides good coverage for short term order replacement scenarios. No major issues or improvements identified.
Line range hint
780-957: Comprehensive and well-structured test for canceling short term orders.The test function
TestCancelShortTermOrdercomprehensively covers various scenarios for canceling short term orders, such as:
- Canceling unfilled orders
- Canceling partially filled orders in the same block and next block
- Handling GTB (Good-Til-Block) values for cancels
- Canceling fully filled orders
The test uses a clean table-driven approach, making it easy to understand and extend with additional test cases. The assertions check the expected order existence in memclob, cancel expirations, and fill amounts, providing a thorough verification of the cancellation behavior.
Overall, the test is well-structured, readable, and provides good coverage for short term order cancellation scenarios. No major issues or improvements identified.
Line range hint
959-1186: Comprehensive and well-structured test for advanced short term orders.The test function
TestShortTermAdvancedOrderscomprehensively covers various scenarios for advanced short term order types, specifically IOC (Immediate-Or-Cancel) and PO (Post-Only) orders. The test cases cover scenarios such as:
- IOC orders fully matching and partially matching
- IOC orders not being placed on the book when not fully filled
- PO orders not crossing and being placed on the book
- PO orders crossing and not being placed on the book
The test uses a clean table-driven approach, making it easy to understand and extend with additional test cases. The assertions check the expected order existence in memclob and fill amounts, providing a good verification of the advanced order behavior.
Overall, the test is well-structured, readable, and provides good coverage for IOC and PO short term order scenarios. No major issues or improvements identified.
protocol/x/clob/e2e/long_term_orders_test.go (5)
Line range hint
321-1336: LGTM!The
TestPlaceLongTermOrderfunction is well-structured and comprehensive. It covers various scenarios for placing long-term orders and verifies the expected behavior of the system. The use of table-driven tests improves readability and maintainability.
Line range hint
1338-1735: LGTM!The
TestRegression_InvalidTimeInForcefunction covers an important regression scenario related to orders with invalid time in force. The test setup is appropriate, and the expectations are clearly defined. It verifies the expected behavior of the system when matching against an invalid order.
Line range hint
1737-1760: LGTM!The
TestLongTermOrderExpiresfunction covers the expiration behavior of long-term orders. It advances blocks with different block times and verifies the presence of the order in the state. The test is concise and focused on a specific aspect of long-term order behavior.
Line range hint
1762-1799: LGTM!The
TestImmediateExecutionLongTermOrdersfunction covers the behavior of long-term orders with immediate execution requirements. It attempts to place orders with IOC and FOK time in force and verifies that they are rejected. The test checks both CheckTx and DeliverTx to ensure consistent behavior.
Line range hint
1801-1875: LGTM!The
TestCancelStatefulOrderfunction is well-structured and comprehensive. It covers various scenarios for canceling stateful orders and verifies the expected behavior of order cancellation. The use of table-driven tests improves readability and maintainability.protocol/app/app.go (1)
1130-1130: Approve injectingRevShareKeeperintoClobKeeper.The change correctly passes the
RevShareKeeperinstance to theClobKeeperduring initialization. This satisfies the new dependency of theClobKeeperon theRevShareKeeper, allowing the clob module to interact with the revshare module.Verify that the
ClobKeepercorrectly uses the injectedRevShareKeeperin its implementation. Run the following command to grep forRevShareKeeperusage within theClobKeeper:Expect to see the
RevShareKeeperbeing used within theClobKeepercode, such as in methods likeAddRewardSharesForFill. If no usage is found, it could indicate dead code.Verification successful
To further verify the usage of
RevShareKeeperwithin theClobKeeper, we need to search for actual method calls or interactions with therevshareKeeperfield. Let's run another search:This command will search for occurrences of
k.revshareKeeper(assumingkis the common receiver name forClobKeepermethods) within theclob/keeperdirectory. This should help us identify if and how theRevShareKeeperis being used in theClobKeeper's implementation.
RevShareKeeperis correctly injected and used inClobKeeperThe verification process confirms that the
RevShareKeeperis properly injected into theClobKeeperand is being used in its implementation. Specifically:
RevShareKeeperis declared as a field in theClobKeeperstruct inprotocol/x/clob/keeper/keeper.go.- It's used in
protocol/x/clob/keeper/process_single_match.goto retrieve revenue shares for a fill:revshares, err := k.revshareKeeper.GetAllRevShares(ctx, fill)This usage aligns with the PR's goal of adding revenue sharing logic to the clob module. The
RevShareKeeperis utilized during the processing of matches in the CLOB system, likely to apply revenue sharing calculations to filled orders.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if RevShareKeeper is used after being injected rg --type go 'RevShareKeeper' protocol/x/clob/keeperLength of output: 197
Script:
#!/bin/bash # Search for usage of revshareKeeper within ClobKeeper methods rg --type go '\bk\.revshareKeeper\b' protocol/x/clob/keeperLength of output: 171
protocol/x/clob/keeper/process_operations_test.go (4)
Line range hint
2688-2699: LGTM!The
isClobMatchPerpetualDeleveragingfunction is correctly implemented. It uses type assertions to check if the operation is a ClobMatch with a MatchPerpetualDeleveraging, and returns a boolean value accordingly. The function name and return values are clear and descriptive.
Line range hint
2701-2716: LGTM!The
assertSubaccountStatefunction is correctly implemented. It asserts the state of subaccounts after processing operations by checking the USDC balance and perpetual positions of each subaccount. It compares the actual values with the expected values provided in the test case using therequire.Equalandrequire.ElementsMatchfunctions from thetestifypackage. The function name and parameters are clear and descriptive.
Line range hint
2718-2854: LGTM!The
setupProcessProposerOperationsTestCasefunction is well-structured and readable, despite being quite long and complex. It correctly sets up the test context and keepers for aProcessProposerOperationstest case based on the provided configuration. It uses helper functions from thekeepertestpackage to create test markets, liquidity tiers, perpetuals, subaccounts, and CLOBs. It also correctly sets up the mock bank keeper and indexer event manager based on the test case expectations. The function is an important part of the test setup and is implemented correctly.
Line range hint
2856-2920: LGTM!The
runProcessProposerOperationsTestCasefunction is well-structured and readable. It correctly sets up the test context and keepers using thesetupProcessProposerOperationsTestCasefunction, and then calls theProcessProposerOperationsfunction with the test case operations. It checks the results by verifying the process proposer matches events, stateful orders, subaccount liquidation info, subaccount state, order fill amounts, and negative TNC subaccount seen blocks. It uses therequirepackage to assert the expected values against the actual values, and also verifies the mock indexer event manager expectations usingmockIndexerEventManager.AssertExpectations(t). The function is an important part of the test execution and is implemented correctly.
affanv14
force-pushed
the
affan/revshare-logic
branch
from
fdc83a5 to
0609f5b
Compare
| @@ -1331,6 +1333,7 @@ func TestPlaceLongTermOrder(t *testing.T) { | |||
| 25_000_000, | |||
| LongTermPlaceOrder_Alice_Num0_Id0_Clob0_Buy2_Price50000_GTBT5.Order.GetBaseQuantums(), | |||
| PlaceOrder_Bob_Num0_Id1_Clob0_Sell1_Price50000_GTB20.Order.GetBaseQuantums(), | |||
| big.NewInt(0), | |||
There was a problem hiding this comment.
For these tests, felt like this was fine to do since the default path(no affiliates) will have 0 revshare
There was a problem hiding this comment.
Let's cover the full path in some e2e test https://linear.app/dydx/issue/OTE-730/add-e2e-tests-for-affiliates. Might make sense to add to some of these existing tests instead of creating entirely new ones
| @@ -1696,7 +1696,7 @@ func (m *MemClobPriceTimePriority) mustPerformTakerOrderMatching( | |||
| FillAmount: matchedAmount, | |||
| } | |||
|
|
|||
| success, takerUpdateResult, makerUpdateResult, err := m.clobKeeper.ProcessSingleMatch(ctx, &matchWithOrders) | |||
| success, takerUpdateResult, makerUpdateResult, _, err := m.clobKeeper.ProcessSingleMatch(ctx, &matchWithOrders) | |||
There was a problem hiding this comment.
order fill event was not being emitted by memclob - skipping
There was a problem hiding this comment.
FYI MemClob does emit offchain update events (for indexer and grpc streaming) here. It just that this rev share info is not needed in that event
| } | ||
|
|
||
| // Remaining amount goes to the fee collector | ||
| feeCollectorShare := new(big.Int).Sub(quantums, marketMapperShare) | ||
| feeCollectorShare := new(big.Int).Sub(totalFeeQuoteQuantums, totalRevShareQuoteQuantums) |
There was a problem hiding this comment.
Nit: explicitly check if feeCollectorsShare <= 0. If so panic
There was a problem hiding this comment.
Too many unrelated tests rely on feeCollectorsShare = 0. Setting it to < 0 rather than <=0
There was a problem hiding this comment.
Too many unrelated tests rely on feeCollectorsShare = 0.
Why is this the case? Wouldn't feeCollectorsShare = 0 indicate totalFeeQuoteQuantums = totalRevShareQuoteQuantums? Is it because in a lot of test cases both are zero? (We the protocol constraint that every fill results in protocol revenue)
protocol/x/rewards/keeper/keeper.go
Outdated
| if revshare.RevShareFeeSource == revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE { | ||
| totalNetFeeRevSharePpm += revshare.RevSharePpm | ||
| } | ||
| if revshare.RevShareType == revsharetypes.REV_SHARE_TYPE_AFFILIATE { |
There was a problem hiding this comment.
Think we are conflating two concepts here. We should only care about FeeSource in this logic (i.e. Reward keeper just cares about whether its a taker fee share instead of whether it goes to an affiliates)
There was a problem hiding this comment.
Also think we can avoid this loop by return additional fields here
protocol/x/rewards/keeper/keeper.go
Outdated
| @@ -126,26 +128,45 @@ func (k Keeper) GetRewardShare( | |||
| // by quote quantums of the fill. | |||
There was a problem hiding this comment.
Let's modified comment according to this
protocol/x/rewards/keeper/keeper.go
Outdated
| takerWeight := new(big.Int).Add( | ||
| bigTakerFeeQuoteQuantums, | ||
| makerRebateMulTakerVolume := lib.BigMulPpm(fill.FillQuoteQuantums, lib.BigI(maxMakerRebatePpm), false) | ||
| netTakerFeeWithoutAffiliateRevshare := new(big.Int).Add( |
There was a problem hiding this comment.
Is this variable name correct? You didn't subtract netTakerFeeWithoutAffiliateRevshare yet
There was a problem hiding this comment.
Also we can avoid allocating additional pointer by :
netTakerFee := new(big.Int).Add(
fill.TakerFeeQuoteQuantums,
makerRebateMulTakerVolume,
)
netTakerFee.Sub(
netTakerFeeWithoutAffiliateRevshare,
affiliateRevShareQuoteQuantums,
)
protocol/x/rewards/keeper/keeper.go
Outdated
| affiliateRevShareQuoteQuantums = revshare.QuoteQuantums | ||
| } | ||
| } | ||
| remainingRevSharePpm := lib.OneMillion - totalNetFeeRevSharePpm |
There was a problem hiding this comment.
Variable naming doesn't make sense. We aren't computing remaining revenue shares
| remainingRevSharePpm := lib.OneMillion - totalNetFeeRevSharePpm | |
| remainingRevSharePpm := lib.OneMillion - totalNetFeeRevSharePpm |
There was a problem hiding this comment.
Terrible naming on my part
| } | ||
|
|
||
| totalRevShareQuoteQuantums := big.NewInt(0).Add( | ||
| revSharesForFill.FeeSourceToQuoteQuantums[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE], |
There was a problem hiding this comment.
The keys revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE and revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE aren't guaranteed to exist right?
There was a problem hiding this comment.
Since revSharesForFill are gotten from GetAllRevShares - the only way these keys arent set are if there is an error. Otherwise we know they exist since I explicitly set them
There was a problem hiding this comment.
But frankly, better to check either way - so will update!
protocol/x/rewards/keeper/keeper.go
Outdated
| totalNetFeeRevSharePpm = revSharesForFill.FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE] | ||
| } | ||
| totalTakerFeeRevShareQuantums := big.NewInt(0) | ||
| if _, ok := revSharesForFill.FeeSourceToQuoteQuantums[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE]; ok { |
There was a problem hiding this comment.
You can simply this by:
if value, ok := revSharesForFill.FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE]; ok {
totalNetFeeRevSharePpm = value
}
Changelist
gets revshares in clob
Updates distribute fees logic to take account of revshare
Updates AddRewardSharesForFill
Emits indexer event on orderfills
Test Plan
Adds unit 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
RevSharePpm.MarketIdfield for better tracking of fills related to different markets.Bug Fixes
Documentation
Refactor