[OTE-760] implement comlink affiliate metadata endpoint #2243
Conversation
|
Warning Rate limit exceeded@jerryfan01234 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 36 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes implemented in this pull request enhance the functionality of the Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
indexer/services/comlink/src/config.ts (1)
63-65: LGTM! Consider adding a comment to clarify the purpose of the threshold.The addition of the
VOLUME_ELIGIBILITY_THRESHOLDparameter to the configuration looks good. It allows for flexibility in adjusting the threshold value without modifying the code, which is a good practice.To improve the clarity of the code, consider adding a comment to explain the purpose and unit of the threshold value. For example:
// The minimum volume required for an affiliate to be eligible for rewards (in USD). VOLUME_ELIGIBILITY_THRESHOLD: parseInteger({ default: 10_000 }),Also, were there any specific considerations for choosing the default value of 10,000? It would be helpful to understand the rationale behind this choice.
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/config.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (3 hunks)
Additional comments not posted (7)
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (7)
33-46: LGTM!The test case is correctly implemented and checks the expected response. It also uses the
beforeEachandafterEachhooks to set up and tear down the test data.
48-55: LGTM!The test case is correctly implemented and checks the expected 404 response for a non-existent wallet address.
57-75: LGTM!The test case is correctly implemented and checks the expected response with
isVolumeEligibleset tofalsewhen the wallet has a total volume of 0. It also updates the wallet data to set up the test scenario.
77-95: LGTM!The test case is correctly implemented and checks the expected response with
isVolumeEligibleset totruewhen the wallet has a total volume of 100000. It also updates the wallet data to set up the test scenario.
97-109: LGTM!The test case is correctly implemented and checks the expected response with
isAffiliateset tofalsewhen theAffiliateReferredUsersTableis empty. It relies on the table being empty to set up the test scenario.
111-127: LGTM!The test case is correctly implemented and checks the expected response with
isAffiliateset totruewhen theAffiliateReferredUsersTablehas an entry for the wallet address. It creates the entry to set up the test scenario.
129-137: LGTM!The test case is correctly implemented and checks the expected 500 response when the wallet does not have a subaccount username. It creates the wallet without a subaccount username to set up the test scenario.
| @Query() address: string, | ||
| ): Promise<AffiliateMetadataResponse> { | ||
| // simulate a delay | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| // Check that the address exists | ||
| const walletRow = await WalletTable.findById(address); | ||
| if (!walletRow) { | ||
| throw new NotFoundError(`Wallet with address ${address} not found`); | ||
| } | ||
| const isVolumeEligible = Number(walletRow.totalVolume) >= config.VOLUME_ELIGIBILITY_THRESHOLD; | ||
|
|
||
| // Check if the address is an affiliate (has referred users) | ||
| const referredUserRows = await AffiliateReferredUsersTable.findByAffiliateAddress(address); | ||
| const isAffiliate = referredUserRows !== undefined ? referredUserRows.length > 0 : false; | ||
|
|
||
| // Get referral code (subaccount 0 username) | ||
| const subaccountRows = await SubaccountTable.findAll( | ||
| { | ||
| address, | ||
| subaccountNumber: 0, | ||
| }, | ||
| [], | ||
| ); | ||
| // No need to check subaccountRows.length > 1 as subaccountNumber is unique for an address | ||
| if (subaccountRows.length === 0) { | ||
| throw new UnexpectedServerError(`Subaccount 0 not found for address ${address}`); | ||
| } | ||
| const subaccountId = subaccountRows[0].id; | ||
|
|
||
| const usernameRows = await SubaccountUsernamesTable.findAll( | ||
| { | ||
| subaccountId: [subaccountId], | ||
| }, | ||
| [], | ||
| ); | ||
| // No need to check usernameRows.length > 1 as subAccountId is unique (foreign key constraint) | ||
| if (usernameRows.length === 0) { | ||
| throw new UnexpectedServerError(`Username not found for subaccount ${subaccountId}`); | ||
| } | ||
| const referralCode = usernameRows[0].username; | ||
|
|
||
| return { | ||
| referralCode: 'TempCode123', | ||
| isVolumeEligible: true, | ||
| isAffiliate: false, | ||
| referralCode, | ||
| isVolumeEligible, | ||
| isAffiliate, | ||
| }; |
There was a problem hiding this comment.
LGTM with suggestions!
The getMetadata method correctly retrieves the metadata for a given address. The logic for checking wallet existence, volume eligibility, and affiliate status is implemented correctly. The referral code is also retrieved correctly from the subaccount username. The error handling is appropriate and informative.
However, I have a few suggestions for improvement:
- Consider optimizing the database queries by using joins or subqueries to reduce the number of round trips to the database.
- Consider breaking down the method into smaller, reusable functions to improve modularity and readability.
- Consider extracting the database queries into a separate data access layer to improve separation of concerns.
- Consider performing input validation and sanitization on the
addressparameter to prevent potential SQL injection attacks.
| } | ||
| const subaccountId = subaccountRows[0].id; | ||
|
|
||
| const usernameRows = await SubaccountUsernamesTable.findAll( |
There was a problem hiding this comment.
instead of doing multiple awaits, can we do promise.All instead?
There was a problem hiding this comment.
Im not too famliar with ts, but the output of these functions is processed (for errors) and then used in subsequent functions. Would this be suitable for promise.all? My understanding is that promise.all should not be used for such cases?
There was a problem hiding this comment.
Linked the wrong line
but cant we combine
const walletRow = await WalletTable.findById(address);
const referredUserRows = await AffiliateReferredUsersTable.findByAffiliateAddress(address);
const subaccountRows = await SubaccountTable.findAll(
{
address,
subaccountNumber: 0,
},
[],
);
into one promise.all?
since all of these are independent and make db queries
| @@ -60,6 +60,9 @@ export const configSchema = { | |||
| // vaults table is added. | |||
| EXPERIMENT_VAULTS: parseString({ default: '' }), | |||
| EXPERIMENT_VAULT_MARKETS: parseString({ default: '' }), | |||
|
|
|||
| // Affiliates config | |||
| VOLUME_ELIGIBILITY_THRESHOLD: parseInteger({ default: 10_000 }), | |||
There was a problem hiding this comment.
Can you verify volume is stored in dollars and not quote quantums?
There was a problem hiding this comment.
I believe the quantum conversion is made before writing to fills db
fill_amount = dydx_trim_scale(dydx_from_jsonlib_long(event_data->'fillAmount') *
power(10, perpetual_market_record."atomicResolution")::numeric);
maker_price = dydx_trim_scale(dydx_from_jsonlib_long(maker_order->'subticks') *
power(10, perpetual_market_record."quantumConversionExponent" +
asset_record."atomicResolution" -
perpetual_market_record."atomicResolution")::numeric);
This screenshot also suggests it is converted
| if (usernameRows.length === 0) { | ||
| throw new UnexpectedServerError(`Username not found for subaccount ${subaccountId}`); | ||
| } | ||
| const referralCode = usernameRows[0].username; |
There was a problem hiding this comment.
Do you mean logging to logs eg. in logging.info OR logging as metric?
There was a problem hiding this comment.
Maybe for a future PR... but I meant sending this up to dd
21481ec to
a495b25
Compare
a495b25 to
7d36faa
Compare
| const subaccountRows = await SubaccountTable.findAll( | ||
| { | ||
| address, | ||
| subaccountNumber: 0, | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Do we want to add this too to the above promise.all statement?
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/tests/controllers/api/v4/affiliates-controller.test.ts
Additional comments not posted (2)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2)
48-84: Error handling looks good!The error handling in the
getMetadatamethod is appropriate and informative. The method throws specific errors with clear error messages in the following scenarios:
NotFoundErrorif the wallet is not found.UnexpectedServerErrorif subaccount 0 is not found for the address.UnexpectedServerErrorif the username is not found for the subaccount.This helps in identifying and debugging issues effectively.
87-91: LGTM!The return statement in the
getMetadatamethod correctly provides the required metadata information. ThereferralCode,isVolumeEligible, andisAffiliatevalues are dynamically retrieved based on the database queries and conditions, replacing the previous hardcoded placeholders. This enhances the functionality of the method and provides accurate metadata for the given address.
| const [walletRow, referredUserRows] = await Promise.all([ | ||
| WalletTable.findById(address), | ||
| AffiliateReferredUsersTable.findByAffiliateAddress(address), | ||
| ]); | ||
|
|
||
| // Check that the address exists | ||
| if (!walletRow) { | ||
| throw new NotFoundError(`Wallet with address ${address} not found`); | ||
| } | ||
|
|
||
| // Check if the address is an affiliate (has referred users) | ||
| const isVolumeEligible = Number(walletRow.totalVolume) >= config.VOLUME_ELIGIBILITY_THRESHOLD; | ||
| const isAffiliate = referredUserRows !== undefined ? referredUserRows.length > 0 : false; | ||
|
|
||
| // Get referral code (subaccount 0 username) | ||
| const subaccountRows = await SubaccountTable.findAll( | ||
| { | ||
| address, | ||
| subaccountNumber: 0, | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Optimize database queries by using Promise.all.
The getMetadata method currently performs multiple independent database queries sequentially. To improve performance, consider combining the following queries using Promise.all:
const walletRow = await WalletTable.findById(address);
const referredUserRows = await AffiliateReferredUsersTable.findByAffiliateAddress(address);
const subaccountRows = await SubaccountTable.findAll(
{
address,
subaccountNumber: 0,
},
[],
);By executing these queries in parallel using Promise.all, you can reduce the overall execution time of the method.
Apply this diff to combine the queries:
- const walletRow = await WalletTable.findById(address);
- const referredUserRows = await AffiliateReferredUsersTable.findByAffiliateAddress(address);
+ const [walletRow, referredUserRows, subaccountRows] = await Promise.all([
+ WalletTable.findById(address),
+ AffiliateReferredUsersTable.findByAffiliateAddress(address),
+ SubaccountTable.findAll(
+ {
+ address,
+ subaccountNumber: 0,
+ },
+ [],
+ ),
+ ]);
- const subaccountRows = await SubaccountTable.findAll(
- {
- address,
- subaccountNumber: 0,
- },
- [],
- );
Changelist
replace /affiliates/metadata stub with real logic
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
Bug Fixes
Tests