Split affiliate info fees by taker and maker #2439
Conversation
WalkthroughThe pull request introduces significant changes to the handling of affiliate information across multiple files. It restructures constants and database schemas to separate maker and taker fees, replacing the aggregated Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| table.dropColumn('totalReferredFees'); | ||
| table.dropColumn('referredNetProtocolEarnings'); | ||
| table.decimal('totalReferredTakerFees', null).notNullable().defaultTo(0); | ||
| table.decimal('totalReferredMakerFees', null).notNullable().defaultTo(0); |
There was a problem hiding this comment.
Can we separate positive maker fees from negative maker fees?
There was a problem hiding this comment.
I will add totalReferredMakerRebates for negative, and keep totalReferredMakerFees for positive
| table.decimal('totalReferredFees', null).notNullable().defaultTo(0).alter(); | ||
| table.decimal('referredNetProtocolEarnings', null).notNullable().defaultTo(0).alter(); |
There was a problem hiding this comment.
totalReferredFees and referredNetProtocolEarnings dont exist in the table - so you shouldnt be able to alter them in this case. Should remove alter() here
![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 (4)
indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (2)
3-12: LGTM! Consider data migration and system-wide impacts.The
upfunction correctly implements the PR objective of splitting affiliate info fees by taker and maker. The use ofdecimaltype and non-nullable columns with default values is appropriate for financial data.However, please consider the following:
- Data Migration: Ensure there's a plan to migrate existing data from the dropped columns to the new ones if necessary.
- System-wide Impact: Verify that all parts of the system that previously used 'totalReferredFees' and 'referredNetProtocolEarnings' are updated to use the new columns.
- Indexing: Consider if any indexes need to be created on these new columns for query performance.
1-23: Overall assessment: Implements required changes with room for improvementThis migration file successfully implements the splitting of affiliate info fees into taker and maker components as per the PR objectives. However, there are a few points to address:
- Data Migration: Ensure there's a plan to handle existing data during the migration.
- System-wide Impact: Verify that all parts of the system are updated to use the new column structure.
- Rollback Mechanism: Improve the
downfunction to handle potential issues with non-existent columns and data loss during rollback.- Testing: Implement thorough testing to ensure the migration works as expected in both directions (up and down).
Please address these points to ensure a smooth and reliable database migration.
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
167-175: LGTM! Consider minor readability improvements.The changes successfully implement the splitting of affiliate info fees by taker and maker, aligning with the PR objectives. The calculations for
affiliateTotalReferredFeesandaffiliateReferredNetProtocolEarningshave been updated correctly, and the new propertiesaffiliateReferredMakerFeesandaffiliateReferredTakerFeeshave been added.To improve readability, consider using temporary variables for repeated calculations. For example:
const totalReferredFees = Number(info.totalReferredMakerFees) + Number(info.totalReferredTakerFees); return { // ... other properties affiliateTotalReferredFees: totalReferredFees, affiliateReferredNetProtocolEarnings: totalReferredFees - Number(info.affiliateEarnings), // ... other properties };This approach reduces repetition and makes the code easier to maintain.
indexer/services/comlink/src/types.ts (1)
734-735: Consider adding comments and grouping related properties.To improve code readability and maintainability:
- Add comments explaining the purpose and usage of the new properties.
- Consider grouping related fee properties together within the interface.
Example:
export interface AffiliateSnapshotResponseObject { // ... other properties ... // Affiliate fee-related properties affiliateTotalReferredFees: number, /** Total fees from maker orders referred by the affiliate */ affiliateReferredMakerFees: number, /** Total fees from taker orders referred by the affiliate */ affiliateReferredTakerFees: number, // ... other properties ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- indexer/packages/postgres/tests/helpers/constants.ts (3 hunks)
- indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts (8 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (1 hunks)
- indexer/packages/postgres/src/models/affiliate-info-model.ts (5 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (5 hunks)
- indexer/packages/postgres/src/types/affiliate-info-types.ts (2 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (4 hunks)
- indexer/services/comlink/public/swagger.json (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/update-affiliate-info.test.ts (4 hunks)
🔇 Additional comments (31)
indexer/packages/postgres/src/types/affiliate-info-types.ts (1)
18-19: LGTM! Verify enum usage in the codebase.The changes in the
AffiliateInfoColumnsenum correctly reflect the modifications made to theAffiliateInfoCreateObjectinterface. This ensures consistency between the two structures.To ensure that these changes don't break existing code, please run the following script to check for any usage of the old enum entries:
#!/bin/bash # Description: Check for usage of old enum entries # Test: Search for 'AffiliateInfoColumns.totalReferredFees' and 'AffiliateInfoColumns.referredNetProtocolEarnings' rg --type typescript 'AffiliateInfoColumns\.(totalReferredFees|referredNetProtocolEarnings)'If there are any occurrences, update them to use the new enum entries
totalReferredMakerFeesandtotalReferredTakerFeesas appropriate.indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (1)
14-23:⚠️ Potential issuePotential issues in rollback logic.
The
downfunction provides a rollback mechanism, but there are a few concerns:
Use of
alter(): The function usesalter()instead ofadd()for 'totalReferredFees' and 'referredNetProtocolEarnings'. This might cause issues if these columns don't exist when rolling back. Consider usingadd()instead.Data Loss Risk: Rolling back will result in the loss of the split fee data (taker and maker fees). Consider adding a comment or log warning about this data loss.
Data Aggregation: If possible, consider aggregating the taker and maker fees into the total fees column during rollback to preserve data.
Here's a suggested improvement:
export async function down(knex: Knex): Promise<void> { return knex .schema .alterTable('affiliate_info', (table) => { // Add a warning log console.warn('Rolling back will result in loss of split fee data'); // Add columns instead of altering table.decimal('totalReferredFees', null).notNullable().defaultTo(0); table.decimal('referredNetProtocolEarnings', null).notNullable().defaultTo(0); // Optionally, aggregate data before dropping columns // This is pseudo-code and would need to be adjusted based on your specific Knex setup // knex.raw(` // UPDATE affiliate_info // SET totalReferredFees = totalReferredTakerFees + totalReferredMakerFees // `); table.dropColumn('totalReferredTakerFees'); table.dropColumn('totalReferredMakerFees'); }); }To ensure the migration works as expected, consider running the following verification:
Replace
your_database_namewith the actual database name used in your environment.indexer/packages/postgres/src/models/affiliate-info-model.ts (5)
1-1: LGTM: Import statement updated correctly.The addition of
NumericPatternimport is consistent with its usage in the updated schema for validating both maker and taker fees.
54-55: LGTM: SQL to JSON conversions updated correctly.The changes in the
sqlToJsonConversionsmethod are consistent with the updates made in thejsonSchemamethod, correctly splittingtotalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFees.
72-74: LGTM: Class properties updated correctly.The changes in the class properties are consistent with the updates made in the
jsonSchemaandsqlToJsonConversionsmethods, correctly splittingtotalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFees.
Line range hint
1-81: Verify the impact of changes on the broader codebase.The changes consistently implement the splitting of
totalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFeesthroughout theAffiliateInfoModelclass. This aligns well with the PR objective.However, please ensure:
- All references to
totalReferredFeesin other parts of the codebase are updated to use the new split fields.- The removal of
referredNetProtocolEarningsdoesn't break any existing functionality or queries.To check for any remaining usage of
totalReferredFeesandreferredNetProtocolEarnings:#!/bin/bash # Search for usage of totalReferredFees and referredNetProtocolEarnings rg --type typescript 'totalReferredFees|referredNetProtocolEarnings'
22-23: Verify the use ofNumericPatternand the removal ofreferredNetProtocolEarnings.The changes to split
totalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFeesalign with the PR objective. However, please confirm:
- Is the use of
NumericPattern(which allows negative values) intentional for both maker and taker fees?- What is the impact of removing
referredNetProtocolEarningsfrom the schema?To check the usage of
referredNetProtocolEarningsin the codebase:Also applies to: 33-34
✅ Verification successful
Re-verify the removal of
referredNetProtocolEarningsby searching directly within TypeScript files.
referredNetProtocolEarningsremoval confirmed in schema and codebase.Only references to
referredNetProtocolEarningsare found in migration files, ensuring its proper removal.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of referredNetProtocolEarnings rg --type typescript 'referredNetProtocolEarnings'Length of output: 105
Script:
#!/bin/bash # Search for usage of referredNetProtocolEarnings in TypeScript files fd --extension ts --extension tsx --type f | xargs rg 'referredNetProtocolEarnings'Length of output: 1426
indexer/packages/postgres/src/types/db-model-types.ts (1)
291-292: LGTM! Verify related code updates.The changes to the
AffiliateInfoFromDatabaseinterface align well with the PR objectives to split affiliate info fees by taker and maker. The renaming oftotalReferredFeestototalReferredMakerFeesand the addition oftotalReferredTakerFeesimprove the granularity of fee tracking.Please ensure the following:
- Update any code that references the old
totalReferredFeesproperty to usetotalReferredMakerFeesinstead.- Implement necessary database schema changes to accommodate the new column.
- If required, create a data migration script to populate the new
totalReferredTakerFeescolumn for existing records.Run the following script to verify related code updates:
✅ Verification successful
Verified! All references to
totalReferredFeeshave been appropriately handled, and the new propertiestotalReferredMakerFeesandtotalReferredTakerFeesare correctly integrated throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to `totalReferredFees` and find usages of the new properties. echo "Checking for remaining references to totalReferredFees:" rg --type typescript 'totalReferredFees' echo "\nChecking usages of new properties:" rg --type typescript 'totalReferredMakerFees|totalReferredTakerFees'Length of output: 385
Script:
#!/bin/bash # Updated script to search TypeScript files correctly echo "Checking for remaining references to totalReferredFees:" rg 'totalReferredFees' --glob '*.ts' --glob '*.tsx' echo "\nChecking usages of new properties:" rg 'totalReferredMakerFees|totalReferredTakerFees' --glob '*.ts' --glob '*.tsx'Length of output: 9515
indexer/services/roundtable/__tests__/tasks/update-affiliate-info.test.ts (5)
75-76: LGTM: Fees split correctlyThe changes correctly implement the split of
totalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFees, aligning with the PR objective. The initialization with '0' is consistent with the previous implementation.
124-125: LGTM: Fees correctly assignedThe changes correctly assign the fees to
totalReferredTakerFees, reflecting the test scenario where a taker fill with a fee of '1000' was added. ThetotalReferredMakerFeesis correctly set to '0' as no maker fees were involved in this test case.
192-193: LGTM: Fees correctly accumulatedThe changes correctly accumulate the fees in
totalReferredTakerFees, reflecting the test scenario where two taker fills, each with a fee of '1000', were added over a two-week period. ThetotalReferredMakerFeesis correctly set to '0' as no maker fees were involved in this test case.
258-259: LGTM: Fees correctly accumulated in backfill scenarioThe changes correctly accumulate the fees in
totalReferredTakerFees, reflecting the test scenario where two taker fills, each with a fee of '1000', were added over a seven-day period during a backfill operation. ThetotalReferredMakerFeesis correctly set to '0' as no maker fees were involved in this test case.
Line range hint
1-300: Summary: Successful implementation of fee splittingThe changes in this file successfully implement the splitting of
totalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFeesacross all relevant test cases. This implementation aligns well with the PR objective of enhancing the tracking of affiliate information fees by introducing separate columns for taker and maker fees.Key observations:
- The changes are consistent across all test scenarios (multiple updates, backfilling, and first run backfill).
- The new fee structure correctly reflects the different test scenarios, accurately accumulating taker fees while maintaining maker fees at zero (as no maker fees were involved in these tests).
- The changes do not introduce any apparent issues or inconsistencies.
These modifications enhance the granularity of fee tracking, which should improve the clarity of affiliate earnings structure by distinguishing between maker and taker fees.
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
372-373: LGTM! Changes align with PR objectives.The modifications to the
affiliateInfoCreateToResponseObjectfunction effectively implement the PR's objective of splitting affiliate info fees by taker and maker:
- The
affiliateTotalReferredFeescalculation now correctly sumstotalReferredMakerFeesandtotalReferredTakerFees.- The
affiliateReferredNetProtocolEarningscalculation has been updated to reflect the new fee structure.- New properties
affiliateReferredMakerFeesandaffiliateReferredTakerFeesprovide more granular fee information.These changes enhance the clarity and granularity of fee tracking, which is the primary goal of this PR.
Also applies to: 375-377, 379-380
indexer/services/comlink/src/types.ts (1)
734-735: LGTM! Changes align with PR objectives.The addition of
affiliateReferredMakerFeesandaffiliateReferredTakerFeesproperties to theAffiliateSnapshotResponseObjectinterface successfully implements the PR objective of splitting affiliate info fees by taker and maker. This change enhances the granularity of fee tracking and provides a clear distinction between maker and taker fees.indexer/packages/postgres/__tests__/helpers/constants.ts (4)
993-994: LGTM! Affiliate fee structure updated.The changes to
defaultAffiliateInfocorrectly implement the split between maker and taker fees. The negative value fortotalReferredMakerFeesand positive value fortotalReferredTakerFeesaccurately represent the typical fee structure where makers receive rebates and takers pay fees. This change aligns with the PR objective of enhancing fee tracking granularity.
1005-1006: LGTM! Consistent fee structure update.The changes to
defaultAffiliateInfo2are consistent with the updates made todefaultAffiliateInfo. The split between maker and taker fees is correctly implemented, with appropriate test values. This consistency ensures that the test data covers multiple scenarios for the new fee structure.
1017-1018: LGTM! Comprehensive fee structure update across test data.The changes to
defaultAffiliateInfo3complete the consistent update of the affiliate fee structure across all test data objects. The incremental changes in values (-10, -11, -12 for maker fees and 10, 11, 12 for taker fees) provide a good variety for testing different scenarios. These modifications collectively ensure that the new split fee structure is thoroughly represented in the test constants.
993-994: Summary: Affiliate fee structure successfully updated across all test data.The changes consistently implement the split between maker and taker fees across all
AffiliateInfoCreateObjectinstances (defaultAffiliateInfo,defaultAffiliateInfo2, anddefaultAffiliateInfo3). This update aligns with the PR objective of enhancing fee tracking granularity. The removal of thereferredNetProtocolEarningsproperty suggests a change in how net earnings are calculated or represented.To ensure completeness:
Would you like me to create a follow-up task to update any documentation or other parts of the codebase that might reference the old
referredNetProtocolEarningsproperty?Also applies to: 1005-1006, 1017-1018
✅ Verification successful
Verification Successful: Affiliate fee structure and property removal confirmed.
All
AffiliateInfoCreateObjectinstances have been correctly updated with the newtotalReferredMakerFeesandtotalReferredTakerFeesproperties, and thereferredNetProtocolEarningsproperty has been successfully removed fromindexer/packages/postgres/__tests__/helpers/constants.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all AffiliateInfoCreateObject instances have been updated # and that referredNetProtocolEarnings has been removed echo "Checking for updated fee structure and removed referredNetProtocolEarnings:" grep -n "AffiliateInfoCreateObject" indexer/packages/postgres/__tests__/helpers/constants.ts -A 15 | grep -E "totalReferredMakerFees|totalReferredTakerFees|referredNetProtocolEarnings" echo "\nChecking for any remaining instances of referredNetProtocolEarnings:" grep -n "referredNetProtocolEarnings" indexer/packages/postgres/__tests__/helpers/constants.tsLength of output: 814
indexer/services/comlink/public/swagger.json (1)
Line range hint
306-326: LGTM! New properties added to improve fee tracking granularity.The changes to the
AffiliateSnapshotResponseObjectschema look good. The addition ofaffiliateReferredMakerFeesandaffiliateReferredTakerFeesproperties aligns with the PR objective of splitting affiliate info fees by taker and maker. This will provide more detailed information about the fees associated with referred trades.To ensure this change doesn't break existing API consumers, please run the following script to check for any direct usage of the
AffiliateSnapshotResponseObject:This will help identify any areas of the codebase that might need updates to handle the new properties.
✅ Verification successful
LGTM! The new properties are successfully integrated and utilized across the codebase.
The additions of
affiliateReferredMakerFeesandaffiliateReferredTakerFeesto theAffiliateSnapshotResponseObjectschema have been properly incorporated and are reflected in both the implementation and tests. This ensures enhanced fee tracking without disrupting existing functionalities.
- Recommendation: Please confirm with all API consumers to ensure they have updated their integrations to accommodate the new properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of AffiliateSnapshotResponseObject in the codebase # Search for AffiliateSnapshotResponseObject usage echo "Searching for AffiliateSnapshotResponseObject usage:" rg --type typescript --type javascript -i 'AffiliateSnapshotResponseObject' # Search for affiliate snapshot API calls echo "Searching for affiliate snapshot API calls:" rg --type typescript --type javascript -i 'affiliateSnapshot|affiliate.*snapshot'Length of output: 441
Script:
#!/bin/bash # Description: Check for usage of AffiliateSnapshotResponseObject and affiliate snapshot API calls in the codebase # Search for AffiliateSnapshotResponseObject usage in .ts and .js files echo "Searching for AffiliateSnapshotResponseObject usage:" rg -g '*.ts' -g '*.js' 'AffiliateSnapshotResponseObject' # Search for affiliate snapshot API calls in .ts and .js files echo "Searching for affiliate snapshot API calls:" rg -g '*.ts' -g '*.js' 'affiliateSnapshot|affiliate.*snapshot'Length of output: 2413
indexer/services/comlink/public/api-documentation.md (3)
4249-4251: Approved: Enhanced fee tracking for affiliatesThe addition of
affiliateReferredMakerFeesandaffiliateReferredTakerFeesfields to theAffiliateSnapshotResponseObjectschema is a positive change. This enhancement allows for more granular tracking of affiliate fees, separating maker and taker fees. This will provide better insights into affiliate performance and enable more detailed reporting.
4289-4291: Approved: Consistent fee tracking update in response schemaThe addition of
affiliateReferredMakerFeesandaffiliateReferredTakerFeesfields to theAffiliateSnapshotResponseschema is consistent with the changes made to theAffiliateSnapshotResponseObject. This ensures that the new fee information is properly included in the API response, maintaining consistency across the documentation.
Line range hint
1-4291: Confirmed: Changes are consistent throughout the documentAfter reviewing the entire document, I can confirm that the changes to include
affiliateReferredMakerFeesandaffiliateReferredTakerFeesare consistent and complete. The modifications are appropriately localized to theAffiliateSnapshotResponseObjectandAffiliateSnapshotResponseschemas, and no other parts of the documentation require updates related to these new fields.indexer/packages/postgres/src/stores/affiliate-info-table.ts (6)
195-196: Correctly Aggregate Maker and Taker FeesThe addition of
totalReferredMakerFeesandtotalReferredTakerFeesusingSUM(CASE WHEN...)clauses accurately separates fees based on liquidity type. This enhances fee tracking granularity in the affiliate stats.
214-215: Handle Null Values with COALESCE FunctionUsing
COALESCEensures thattotalReferredMakerFeesandtotalReferredTakerFeesdefault to0when there are no associated fees, preventing potential null value issues in subsequent calculations.
235-236: Include New Fee Columns in INSERT StatementAdding
totalReferredMakerFeesandtotalReferredTakerFeesto theINSERT INTO affiliate_infostatement ensures these new fields are correctly populated in the database.
246-247: Select New Fee Columns for Database InsertionSelecting
totalReferredMakerFeesandtotalReferredTakerFeesfromaffiliate_info_updateguarantees that the calculated fees are inserted into theaffiliate_infotable.
258-259: Accurately Update Fee Totals on ConflictThe
ON CONFLICTclause now updatestotalReferredMakerFeesandtotalReferredTakerFeesby adding the new values to existing totals. This ensures accurate accumulation of fees over time.
Line range hint
195-259: Verify Database Schema Updates for New ColumnsThe introduction of
totalReferredMakerFeesandtotalReferredTakerFeesrequires corresponding columns in theaffiliate_infotable. Please ensure that database migration scripts have been created to add these new columns to the schema.Run the following script to check for migration files adding these columns:
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (2)
394-394: Verify the intentional use of negativefeevalues in fill dataIn the
populateFillsAndReferralsfunction, thefeeproperty is set to negative values (e.g.,fee: '-1000') for fills withliquidity: Liquidity.MAKER. Negative fees might represent maker rebates or indicate that makers receive a fee. Please verify that this is the intended behavior and that the business logic correctly handles negative fee amounts.To ensure consistency in fee representation, you can run the following script:
#!/bin/bash # Description: Find occurrences where fill fees are assigned negative values. # Search for FillTable.create calls with negative fee values rg --type ts --no-heading --line-number "FillTable\.create\(\{[^\}]*fee:\s*'-\d+'"Also applies to: 405-405, 416-416
116-117: Clarify the use of negative values fortotalReferredMakerFeesIn the test cases, the
totalReferredMakerFeesproperty is assigned negative values (e.g.,'-1000','-2000','-3000'). Negative fee values can be confusing, as fees are typically represented as positive numbers. If negative values are intended to represent maker rebates or negative fees, please ensure that this convention is consistently applied and clearly documented to improve code readability and maintainability.To confirm the usage of negative maker fees across the codebase, you can run the following script:
Also applies to: 143-144, 165-166, 191-192
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
indexer/packages/postgres/src/types/affiliate-info-types.ts (1)
6-8: Overall impact: Improved fee tracking granularity with potential wider implications.The changes successfully split affiliate fees into maker and taker categories, improving the granularity of fee tracking. This aligns well with the PR objectives. However, there are a few points to consider:
The introduction of
totalReferredMakerRebatessuggests a more complex fee structure. Ensure that all related business logic and calculations are updated to accommodate this new concept.The removal of
referredNetProtocolEarningsmight impact existing queries or calculations that rely on this aggregated value. Verify that all dependent code is updated accordingly.To ensure a smooth transition:
- Review and update any queries or aggregations that previously used
totalReferredFeesorreferredNetProtocolEarnings.- Update any documentation or API specifications that reference the changed or removed fields.
- Consider adding a migration script to populate the new fields based on existing data, if applicable.
Also applies to: 19-21
indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (2)
3-13: LGTM! The migration aligns well with the PR objectives.The
upfunction successfully implements the required changes:
- It removes the 'totalReferredFees' and 'referredNetProtocolEarnings' columns as suggested in the past review.
- It adds separate columns for taker fees, maker fees, and maker rebates, addressing the need to split affiliate info fees and separate positive maker fees from negative ones (rebates).
The use of
decimaltype and non-nullable columns with default values is appropriate for financial data.Consider adding a comment at the beginning of the file explaining the purpose of this migration for better maintainability. For example:
/** * Migration: Split affiliate info fees by taker and maker * * This migration modifies the affiliate_info table to provide more granular * fee tracking by separating taker fees, maker fees, and maker rebates. */
1-25: Consider the broader implications of this schema changeWhile this migration successfully implements the required changes and improves the granularity of fee tracking, there are some important considerations to keep in mind:
Existing Queries: Any existing queries or applications that rely on the 'totalReferredFees' or 'referredNetProtocolEarnings' columns will need to be updated to use the new column structure.
Data Migration: Consider whether there's a need to migrate existing data from the old structure to the new one. If there's valuable historical data, you might want to add a data migration step in the
upfunction.Indexing: Evaluate whether the new columns require indexing for query performance, especially if they'll be frequently used in WHERE clauses or joins.
Reporting: Update any reporting tools or dashboards that might be using the old column structure.
API Changes: If this table is exposed through any APIs, ensure that the API contracts are updated and versioned appropriately.
To address these points:
- Create a follow-up task to update all affected queries and applications.
- If needed, add a data migration step in the
upfunction to populate the new columns based on existing data.- Analyze query patterns and add appropriate indexes if necessary.
- Update relevant documentation, especially focusing on any breaking changes for downstream consumers of this data.
Would you like assistance in creating these follow-up tasks or implementing any of these suggestions?
indexer/services/roundtable/__tests__/tasks/update-affiliate-info.test.ts (2)
75-77: LGTM! Consider adding a comment for clarity.The changes correctly implement the split of affiliate info fees into maker and taker categories, aligning with the PR objectives. The new fields
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesprovide a more granular view of the fees.Consider adding a brief comment explaining the significance of these new fields, especially
totalReferredMakerRebates, to improve code readability and maintainability.Also applies to: 125-127
261-263: LGTM! Consider parameterizing test data.The changes in the "Successfully backfills on first run" test case correctly implement the split of affiliate info fees, consistent with the previous test cases.
Consider parameterizing the test data for the fill creation and expected affiliate info. This would make it easier to add more test scenarios in the future and reduce code duplication. For example:
const testScenarios = [ { name: 'two taker fills', fills: [ { liquidity: Liquidity.TAKER, fee: '1000', affiliateRevShare: '500' }, { liquidity: Liquidity.TAKER, fee: '1000', affiliateRevShare: '500' }, ], expectedInfo: { totalReferredMakerFees: '0', totalReferredTakerFees: '2000', totalReferredMakerRebates: '0', // ... other expected fields }, }, // ... other scenarios ]; for (const scenario of testScenarios) { it(`Successfully backfills on first run: ${scenario.name}`, async () => { // ... test setup for (const fill of scenario.fills) { await FillTable.create({ ...testConstants.defaultFill, ...fill }); } // ... run task expect(updatedInfo).toEqual(scenario.expectedInfo); }); }This approach would make it easier to test various combinations of maker and taker fees in the future.
indexer/packages/postgres/src/stores/affiliate-info-table.ts (1)
195-197: LGTM! Consider a minor optimization.The logic for separating maker fees, taker fees, and maker rebates is correct and aligns with the PR objective. Well done!
Consider combining the
totalReferredMakerFeesandtotalReferredMakerRebatescalculations into a single CASE statement for slightly improved readability:SUM(CASE WHEN affiliate_fills."liquidity" = '${Liquidity.MAKER}' THEN CASE WHEN affiliate_fills."fee" > 0 THEN affiliate_fills."fee" ELSE 0 END ELSE 0 END) AS "totalReferredMakerFees", SUM(CASE WHEN affiliate_fills."liquidity" = '${Liquidity.MAKER}' THEN CASE WHEN affiliate_fills."fee" < 0 THEN affiliate_fills."fee" ELSE 0 END ELSE 0 END) AS "totalReferredMakerRebates",This change is optional and doesn't affect the functionality, but it might make the code slightly more concise.
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (3)
116-118: LGTM! Consider updating the test description.The new properties
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesprovide a more granular breakdown of affiliate fees, which aligns with the PR objective of splitting affiliate info fees by taker and maker.Consider updating the test description to reflect that it now also checks for the new fee breakdown properties. This will make the test's purpose clearer to other developers.
144-146: LGTM! Consider improving variable naming for clarity.The changes correctly implement the new fee breakdown structure across multiple update scenarios. The values accumulate as expected, demonstrating the correct behavior of the
updateInfofunction.For improved readability, consider renaming the variables
updatedInfo1,updatedInfo2, etc., to more descriptive names that reflect the state they represent, such asupdatedInfoAfterFirstUpdate,updatedInfoAfterSecondUpdate, etc.Also applies to: 167-169
Line range hint
1-433: Summary: Comprehensive update to affiliate fee structureThe changes in this file consistently implement a new fee breakdown structure across all test cases, aligning well with the PR objective of splitting affiliate info fees by taker and maker. The introduction of
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesprovides a more granular view of affiliate earnings.A notable change is the introduction of negative fees to represent maker rebates. While this aligns with the new fee structure, it represents a significant shift in how fees are handled.
Given the introduction of negative fees and the new fee breakdown structure:
- Ensure that all components interacting with affiliate fees can handle negative values correctly.
- Update any documentation or comments in related files to explain the new fee structure and the concept of maker rebates.
- Consider adding integration tests to verify that the new fee structure works correctly with other components of the system.
- Review and update any reporting or analytics systems that may be affected by this change in fee representation.
These steps will help ensure that the new fee structure is consistently implemented and understood across the entire system.
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
372-382: LGTM! Consider minor refactoring for improved readability.The changes correctly implement the splitting of affiliate info fees by taker and maker, aligning with the PR objectives. The calculations for
affiliateTotalReferredFeesandaffiliateReferredNetProtocolEarningshave been updated to include the separate fee components, and new propertiesaffiliateReferredMakerFeesandaffiliateReferredTakerFeeshave been added to provide detailed fee information.Consider refactoring the calculations of
affiliateTotalReferredFeesandaffiliateReferredNetProtocolEarningsfor improved readability:const totalReferredFees = Number(info.totalReferredMakerFees) + Number(info.totalReferredTakerFees) + Number(info.totalReferredMakerRebates); return { // ... other properties affiliateTotalReferredFees: totalReferredFees, affiliateReferredNetProtocolEarnings: totalReferredFees - Number(info.affiliateEarnings), // ... remaining properties };This refactoring reduces duplication and makes the calculation more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- indexer/packages/postgres/tests/helpers/constants.ts (3 hunks)
- indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts (6 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (1 hunks)
- indexer/packages/postgres/src/models/affiliate-info-model.ts (5 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (5 hunks)
- indexer/packages/postgres/src/types/affiliate-info-types.ts (2 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/update-affiliate-info.test.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- indexer/packages/postgres/src/types/db-model-types.ts
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
🔇 Additional comments (16)
indexer/packages/postgres/src/types/affiliate-info-types.ts (1)
19-21: LGTM! Enum changes are consistent with interface modifications.The additions of
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesto theAffiliateInfoColumnsenum are consistent with the changes made to theAffiliateInfoCreateObjectinterface. This maintains the structural integrity and consistency between the interface and the enum.indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (1)
15-25: Thedownfunction correctly reverses the migration, but consider data preservation.The
downfunction successfully rolls back the changes made in theupfunction, which is crucial for maintaining database flexibility. However, there's a potential issue to address:When rolling back, the data from the new columns (totalReferredTakerFees, totalReferredMakerFees, totalReferredMakerRebates) will be lost. Consider implementing a data migration strategy to preserve this information when rolling back. For example:
- Before dropping the new columns, you could aggregate their values into the old columns.
- Alternatively, you could create a backup table to store the data temporarily.
To verify the impact, please run the following script to check for existing data in the affected columns:
If the count is greater than 0, consider implementing a data preservation strategy in the
downfunction.indexer/services/roundtable/__tests__/tasks/update-affiliate-info.test.ts (3)
194-196: LGTM! Consistent implementation of fee splitting.The changes in the "Successfully backfills from past date" test case correctly implement the split of affiliate info fees. The values accurately reflect the test scenario with two taker fills (totalReferredTakerFees: '2000') and no maker fills or rebates.
Line range hint
1-305: Overall, the changes look good and align with the PR objectives.The modifications to this test file consistently implement the split of affiliate info fees into maker and taker categories. The test cases have been appropriately updated to reflect the new fee structure, ensuring that the
update-affiliate-infotask is working as expected with the new changes.Some suggestions for further improvement:
- Add comments explaining the significance of the new fields, especially
totalReferredMakerRebates.- Consider parameterizing test data to make it easier to add more test scenarios in the future.
- Verify if new metrics should be added for the new fee categories and update the "Successfully records metrics" test case accordingly.
These improvements would enhance the maintainability and extensibility of the test suite.
Line range hint
280-292: Consider updating metrics for new fee categories.While the "Successfully records metrics" test case remains unchanged, it might be beneficial to update the metrics to reflect the new fee categories introduced in this PR.
Could you verify if any new metrics should be added for the
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatescategories? If so, please update this test case accordingly.If these new metrics are being recorded elsewhere in the codebase, consider adding corresponding assertions in this test case to ensure they are being recorded correctly.
indexer/packages/postgres/src/stores/affiliate-info-table.ts (5)
215-217: LGTM! Proper handling of potential NULL values.The addition of
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesfields with COALESCE is correct and consistent with the PR objective. The use of COALESCE ensures that NULL values are properly handled, defaulting to 0 for these numerical fields.
237-239: LGTM! Consistent addition of new columns.The new columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly added to the INSERT statement. The order of these columns matches the SELECT statement that follows, ensuring data consistency.
249-251: LGTM! Consistent selection of new fields.The new fields
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly added to the SELECT statement. The order and naming of these fields match the INSERT statement, ensuring data integrity.
262-264: LGTM! Consider adding a consistency check.The update logic for
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatescorrectly accumulates the new fee values with the existing ones. This approach ensures that the total fees are properly updated over time.To ensure data consistency, consider adding a check to verify that the sum of
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesequals the total referred fees. This can be done by adding a trigger or a periodic consistency check job. Here's a sample query to verify this:SELECT address, totalReferredMakerFees + totalReferredTakerFees + totalReferredMakerRebates AS calculated_total, totalReferredFees AS stored_total, CASE WHEN totalReferredMakerFees + totalReferredTakerFees + totalReferredMakerRebates != totalReferredFees THEN 'Inconsistent' ELSE 'Consistent' END AS consistency_status FROM affiliate_info WHERE totalReferredMakerFees + totalReferredTakerFees + totalReferredMakerRebates != totalReferredFees;This query will help identify any inconsistencies between the split fees and the total fees.
Line range hint
1-300: Overall, excellent implementation of fee splitting for affiliates.The changes in this file successfully implement the splitting of affiliate info fees by taker and maker, aligning perfectly with the PR objectives. The modifications are consistent throughout the SQL query, from the initial calculations in the CTEs to the final INSERT and UPDATE statements.
Key points:
- Proper separation of maker fees, taker fees, and maker rebates.
- Consistent addition of new fields across all relevant parts of the query.
- Correct handling of potential NULL values using COALESCE.
- Appropriate accumulation of fees in the UPDATE statement.
These changes will provide more granular insights into affiliate performance, distinguishing between maker and taker activities. This enhanced data structure will likely support more detailed analysis and potentially more sophisticated affiliate reward strategies in the future.
To ensure the changes don't introduce any regressions, consider adding or updating unit tests to cover the new fee splitting logic. This could include tests for:
- Correct calculation of split fees.
- Proper handling of NULL values.
- Accurate accumulation of fees over multiple updates.
Additionally, it might be beneficial to update any related documentation or API specifications to reflect these new fields and their meanings.
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (2)
194-196: LGTM! Correctly handles fills before referral block height.The changes maintain consistency with the new fee breakdown structure. The test correctly verifies that fills occurring before the referral block height are not counted in the fee calculations, as all fee-related values are set to '0'.
399-399: LGTM! Please provide more context on negative fees.The introduction of a negative fee for a maker fill aligns with the concept of maker rebates and the PR objective of splitting affiliate info fees by taker and maker.
Could you provide more context on the implications of negative fees in the system? Are there any other components that need to be updated to handle negative fees correctly?
Consider adding a comment explaining the purpose of the negative fee, as it represents a significant change in fee representation. This will help other developers understand the intention behind this change.
indexer/packages/postgres/__tests__/helpers/constants.ts (4)
993-995: LGTM! Improved fee tracking granularity.The changes in
defaultAffiliateInfoalign well with the PR objective. SplittingtotalReferredFeesintototalReferredMakerFeesandtotalReferredTakerFeesprovides more detailed information. The addition oftotalReferredMakerRebatesintroduces a new concept of rebates for makers, which enhances the affiliate info structure.
1006-1008: LGTM! Consistent changes indefaultAffiliateInfo2.The modifications in
defaultAffiliateInfo2mirror those indefaultAffiliateInfo, maintaining consistency across the test data. The split fee structure and the addition of maker rebates are implemented correctly, with values that align with the object's existing pattern (using '11').
1019-1021: LGTM! Consistent implementation across allAffiliateInfoCreateObjectinstances.The changes in
defaultAffiliateInfo3complete the consistent implementation of the new fee structure across all threeAffiliateInfoCreateObjectinstances. The split fee properties and the new maker rebates property are correctly added, with values that match the object's existing pattern (using '12').
993-995: Summary: Successful implementation of split fee structure in test data.The changes in this file successfully implement the PR objective of splitting affiliate info fees by taker and maker. The modifications are consistently applied across all three
AffiliateInfoCreateObjectinstances (defaultAffiliateInfo,defaultAffiliateInfo2, anddefaultAffiliateInfo3).Key improvements:
- Replaced
totalReferredFeeswithtotalReferredMakerFeesandtotalReferredTakerFees.- Added
totalReferredMakerRebatesto track rebates for makers.These changes enhance the granularity of fee tracking in the test data, which should better support testing scenarios related to the new fee structure.
Also applies to: 1006-1008, 1019-1021
| totalReferredMakerFees: string, | ||
| totalReferredTakerFees: string, | ||
| totalReferredMakerRebates: string, |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Residual References Found for referredNetProtocolEarnings
The property referredNetProtocolEarnings is still referenced in several migration files:
indexer/packages/postgres/src/db/migrations/migration_files/20240919142157_change_affiliate_info_default_value.tsindexer/packages/postgres/src/db/migrations/migration_files/20240830165511_create_affiliate_info_table.tsindexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.tsindexer/packages/postgres/src/db/migrations/migration_files/20240913142157_change_affiliate_info_decimal_precision_and_add_total_referred_volume.ts
Please address these residual references to ensure database migrations function correctly without the removed property.
🔗 Analysis chain
LGTM! Changes align with PR objectives.
The addition of totalReferredMakerFees, totalReferredTakerFees, and totalReferredMakerRebates aligns well with the PR objective of splitting affiliate info fees by taker and maker. This change provides more granular tracking of fees and rebates.
Please verify the impact of removing the referredNetProtocolEarnings property. A previous review comment indicated its usage in migration scripts. Run the following script to check for any remaining usage of this property in the codebase:
If there are any occurrences, ensure they are updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of removed property 'referredNetProtocolEarnings'
# Test: Search for 'referredNetProtocolEarnings' in TypeScript files
rg --type typescript 'referredNetProtocolEarnings'
Length of output: 85
Script:
#!/bin/bash
# Description: Check for usage of removed property 'referredNetProtocolEarnings'
# Test: Search for 'referredNetProtocolEarnings' in TypeScript files
rg 'referredNetProtocolEarnings' --glob '*.ts' --glob '*.tsx'
Length of output: 1396
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts
Show resolved
Hide resolved
jerryfan01234
force-pushed
the
affiliates-change-affiliate-info-columns
branch
from
fe72dc8 to
70f0585
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
indexer/packages/postgres/src/stores/affiliate-info-table.ts (2)
195-197: LGTM! Consider adding comments for clarity.The new columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatescorrectly implement the split between maker and taker fees, aligning with the PR objectives. The logic for calculating these values is accurate.Consider adding inline comments to explain the logic for each new column, especially for
totalReferredMakerRebates, as it might not be immediately obvious why negative fees are considered here.
262-264: LGTM! Correct accumulation of new values.The new columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly included in the ON CONFLICT DO UPDATE section. The update logic properly accumulates the new values by adding them to the existing ones, which is the correct approach for maintaining totals across multiple updates.For consistency, consider adding a comment above this section explaining the accumulation logic, similar to the comments in other parts of the query.
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (3)
144-146: LGTM: Test case correctly updated for new fee structureThe test case has been appropriately modified to check for the correct accumulation of maker and taker fees separately, as well as the introduction of maker rebates. This accurately reflects the new fee structure and its expected behavior over multiple updates.
For improved clarity, consider adding a comment explaining the significance of negative values for
totalReferredMakerRebates.Also applies to: 167-169
399-399: LGTM: Test data updated to include maker rebatesThe modification to set a negative fee for a maker fill is consistent with the new concept of maker rebates introduced in the test cases. This change ensures that the test data accurately reflects the new fee structure, allowing for comprehensive testing of the updated fee calculation logic.
Consider adding a comment explaining the significance of the negative fee value for clarity.
Line range hint
1-424: Overall assessment: Test suite successfully updated for new fee structureThe changes in this file consistently and accurately reflect the PR objective of splitting affiliate info fees by taker and maker. The test cases have been comprehensively updated to cover various scenarios, including:
- Basic fee accumulation for makers and takers
- Handling of maker rebates (negative fees)
- Multiple updates to fee accumulation
- Edge cases like new referrals without new fills
The modifications to both the test cases and the test data generation function ensure thorough testing of the new fee calculation logic. These changes provide confidence in the correct implementation of the new fee structure in the main code.
To further enhance the test suite:
- Consider adding comments to explain the significance of negative fee values (maker rebates) where they appear.
- Ensure that there are sufficient test cases to cover all possible combinations of maker/taker fees and rebates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- indexer/packages/postgres/tests/helpers/constants.ts (3 hunks)
- indexer/packages/postgres/tests/stores/affiliate-info-table.test.ts (6 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts (1 hunks)
- indexer/packages/postgres/src/models/affiliate-info-model.ts (5 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (5 hunks)
- indexer/packages/postgres/src/types/affiliate-info-types.ts (2 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/update-affiliate-info.test.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- indexer/packages/postgres/src/db/migrations/migration_files/20241002144813_change_affiliate_info_fee_columns.ts
- indexer/packages/postgres/src/models/affiliate-info-model.ts
- indexer/packages/postgres/src/types/affiliate-info-types.ts
- indexer/packages/postgres/src/types/db-model-types.ts
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
- indexer/services/roundtable/tests/tasks/update-affiliate-info.test.ts
🔇 Additional comments (8)
indexer/packages/postgres/src/stores/affiliate-info-table.ts (3)
215-217: LGTM! Proper handling of new columns.The new columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly included in theaffiliate_info_updateCTE. The use of COALESCE to handle potential NULL values is a good practice, ensuring that these fields default to 0 when no data is available.
237-239: LGTM! Consistent inclusion of new columns.The new columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly included in both the INSERT INTO statement and the corresponding SELECT statement. The order and naming of columns are consistent, ensuring proper data insertion.Also applies to: 249-251
Line range hint
195-264: Overall LGTM! Successfully implemented fee splitting.The changes in this file successfully implement the PR objective of splitting affiliate info fees by taker and maker. The modifications are consistent throughout the SQL query, including the
affiliate_statsCTE,affiliate_info_updateCTE, INSERT INTO statement, and ON CONFLICT DO UPDATE section.Key points:
- New columns
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesare correctly introduced and calculated.- The logic for differentiating between maker and taker fees is accurate.
- Proper handling of NULL values and accumulation of totals is implemented.
These changes will provide more granular tracking of affiliate fees, which aligns with the PR's goals. The implementation is solid and follows good practices.
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts (2)
116-118: LGTM: New fee structure aligns with PR objectivesThe introduction of
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesaccurately reflects the PR's goal of splitting affiliate info fees by taker and maker. The negative value fortotalReferredMakerRebatessuggests a new concept of rebates for makers, which is an important distinction in the fee structure.
194-196: LGTM: Test case correctly handles new referral without new fillsThe test case has been appropriately updated to reflect the new fee structure. It correctly maintains the previous fee values when no new fills are processed, while only incrementing the
totalReferredUserscount for a new affiliate referral. This accurately tests the behavior of the new fee structure in this specific scenario.indexer/packages/postgres/__tests__/helpers/constants.ts (3)
993-995: LGTM! Improved fee tracking for affiliates.The changes in
defaultAffiliateInfosuccessfully implement the split between maker and taker fees, and introduce maker rebates. This enhancement provides more granular financial data for affiliate tracking, aligning with the PR objectives.
1006-1008: LGTM! Consistent implementation of fee splitting.The changes in
defaultAffiliateInfo2correctly implement the fee splitting and rebate addition, maintaining consistency withdefaultAffiliateInfo. The incremented values (11 instead of 10) preserve the existing pattern in the test data.
1019-1021: LGTM! Consistent implementation across all test data.The changes in
defaultAffiliateInfo3correctly implement the fee splitting and rebate addition, maintaining consistency with the previous objects. This completes the update of all test data for affiliate info.Summary of changes:
totalReferredFeesis split intototalReferredMakerFeesandtotalReferredTakerFees.- A new field
totalReferredMakerRebatesis added with negative values.- The incremental pattern in test data is preserved (10 → 11 → 12).
These modifications ensure that the test data accurately reflects the new structure for tracking affiliate fees, supporting the PR's objective of splitting fees by taker and maker.
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 12c2ee9)
Co-authored-by: jerryfan01234 <[email protected]>
Changelist
Add/replace columns to track taker/maker fees separately in affiliate-info
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
New Features
totalReferredMakerFees,totalReferredTakerFees, andtotalReferredMakerRebatesin affiliate information, enhancing clarity in fee categorization.Bug Fixes
Documentation
Tests