add afflaiteReferredMakerRebates field to response #2473
Conversation
WalkthroughThis pull request introduces extensive testing for the Changes
Possibly related PRs
Suggested labels
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 (
|
![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 (5)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
178-178: LGTM with a minor suggestion.The addition of
affiliateReferredMakerRebatesto the response object aligns with the PR objective. Good job on usingNumber()to ensure type consistency.Consider updating the PR description to correct the spelling from 'afflaiteReferredMakerRebates' to 'affiliateReferredMakerRebates' for consistency with the implemented code.
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
383-383: Typo in PR objectiveThere's a typo in the PR objective. It mentions "afflaiteReferredMakerRebates" instead of "affiliateReferredMakerRebates". Please update the PR description to correct this typo for clarity.
indexer/services/comlink/src/types.ts (1)
736-736: LGTM! Consider addressing the typo in the PR objective.The addition of the
affiliateReferredMakerRebatesproperty to theAffiliateSnapshotResponseObjectinterface is correct and aligns with the PR objective. The property name follows the existing naming convention, and thenumbertype is appropriate for representing rebates.There appears to be a typo in the PR objective: 'afflaiteReferredMakerRebates' should be 'affiliateReferredMakerRebates'. Consider updating the PR description for clarity.
indexer/services/comlink/public/swagger.json (1)
Line range hint
314-331: LGTM! Consider adding a description for the new property.The addition of the
affiliateReferredMakerRebatesproperty to theAffiliateSnapshotResponseObjectschema is well-integrated and consistent with the existing structure. The type, format, and inclusion in the required fields are appropriate.Consider adding a description for the new
affiliateReferredMakerRebatesproperty to provide more context about its purpose and usage. This would improve the API documentation and make it easier for consumers to understand the meaning of this field.indexer/services/comlink/public/api-documentation.md (1)
4252-4253: New fields added to AffiliateSnapshotResponseObject. Please provide documentation.Two new fields,
affiliateReferredTakerFeesandaffiliateReferredMakerRebates, have been added to the AffiliateSnapshotResponseObject. This addition aligns with the PR objectives.Could you please add a brief description for these new fields in the documentation? This will help API users understand the purpose and content of these fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (79 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)
🔇 Additional comments (12)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2)
Line range hint
321-321: Improved error message clarity.The updated error message for
addressFilterprovides clearer guidance on the expected input format. This change enhances the API's user-friendliness and aligns with best practices for clear error messages.
Line range hint
1-424: Overall assessment: Changes look good and align with PR objectives.The modifications to add the
affiliateReferredMakerRebatesfield to the response and update the error message foraddressFilterhave been implemented correctly. These changes enhance the functionality and user experience of the API without introducing any apparent issues.To ensure the changes are properly integrated, please run the existing test suite and consider adding new tests to cover the added
affiliateReferredMakerRebatesfield in the response.indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (3)
383-383: LGTM: New field added correctlyThe new field
affiliateReferredMakerRebateshas been correctly added to the response object in theaffiliateInfoCreateToResponseObjecthelper function. This change aligns with the PR objective to include this information in the response.
Line range hint
1-385: Summary of review findings
- The new field
affiliateReferredMakerRebateshas been correctly added to theaffiliateInfoCreateToResponseObjecthelper function.- Existing snapshot tests may need to be updated to include assertions for the new field. Please verify and update if necessary.
- There's a typo in the PR objective ("afflaiteReferredMakerRebates" instead of "affiliateReferredMakerRebates").
Next steps:
- Verify and update snapshot tests if needed.
- Correct the typo in the PR description.
- Ensure that the changes are tested thoroughly, as the "Test Plan" section in the PR description is empty.
Once these points are addressed, the changes look good to merge.
Line range hint
136-307: Verify impact of helper function changes on snapshot testsThe
affiliateInfoCreateToResponseObjecthelper function has been modified to include a new fieldaffiliateReferredMakerRebates. Please ensure that the snapshot tests in this section accurately reflect this change and that the expected responses include the new field where appropriate.To verify this, you can run the following script:
If the script doesn't return any results, consider updating the relevant test cases to include assertions for the new field.
✅ Verification successful
Action Required: Update Shell Script to Search TypeScript Files
Please run the following script to check if the
affiliateReferredMakerRebatesfield is included in the snapshot tests:
Snapshot Tests Updated Correctly
The snapshot tests now include the
affiliateReferredMakerRebatesfield as required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if snapshot tests include the new affiliateReferredMakerRebates field # Test: Search for affiliateReferredMakerRebates in snapshot tests rg --type typescript 'affiliateReferredMakerRebates' indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.tsLength of output: 171
Script:
# #!/bin/bash # Description: Check if snapshot tests include the new affiliateReferredMakerRebates field # Search for affiliateReferredMakerRebates in TypeScript test files rg 'affiliateReferredMakerRebates' --glob '*.ts' --glob '*.tsx' indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.tsLength of output: 222
indexer/services/comlink/src/types.ts (2)
736-736: Confirm database schema changes and migration planThe addition of
affiliateReferredMakerRebatesto theAffiliateSnapshotResponseObjectinterface might require corresponding changes in the database schema. To ensure data consistency and avoid potential issues:
- Verify if a database schema update is needed to store this new information.
- If a schema change is required, ensure a data migration plan is in place.
- Update the PR description to include information about any necessary database changes and migration steps.
Could you please confirm if any database schema changes are needed for this new field? If so, please outline the migration plan in the PR description.
736-736: Verify integration of the new property in the codebaseWhile the addition of
affiliateReferredMakerRebatesto theAffiliateSnapshotResponseObjectinterface is correct, we should ensure proper integration across the codebase:
- Update any code constructing or using
AffiliateSnapshotResponseObjectto include the new property.- Revise API documentation to reflect this new field.
- Adjust any serialization/deserialization logic for this object if necessary.
- Update or add test cases to cover the new property.
To help verify the integration, you can run the following script:
indexer/services/comlink/public/swagger.json (1)
Line range hint
1-2893: Summary: API specification updated with new affiliate-related fieldThe changes to the Swagger/OpenAPI specification are focused and well-implemented. The addition of the
affiliateReferredMakerRebatesfield to theAffiliateSnapshotResponseObjectschema enhances the API's capability to provide more detailed affiliate-related information.This update aligns with the PR objectives to modify the response structure based on discussions with Fuul. The change is backwards-compatible and should not break existing integrations. However, API consumers should be notified of this addition to take advantage of the new field.
To ensure the new field is properly implemented, please run the following verification script:
This script will help verify that the new field is properly implemented in handlers, services, and tests, as well as check for any related database changes.
indexer/services/comlink/public/api-documentation.md (4)
31-31: Python code sample updated correctly.The base URL in the Python code sample has been properly updated to match the new Testnet URL.
49-49: JavaScript code sample updated correctly.The base URL in the JavaScript code sample has been properly updated to match the new Testnet URL.
4294-4295: AffiliateSnapshotResponse updated consistently.The new fields
affiliateReferredTakerFeesandaffiliateReferredMakerRebateshave been correctly added to the AffiliateSnapshotResponse schema, maintaining consistency with the AffiliateSnapshotResponseObject changes.
9-9: Testnet base URL updated. Please verify the new endpoint.The base URL for the Testnet environment has been updated. This change aligns with the PR objectives.
Please run the following script to verify that the new Testnet URL is accessible and returns a valid response:
Ensure that the response includes a 200 OK status code, indicating that the endpoint is functional.
✅ Verification successful
Testnet URL is accessible and functional.
The new Testnet endpoint was successfully reached and returns a valid response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new Testnet URL curl -I https://dydx-testnet.imperator.co/v4/heightLength of output: 945
jerryfan01234
force-pushed
the
add-comlink-affiliates-snapshot-makerRebate
branch
from
415518c to
47c909e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
178-178: LGTM! Consider adding a comment for clarity.The implementation of
affiliateReferredMakerRebateslooks correct and aligns with the PR objectives. Good job on enhancing the response object with this additional information.A couple of suggestions:
- There seems to be a typo in the PR description: "afflaiteReferredMakerRebates" vs "affiliateReferredMakerRebates" in the code. Consider updating the PR description for consistency.
- It might be helpful to add a brief comment explaining the purpose of this field for future maintainers.
Consider adding a comment above this line to explain the purpose of the
affiliateReferredMakerRebatesfield:// Total maker rebates earned by users referred by this affiliate affiliateReferredMakerRebates: Number(info.totalReferredMakerRebates),indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (2)
383-383: LGTM with a minor suggestion.The addition of the
affiliateReferredMakerRebatesfield to the response object is correct and aligns with the PR objective. The field is properly derived frominfo.totalReferredMakerRebatesand is consistent in type (Number) with other fields in the response object.Consider adding a test case in the
/snapshotendpoint tests to verify that this new field is correctly included in the response. This would ensure that the change is properly reflected in the API output.
Line range hint
1-383: Enhance test coverage for the new field.While the addition of the
affiliateReferredMakerRebatesfield to the response object is correct, the existing test cases have not been updated to verify this new field. To ensure comprehensive test coverage:
- Update the
should return snapshots when all params specifiedtest in the/snapshotendpoint to include an assertion for the newaffiliateReferredMakerRebatesfield.- Consider adding a new test case that specifically focuses on scenarios where the
affiliateReferredMakerRebatesvalue is non-zero, to ensure it's being calculated and returned correctly.These additions will help validate that the new field is correctly integrated into the API responses and behaves as expected under various conditions.
indexer/services/comlink/public/api-documentation.md (1)
Line range hint
1-1000: Base URL for Testnet environment has been updated.The Testnet base URL has been changed from
https://indexer.v4testnet.dydx.exchange/v4tohttps://dydx-testnet.imperator.co/v4throughout the documentation. This change is consistent across all code samples.Please ensure that:
- All Testnet users are notified of this change.
- There's a migration plan or grace period for users still using the old URL.
- The new URL is properly set up and functioning as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- indexer/services/comlink/public/swagger.json
- indexer/services/comlink/src/types.ts
🔇 Additional comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
Line range hint
1-383: Overall assessment: LGTM with suggestions for improved test coverage.The changes to add the
affiliateReferredMakerRebatesfield are implemented correctly in theaffiliateInfoCreateToResponseObjectfunction. The existing test structure is well-organized and comprehensive for the current functionality.To further improve the quality of this PR:
- Implement the suggested additions to test coverage for the new field.
- Ensure that the changes are reflected in any relevant documentation or API specifications.
Once these suggestions are addressed, the PR will be ready for merging.
indexer/services/comlink/public/api-documentation.md (1)
Line range hint
1-1000: Summary of changes to the API documentation
- The Testnet base URL has been updated throughout the document.
- A new field
affiliateReferredMakerRebateshas been added to affiliate-related responses.These changes appear to be intentional and consistently applied. However, they may have the following impacts:
- Existing Testnet users will need to update their API endpoint URLs.
- Systems consuming affiliate data will need to be updated to handle the new
affiliateReferredMakerRebatesfield.To ensure these changes are properly implemented and documented, please run the following verification steps:
Please review the output of these commands to ensure all changes are correctly implemented and no unintended modifications remain.
| @@ -175,6 +175,7 @@ class AffiliatesController extends Controller { | |||
| affiliateReferredTotalVolume: Number(info.referredTotalVolume), | |||
| affiliateReferredMakerFees: Number(info.totalReferredMakerFees), | |||
| affiliateReferredTakerFees: Number(info.totalReferredTakerFees), | |||
| affiliateReferredMakerRebates: Number(info.totalReferredMakerRebates), | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing AffiliateSnapshotResponseObject type definition
The type AffiliateSnapshotResponseObject is not defined in the codebase. Please add this type definition and include the new affiliateReferredMakerRebates field to ensure type consistency.
Additionally, ensure that unit tests properly assert the affiliateReferredMakerRebates field.
🔗 Analysis chain
Verify consistency across related files
While the change looks good, please ensure that:
- Any related type definitions or interfaces (e.g.,
AffiliateSnapshotResponseObject) are updated to include this new field. - API documentation or OpenAPI/Swagger specifications are updated to reflect this change in the response structure.
- Any unit tests covering the
getSnapshotmethod are updated to include assertions for this new field.
To check for related type definitions, you can run:
To check for API documentation updates:
To check for relevant test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AffiliateSnapshotResponseObject type definition
rg --type typescript "type AffiliateSnapshotResponseObject" -A 20
Length of output: 102
Script:
#!/bin/bash
# Search for API documentation mentioning affiliate snapshots
rg --type markdown "affiliate.*snapshot" -A 10
Length of output: 6638
Script:
#!/bin/bash
# Search for test files related to AffiliatesController
fd -e test.ts affiliates-controller
Length of output: 121
Script:
#!/bin/bash
# Corrected search for AffiliateSnapshotResponseObject type definition using file extensions
rg "type AffiliateSnapshotResponseObject" --glob "*.ts" -A 20
Length of output: 63
Script:
#!/bin/bash
# Search within test files for assertions related to affiliateReferredMakerRebates
rg "affiliateReferredMakerRebates" indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts
Length of output: 193
| "affiliateReferredTakerFees": 0.1, | ||
| "affiliateReferredMakerRebates": 0.1 |
There was a problem hiding this comment.
New field affiliateReferredMakerRebates added to affiliate responses.
A new field affiliateReferredMakerRebates has been added to the AffiliateSnapshotResponseObject and is included in the example response for AffiliateSnapshotResponse.
Please ensure that:
- The purpose and meaning of this new field are clearly documented.
- Any relevant calculations or business logic related to this field are explained.
- The API versioning is updated if this change is not backward compatible.
- The change is reflected in all relevant sections of the documentation, including any schema definitions or data models.
Also applies to: 4252-4253, 4272-4272, 4294-4295
|
@Mergifyio release/indexer/v7.x |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 264cb06)
…2474) Co-authored-by: jerryfan01234 <[email protected]>
Changelist
modify response according to discussion with Fuul
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
affiliateReferredMakerRebatesin affiliate snapshots.Bug Fixes
addressFiltervalidation to clarify input requirements.Documentation
affiliateReferredMakerRebatesfield.Tests