[OTE-793] modify affiliate api stubs #2234
Conversation
WalkthroughThe changes involve a significant update to the API's Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant Service
Client->>API: GET /metadata?address={address}
API->>Controller: Call getMetadata(address)
Controller->>Service: Fetch metadata for address
Service-->>Controller: Return metadata
Controller-->>API: Return response with metadata
API-->>Client: Respond with 200 and metadata
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: 1
Outside diff range and nitpick comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (1)
6-6: Update the test case description.The change from
GET /referral_codetoGET /metadatais approved as it aligns with the updated endpoint path.However, please update the test case description to reflect the purpose of the
/metadataendpoint. For example:-it('should return referral code for a valid address string', async () => { +it('should return metadata for a valid address string', async () => {indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
Line range hint
32-40: LGTM, but replace the stub implementation with real logic.The changes to the method signature and endpoint are approved.
However, the method is currently a stub implementation that does not contain any real logic. Please replace the stub implementation with real logic that retrieves the metadata for the given address.
Do you want me to open a GitHub issue to track the task of replacing the stub implementation with real logic?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (4 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Additional comments not posted (2)
indexer/services/comlink/src/types.ts (2)
707-712: Approve addition ofaffiliateReferredTotalVolume, verify property order change.The addition of the
affiliateReferredTotalVolumeproperty to theAffiliateSnapshotResponseObjectinterface is approved as it expands the data structure to include total volume metrics related to referred affiliates.However, please verify if the change in the order of the
affiliateEarningsproperty is intentional and consistent with the order of properties across the codebase. If not, consider reverting the order change for consistency.
Line range hint
715-717: LGTM!The addition of the new interface
AffiliateTotalVolumeResponsewith thetotalVolumeproperty of typenumber | nullis approved.
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts
Show resolved
Hide resolved
jerryfan01234
force-pushed
the
affiliates-comlink-modify-stubs
branch
from
b2d0e59 to
368742b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
Line range hint
32-40: LGTM! Replace the stub with real logic.The method renaming from
getReferralCodetogetMetadataand the endpoint change from/referral_codeto/metadataare approved. These changes align with the updated specifications related to the Affiliates Indexer.However, the method implementation is currently a stub. Please replace the stub with real logic to retrieve the metadata.
Do you want me to open a GitHub issue to track the task of replacing the stub with real logic?
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 (8 hunks)
- indexer/services/comlink/public/swagger.json (3 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (4 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts
Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/src/types.ts
Additional comments not posted (9)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2)
Line range hint
106-141: LGTM!The router configuration for the
/metadataendpoint has been updated correctly to reflect the changes in thegetMetadatamethod. The error handling and timing statistics have also been adjusted to align with the new method name and endpoint.The changes are approved.
71-76: LGTM!The response structure has been updated to include a new property,
affiliateReferredTotalVolume, in theAffiliateSnapshotResponseObject. This addition enhances the data returned by the controller, providing more comprehensive insights into affiliate metrics.The changes are approved.
indexer/services/comlink/public/swagger.json (3)
274-276: LGTM!The addition of the
affiliateReferralCodeproperty to theAffiliateSnapshotResponseObjectschema is approved. It aligns with the goal of providing more comprehensive affiliate data in the API response.
297-299: LGTM!The addition of the
affiliateReferredTotalVolumeproperty to theAffiliateSnapshotResponseObjectschema is approved. It enhances the data model by including total volume metrics related to referred trades, providing more insights into affiliate performance.
1676-1678: LGTM, but verify the endpoint usage.The API endpoint and operation ID changes are approved.
However, ensure that this change is thoroughly tested to confirm that it behaves as expected and that all API calls to the affiliate metadata endpoint have been updated to use the new path.
Run the following script to verify the endpoint usage:
Verification successful
Endpoint change successfully implemented and tested.
The new endpoint
/affiliates/metadatais correctly used in the API definition, documentation, and test files, indicating that the change has been thoroughly integrated and tested.
- Files updated with new endpoint:
indexer/services/comlink/public/swagger.jsonindexer/services/comlink/public/api-documentation.mdindexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all API calls to the affiliate metadata endpoint use the new path. # Test: Search for the endpoint usage. Expect: Only occurrences of the new path. rg --type typescript -A 5 $'/affiliates/metadata'Length of output: 83
Script:
#!/bin/bash # Description: Verify all API calls to the affiliate metadata endpoint use the new path. # Test: Search for the endpoint usage without specifying a file type. rg -A 5 '/affiliates/metadata'Length of output: 2436
indexer/services/comlink/public/api-documentation.md (4)
Line range hint
371-419: LGTM!The code changes that rename the
GetReferralCodeoperation toGetMetadataand update the corresponding API endpoint and parameters look good. The changes align well with the broader shift in the API's functionality.
Line range hint
4074-4095: Looks good!The addition of the new
affiliateReferredTotalVolumeproperty and the repositioning of theaffiliateReferredNetProtocolEarningsproperty in theAffiliateSnapshotResponseObjectschema look good. The changes match the description in the summary and enhance the data returned by the API.
4110-4115: LGTM!The updates to the
affiliateListarray in theAffiliateSnapshotResponseschema look good. The changes ensure theAffiliateSnapshotResponseschema matches the updatedAffiliateSnapshotResponseObjectschema.
Line range hint
1-5000: Overall, the changes look great!The modifications made to the API documentation, including renaming operations, updating endpoints, adjusting request parameters, and modifying response schemas, are consistent and align well with the broader functionality shift. The changes enhance the data returned by the API and improve its overall structure.
Great job on this PR! The changes are approved.
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
Show resolved
Hide resolved
jerryfan01234
force-pushed
the
affiliates-comlink-modify-stubs
branch
from
368742b to
d834a24
Compare
jerryfan01234
force-pushed
the
affiliates-comlink-modify-stubs
branch
from
d834a24 to
e61c6c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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 (12 hunks)
- indexer/services/comlink/public/swagger.json (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (5 hunks)
- indexer/services/comlink/src/types.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts
Additional context used
LanguageTool
indexer/services/comlink/public/api-documentation.md
[misspelling] ~4027-~4027: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...="schemaaffiliatemetadataresponse"> <...(EN_A_VS_AN)
[misspelling] ~4028-~4028: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..."schema_AffiliateMetadataResponse"> ...(EN_A_VS_AN)
Additional comments not posted (13)
indexer/services/comlink/src/types.ts (4)
672-674: LGTM!The interface changes are approved.
690-694: LGTM!The interface changes are approved.
Line range hint
717-719: LGTM!The code is approved.
696-698: LGTM, but verify the property usage.The interface changes are approved.
However, ensure that this change is thoroughly tested to confirm that it behaves as expected and that all usages of the
addressproperty have been updated to handle the non-nullable type.Run the following script to verify the property usage:
indexer/services/comlink/public/swagger.json (3)
243-259: LGTM!The changes to the
AffiliateMetadataResponseschema definition look good. The newisVolumeEligibleandisAffiliateboolean properties have been added to provide additional metadata, and therequiredarray has been updated accordingly.
Line range hint
273-318: LGTM!The changes to the
AffiliateSnapshotResponseObjectschema definition look good. The newaffiliateReferralCodestring property andaffiliateReferredTotalVolumenumber property have been added to provide additional data points in the affiliate snapshot response, and therequiredarray has been updated accordingly.
Line range hint
1682-1706: LGTM, but verify the API usage.The changes to the
/affiliates/metadataendpoint definition look good. The endpoint path,operationId, and response schema have been updated to reflect the broader scope of retrieving affiliate metadata.However, ensure that this change is thoroughly tested to confirm that it behaves as expected and that all API calls have been updated to use the new endpoint.
Run the following script to verify the API usage:
Verification successful
API Usage Verified: Transition to
/affiliates/metadataCompleteThe new
/affiliates/metadataendpoint is correctly used across the codebase, including in theswagger.json, API documentation, and test files. There are no occurrences of the old/affiliates/referral_codeendpoint, confirming that the transition has been successfully completed.
indexer/services/comlink/public/swagger.jsonindexer/services/comlink/public/api-documentation.mdindexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all API calls use the new `/affiliates/metadata` endpoint. # Test: Search for the old and new endpoint usage. Expect: Only occurrences of the new endpoint. rg --type-add 'api:*.go' --type api -A 5 $'/affiliates/referral_code|/affiliates/metadata'Length of output: 91
Script:
#!/bin/bash # Description: Verify all API calls use the new `/affiliates/metadata` endpoint across all file types. # Test: Search for the old and new endpoint usage across all files. Expect: Only occurrences of the new endpoint. rg '/affiliates/referral_code|/affiliates/metadata' -A 5Length of output: 2464
indexer/services/comlink/public/api-documentation.md (6)
371-371: LGTM!The code segment is approved.
Line range hint
373-387: LGTM!The renaming of the operation and endpoint to
GetMetadatalooks good.
Line range hint
405-419: LGTM!The API endpoint update in the code samples looks good.
433-435: LGTM!The addition of the
isVolumeEligibleandisAffiliatefields to the response looks good.
443-443: LGTM!The response schema reference update looks good.
Line range hint
4025-4047: LGTM!The new schema definition for
AffiliateMetadataResponselooks good.Tools
LanguageTool
[misspelling] ~4027-~4027: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...="schemaaffiliatemetadataresponse"> <...(EN_A_VS_AN)
[misspelling] ~4028-~4028: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..."schema_AffiliateMetadataResponse"> ...(EN_A_VS_AN)
|
|
||
| export interface AffiliateSnapshotResponseObject { | ||
| affiliateAddress: string, | ||
| affiliateEarnings: number, | ||
| affiliateReferralCode: string, | ||
| affiliateEarnings: number, | ||
| affiliateReferredTrades: number, | ||
| affiliateTotalReferredFees: number, | ||
| affiliateReferredUsers: number, | ||
| affiliateReferredNetProtocolEarnings: number, | ||
| affiliateReferredTotalVolume: number, | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent affiliateEarnings Type Definition
The affiliateEarnings property is defined as a number in the AffiliateSnapshotResponseObject interface but is still defined as a string in several files within the postgres package. This inconsistency could lead to type mismatches and potential runtime errors.
- Files with
stringtype foraffiliateEarnings:indexer/packages/postgres/src/types/db-model-types.tsindexer/packages/postgres/src/types/affiliate-info-types.tsindexer/packages/postgres/src/models/affiliate-info-model.tsindexer/packages/postgres/__tests__/helpers/constants.ts
Please review and update these files to ensure consistency with the number type definition in the AffiliateSnapshotResponseObject interface.
Analysis chain
LGTM, but verify the property usages.
The interface changes are approved.
However, ensure that these changes are thoroughly tested to confirm that they behave as expected and that all usages of the modified properties have been updated to handle the number type.
Run the following script to verify the property usages:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all usages of the modified properties in `AffiliateSnapshotResponseObject` handle the `number` type.
# Test: Search for the interface usage. Expect: No type errors.
fd --type typescript --exec sh -c 'npx tsc --noEmit "$1"' sh {} \;
Length of output: 249
Script:
#!/bin/bash
# Description: Search for the usage of `AffiliateSnapshotResponseObject` and its properties in TypeScript files.
# Find all TypeScript files and search for the usage of `AffiliateSnapshotResponseObject`.
fd -e ts | xargs rg 'AffiliateSnapshotResponseObject'
# Search for the usage of each property to ensure they are handled as numbers.
properties=(
"affiliateEarnings"
"affiliateReferredTrades"
"affiliateTotalReferredFees"
"affiliateReferredUsers"
"affiliateReferredNetProtocolEarnings"
"affiliateReferredTotalVolume"
)
for property in "${properties[@]}"; do
echo "Searching for property: $property"
fd -e ts | xargs rg "$property"
done
Length of output: 4122
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.body).toEqual({ | ||
| referralCode: 'TempCode123', | ||
| isVolumeEligible: true, |
There was a problem hiding this comment.
Is this field needed since referral code already checks the volume eligibility before returning the code?
| isVolumeEligible: true, | ||
| isAffiliate: false, |
There was a problem hiding this comment.
What will this be used for?
There was a problem hiding this comment.
Lets sync offline, had discussion with Bill and Yi on this
Changelist
Changed api stubs according to modified design doc:
https://www.notion.so/dydx/Affiliates-Indexer-spec-78b1383d1e9a40178f021c3df006a06d#7f1568bef1834413a199588b84b1ea63
stubs enable testing for FE team
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
/affiliates/referral_codeto/affiliates/metadata, enhancing the functionality to retrieve metadata.isVolumeEligibleandisAffiliate, in the API response for more comprehensive affiliate metrics.Documentation
GetReferralCodetoGetMetadatato reflect the updated functionality.Bug Fixes
/metadataendpoint, ensuring accurate validation of API responses.