[OTE-762] Affiliates comlink affiliate info #2269
Conversation
WalkthroughThe changes involve significant updates across various files in the Postgres package, focusing on enhancing constants related to affiliate information, introducing new functionalities for filtering and retrieving affiliate data, and improving the testing suite for these components. New methods are added to models and controllers, while existing structures are modified to accommodate these enhancements. Additionally, API documentation is updated to reflect new query parameters and changes in sorting logic. Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (4)
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: 0
Outside diff range and nitpick comments (1)
indexer/packages/postgres/src/db/migrations/migration_files/20240913142157_change_affiliate_info_decimal_precision_and_add_total_referred_volume.ts (1)
3-12: LGTM!The migration logic is correct and follows the Knex.js API. The changes allow for more flexibility in storing decimal values and introduce a new non-nullable field to track the total referred volume.
Ensure that the application code is updated to provide a value for the
referredTotalVolumecolumn when inserting or updating rows in theaffiliate_infotable. Consider adding validation checks to enforce this constraint.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts (4 hunks)
- indexer/packages/postgres/tests/stores/subaccount-usernames-table.test.ts (2 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240913142157_change_affiliate_info_decimal_precision_and_add_total_referred_volume.ts (1 hunks)
- indexer/packages/postgres/src/index.ts (2 hunks)
- indexer/packages/postgres/src/models/affiliate-info-model.ts (4 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (2 hunks)
- indexer/packages/postgres/src/stores/subaccount-usernames-table.ts (3 hunks)
- indexer/packages/postgres/src/types/affiliate-info-types.ts (2 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (2 hunks)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (5 hunks)
- indexer/services/comlink/public/api-documentation.md (1 hunks)
- indexer/services/comlink/public/swagger.json (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (3 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Additional comments not posted (38)
indexer/packages/postgres/src/types/affiliate-info-types.ts (2)
10-10: LGTM! Verify the usage of the newreferredTotalVolumeproperty.The addition of the
referredTotalVolumeproperty to theAffiliateInfoCreateObjectinterface is a good enhancement for tracking affiliate performance metrics. The typestringis consistent with similar properties.Please ensure that all code paths that create or use
AffiliateInfoCreateObjectare updated to populate and handle the newreferredTotalVolumeproperty correctly. You can use the following script to search for relevant occurrences:Verification successful
LGTM! The
referredTotalVolumeproperty is correctly implemented and used.The new
referredTotalVolumeproperty has been successfully added to theAffiliateInfoCreateObjectinterface and is being used consistently throughout the codebase. Here's a summary of the changes:
- The property is correctly defined in the interface in
indexer/packages/postgres/src/types/affiliate-info-types.ts.- It's included in the
AffiliateInfoModelinindexer/packages/postgres/src/models/affiliate-info-model.ts.- The property is used in API responses in
affiliates-controller.ts.- Test constants have been updated to include the new property.
- A database migration file has been created to add the
referredTotalVolumecolumn to the database schema.These changes ensure that the new property is properly integrated into the existing codebase and database structure.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `referredTotalVolume` property. # Test: Search for the interface usage. Expect: Occurrences where the new property needs to be populated. rg --type typescript -A 5 $'AffiliateInfoCreateObject'Length of output: 88
Script:
#!/bin/bash # Description: Verify the usage of the new `referredTotalVolume` property in TypeScript files. # Find TypeScript files and search for AffiliateInfoCreateObject echo "Searching for AffiliateInfoCreateObject in TypeScript files:" fd -e ts -e tsx | xargs rg -n -A 5 'AffiliateInfoCreateObject' # Search for the interface definition echo -e "\nSearching for the AffiliateInfoCreateObject interface definition:" fd -e ts -e tsx | xargs rg -n -A 10 'interface AffiliateInfoCreateObject' # Search for potential usage of referredTotalVolume echo -e "\nSearching for potential usage of referredTotalVolume:" fd -e ts -e tsx | xargs rg -n -A 5 'referredTotalVolume'Length of output: 15938
22-22: LGTM! Verify the usage of the newreferredTotalVolumeenum value.The addition of the
referredTotalVolumevalue to theAffiliateInfoColumnsenum is necessary to ensure consistency with the new property added to theAffiliateInfoCreateObjectinterface.Please ensure that all code paths that use
AffiliateInfoColumnsare updated to handle the newreferredTotalVolumevalue correctly. You can use the following script to search for relevant occurrences:indexer/packages/postgres/src/db/migrations/migration_files/20240913142157_change_affiliate_info_decimal_precision_and_add_total_referred_volume.ts (1)
14-22: LGTM!The migration rollback logic is correct and follows the Knex.js API. It correctly reverts the changes made by the
upfunction.Verify that the
downfunction is only used in a controlled environment, such as local development or staging, to avoid potential data loss in production. If the migration needs to be rolled back in production, ensure that the necessary precautions are taken to backup and restore the data in thereferredTotalVolumecolumn.indexer/packages/postgres/src/models/affiliate-info-model.ts (4)
26-26: LGTM!The addition of the
referredTotalVolumeproperty to therequiredarray in thejsonSchemais correct and aligns with the PR objective of enhancing affiliate information functionality.
37-37: LGTM!The addition of the
referredTotalVolumeproperty to thepropertiesobject in thejsonSchemais correct. The typestringand theNonNegativeNumericPatternvalidation ensure that the property value is a non-negative numeric string, which is appropriate for the referred total volume metric.
58-58: LGTM!The addition of the
referredTotalVolumeproperty to thesqlToJsonConversionsobject with thestringconversion type is correct. This mapping ensures that the property value is correctly converted from the SQL representation to the JSON representation when retrieving data from the database.
80-80: LGTM!The addition of the
referredTotalVolumeclass member variable of typestringis correct and corresponds with the new property introduced in thejsonSchemaandsqlToJsonConversions. The typestringmatches the type specified in thejsonSchemaandsqlToJsonConversions.indexer/packages/postgres/src/stores/affiliate-info-table.ts (1)
97-124: LGTM!The new
paginatedFindWithAddressFilterfunction is a great addition that enhances the flexibility and functionality of the affiliate info table store. It allows for retrieving a paginated list of affiliate information filtered by a specified array of addresses, with options for sorting by affiliate earnings.The implementation looks solid and follows the existing patterns in the file. The function:
- Sets up a base query using the
setupBaseQueryfunction.- Applies the address filter if provided, using a
WHERE INclause.- Sorts the results by affiliate earnings if specified, or defaults to sorting by address.
- Applies pagination using the provided
offsetandlimitvalues.- Executes the query and returns all fields.
This functionality can be particularly useful for applications that require filtering and sorting of affiliate information based on specific criteria.
Great job on this addition!
indexer/packages/postgres/src/stores/subaccount-usernames-table.ts (1)
119-144: LGTM!The
findByAddressfunction is well-implemented and follows good practices:
- It handles the case of an empty input array correctly.
- The SQL query is constructed using a CTE for readability and performance.
- The query filters for subaccounts with a
subaccountNumberof 0, which seems to be a specific requirement.- The left join ensures that all addresses are returned, even if they don't have an associated username.
- It uses the
knexReadReplicaconnection, which is appropriate for read-only queries.- It returns the result rows directly, which is fine since the query already selects the required columns.
The function adds a new capability to retrieve usernames based on addresses, which seems to be a required feature. It is asynchronous, which is appropriate for database queries, and uses the read replica connection for load balancing.
indexer/packages/postgres/__tests__/stores/subaccount-usernames-table.test.ts (1)
88-107: LGTM!The new test case
'Get username using address'is a valuable addition to the test suite. It verifies that thefindByAddressmethod ofSubaccountUsernamesTablecorrectly retrieves the username associated with a wallet's subaccount, even when multiple usernames exist for different wallets.The test case sets up the necessary data by creating entries in
SubaccountUsernamesTable,WalletTable, andSubaccountsTable. It then asserts that only the expected username is returned when queried by the wallet's address.This test case enhances the coverage of the
SubaccountUsernamesTablefunctionality and ensures that the retrieval logic behaves as intended.indexer/packages/postgres/src/index.ts (2)
23-23: LGTM!The new export for
AffiliateInfoModelis correctly added and expands the module's capabilities for affiliate-related operations without altering existing functionality.
52-52: LGTM!The new export for
AffiliateInfoTableis correctly added and expands the module's capabilities for affiliate-related operations without altering existing functionality.indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (5)
3-3: LGTM!The import statement is syntactically correct and the imported constant is likely used to provide additional test data.
35-37: LGTM!The test case correctly uses
defaultAffiliateInfo2to test the upsert functionality of theAffiliateInfoTable.
43-43: LGTM!The test case correctly uses
defaultAffiliateInfo2to create additional test data for testing thefindAllfunctionality.
55-55: LGTM!The test case correctly uses
defaultAffiliateInfo2in the expectation to verify the data retrieved by thefindAllfunctionality.
68-163: Excellent test coverage!The new describe block
paginatedFindWithAddressFiltercontains well-structured test cases that cover various scenarios for thepaginatedFindWithAddressFilterfunctionality of theAffiliateInfoTable. The test cases are using the correct parameters, expectations, and types to verify the functionality. The beforeEach block is a good practice for setting up the test data.indexer/packages/postgres/src/types/db-model-types.ts (2)
267-270: LGTM!The
AddressUsernameFromDatabaseinterface provides a clear and structured way to represent the relationship between an address and a username stored in the database. The property names are descriptive and the types are appropriate.Exporting the interface allows it to be used in other parts of the codebase, promoting type safety and consistency when working with address-username mappings.
294-294: LGTM!The addition of the
referredTotalVolumeproperty to theAffiliateInfoFromDatabaseinterface enhances the data structure's ability to capture more comprehensive affiliate-related metrics. The property name is descriptive and the type is consistent with other volume-related properties in the interface.This modification allows for tracking and analyzing the total volume referred by affiliates, providing valuable insights into affiliate performance and contributions. The change is also backward compatible, minimizing the risk of breaking changes in the codebase.
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2)
122-183: LGTM! The changes to thegetSnapshotfunction enhance the functionality and usability of the API.The implementation correctly handles the new
addressFilterquery parameter, allowing users to retrieve affiliate snapshots based on specific addresses. The use ofAffiliateInfoTable.paginatedFindWithAddressFilterensures efficient retrieval of affiliate information based on the provided filters.The function appropriately handles edge cases, such as when no results are found, by returning an empty affiliate list with a total count of 0. The mapping of affiliate information to the response format is done correctly, including the retrieval of referral codes for each affiliate address.
Logging a warning when an affiliate address does not have a corresponding referral code helps in identifying potential data inconsistencies.
The renamed query parameter
sortByAffiliateEarningimproves the clarity of the sorting option, making it more intuitive for API users.The updated response structure provides more accurate information about the total count of affiliates returned, enhancing the usability of the API.
Overall, the changes introduce valuable enhancements to the
getSnapshotfunction, improving its functionality, flexibility, and usability.
284-326: The updates to the validation schema for thegetSnapshotfunction enhance the robustness, reliability, and usability of the API.The inclusion of validation checks for the new
addressFilterquery parameter ensures that the API receives valid input and prevents potential errors due to invalid data. The custom sanitizer correctly splits the comma-separated string into an array, allowing users to provide multiple addresses as a single string, which improves the convenience and usability of the API.The custom validator for
addressFilterensures that the provided value is a non-empty array of strings, preventing invalid input from being processed. This enhances the robustness and reliability of the API by reducing the chances of unexpected errors.The renaming of the
sortByReferredFeesparameter tosortByAffiliateEarningimproves the clarity and consistency of the API, aligning it with the actual sorting logic. This makes it easier for users to understand and use the sorting functionality, enhancing the overall usability of the API.Overall, the changes to the validation schema introduce valuable improvements to the
getSnapshotfunction, ensuring that the API receives valid input, prevents potential errors, and provides a more intuitive and user-friendly experience.indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (5)
360-375: LGTM!The
affiliateInfoCreateToResponseObjectfunction correctly transforms anAffiliateInfoCreateObjectinstance into anAffiliateSnapshotResponseObject. The property mappings and type conversions are accurate.
Line range hint
33-149: LGTM!The
GET /metadatasub-suite comprehensively tests the/metadataendpoint. The test cases cover all the important scenarios and make the appropriate assertions.
Line range hint
151-178: LGTM!The
GET /addresssub-suite tests the/addressendpoint for the success and failure scenarios. The test cases make the appropriate assertions.
180-318: LGTM!The
GET /snapshotsub-suite comprehensively tests the/snapshotendpoint. The test cases cover all the important scenarios, including filtering, pagination, and sorting. The test cases make the appropriate assertions and use theaffiliateInfoCreateToResponseObjectfunction to generate the expected response objects.
Line range hint
320-357: LGTM!The
GET /total_volumesub-suite tests the/total_volumeendpoint for the success and failure scenarios. The test cases make the appropriate assertions.indexer/services/comlink/src/types.ts (2)
687-690: LGTM!The changes to the
AffiliateSnapshotRequestinterface look good:
- Adding the optional
addressFilterproperty allows for more flexible querying of affiliate snapshots by specific addresses.- Renaming
sortByReferredFeestosortByAffiliateEarningimproves clarity around the sorting criteria.The updates enhance the interface's functionality and readability.
Line range hint
705-714: Looks good!The
AffiliateSnapshotResponseObjectinterface is well-structured and includes all the relevant properties to represent an affiliate's performance snapshot. The property names are descriptive, and the data types are appropriate.indexer/packages/postgres/__tests__/helpers/constants.ts (6)
78-78: LGTM!The constant declaration is straightforward and the value is a simple string literal.
101-107: LGTM!The constant declaration is correct and the object is created using the
defaultWalletAddressconstant and other literal values.
149-152: LGTM!The constant declaration is correct and the
SubaccountTable.uuidfunction is called with the appropriate arguments to generate a unique identifier.
920-924: LGTM!The constant declaration is correct and the object is created using the
defaultSubaccountIdDefaultWalletAddressconstant and a literal value for theusernameproperty.
926-930: LGTM!The constant declaration is correct and the object is created using the
defaultSubaccountIdWithAlternateAddressconstant and a literal value for theusernameproperty.
988-1019: Verify the impact of the numeric format changes.The changes to the existing
defaultAffiliateInfoanddefaultAffiliateInfo2constants, as well as the addition of the newdefaultAffiliateInfo3constant, look good.However, please ensure that the code that consumes these constants is updated to handle the change in the format of numeric values from decimal strings to whole number strings, if necessary.
Run the following script to verify the usage of these constants:
Verification successful
Changes verified: No negative impact on codebase
The modifications to the
defaultAffiliateInfoconstants and the addition ofdefaultAffiliateInfo3are safe and do not negatively impact the codebase. The changes are only used in a single test file: 'indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts'.To ensure everything is up-to-date:
- Review the 'affiliate-info-table.test.ts' file to confirm it's using the new whole number string format correctly.
- Verify that the test cases still accurately represent the expected behavior with the updated numeric format.
No instances of
parseFloat()ortoFixed()were found using these constants, indicating that the change from decimal strings to whole number strings doesn't cause immediate issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the affiliate info constants and check for potential issues with the numeric format changes. # Test 1: Search for the usage of the constants. Expect: Only occurrences with the new numeric formats. rg --type typescript -A 5 $'defaultAffiliateInfo[123]?\.' # Test 2: Search for potential issues with the numeric format changes. Expect: No occurrences of parsing the constant values as floats or decimals. rg --type typescript -A 5 $'parseFloat\(.*defaultAffiliateInfo[123]?\..*\)|toFixed\(.*defaultAffiliateInfo[123]?\..*\)'Length of output: 249
Script:
#!/bin/bash # Description: Verify the usage of the affiliate info constants and check for potential issues with the numeric format changes. # Test 1: Search for the usage of the constants without specifying file type echo "Test 1: Usage of affiliate info constants" rg 'defaultAffiliateInfo[123]?\.' -A 5 # Test 2: Search for potential issues with the numeric format changes in TypeScript files echo "Test 2: Potential issues with numeric format changes" fd -e ts -e tsx | xargs rg 'parseFloat\(.*defaultAffiliateInfo[123]?\..*\)|toFixed\(.*defaultAffiliateInfo[123]?\..*\)' -A 5 # Test 3: List all TypeScript files that use the affiliate info constants echo "Test 3: TypeScript files using affiliate info constants" fd -e ts -e tsx | xargs rg 'defaultAffiliateInfo[123]?\.' --files-with-matchesLength of output: 2420
indexer/services/comlink/public/swagger.json (2)
1819-1829: LGTM!The addition of the
addressFilterquery parameter is a valuable enhancement to the API. It allows clients to filter results based on multiple addresses, providing more flexibility in querying. The parameter is correctly defined as an optional array of strings.
1850-1850: Renaming ofsortByReferredFeestosortByAffiliateEarninglooks good, but requires coordination.The renaming aligns the parameter name more closely with the business domain. However, this is a breaking change for clients currently using
sortByReferredFees.Please ensure that:
- The corresponding server-side implementation has been updated to handle the new
sortByAffiliateEarningparameter name.- This breaking change is clearly communicated to the API users, giving them time to update their client code.
Run the following script to verify the handling of the new parameter name in the implementation:
Verification successful
Parameter renaming from 'sortByReferredFees' to 'sortByAffiliateEarning' has been successfully implemented
The renaming of the query parameter has been thoroughly and consistently implemented across the codebase. Key observations:
- The new parameter 'sortByAffiliateEarning' is present in type definitions, controller logic, database queries, and tests.
- No occurrences of the old parameter 'sortByReferredFees' were found.
- The API documentation (swagger.json) has been updated to reflect the new parameter name.
This comprehensive implementation suggests that the change has been handled correctly throughout the system. However, as this is a breaking change, please ensure that:
- This change is clearly communicated to all API users, giving them sufficient time to update their client code.
- Any external documentation or client libraries are updated to reflect this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of `sortByAffiliateEarning` parameter. # Test: Search for the new parameter name usage. Expect: At least one occurrence in a relevant code file. rg --type python --type go --type js --type ts -w $'sortByAffiliateEarning' -A 5Length of output: 110
Script:
#!/bin/bash # Description: Verify handling of `sortByAffiliateEarning` parameter and check for remnants of `sortByReferredFees`. # Search for both old and new parameter names echo "Searching for sortByAffiliateEarning:" rg --type go --type js --type ts -w 'sortByAffiliateEarning' -A 5 echo -e "\nSearching for sortByReferredFees:" rg --type go --type js --type ts -w 'sortByReferredFees' -A 5 # Search for partial matches of both parameter names echo -e "\nSearching for partial matches:" rg --type go --type js --type ts 'sort[Bb]y(Affiliate|Referred)' # Investigate the swagger.json file for context echo -e "\nInvestigating swagger.json:" rg 'sortByAffiliateEarning' indexer/services/comlink/public/swagger.json -C 5Length of output: 9043
indexer/services/comlink/public/api-documentation.md (2)
636-638: LGTM!The addition of the optional
addressFilterquery parameter to filter affiliate snapshot results by specific addresses looks good.
639-639: Verify the impact of the query parameter renaming.The
sortByReferredFeesquery parameter has been renamed tosortByAffiliateEarning. While the change itself looks fine, please ensure that:
- Existing API clients using the old parameter name are updated to use the new name.
- The backend implementation of the sorting logic is updated to match the new parameter name and behavior.
Run the following script to verify the usage and implementation of the renamed parameter:
Verification successful
Parameter renaming successfully implemented
The renaming of
sortByReferredFeestosortByAffiliateEarninghas been properly implemented across the codebase. Here's a summary of the changes:
- The old parameter name
sortByReferredFeeshas been completely removed.- The new parameter name
sortByAffiliateEarningis consistently used in:
- Backend implementation (affiliate-info-table.ts)
- API controller (affiliates-controller.ts)
- Type definitions (types.ts)
- Tests (affiliates-controller.test.ts)
- API documentation (api-documentation.md)
The change appears to be thorough and consistent. However, as a final step, I recommend manually verifying the API endpoints to ensure everything is working as expected in the production environment.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and implementation of the renamed `sortByAffiliateEarning` parameter. # Test 1: Search for usage of the old `sortByReferredFees` parameter in the codebase. # Expect: No occurrences of the old parameter name. rg --type typescript --type javascript --type python $'sortByReferredFees' # Test 2: Search for usage of the new `sortByAffiliateEarning` parameter in the codebase. # Expect: Occurrences in the backend implementation and any relevant API client code. rg --type typescript --type javascript --type python $'sortByAffiliateEarning' # Test 3: Verify the backend implementation of the sorting logic for the `sortByAffiliateEarning` parameter. # Expect: The sorting logic should match the new parameter name and behavior. ast-grep --lang typescript --pattern $' $$$ .sort(($_, $_) => { $$$ affiliateEarning $$$ }) $$$ 'Length of output: 840
Script:
#!/bin/bash # Description: Verify the usage of the renamed parameter and related sorting logic # Search for the old parameter name echo "Searching for old parameter name 'sortByReferredFees':" grep -r "sortByReferredFees" --include=\*.{js,ts,py,md} . # Search for the new parameter name echo -e "\nSearching for new parameter name 'sortByAffiliateEarning':" grep -r "sortByAffiliateEarning" --include=\*.{js,ts,py,md} . # Search for sorting logic related to affiliate earnings echo -e "\nSearching for sorting logic related to affiliate earnings:" grep -r -A 5 "sort.*affiliate" --include=\*.{js,ts} . # Search for API-related files that might contain the parameter names echo -e "\nSearching for API-related files with parameter names:" grep -r -i "api.*sort.*\(referredfees\|affiliateearning\)" --include=\*.{js,ts,py,md} .Length of output: 3245
| table.decimal('affiliateEarnings', null).alter(); | ||
| table.decimal('totalReferredFees', null).alter(); | ||
| table.decimal('referredNetProtocolEarnings', null).alter(); |
There was a problem hiding this comment.
what does variable precision mean? Thinking about this more, we probably want scale of 6 for all fees, thats the smallest unit of USDC
There was a problem hiding this comment.
For variable precision it will not append any 0s after the decimal. Eg. 2 stays 2 and 2.123 stays 2.123. Variable precision allows for up to the maximum postgres limit.
If you set the precision to 6. It will pad 0s so 2 -> 2.000000.
We can do it either way, the main change will be that our unit tests use the string conversion so we have to change those values. I prefer to stick with variable, as that is convention other columns in our tables use.
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
Outdated
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
Outdated
Show resolved
Hide resolved
| if (addressUsernames.length !== infos.length) { | ||
| logger.warning({ | ||
| at: 'affiliates-controller#snapshot', | ||
| message: `Could not find referral code for following addresses: ${infos.map((info) => info.address).filter((address) => !(address in addressUsernameMap)).join(', ')}`, |
There was a problem hiding this comment.
nit: save formula into a separate variable
There was a problem hiding this comment.
_.chain(infos)
.map(AffiliateInfoColumns.address)
.filter((address) => !(address in addressUsernameMap))
.join(', ')
.values()
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
122-188: LGTM!The
getSnapshotmethod introduces useful enhancements to the affiliate snapshots API, including:
- Filtering affiliate snapshots based on specific addresses
- Sorting affiliate snapshots based on affiliate earnings
- Graceful handling of potential data inconsistencies
The method is well-structured and follows good practices such as:
- Providing default values for optional query parameters
- Returning an empty result set when no results are found
- Logging warnings for potential data inconsistencies instead of throwing errors
Consider extracting the logic for mapping affiliate information to the response format into a separate function to improve readability and maintainability. For example:
function mapAffiliateInfoToSnapshot(info: AffiliateInfoFromDatabase, referralCode: string): AffiliateSnapshotResponseObject { return { affiliateAddress: info.address, affiliateReferralCode: referralCode, affiliateEarnings: Number(info.affiliateEarnings), affiliateReferredTrades: Number(info.referredMakerTrades) + Number(info.referredTakerTrades), affiliateTotalReferredFees: Number(info.totalReferredFees), affiliateReferredUsers: Number(info.totalReferredUsers), affiliateReferredNetProtocolEarnings: Number(info.referredNetProtocolEarnings), affiliateReferredTotalVolume: Number(info.referredTotalVolume), }; } // Usage in getSnapshot method const affiliateSnapshots: AffiliateSnapshotResponseObject[] = infos.map((info) => mapAffiliateInfoToSnapshot(info, addressUsernameMap[info.address] || '') );indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
203-207: Consider parallelizing the affiliate info creation.To speed up the test setup, you can parallelize the creation of affiliate info records using
Promise.all:await Promise.all([ AffiliateInfoTable.create(defaultInfo), AffiliateInfoTable.create(defaultInfo2), AffiliateInfoTable.create(defaultInfo3), ]);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts (4 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (2 hunks)
- indexer/packages/postgres/src/stores/subaccount-usernames-table.ts (3 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (2 hunks)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (5 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts
- indexer/packages/postgres/src/stores/subaccount-usernames-table.ts
- indexer/packages/postgres/src/types/db-model-types.ts
Additional comments not posted (13)
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (6)
3-3: LGTM!The import statement for
defaultAffiliateInfo2looks good.
35-37: LGTM!The updated test case using
defaultAffiliateInfo2looks good.
Line range hint
43-55: LGTM!The updated test case including
defaultAffiliateInfo2in the test data looks good.
69-177: LGTM!The new test cases for
paginatedFindWithAddressFilterfunctionality look good. They cover various scenarios and check for the expected data to be returned correctly.
83-98: LGTM!The new test case for successfully filtering by address looks good. It checks that the
paginatedFindWithAddressFiltermethod correctly filters the results and asserts that the expected data is returned.
100-120: LGTM!The new test case for successfully sorting by affiliate earning looks good. It checks that the
paginatedFindWithAddressFiltermethod correctly sorts the results and asserts that the expected data is returned in the correct order.indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (7)
Line range hint
23-31: LGTM!The test suite setup and teardown logic looks good. The
beforeAllandafterAllhooks correctly migrate and tear down the test database.
Line range hint
33-149: LGTM!The test cases for the
/metadataendpoint look comprehensive and well-structured. The setup and teardown logic is correctly implemented, and the test cases cover various scenarios, including edge cases and error handling.
Line range hint
151-178: LGTM!The test cases for the
/addressendpoint look good. They cover the happy path and the error scenario when the referral code is not found. The setup and teardown logic is correctly implemented.
180-320: LGTM!The test cases for the
/snapshotendpoint are comprehensive and well-structured. They cover various scenarios, including filtering, pagination, and edge cases. The setup and teardown logic is correctly implemented, and the test expectations are clearly defined.
362-377: LGTM!The
affiliateInfoCreateToResponseObjectfunction is a useful utility for convertingAffiliateInfoCreateObjectinstances to the expected response format. It improves code readability and maintainability by centralizing the conversion logic.
208-208: Skip the past review comment.The code already utilizes
Promise.allto parallelize the affiliate info creation, as suggested in the past review comment. No further action is needed.
Line range hint
322-359: LGTM!The test cases for the
/total_volumeendpoint look good. They cover the happy path and the error scenario when the address does not exist. The setup and teardown logic is correctly implemented.
jerryfan01234
force-pushed
the
affiliates-comlink-affiliate-info
branch
from
7ff6e5d to
6ca6830
Compare
| limit: number, | ||
| sortByAffiliateEarning: boolean, | ||
| options: Options = DEFAULT_POSTGRES_OPTIONS, | ||
| ): Promise<AffiliateInfoFromDatabase[] | undefined> { |
There was a problem hiding this comment.
looks like it returns empty array, when will this ever return undefined?
There was a problem hiding this comment.
it cant be undefined, i will remove | undefined in definition and call sites.
There was a problem hiding this comment.
I will fix this for other affiliate endpoints in upcoming PR
| affiliateReferredTotalVolume: 1000000, | ||
| }; | ||
| // No results found | ||
| if (infos === undefined) { |
There was a problem hiding this comment.
if no results are found, isn't infos === []? It should never be undefined
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
Outdated
Show resolved
Hide resolved
Changelist
replace snapshot endpoint stub with real logic
add supporting method in affiliate info table store
modify affiliate info table decimal precision and add column via migration
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests