[OTE-790]Implement affiliates whitelist logic #2258
Conversation
WalkthroughThe changes introduce enhancements to the affiliate management system across various components, including the addition of methods for updating affiliate whitelists, new message types, and modifications to existing functions. Key functionalities include the ability to manage affiliate relationships more dynamically, implement whitelists, and improve the handling of revenue shares based on affiliate status. The updates also include extensive testing to ensure the reliability of these new features. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (2)
protocol/testutil/memclob/keeper.go (1)
330-330: Approve the addition ofaffiliatesWhitelistMapparameter.The new
affiliatesWhitelistMapparameter has been added to theProcessSingleMatchfunction signature, which is a step towards implementing the affiliate whitelisting logic.However, the parameter is currently unused within the function body. Consider utilizing it to filter or process matches based on the affiliate whitelist. For example:
// Check if the maker or taker order belongs to a whitelisted affiliate if _, ok := affiliatesWhitelistMap[makerOrder.AffiliateAddress]; !ok { // Handle case when maker order is not from a whitelisted affiliate } if _, ok := affiliatesWhitelistMap[takerOrder.AffiliateAddress]; !ok { // Handle case when taker order is not from a whitelisted affiliate }protocol/app/app.go (1)
1124-1124: Consider defining an interface for the AffiliatesKeeper dependency.Passing the concrete
AffiliatesKeeperto theClobKeepercreates a direct dependency between these modules. If possible, consider defining an interface that encapsulates just the functionality theClobKeeperneeds from theAffiliatesKeeper. This would decouple the modules and improve testability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/affiliates/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (80)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.rpc.msg.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.ts (3 hunks)
- proto/dydxprotocol/affiliates/tx.proto (2 hunks)
- protocol/app/app.go (1 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/lib/ante/internal_msg.go (1 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 (2 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 (3 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/testutil/keeper/clob.go (6 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/memclob/keeper.go (1 hunks)
- protocol/x/affiliates/keeper/grpc_query.go (3 hunks)
- protocol/x/affiliates/keeper/grpc_query_test.go (4 hunks)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (5 hunks)
- protocol/x/affiliates/keeper/msg_server.go (1 hunks)
- protocol/x/affiliates/keeper/msg_server_test.go (1 hunks)
- protocol/x/affiliates/types/errors.go (1 hunks)
- protocol/x/affiliates/types/keys.go (1 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (6 hunks)
- protocol/x/clob/keeper/process_single_match.go (4 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 (1 hunks)
- protocol/x/clob/types/mem_clob_keeper.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (2 hunks)
- protocol/x/revshare/keeper/revshare_test.go (3 hunks)
Files skipped from review due to trivial changes (49)
- 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
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.ts
[error] 72-72: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 75-75: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (59)
protocol/x/affiliates/types/keys.go (1)
19-20: LGTM!The introduction of the
AffiliateWhitelistKeyconstant is a good addition. It follows the naming convention of the existing constants and is assigned an appropriate string value.This constant lays the foundation for implementing affiliate whitelist functionality in the module.
protocol/x/affiliates/types/errors.go (2)
9-10: LGTM!The new error declaration is syntactically correct, and the error message is clear and descriptive. The formatting change improves readability without altering the functionality.
15-16: Verify the error handling in the whitelisting logic.The new error declaration for duplicate affiliate addresses in the whitelist is clear and consistent with the existing code. However, please ensure that this error is handled appropriately wherever the whitelisting logic is implemented.
Run the following script to verify the error handling:
Verification successful
Error handling for duplicate affiliate addresses is implemented correctly
The
ErrDuplicateAffiliateAddressForWhitelisterror is well-defined and properly handled in the codebase:
- Defined in
protocol/x/affiliates/types/errors.go- Used in test cases in
protocol/x/affiliates/keeper/keeper_test.go- Correctly implemented in
protocol/x/affiliates/keeper/keeper.goThe implementation efficiently checks for duplicate addresses using a map and returns the error with additional context when a duplicate is found. This approach ensures proper handling of the duplicate affiliate address scenario in the whitelist logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of `ErrDuplicateAffiliateAddressForWhitelist` in the whitelisting logic. # Test: Search for the error usage. Expect: Occurrences in the whitelisting logic with proper error handling. rg --type go -A 5 $'ErrDuplicateAffiliateAddressForWhitelist'Length of output: 1133
proto/dydxprotocol/affiliates/tx.proto (3)
19-21: LGTM!The new RPC method
UpdateAffiliateWhitelistis declared correctly in theMsgservice. The method signature, input message, and output message are consistent with the existing code style and naming conventions.
51-58: Looks good!The
MsgUpdateAffiliateWhitelistmessage is defined correctly with appropriate fields and types. Theauthorityfield uses thecosmos.AddressStringscalar type, and thewhitelistfield is marked as non-nullable. The message also includes the necessary Cosmos-specific option to indicate the required signer.
60-61: Approved.The
MsgUpdateAffiliateWhitelistResponsemessage is defined correctly as an empty message. This is acceptable since no specific response data is required for theUpdateAffiliateWhitelistRPC method.indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.rpc.msg.ts (4)
3-3: LGTM!The import statement correctly adds the new message types for updating affiliate whitelists, which aligns with the PR objective.
12-14: LGTM!The new
updateAffiliateWhitelistmethod is correctly added to theMsginterface with the appropriate request and response types, which aligns with the PR objective.
23-23: LGTM!The
updateAffiliateWhitelistmethod is correctly bound in the constructor of theMsgClientImplclass, which is necessary for its implementation.
38-43: LGTM!The
updateAffiliateWhitelistmethod is correctly implemented in theMsgClientImplclass, following the same pattern as the other methods. It properly encodes the request, sends the RPC request, and decodes the response.protocol/x/affiliates/keeper/msg_server.go (1)
62-74: LGTM!The
UpdateAffiliateWhitelistfunction is implemented correctly and follows the expected behavior:
- It checks for the caller's authority before performing the update, ensuring only authorized modifications.
- It returns appropriate errors in case of failure, providing informative error messages.
- It updates the whitelist by calling the keeper's
SetAffiliateWhitelistmethod, ensuring the update is performed correctly.- It returns the expected response type
MsgUpdateAffiliateWhitelistResponseupon successful execution.The function enhances the functionality of the
msgServerby allowing for dynamic updates to the affiliate whitelist while maintaining proper authorization checks.protocol/x/clob/types/mem_clob_keeper.go (1)
24-24: LGTM! Consider the implementation details.The addition of the
affiliatesWhitelistMapparameter to theProcessSingleMatchmethod is a good change that enhances the affiliate management capabilities of the system. This change enables more granular control over affiliate involvement in the matching process.When implementing this change, ensure that:
- The
affiliatesWhitelistMapis properly populated and maintained with the correct affiliate IDs and their corresponding whitelist status.- The
ProcessSingleMatchmethod implementation is updated to utilize theaffiliatesWhitelistMapeffectively, filtering or managing matches based on the affiliate whitelist status.- The change is thoroughly tested to verify that the affiliate whitelisting logic works as intended and does not introduce any unintended side effects.
Overall, this is a positive change that aligns with the goal of improving affiliate management in the system.
protocol/x/affiliates/keeper/grpc_query.go (2)
25-40: LGTM! The new whitelist functionality enhances affiliate management.The changes introduce a new way to manage affiliates using a whitelist, which takes precedence over the tier-based system. This allows for more flexibility in managing affiliate relationships and revenue shares.
The implementation is well-structured and does not introduce any apparent issues or bugs. The function first checks the affiliate whitelist map and retrieves the fee share ppm if the address is whitelisted. If not, it falls back to the original tier-based logic.
Overall, the changes improve the affiliate management system by providing a more dynamic and flexible approach.
92-101: Great job implementing the previously unimplemented function!The changes complete the implementation of the
AffiliateWhitelistfunction, which was previously marked as a TODO. The function now retrieves the affiliate whitelist using theGetAffiliateWhitelistmethod and returns it in the response.The implementation is straightforward and does not introduce any apparent issues or bugs. This addition provides a concrete way to access the affiliate whitelist, enhancing the functionality of the RPC method.
protocol/x/clob/types/clob_keeper.go (1)
84-84: Approve the addition of theaffiliatesWhitelistMapparameter to theProcessSingleMatchmethod.The inclusion of the
affiliatesWhitelistMapparameter in theProcessSingleMatchmethod signature is a reasonable change to incorporate affiliate whitelisting logic into the match processing workflow.However, please ensure the following:
The
affiliatesWhitelistMapis correctly populated and maintained throughout the system. Verify that the map is updated whenever there are changes to the affiliate whitelist.The match processing logic within the
ProcessSingleMatchmethod has been updated to handle the new affiliate whitelisting behavior correctly. Ensure that matches are processed according to the affiliate status of the involved parties.The performance impact of this change has been considered and deemed acceptable. Assess whether the addition of affiliate whitelisting logic significantly affects the execution time of the
ProcessSingleMatchmethod, especially for large whitelists.You can use the following script to verify the points mentioned above:
Please let me know if you have any further concerns or if you need assistance with the verification process.
protocol/x/clob/types/expected_keepers.go (1)
173-176: LGTM!The introduction of the
AffiliatesKeeperinterface and theGetAffiliateWhitelistMapmethod aligns with the PR objectives and enhances the modularity and extensibility of the codebase. The method signature is clear and follows the naming convention of the codebase.The changes provide a clear contract for affiliate-related operations and enable the retrieval of a structured representation of affiliates, which could be leveraged in other parts of the application for validation, access control, or business logic involving partnerships or revenue sharing.
protocol/lib/ante/internal_msg.go (1)
147-148: LGTM!The addition of
*affiliates.MsgUpdateAffiliateWhitelistto the list of internal message types in theIsInternalMsgfunction is consistent with the existing pattern and appears to be a valid change.This will ensure that messages of type
*affiliates.MsgUpdateAffiliateWhitelistare correctly identified as internal messages, which may affect their processing and handling within the application.protocol/x/affiliates/keeper/msg_server_test.go (1)
136-182: LGTM!The
TestMsgServer_UpdateAffiliateWhitelistfunction is a well-structured and comprehensive test that covers the essential scenarios for updating the affiliate whitelist. It ensures that only authorized entities (i.e., the governance module) can modify the whitelist, while unauthorized attempts are correctly rejected.The test cases are clearly defined, with appropriate setup and assertions to validate the expected behavior. This addition enhances the overall test coverage and reinforces the integrity of the affiliate management system.
protocol/testutil/keeper/listing.go (1)
152-155: LGTM!The addition of
affiliatesKeeperto theListingKeepersfunction and passing it to thecreateClobKeeperfunction is consistent with the provided summary. This change enables theclobKeeperto interact with theaffiliatesKeeper, potentially for managing affiliate-related data or operations.The change is unlikely to have any negative impact on the existing functionality.
protocol/app/msgs/internal_msgs_test.go (1)
69-70: LGTM!The added message paths expand the test coverage for affiliate-related functionalities, improving the robustness of the codebase. The changes are well-placed within the
TestInternalMsgSamples_Gov_Keyfunction and align with the existing code structure.protocol/x/affiliates/keeper/grpc_query_test.go (2)
32-32: LGTM!The changes to the
TestAffiliateInfofunction enhance the test coverage by including a new test case for a whitelisted affiliate. The test setup for the whitelisted case ensures that the system behaves as expected when an affiliate is part of the whitelist. Setting theIsWhitelistedfield tofalsefor existing test cases ensures that the system correctly identifies non-whitelisted affiliates.Also applies to: 62-62, 94-131
228-249: LGTM!The new
TestAffiliateWhitelistfunction provides coverage for theSetAffiliateWhitelistandAffiliateWhitelistmethods of thekeeperpackage. It ensures that the system behaves as expected when setting and retrieving the affiliate whitelist.protocol/x/revshare/keeper/revshare.go (2)
153-161: LGTM!The addition of the
affiliatesWhitelistMapparameter toGetAllRevSharesand the corresponding update to thegetAffiliateRevSharesfunction call enable more granular control over affiliate revenue sharing. This change allows for filtering affiliate revenue shares based on a predefined whitelist, providing better management of affiliate relationships and fee distributions.The core logic of the function remains unchanged, minimizing the risk of introducing correctness issues or bugs.
194-203: Looks good!The addition of the
affiliatesWhitelistMapparameter togetAffiliateRevSharesand the corresponding update to theGetTakerFeeSharefunction call from theaffiliatesKeeperallow for more granular control over affiliate fee sharing. This change enables filtering of affiliate fee shares based on a predefined whitelist, providing better management of affiliate relationships and fee distributions.The core logic of the function remains unchanged, minimizing the risk of introducing correctness issues or bugs.
protocol/testutil/keeper/clob.go (4)
20-20: LGTM!The import statement for the
affiliateskeeperpackage is necessary and looks good.
52-52: LGTM!The
AffiliatesKeeperfield is correctly added to theClobKeepersTestContextstruct.
110-112: LGTM!The
AffiliatesKeeperandrevShareKeeperare initialized correctly in theNewClobKeepersTestContextWithUninitializedMemStorefunction.
189-189: LGTM!The
createClobKeeperfunction is correctly modified to include theaffiliatesKeeperparameter and pass it to theNewKeeperfunction.Also applies to: 228-228, 260-260
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.ts (2)
52-60: LGTM!The
MsgUpdateAffiliateWhitelistinterface is well-defined and follows the expected structure for a message type. Theauthorityand optionalwhitelistproperties are appropriately typed.
61-69: LGTM!The
MsgUpdateAffiliateWhitelistSDKTypeinterface correctly mirrors the structure ofMsgUpdateAffiliateWhitelistand uses the appropriate SDK type for thewhitelistproperty.protocol/x/affiliates/keeper/keeper.go (4)
Line range hint
164-184: LGTM!The changes to the
GetTakerFeeSharefunction look good. The addition of theaffiliatesWhitelistMapparameter and the logic to override the fee share for whitelisted affiliates is implemented correctly.
270-282: Looks good!The new
GetAffiliateWhitelistMapfunction is a useful addition. It correctly retrieves the affiliate whitelist and constructs a map of addresses to their fee shares, which can be used for efficient lookups.
284-299: Nice work!The new
SetAffiliateWhitelistfunction is implemented well. It correctly checks for duplicate addresses in the provided whitelist and returns an appropriate error if any are found. The marshaling and storing of the whitelist in the key-value store is handled properly.
301-315: Excellent!The new
GetAffiliateWhitelistfunction is implemented correctly. It properly retrieves the affiliate whitelist from the key-value store and handles the case when no whitelist exists by returning an empty whitelist. The unmarshaling of the whitelist and error propagation is handled appropriately.protocol/x/clob/keeper/keeper.go (2)
45-45: LGTM!The addition of the
affiliatesKeeperfield to theKeeperstruct is a valid change that aligns with the summary. This change introduces a new dependency on the affiliates module, allowing theKeeperto manage or interact with affiliate-related data or operations.
89-89: LGTM!The addition of the
affiliatesKeeperparameter to theNewKeeperfunction and its assignment to the corresponding field in theKeeperstruct are valid changes that align with the summary. These changes ensure that theKeeperis properly initialized with theaffiliatesKeeperdependency, allowing it to be used within the keeper's methods for managing or interacting with affiliate-related data or operations.Also applies to: 114-114
protocol/app/msgs/internal_msgs.go (1)
111-114: LGTM!The addition of the new message types
MsgUpdateAffiliateWhitelistandMsgUpdateAffiliateWhitelistResponseto theInternalMsgSamplesDydxCustommap looks good. This change enhances the affiliate management functionality by allowing updates to an affiliate whitelist.The code follows the existing conventions and is consistent with the AI-generated summary.
protocol/mocks/SubaccountsKeeper.go (2)
190-205: LGTM!The mock function
GetStreamSubaccountUpdateis implemented correctly. It properly handles the case when no return value is specified by panicking with a descriptive message. The function returns the predefined value if specified, otherwise it panics as expected.
254-256: LGTM!The mock function
SendFinalizedSubaccountUpdatesis implemented correctly. It properly registers the call using theCalledmethod with the provided arguments. The function doesn't return any value, which is expected for a function that sends updates.protocol/mocks/MemClobKeeper.go (1)
Line range hint
323-362: LGTM!The changes to the
ProcessSingleMatchmethod are consistent with the mock's purpose of simulating the behavior of the actual implementation. The method signature has been updated to include a new parameteraffiliatesWhitelistMapof typemap[string]uint32, and the method body has been updated to pass the new parameter to the mock'sCalledmethod and retrieve the return values using the new parameter.protocol/app/msgs/all_msgs.go (1)
152-157: LGTM! The addition of new message types enhances the affiliate management functionality.The introduction of
MsgUpdateAffiliateWhitelistandMsgUpdateAffiliateWhitelistResponsemessage types suggests an expansion of the affiliate management capabilities. These new message types likely enable the updating of affiliate whitelists, providing more granular control over affiliate participation and compliance.The existing message types for registering affiliates and updating affiliate tiers remain unchanged, indicating that the new whitelist functionality is a supplementary feature rather than a modification of the core affiliate management flow.
Overall, these changes reflect a well-structured enhancement of the affiliate management system.
protocol/x/affiliates/keeper/keeper_test.go (7)
396-464: LGTM!The test cases for
SetAffiliateWhitelistare comprehensive and cover various scenarios, including single tier with single address, multiple tiers with multiple addresses, and duplicate addresses across tiers. The test cases ensure that the function behaves as expected and handles both valid and invalid whitelist configurations correctly.
466-540: LGTM!The test cases for
GetAffiliateWhiteListMapare comprehensive and cover various scenarios, including multiple tiers with multiple addresses, single tier with single address, empty tiers, and nil whitelist. The test cases ensure that the function correctly converts the affiliate whitelist to a map format and handles different whitelist configurations as expected.
542-617: LGTM!The test cases for
GetTakerFeeShareViaWhitelistare comprehensive and cover various scenarios, including affiliate in whitelist, affiliate not in whitelist, and referee not registered. The test cases ensure that theGetTakerFeeSharefunction correctly retrieves the taker fee share based on the affiliate whitelist and handles different scenarios as expected.
188-188: Verify the usage of the additional parameter in the codebase.The changes look good. However, ensure that the additional parameter passed to
GetTakerFeeShareis being used correctly and consistently across the codebase.Run the following script to verify the usage of the additional parameter:
Verification successful
Verification successful: Additional parameter usage is consistent
The additional parameter
affiliatesWhitelistMapis being used correctly and consistently across the codebase. All calls toGetTakerFeeShareinclude this parameter, and the implementation in the keeper uses it as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the additional parameter in `GetTakerFeeShare` calls. # Test: Search for `GetTakerFeeShare` calls. Expect: Consistent usage of the additional parameter. rg --type go -A 5 $'GetTakerFeeShare'Length of output: 6391
175-175: Verify the usage of the additional parameter in the codebase.The changes look good. However, ensure that the additional parameter passed to
GetTakerFeeShareis being used correctly and consistently across the codebase.Run the following script to verify the usage of the additional parameter:
Verification successful
Additional parameter usage verified and consistent
The
affiliatesWhitelistMapparameter is consistently used in theGetTakerFeeSharefunction definition and all calls to the function throughout the codebase. In production code, it's passed as expected, while in test files, an empty map is used, which is a valid scenario. This consistent usage indicates that the changes are intentional and correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the additional parameter in `GetTakerFeeShare` calls. # Test: Search for `GetTakerFeeShare` calls. Expect: Consistent usage of the additional parameter. rg --type go -A 5 $'GetTakerFeeShare'Length of output: 6391
224-224: Verify the usage of the additional parameter in the codebase.The changes look good. However, ensure that the additional parameter passed to
GetTakerFeeShareis being used correctly and consistently across the codebase.Run the following script to verify the usage of the additional parameter:
Verification successful
Approve changes: Additional parameter consistently used
The additional
affiliatesWhitelistMapparameter inGetTakerFeeShareis consistently used across the codebase. It's properly implemented in the keeper and utilized in production code (revshare.go). In test files, an empty map is passed, which is valid but might not fully test the whitelist functionality. Consider adding tests with non-empty whitelist maps to ensure comprehensive coverage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the additional parameter in `GetTakerFeeShare` calls. # Test: Search for `GetTakerFeeShare` calls. Expect: Consistent usage of the additional parameter. rg --type go -A 5 $'GetTakerFeeShare'Length of output: 6391
242-242: Verify the usage of the additional parameter in the codebase.The changes look good. However, ensure that the additional parameter passed to
GetTakerFeeShareis being used correctly and consistently across the codebase.Run the following script to verify the usage of the additional parameter:
Verification successful
The additional parameter is used correctly and consistently across the codebase.
The
GetTakerFeeSharefunction is called with the newaffiliatesWhitelistMapparameter in all relevant locations:
- In
protocol/x/revshare/keeper/revshare.go- In multiple test cases in
protocol/x/affiliates/keeper/keeper_test.goThe usage is consistent with the function signature defined in
protocol/x/affiliates/keeper/keeper.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the additional parameter in `GetTakerFeeShare` calls. # Test: Search for `GetTakerFeeShare` calls. Expect: Consistent usage of the additional parameter. rg --type go -A 5 $'GetTakerFeeShare'Length of output: 6391
protocol/x/clob/keeper/process_single_match.go (2)
44-44: LGTM! Consider implementing the affiliate whitelist logic.The addition of the
affiliatesWhitelistMapparameter is a preparatory change for implementing the affiliate whitelist logic. The parameter is not used within the function, indicating that the whitelist logic is not yet implemented.Consider implementing the whitelist logic in a future PR to complete the feature.
309-309: LGTM! Implement the affiliate whitelist logic in thegetAllRevSharesfunction.The addition of the
affiliatesWhitelistMapparameter is a preparatory change for implementing the affiliate whitelist logic. The parameter is not used within the function, indicating that the whitelist logic is not yet implemented.The TODO comment suggests that the whitelist map should be used in the
getAllRevSharesfunction. Consider implementing the whitelist logic in thegetAllRevSharesfunction to complete the feature.Also applies to: 441-442
protocol/x/revshare/keeper/revshare_test.go (2)
556-606: LGTM!The new test case is well-structured and follows the existing pattern. It correctly sets up the necessary parameters for market mappers, affiliates, and affiliate whitelists. The expected revenue shares are properly defined based on the whitelisted affiliate's share and the market mapper's share. The test case verifies that the returned revenue shares match the expected values.
638-641: LGTM!The changes to retrieve the affiliate whitelist map and pass it to the
GetAllRevSharesfunction are necessary and consistent with the new test case and the overall functionality of the revenue share system. Retrieving the affiliate whitelist map from the affiliates keeper ensures that the latest whitelist information is used when calculating the revenue shares.protocol/x/clob/keeper/process_operations.go (4)
167-173: LGTM!The code segment correctly retrieves the affiliates whitelist map from the affiliates keeper and handles the error case appropriately.
175-175: LGTM!The
PersistMatchToStatefunction call has been updated to include theaffiliatesWhitelistMapargument, which is consistent with the function signature change.
231-243: LGTM!The
PersistMatchToStatefunction signature has been updated to include theaffiliatesWhitelistMapparameter, which is then passed to the respective functions for processing match orders and liquidations. The changes are consistent and ensure the availability of theaffiliatesWhitelistMapin the downstream functions.
474-474: LGTM!The
PersistMatchOrdersToStateandPersistMatchLiquidationToStatefunctions have been updated to include theaffiliatesWhitelistMapparameter, which is then passed to theProcessSingleMatchfunction for each match fill. The changes are consistent and ensure the availability of theaffiliatesWhitelistMapin theProcessSingleMatchfunction for processing individual match fills.Also applies to: 519-519, 590-631
protocol/mocks/ClobKeeper.go (1)
Line range hint
1002-1040: Approve the mock function signature update.The addition of the
affiliatesWhitelistMapparameter to theProcessSingleMatchmock function signature looks good. The parameter is correctly propagated to the underlying mock call.Verify that the actual implementation of
ProcessSingleMatchcorrectly handles theaffiliatesWhitelistMapparameter and applies the affiliates whitelist logic as intended. Pay close attention to how the whitelist is used during match processing and ensure it aligns with the expected behavior.You can use the following script to search for the
ProcessSingleMatchfunction in the codebase:protocol/app/app.go (1)
1124-1124: Integration of AffiliatesKeeper looks good.The
AffiliatesKeeperis now being passed to theClobKeeper, allowing theClobKeeperto interact with the affiliates module as needed.protocol/x/clob/memclob/memclob.go (1)
1699-1701: LGTM! The new parameter enables affiliate whitelisting logic in order matching.The introduction of the
affiliatesWhitelistMapparameter, initialized as an empty map, suggests an enhancement to handle affiliate-related logic during the order matching process. By passing this map to theProcessSingleMatchmethod, it allows the utilization of the affiliates whitelist data within the matching logic.The map type
map[string]uint32indicates that the keys represent affiliate identifiers, while the values hold some numeric metadata associated with each affiliate.This change expands the method signature to accommodate additional functionality without altering the existing control flow or error handling.
| /** Response to MsgUpdateAffiliateWhitelist */ | ||
|
|
||
| export interface MsgUpdateAffiliateWhitelistResponseSDKType {} |
There was a problem hiding this comment.
Consider using a type alias instead of an empty interface.
Similar to the MsgUpdateAffiliateWhitelistResponse interface, the MsgUpdateAffiliateWhitelistResponseSDKType interface is empty. Consider using a type alias instead to improve code clarity and maintainability.
-export interface MsgUpdateAffiliateWhitelistResponseSDKType {}
+export type MsgUpdateAffiliateWhitelistResponseSDKType = Record<string, never>;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Response to MsgUpdateAffiliateWhitelist */ | |
| export interface MsgUpdateAffiliateWhitelistResponseSDKType {} | |
| /** Response to MsgUpdateAffiliateWhitelist */ | |
| export type MsgUpdateAffiliateWhitelistResponseSDKType = Record<string, never>; |
Tools
Biome
[error] 75-75: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| /** Response to MsgUpdateAffiliateWhitelist */ | ||
|
|
||
| export interface MsgUpdateAffiliateWhitelistResponse {} |
There was a problem hiding this comment.
Consider using a type alias instead of an empty interface.
The MsgUpdateAffiliateWhitelistResponse interface is empty, which is equivalent to {}. To improve code clarity and maintainability, consider using a type alias instead.
-export interface MsgUpdateAffiliateWhitelistResponse {}
+export type MsgUpdateAffiliateWhitelistResponse = Record<string, never>;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Response to MsgUpdateAffiliateWhitelist */ | |
| export interface MsgUpdateAffiliateWhitelistResponse {} | |
| /** Response to MsgUpdateAffiliateWhitelist */ | |
| export type MsgUpdateAffiliateWhitelistResponse = Record<string, never>; |
Tools
Biome
[error] 72-72: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/affiliates/keeper/keeper_test.go (1)
176-176: Add a comment to clarify the purpose of the empty map argument.The empty map passed as the third argument to
GetTakerFeeShareis not immediately clear without additional context. Consider adding a comment to explain that it represents an affiliate whitelist or any other relevant information.Also applies to: 189-189
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (24)
- protocol/app/app.go (1 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/lib/ante/internal_msg.go (1 hunks)
- protocol/mocks/ClobKeeper.go (2 hunks)
- protocol/mocks/MemClobKeeper.go (2 hunks)
- protocol/testutil/keeper/clob.go (7 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/memclob/keeper.go (1 hunks)
- protocol/x/affiliates/keeper/grpc_query.go (3 hunks)
- protocol/x/affiliates/keeper/grpc_query_test.go (4 hunks)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (5 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (6 hunks)
- protocol/x/clob/keeper/process_single_match.go (4 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 (1 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 (3 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (19)
- protocol/app/app.go
- protocol/app/msgs/all_msgs.go
- protocol/app/msgs/internal_msgs.go
- protocol/app/msgs/internal_msgs_test.go
- protocol/lib/ante/internal_msg.go
- protocol/mocks/ClobKeeper.go
- protocol/mocks/MemClobKeeper.go
- protocol/testutil/keeper/clob.go
- protocol/testutil/keeper/listing.go
- protocol/testutil/memclob/keeper.go
- protocol/x/affiliates/keeper/keeper.go
- protocol/x/clob/keeper/keeper.go
- protocol/x/clob/keeper/process_operations.go
- protocol/x/clob/keeper/process_single_match.go
- protocol/x/clob/memclob/memclob.go
- protocol/x/clob/types/clob_keeper.go
- protocol/x/clob/types/mem_clob_keeper.go
- protocol/x/revshare/keeper/revshare.go
- protocol/x/revshare/keeper/revshare_test.go
Additional comments not posted (13)
protocol/x/affiliates/keeper/grpc_query.go (2)
25-40: Verify the impact on the affiliate revenue sharing logic.The changes introduce a new way to manage affiliate fee shares through a whitelist, which takes precedence over the tier-based system. This allows for more flexibility in managing affiliates.
Please ensure that the overall affiliate revenue sharing logic is thoroughly tested, considering the following scenarios:
- Affiliates who are on the whitelist
- Affiliates who are not on the whitelist but have a tier level
- Affiliates who are neither on the whitelist nor have a tier level
88-97: LGTM, but note the breaking change in the response type.The
AffiliateWhitelistfunction has been implemented to retrieve the affiliate whitelist from the context and return it in the response. The implementation looks good.However, please note that the response type has been updated to include the whitelist, which is a breaking change for any clients using this RPC method. Ensure that the relevant documentation and clients are updated accordingly.
protocol/x/clob/types/expected_keepers.go (2)
184-186: LGTM!The addition of the
AffiliatesKeeperinterface and theGetAffiliateWhitelistMapmethod is a great way to enhance the affiliate management functionality in a modular and extensible manner. This will enable better integration of affiliate whitelisting across the application.
178-178: Update implementations and callers ofGetAllRevShares.The change to the
GetAllRevSharesmethod signature is a good enhancement for integrating affiliate management with revenue sharing. However, please ensure that:
- All implementations of the
RevShareKeeperinterface are updated to handle the newaffiliateWhitelistMapparameter.- All callers of the
GetAllRevSharesmethod are updated to provide theaffiliateWhitelistMapargument.Run the following script to verify the updates:
Verification successful
Verification successful:
GetAllRevSharesupdates are completeThe implementation of
GetAllRevSharesand its callers have been correctly updated to include the newaffiliateWhitelistMapparameter. This change has been consistently applied across the codebase, ensuring compatibility with the modified interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify implementations and callers of `GetAllRevShares` have been updated. # Test 1: Search for implementations of `RevShareKeeper` interface. Expect: Updated method signature. rg --type go -A 5 $'func \(.*RevShareKeeper\) GetAllRevShares\(' # Test 2: Search for callers of `GetAllRevShares` method. Expect: Updated method invocation with `affiliateWhitelistMap` argument. rg --type go -A 5 $'GetAllRevShares\('Length of output: 2852
protocol/x/affiliates/keeper/grpc_query_test.go (4)
32-32: LGTM!Setting
IsWhitelistedtofalseis correct since the affiliate is not added to the whitelist in this test case.
62-62: LGTM!Setting
IsWhitelistedtofalseis correct since no affiliate relationship is registered in this test case.
94-131: LGTM!The "Whitelisted" test case correctly sets up a whitelisted affiliate and verifies the expected response:
- Registers an affiliate relationship between Alice and Bob.
- Sets a delegation for Alice.
- Adds Alice to the affiliate whitelist with a custom
FeeSharePpmof 120,000 (12%).- Expects the response to include the custom whitelist values and
IsWhitelistedset totrue.The test case looks good and thoroughly verifies the behavior for a whitelisted affiliate.
228-249: LGTM!The new
TestAffiliateWhitelistfunction correctly verifies setting and retrieving the affiliate whitelist:
- Sets up an affiliate whitelist with Alice's address and a custom
TakerFeeSharePpmof 1,000,000.- Calls the
AffiliateWhitelistmethod and verifies the response matches the set whitelist.The test function looks good and thoroughly verifies the behavior of setting and retrieving the affiliate whitelist.
protocol/x/affiliates/keeper/keeper_test.go (4)
397-465: LGTM!The
TestSetAffiliateWhitelistfunction comprehensively tests theSetAffiliateWhitelistmethod with various scenarios, ensuring its correctness and handling of edge cases.
467-541: LGTM!The
TestGetAffiliateWhiteListMapfunction thoroughly tests theGetAffiliateWhitelistMapmethod with different whitelist configurations, ensuring that it returns the expected map representation of the affiliate whitelist.
543-618: LGTM!The
TestGetTakerFeeShareViaWhitelistfunction effectively tests theGetTakerFeeSharemethod when using an affiliate whitelist. It covers important scenarios and ensures that the method correctly determines the taker fee share based on the whitelist and falls back to the default tiers when necessary.
Line range hint
619-719: LGTM!The
TestAggregateAffiliateReferredVolumeForFillsfunction thoroughly tests theAggregateAffiliateReferredVolumeForFillsmethod with different referral scenarios. It ensures that the method correctly aggregates the referred volume for affiliates based on the fill data.protocol/x/subaccounts/keeper/transfer_test.go (1)
1645-1647: Verify theGetAllRevSharesmethod has been updated to handle the newaffiliateWhitelistMapparameter correctly.The change looks good. It retrieves the
affiliateWhitelistMapfrom theaffiliatesKeeperand passes it as an additional parameter to theGetAllRevSharesmethod.Ensure that the
GetAllRevSharesmethod has been updated to:
- Accept the new
affiliateWhitelistMapparameter.- Use the
affiliateWhitelistMapto correctly determine the revenue shares based on affiliate relationships.You can use the following script to verify the
GetAllRevSharesmethod:Verification successful
GetAllRevShares method has been correctly updated to handle the new affiliateWhitelistMap parameter
The verification process has confirmed that the
GetAllRevSharesmethod inprotocol/x/revshare/keeper/revshare.gohas been successfully updated to incorporate theaffiliateWhitelistMapparameter. Key observations:
- The method signature now includes
affiliatesWhitelistMap map[string]uint32as expected.- The new parameter is utilized in the
getAffiliateRevSharesfunction, indicating that affiliate revenue shares are calculated based on this whitelist.- The affiliate rev shares are properly integrated into the overall revenue share calculation for the fill.
These changes align with the intended purpose of using the affiliate whitelist to determine revenue shares. The implementation appears to be correct and consistent with the expected behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `GetAllRevShares` method has been updated to handle the new `affiliateWhitelistMap` parameter. # Test 1: Check if the method signature has been updated to accept the new parameter. # Expect: The method should accept an `affiliateWhitelistMap map[string]bool` parameter. ast-grep --lang go --pattern $'func GetAllRevShares(ctx sdk.Context, fill Fill, affiliateWhitelistMap map[string]bool) (RevShares, error)' # Test 2: Check if the method uses the `affiliateWhitelistMap` to determine revenue shares. # Expect: The method should reference the `affiliateWhitelistMap` when calculating shares. rg --type go -A 5 $'affiliateWhitelistMap'Length of output: 4633
Script:
#!/bin/bash # Description: Locate the implementation of GetAllRevShares and its usage of affiliateWhitelistMap # Search for GetAllRevShares implementation echo "Searching for GetAllRevShares implementation:" ast-grep --lang go --pattern 'func ($_) GetAllRevShares($_) { $$$ }' # Look for usages of affiliateWhitelistMap within the revshare module echo "Searching for affiliateWhitelistMap usage in revshare module:" rg --type go -g 'protocol/x/revshare/**' 'affiliateWhitelistMap'Length of output: 627
Script:
#!/bin/bash # Description: Further investigation to locate GetAllRevShares implementation # Search for interface defining GetAllRevShares echo "Searching for interface with GetAllRevShares:" ast-grep --lang go --pattern 'type $_ interface { $$$ GetAllRevShares($_) $$$ }' # Look for potential RevShareKeeper implementations echo "Searching for potential RevShareKeeper implementations:" rg --type go 'type Keeper struct' # Broader search for GetAllRevShares echo "Broader search for GetAllRevShares:" rg --type go 'func.*GetAllRevShares'Length of output: 797
Script:
#!/bin/bash # Description: Inspect the implementation of GetAllRevShares in revshare.go echo "Content of GetAllRevShares function in revshare.go:" sed -n '/func (k Keeper) GetAllRevShares/,/^}/p' protocol/x/revshare/keeper/revshare.goLength of output: 2681
protocol/x/affiliates/types/keys.go
Outdated
| @@ -16,4 +16,6 @@ const ( | |||
| ReferredVolumeKeyPrefix = "ReferredVolume:" | |||
|
|
|||
| AffiliateTiersKey = "AffiliateTiers" | |||
|
|
|||
| AffiliateWhitelistKey = "AffiliateWhitelist" | |||
There was a problem hiding this comment.
Let's actually optimize the keys and prefixes a bit (for faster insert/read from IAVL tree)
ReferredByKeyPrefix = "RB:"
ReferredVolumeKeyPrefix = "RV:"
AffiliateTiersKey = "AT"
AffiliateWhitelistKey = "AW"
| @@ -307,6 +309,7 @@ func (k Keeper) persistMatchedOrders( | |||
| makerFeePpm int32, | |||
| bigFillQuoteQuantums *big.Int, | |||
| insuranceFundDelta *big.Int, | |||
| affiliatesWhitelistMap map[string]uint32, // nolint: unused | |||
There was a problem hiding this comment.
Why the nolint? Isn't affiliatesWhitelistMapused below?
In general nolint in non-testing files is red flag
There was a problem hiding this comment.
this was a remnant from before the clob pr was merged. Should be removed
| @@ -144,8 +144,15 @@ func (k Keeper) ProcessInternalOperations( | |||
|
|
|||
| switch castedOperation := operation.Operation.(type) { | |||
| case *types.InternalOperation_Match: | |||
| affiliatesWhitelistMap, err := k.affiliatesKeeper.GetAffiliateWhitelistMap(ctx) | |||
There was a problem hiding this comment.
Declare affiliatesWhitelistMap before the for loop, and query it at most once for all matches in the operations queue.
| func (k Keeper) SetAffiliateWhitelist(ctx sdk.Context, whitelist types.AffiliateWhitelist) error { | ||
| store := ctx.KVStore(k.storeKey) | ||
| addressSet := make(map[string]bool) | ||
| for _, tier := range whitelist.Tiers { |
There was a problem hiding this comment.
New change: let's also add a protocol constant = 50% as upper bound of the fee share ppm, for fee security
There was a problem hiding this comment.
I think it would be better to add this to ValidateRevShareSafety
Now that I think about it, Im gonna make ValidateRevShareSafety more accurate - can discuss offline
There was a problem hiding this comment.
To clarify, we explicitly want a constant Cap = 50% for the affiliate whitelist fee share (doesn't hurt to apply this to regular affiliate tier as well) - don't see being added?
protocol/x/clob/memclob/memclob.go
Outdated
| success, takerUpdateResult, makerUpdateResult, _, err := m.clobKeeper.ProcessSingleMatch(ctx, &matchWithOrders) | ||
| affiliatesWhitelistMap := make(map[string]uint32) | ||
| success, takerUpdateResult, makerUpdateResult, _, err := m.clobKeeper.ProcessSingleMatch( | ||
| ctx, &matchWithOrders, affiliatesWhitelistMap) |
There was a problem hiding this comment.
Explicitly pass in an empty map, and document why the input is empty.
| } | ||
| } | ||
|
|
||
| func TestGetTakerFeeShareViaWhitelist(t *testing.T) { |
There was a problem hiding this comment.
Do we have a unit test where the affiliate qualifies through regular affiliate program and gets overridden to the higher whitelist fee share?
There was a problem hiding this comment.
Yes, the first unit test here registers an affiliate(which should have revshare of 15%) and we bump it to 40%
| @@ -136,6 +136,13 @@ func (k Keeper) ProcessInternalOperations( | |||
| // All short term orders in this map have passed validation. | |||
| placedShortTermOrders := make(map[types.OrderId]types.Order, 0) | |||
|
|
|||
| affiliatesWhitelistMap, err := k.affiliatesKeeper.GetAffiliateWhitelistMap(ctx) | |||
There was a problem hiding this comment.
Can you do it in a way that make this state read at most once when there are matches, but zero if there's no match: declare affiliatesWhitelistMap outside of the scope of for loop, but assign to it only if it's not been initialized, under case *types.InternalOperation_Match:
This may look a bit clumsy, so could you add comment where applicable?
protocol/x/clob/memclob/memclob.go
Outdated
| success, takerUpdateResult, makerUpdateResult, _, err := m.clobKeeper.ProcessSingleMatch( | ||
| ctx, &matchWithOrders, affiliatesWhitelistMap) | ||
| ctx, &matchWithOrders, make(map[string]uint32)) |
There was a problem hiding this comment.
Nit: pass in literal for readability
| ctx, &matchWithOrders, make(map[string]uint32)) | |
| ctx, &matchWithOrders, map[string]uint32{}) |
| // check if affiliate whitelist map is nil and initialize it if it is. | ||
| // This is done to avoid getting whitelist map on list of operations | ||
| // where there are no matches. | ||
| if affiliatesWhitelistMap == nil { |
There was a problem hiding this comment.
If the AffiliateWhitelist is empty in state, then this will be nil everytime right? Do we need a separate boolean to track whether the state read was done?
There was a problem hiding this comment.
golang dfiferentiates between a nil map and an empty map
There was a problem hiding this comment.
package main
import "fmt"
func main() {
x := map[string]uint32{}
if x == nil {
fmt.Println("x is nil")
}
var y map[string]uint32
if y == nil {
fmt.Println("y is nil")
}
}
There was a problem hiding this comment.
try this piece of code(confirmed it myself)
protocol/x/clob/memclob/memclob.go
Outdated
| @@ -1698,8 +1698,12 @@ func (m *MemClobPriceTimePriority) mustPerformTakerOrderMatching( | |||
|
|
|||
| // affiliate revshare doesnt affect subaccount balance, we can pass an empty map | |||
There was a problem hiding this comment.
Let's use below comment:
Pass in empty map to avoid reading `AffiliateWhitelist` from state in every `CheckTx`. This deviates from `DeliverTx` which accounts for affiliate whitelist correctly.
This deviation is ok becasue rev shares/fees are distributed to the recipient’s bank balance and not settled at the subaccount level, and won’t affect the collateralization of future operations in the operations queue.
Changelist
Implements whitelisting functionality for affiliates
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
Bug Fixes
Tests
Documentation