-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[OTE-761] Affiliates comlink total volume and address #2254
[OTE-761] Affiliates comlink total volume and address #2254
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested labels
Poem
Tip OpenAI O1 model for chat
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (1)
167-167
: Minor optimization suggestionConsider directly returning the total volume instead of creating an object:
-return { - totalVolume: Number(walletRow.totalVolume), -}; +return Number(walletRow.totalVolume);This will slightly optimize the method by avoiding the creation of an unnecessary object.
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 (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2 hunks)
Additional comments not posted (7)
indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts (5)
141-144
: LGTM!Setting up test data in the
beforeEach
hook is a good practice to ensure that each test case has a consistent starting state. Creating a default subaccount username is necessary for testing the/address
endpoint.
146-148
: LGTM!Clearing the test data in the
afterEach
hook is a good practice to ensure that each test case starts with a clean slate and is not affected by the previous test cases.
150-161
: LGTM!The test case is well-structured and tests the happy path scenario. Using
testConstants.defaultSubaccountUsername.username
as the referral code is a good practice to ensure that the test case is not hardcoded and can be easily updated if the test data changes.
163-169
: LGTM!The test case is well-structured and tests the error scenario when the referral code is not found. The test case ensures that the API returns the appropriate error status code when the referral code is not found.
205-237
: LGTM!Setting up test data in the
beforeEach
hook and clearing it in theafterEach
hook is a good practice to ensure that each test case has a consistent starting state and is not affected by the previous test cases.The test cases are well-structured and test both the happy path and error scenarios. Using
testConstants.defaultWallet.address
as the address is a good practice to ensure that the test case is not hardcoded and can be easily updated if the test data changes.indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts (2)
96-113
: LGTM!The
getAddress
method has been updated to replace the stub implementation with actual logic to retrieve the address based on the provided referral code. It correctly handles the case when the referral code doesn't exist by throwing aNotFoundError
. It also handles the unexpected case when the subaccount is not found, although this should never happen due to the foreign key constraint between thesubaccounts
andsubaccount_usernames
tables.The method achieves the goal outlined in the PR objectives.
158-167
: LGTM!The
getTotalVolume
method has been updated to replace the stub implementation with actual logic to retrieve the total volume based on the provided address. It correctly handles the case when the wallet doesn't exist by throwing aNotFoundError
.The method achieves the goal outlined in the PR objectives.
Changelist
replace total_volume and address endpoint stubs with real logic
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit