-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add gRPC gateway routing for affiliate queries #2554
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (5)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.lcd.ts (2)
6-16
: Add validation for requestClient in constructorConsider adding validation to ensure requestClient is properly initialized before assigning it.
constructor({ requestClient }: { requestClient: LCDClient; }) { + if (!requestClient) { + throw new Error('requestClient is required'); + } this.req = requestClient; this.affiliateInfo = this.affiliateInfo.bind(this); this.referredBy = this.referredBy.bind(this); this.allAffiliateTiers = this.allAffiliateTiers.bind(this); this.affiliateWhitelist = this.affiliateWhitelist.bind(this); }
1-46
: Improve code organization and maintainabilityConsider the following improvements:
- Extract the base URL path into a constant
- Add consistent error handling across all methods
- Fix inconsistent empty line spacing between methods
import { LCDClient } from "@osmonauts/lcd"; import { AffiliateInfoRequest, AffiliateInfoResponseSDKType, ReferredByRequest, ReferredByResponseSDKType, AllAffiliateTiersRequest, AllAffiliateTiersResponseSDKType, AffiliateWhitelistRequest, AffiliateWhitelistResponseSDKType } from "./query"; + +const BASE_PATH = 'dydxprotocol/affiliates'; + +interface QueryError extends Error { + status?: number; + statusText?: string; +} + export class LCDQueryClient { // ... rest of the code ... }proto/dydxprotocol/affiliates/query.proto (2)
24-28
: LGTM: Well-structured HTTP GET endpoint for AllAffiliateTiers.The endpoint path follows RESTful conventions for a collection resource.
Consider consistent line wrapping for the RPC definition:
- rpc AllAffiliateTiers(AllAffiliateTiersRequest) - returns (AllAffiliateTiersResponse) { + rpc AllAffiliateTiers(AllAffiliateTiersRequest) returns (AllAffiliateTiersResponse) {
30-34
: LGTM: Well-structured HTTP GET endpoint for AffiliateWhitelist.The endpoint path follows RESTful conventions for a collection resource.
Consider consistent line wrapping for the RPC definition:
- rpc AffiliateWhitelist(AffiliateWhitelistRequest) - returns (AffiliateWhitelistResponse) { + rpc AffiliateWhitelist(AffiliateWhitelistRequest) returns (AffiliateWhitelistResponse) {protocol/x/affiliates/module.go (1)
81-86
: Consider using clientCtx's context instead of background context.While the implementation correctly registers the gRPC gateway routes for affiliate queries, it could be improved by using the context from
clientCtx
instead ofcontext.Background()
. This would ensure proper context propagation and cancellation handling.func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) { - err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)) + err := types.RegisterQueryHandlerClient(clientCtx.Context(), mux, types.NewQueryClient(clientCtx)) if err != nil { panic(err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
protocol/x/affiliates/types/query.pb.go
is excluded by!**/*.pb.go
protocol/x/affiliates/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
📒 Files selected for processing (5)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.lcd.ts
(1 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/lcd.ts
(1 hunks)proto/dydxprotocol/affiliates/query.proto
(1 hunks)protocol/x/affiliates/module.go
(1 hunks)
🔇 Additional comments (13)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.lcd.ts (1)
34-44
: Endpoints match PR test plan
The implemented endpoints for allAffiliateTiers
and affiliateWhitelist
match exactly with the endpoints mentioned in the PR test plan.
✅ Verification successful
Endpoints correctly match proto definitions
The endpoints in the TypeScript code exactly match the gRPC service definitions in the proto file:
/dydxprotocol/affiliates/all_affiliate_tiers
forAllAffiliateTiers
/dydxprotocol/affiliates/affiliate_whitelist
forAffiliateWhitelist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the endpoints are properly defined in the proto file
rg -A 2 'get: "/dydxprotocol/affiliates/(all_affiliate_tiers|affiliate_whitelist)"' --type proto
Length of output: 128
Script:
#!/bin/bash
# Search for proto files first to understand the file extension
fd -e proto
# Then search for the endpoints in all files since the extension might be different
rg -A 2 'get: "/dydxprotocol/affiliates/(all_affiliate_tiers|affiliate_whitelist)"'
Length of output: 5396
Script:
#!/bin/bash
# Let's check the affiliates query proto file specifically since we found it in the list
cat proto/dydxprotocol/affiliates/query.proto
Length of output: 3717
indexer/packages/v4-protos/src/codegen/dydxprotocol/lcd.ts (1)
15-17
: LGTM! The implementation follows the established pattern.
The addition of the affiliates
query client is consistent with other services and properly integrates with the LCD client architecture.
Let's verify the existence of the imported module and its interface:
✅ Verification successful
✓ The affiliates LCD query client is properly implemented and available
The verification confirms that:
- The
affiliates/query.lcd.ts
module exists and follows the standard LCD client pattern - The
LCDQueryClient
class is properly implemented with:- Constructor accepting a
requestClient
- Four query methods:
affiliateInfo
,referredBy
,allAffiliateTiers
, andaffiliateWhitelist
- Proper method binding in constructor
- Correct endpoint paths and return types
- Constructor accepting a
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of the affiliates LCD query module
# Test 1: Check if the affiliates query.lcd.ts file exists
fd -t f "query.lcd.ts" -p "affiliates"
# Test 2: Verify the LCDQueryClient class implementation
ast-grep --pattern 'export class LCDQueryClient {
$$$
constructor($_) {
$$$
}
$$$
}' "affiliates/query.lcd.ts"
Length of output: 328
Script:
#!/bin/bash
# Let's try searching from the repository root with proper paths
# Test 1: Find the affiliates query.lcd.ts file in the codebase
fd -t f "query.lcd.ts"
# Test 2: Search for imports of affiliates/query.lcd
rg -l "from.*affiliates/query\.lcd"
# Test 3: Check the directory structure under v4-protos
ls -R indexer/packages/v4-protos/src/codegen/dydxprotocol/
Length of output: 6515
Script:
#!/bin/bash
# Let's verify the content of the affiliates query.lcd.ts file
cat indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.lcd.ts
# Also check the LCDQueryClient implementation pattern
ast-grep --pattern 'export class LCDQueryClient {
$$$
}' indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.lcd.ts
Length of output: 7360
proto/dydxprotocol/affiliates/query.proto (4)
7-7
: LGTM: Required import for gRPC-Gateway annotations.
The addition of google/api/annotations.proto
import is correct and necessary for HTTP endpoint annotations.
14-17
: LGTM: Well-structured HTTP GET endpoint for AffiliateInfo.
The endpoint path follows RESTful conventions and correctly maps the address parameter.
19-22
: LGTM: Well-structured HTTP GET endpoint for ReferredBy.
The endpoint path follows RESTful conventions and correctly maps the address parameter.
14-34
: Verify testing coverage for all endpoints.
The PR description mentions testing of two endpoints:
- ✓
/dydxprotocol/affiliates/all_affiliate_tiers
- ✓
/dydxprotocol/affiliates/affiliate_whitelist
Please ensure the following endpoints are also tested:
/dydxprotocol/affiliates/affiliate_info/{address}
/dydxprotocol/affiliates/referred_by/{address}
✅ Verification successful
Let's also check the implementation files to understand the testing scope:
And check for any test files in the same directory as the proto file:
Let's also check the affiliate info table tests since they might contain relevant endpoint testing:
All endpoints have comprehensive test coverage
Based on the test files analysis, all four endpoints have proper test coverage:
-
/dydxprotocol/affiliates/affiliate_info/{address}
:- Comprehensive unit tests in
grpc_query_test.go
covering success cases, non-existent addresses, invalid addresses, and whitelisted affiliates.
- Comprehensive unit tests in
-
/dydxprotocol/affiliates/referred_by/{address}
:- Unit tests in
grpc_query_test.go
covering both successful referral lookups and non-registered affiliate cases.
- Unit tests in
-
/dydxprotocol/affiliates/all_affiliate_tiers
:- Unit tests in
grpc_query_test.go
verifying the retrieval of default affiliate tiers.
- Unit tests in
-
/dydxprotocol/affiliates/affiliate_whitelist
:- Unit tests in
grpc_query_test.go
confirming proper whitelist retrieval.
- Unit tests in
Additionally, there's complementary test coverage in the indexer's postgres stores for affiliate info and referred users, providing end-to-end validation of the affiliate system's data persistence and retrieval.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any existing tests for these endpoints
# Look for test files related to these endpoints
fd -e go -e ts -e js -e py "test|spec" | xargs rg -l "affiliate_info|referred_by"
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search to find any test files related to affiliates
fd -e go -e ts -e js -e py "test|spec" | xargs rg -l "affiliate"
# Also search for the specific endpoint paths
rg -l "/dydxprotocol/affiliates/"
Length of output: 1606
Script:
#!/bin/bash
# Search for implementation files related to these endpoints
rg -l "AffiliateInfo|ReferredBy" --type proto --type go --type ts --type js
Length of output: 107
Script:
#!/bin/bash
# Find test files in the proto directory and its subdirectories
fd -e go -e ts -e js -e py "test|spec" proto/
Length of output: 47
Script:
#!/bin/bash
# Let's check the content of the grpc query test file to see which endpoints are tested
rg -A 5 "func Test.*Affiliate.*Query" protocol/x/affiliates/keeper/grpc_query_test.go
# Also check the content of the API controller test
rg -A 5 "describe.*affiliate" indexer/services/comlink/__tests__/controllers/api/v4/affiliates-controller.test.ts
Length of output: 337
Script:
#!/bin/bash
rg -A 5 "describe|it\(" indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts
rg -A 5 "describe|it\(" indexer/packages/postgres/__tests__/stores/affiliate-referred-users-table.test.ts
Length of output: 5358
Script:
#!/bin/bash
# Let's check the specific content of the grpc query test file
cat protocol/x/affiliates/keeper/grpc_query_test.go
Length of output: 7739
Script:
#!/bin/bash
# Also check for any integration or e2e tests that might cover these endpoints
rg -l "affiliate.*info.*address|referred.*by" protocol/x/affiliates/e2e/
rg -A 5 "TestAffiliate" protocol/x/affiliates/e2e/register_affiliate_test.go
Length of output: 151
protocol/x/affiliates/module.go (1)
81-86
: Implementation aligns with PR objectives.
The gRPC gateway route registration correctly enables the HTTP endpoints mentioned in the test plan:
/dydxprotocol/affiliates/all_affiliate_tiers
/dydxprotocol/affiliates/affiliate_whitelist
Let's verify the query handler registration:
✅ Verification successful
gRPC gateway routes are properly implemented and aligned with requirements
The verification confirms that:
- Both
AllAffiliateTiers
andAffiliateWhitelist
query handlers are correctly implemented in the keeper - The protobuf service definitions and message types are properly generated
- The gRPC gateway registration in
module.go
will correctly expose these endpoints for HTTP/REST access
The implementation matches the PR objectives and will enable the test plan endpoints as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the query handler implementation exists and matches the routes.
# Test: Check if the query service is properly defined
ast-grep --pattern 'service Query {
$$$
rpc AllAffiliateTiers($$$) returns ($$$) {
$$$
}
rpc AffiliateWhitelist($$$) returns ($$$) {
$$$
}
$$$
}'
# Test: Check if the HTTP rules are properly defined
rg -A 2 'option \(google\.api\.http\)' protocol/x/affiliates/
Length of output: 231
Script:
#!/bin/bash
# Let's try a different approach to verify the query service and routes
# Check for query service definition in proto files
fd '\.proto$' protocol/x/affiliates/ --exec cat {}
# Check for query handler implementation
rg -l "QueryServer" protocol/x/affiliates/
# Look for HTTP/REST endpoint definitions
rg -l "google.api.http" protocol/x/affiliates/
Length of output: 302
Script:
#!/bin/bash
# Let's examine the specific query handler implementations and routes
# Check the query handler implementation
rg -A 5 "AllAffiliateTiers|AffiliateWhitelist" protocol/x/affiliates/keeper/grpc_query.go
# Check the generated protobuf definitions
rg -A 5 "AllAffiliateTiers|AffiliateWhitelist" protocol/x/affiliates/types/query.pb.go
# Check the proto file directly for route definitions
fd 'query\.proto$' protocol/x/affiliates/ --exec cat {}
Length of output: 15621
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (6)
126-143
: LGTM: LCD query client imports are properly structured
The new LCD query client imports are well-organized and follow a consistent pattern, including the new affiliates module (_137
) which aligns with the PR's objective of implementing affiliate query routing.
144-164
: LGTM: RPC query client imports maintain consistency
The RPC query client imports follow the same organizational pattern as LCD clients, ensuring consistency across the codebase. The addition of affiliates/query.rpc.Query
at line 156 properly complements the LCD query implementation.
165-182
: LGTM: RPC message client imports are complete
The RPC message client imports include all necessary modules, with affiliates/tx.rpc.msg
at line 177 ensuring full support for affiliate-related transactions.
183-185
: LGTM: Utility imports are properly included
The addition of LCD, RPC query, and RPC transaction utility imports provides the necessary infrastructure for the gRPC gateway implementation.
406-408
: LGTM: ClientFactory exports are properly configured
The ClientFactory exports include all necessary components (lcd
, rpc.query
, and rpc.tx
) required for the gRPC gateway functionality.
201-203
: Verify affiliate module integration
The affiliates module export includes the new LCD and RPC clients. Let's verify the integration is complete.
✅ Verification successful
Let me check the actual integration of the affiliate module in these files to ensure completeness.
Affiliate module integration is complete and properly implemented
The verification shows that the affiliate module is fully integrated across all necessary client interfaces:
- LCD Query Client in
lcd.ts
- RPC Query Client in
rpc.query.ts
- RPC Transaction Client in
rpc.tx.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify affiliate module integration
# Test: Check for any missed affiliate-related files
rg -g '!bundle.ts' -l "affiliates.*\.(lcd|rpc)" ./indexer/packages/v4-protos/src/codegen/dydxprotocol/
Length of output: 296
Script:
#!/bin/bash
# Check LCD client integration
echo "=== LCD Client Integration ==="
rg -A 5 "affiliates" ./indexer/packages/v4-protos/src/codegen/dydxprotocol/lcd.ts
echo -e "\n=== RPC Query Integration ==="
rg -A 5 "affiliates" ./indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.query.ts
echo -e "\n=== RPC Transaction Integration ==="
rg -A 5 "affiliates" ./indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts
Length of output: 1719
async allAffiliateTiers(_params: AllAffiliateTiersRequest = {}): Promise<AllAffiliateTiersResponseSDKType> { | ||
const endpoint = `dydxprotocol/affiliates/all_affiliate_tiers`; | ||
return await this.req.get<AllAffiliateTiersResponseSDKType>(endpoint); | ||
} |
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.
🛠️ Refactor suggestion
Add error handling for list queries
The allAffiliateTiers and affiliateWhitelist methods should include error handling.
async allAffiliateTiers(_params: AllAffiliateTiersRequest = {}): Promise<AllAffiliateTiersResponseSDKType> {
const endpoint = `dydxprotocol/affiliates/all_affiliate_tiers`;
- return await this.req.get<AllAffiliateTiersResponseSDKType>(endpoint);
+ try {
+ return await this.req.get<AllAffiliateTiersResponseSDKType>(endpoint);
+ } catch (error) {
+ throw new Error(`Failed to fetch affiliate tiers: ${error.message}`);
+ }
}
async affiliateWhitelist(_params: AffiliateWhitelistRequest = {}): Promise<AffiliateWhitelistResponseSDKType> {
const endpoint = `dydxprotocol/affiliates/affiliate_whitelist`;
- return await this.req.get<AffiliateWhitelistResponseSDKType>(endpoint);
+ try {
+ return await this.req.get<AffiliateWhitelistResponseSDKType>(endpoint);
+ } catch (error) {
+ throw new Error(`Failed to fetch affiliate whitelist: ${error.message}`);
+ }
}
Also applies to: 41-44
async affiliateInfo(params: AffiliateInfoRequest): Promise<AffiliateInfoResponseSDKType> { | ||
const endpoint = `dydxprotocol/affiliates/affiliate_info/${params.address}`; | ||
return await this.req.get<AffiliateInfoResponseSDKType>(endpoint); | ||
} |
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.
🛠️ Refactor suggestion
Add input validation and error handling for affiliateInfo
The method should validate the address parameter and handle potential request failures.
async affiliateInfo(params: AffiliateInfoRequest): Promise<AffiliateInfoResponseSDKType> {
+ if (!params.address || !params.address.trim()) {
+ throw new Error('Address is required');
+ }
const endpoint = `dydxprotocol/affiliates/affiliate_info/${params.address}`;
- return await this.req.get<AffiliateInfoResponseSDKType>(endpoint);
+ try {
+ return await this.req.get<AffiliateInfoResponseSDKType>(endpoint);
+ } catch (error) {
+ throw new Error(`Failed to fetch affiliate info: ${error.message}`);
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
async affiliateInfo(params: AffiliateInfoRequest): Promise<AffiliateInfoResponseSDKType> { | |
const endpoint = `dydxprotocol/affiliates/affiliate_info/${params.address}`; | |
return await this.req.get<AffiliateInfoResponseSDKType>(endpoint); | |
} | |
async affiliateInfo(params: AffiliateInfoRequest): Promise<AffiliateInfoResponseSDKType> { | |
if (!params.address || !params.address.trim()) { | |
throw new Error('Address is required'); | |
} | |
const endpoint = `dydxprotocol/affiliates/affiliate_info/${params.address}`; | |
try { | |
return await this.req.get<AffiliateInfoResponseSDKType>(endpoint); | |
} catch (error) { | |
throw new Error(`Failed to fetch affiliate info: ${error.message}`); | |
} | |
} |
async referredBy(params: ReferredByRequest): Promise<ReferredByResponseSDKType> { | ||
const endpoint = `dydxprotocol/affiliates/referred_by/${params.address}`; | ||
return await this.req.get<ReferredByResponseSDKType>(endpoint); | ||
} |
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.
🛠️ Refactor suggestion
Add input validation and error handling for referredBy
Similar to affiliateInfo, this method needs proper validation and error handling.
async referredBy(params: ReferredByRequest): Promise<ReferredByResponseSDKType> {
+ if (!params.address || !params.address.trim()) {
+ throw new Error('Address is required');
+ }
const endpoint = `dydxprotocol/affiliates/referred_by/${params.address}`;
- return await this.req.get<ReferredByResponseSDKType>(endpoint);
+ try {
+ return await this.req.get<ReferredByResponseSDKType>(endpoint);
+ } catch (error) {
+ throw new Error(`Failed to fetch referral info: ${error.message}`);
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
async referredBy(params: ReferredByRequest): Promise<ReferredByResponseSDKType> { | |
const endpoint = `dydxprotocol/affiliates/referred_by/${params.address}`; | |
return await this.req.get<ReferredByResponseSDKType>(endpoint); | |
} | |
async referredBy(params: ReferredByRequest): Promise<ReferredByResponseSDKType> { | |
if (!params.address || !params.address.trim()) { | |
throw new Error('Address is required'); | |
} | |
const endpoint = `dydxprotocol/affiliates/referred_by/${params.address}`; | |
try { | |
return await this.req.get<ReferredByResponseSDKType>(endpoint); | |
} catch (error) { | |
throw new Error(`Failed to fetch referral info: ${error.message}`); | |
} | |
} |
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 9d3e3cb)
Co-authored-by: Teddy Ding <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
Tested by visiting http://localhost:1317/dydxprotocol/affiliates/all_affiliate_tiers and http://localhost:1317/dydxprotocol/affiliates/affiliate_whitelist
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
New Features
LCDQueryClient
for querying affiliate-related information.affiliateInfo
,referredBy
,allAffiliateTiers
, andaffiliateWhitelist
.createLCDClient
to include anaffiliates
property.Improvements
query.proto
file to allow HTTP GET requests for affiliate-related RPC methods.Bug Fixes
bundle.ts
file to ensure proper module referencing.