[OTE-780] Add client code for affiliates #2252
Conversation
WalkthroughThe changes in this pull request enhance the affiliate module of the protocol by introducing a new field Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 4
Outside diff range and nitpick comments (1)
protocol/x/affiliates/genesis.go (1)
16-23: Enhance the error handling.The changes to retrieve all affiliate tiers and include them in the exported genesis state are correct and provide a more complete representation of the module's state.
However, consider improving the error handling by returning the error instead of panicking. This allows the caller to handle the error gracefully.
Apply this diff to improve the error handling:
affiliateTiers, err := k.GetAllAffiliateTiers(ctx) if err != nil { - panic(err) + return nil, err } return &types.GenesisState{ AffiliateTiers: affiliateTiers, -} +}, nilAlso, update the function signature to return an error:
-func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState { +func ExportGenesis(ctx sdk.Context, k keeper.Keeper) (*types.GenesisState, error) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/affiliates/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- proto/dydxprotocol/affiliates/genesis.proto (1 hunks)
- protocol/x/affiliates/client/cli/query.go (2 hunks)
- protocol/x/affiliates/client/cli/query_test.go (1 hunks)
- protocol/x/affiliates/client/cli/tx.go (2 hunks)
- protocol/x/affiliates/genesis.go (1 hunks)
- protocol/x/affiliates/keeper/grpc_query.go (1 hunks)
- protocol/x/affiliates/types/genesis.go (1 hunks)
Additional context used
buf
proto/dydxprotocol/affiliates/genesis.proto
3-3: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
Additional comments not posted (10)
protocol/x/affiliates/types/genesis.go (1)
5-7: LGTM!The change to initialize the
AffiliateTiersfield with an emptyAffiliateTiersinstance in the default genesis state is a good improvement. It ensures that the field is explicitly initialized, which may be crucial for the correct functioning of the application that relies on this genesis state.The overall logic of the function remains intact, but the initialization now provides a more complete and potentially safer default state.
proto/dydxprotocol/affiliates/genesis.proto (1)
10-13: LGTM!The addition of the
affiliate_tiersfield to theGenesisStatemessage is a valuable enhancement to thex/affiliatesmodule. It allows for storing and managing different levels of affiliates within the protocol, which is crucial for the module's functionality.The use of the
gogoproto.nullableoption to mark the field as non-nullable is also a good practice to ensure data integrity.protocol/x/affiliates/genesis.go (1)
11-11: LGTM!The change to call
k.UpdateAffiliateTiers(ctx, genState.AffiliateTiers)is correct and ensures that the affiliate tiers are properly initialized during genesis.protocol/x/affiliates/client/cli/tx.go (1)
27-46: LGTM!The
CmdRegisterAffiliatefunction is well-implemented and follows the expected patterns for creating a Cobra command in the Cosmos SDK. Here are the key observations:
- The command is clearly defined with the appropriate usage, description, and argument requirements.
- The transaction handling logic is correct, including retrieving the client context, constructing the
MsgRegisterAffiliatemessage, and generating or broadcasting the transaction.- The necessary transaction flags are added to the command using
AddTxFlagsToCmd, allowing for transaction customization.- The command is properly integrated into the parent command.
Overall, the code changes are sound and align with the module's functionality for registering affiliates.
protocol/x/affiliates/client/cli/query_test.go (2)
20-41: LGTM!The
setupNetworkfunction correctly initializes the genesis state with default values and adds test affiliate tiers. It returns the network and client context, which can be used in the test functions.
43-52: LGTM!The
TestQueryAffiliateTiersfunction correctly tests theGetCmdQueryAffiliateTiersCLI command. It sets up the network, executes the command, unmarshals the response, and asserts that the returned affiliate tiers match the default tiers.protocol/x/affiliates/client/cli/query.go (4)
24-30: LGTM!The function is correctly adding the new commands to the
cmdvariable and returning the updatedcmdvariable.
32-50: LGTM!The function is correctly implementing the logic to query all affiliate tiers and print the response.
52-73: LGTM!The function is correctly implementing the logic to query affiliate info for a given address and print the response.
75-94: LGTM!The function is correctly implementing the logic to query the referee that referred the given address and print the response.
| @@ -1,7 +1,13 @@ | |||
| syntax = "proto3"; | |||
| package dydxprotocol.affiliates; | |||
| import "gogoproto/gogo.proto"; | |||
There was a problem hiding this comment.
Fix the missing import for "gogoproto/gogo.proto".
The static analysis tool buf reports a missing import for "gogoproto/gogo.proto". This issue needs to be resolved to ensure successful compilation of the code.
Please check the following:
- Ensure that the
gogoprotolibrary is properly installed and configured in your project's build system. - Verify that the import path is correct and the "gogo.proto" file exists in the specified location.
If you need further assistance, I'd be happy to help you resolve this issue. Let me know if you'd like me to investigate further or provide more specific guidance.
Tools
buf
3-3: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
| "affiliate not found for address: %s", | ||
| req.GetAddress(), | ||
| ) | ||
| return &types.ReferredByResponse{}, nil |
There was a problem hiding this comment.
Tip
Codebase Verification
Based on the analysis of the test cases, it appears that the tests for the ReferredBy function have not been updated to reflect the new behavior of returning an empty response instead of an error when no affiliate is found. This inconsistency could lead to potential issues and false negative test results.
To address this, we should recommend updating the test cases to align with the new implementation. Here's the final response:
Update test cases for ReferredBy function to expect empty response
The test cases for the ReferredBy function in protocol/x/affiliates/keeper/grpc_query_test.go need to be updated to reflect the new behavior of returning an empty response instead of an error when no affiliate is found. Specifically, the "Affiliate not found" test case should be modified as follows:
- Remove the
expectError: types.ErrAffiliateNotFoundline. - Update the
expectedfield to be an emptyReferredByResponseinstead ofnil. - Modify the assertion to check for an empty response rather than an error.
This update will ensure that the tests accurately reflect the new implementation and prevent potential false negative results.
Analysis chain
Verify the impact of returning an empty response on the calling code.
The change simplifies the control flow by removing the error handling when an affiliate is not found and returns an empty ReferredByResponse instead. This may impact how the calling code interprets the absence of an affiliate and may lead to different handling of the response in the broader application context.
Please verify that the calling code and the broader application can handle the empty response correctly. You can use the following script to search for the usage of the ReferredBy function and analyze the impact of the change:
To complete the verification process, we need to check if the test cases for the ReferredBy function have been updated to expect an empty response instead of an error when no affiliate is found. Let's examine the relevant test file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of returning an empty response on the calling code.
# Test: Search for the function usage. Expect: Analyze how the response is handled.
rg --type go -A 10 $'ReferredBy\('
Length of output: 18042
Script:
#!/bin/bash
# Description: Check if test cases for ReferredBy have been updated
# Test: Examine the test cases for ReferredBy function
cat protocol/x/affiliates/keeper/grpc_query_test.go
Length of output: 5723
| func TestQueryAffiliateInfo(t *testing.T) { | ||
| net, ctx := setupNetwork(t) | ||
|
|
||
| testAddress := constants.AliceAccAddress.String() | ||
| out, err := clitestutil.ExecTestCLICmd(ctx, cli.GetCmdQueryAffiliateInfo(), []string{testAddress}) | ||
| require.NoError(t, err) | ||
|
|
||
| var resp types.AffiliateInfoResponse | ||
| require.NoError(t, net.Config.Codec.UnmarshalJSON(out.Bytes(), &resp)) | ||
| } |
There was a problem hiding this comment.
Add assertions for the returned affiliate info.
The TestQueryAffiliateInfo function correctly sets up the network and executes the GetCmdQueryAffiliateInfo CLI command with a test address. It also unmarshals the response into an AffiliateInfoResponse struct.
However, it does not assert anything about the returned affiliate info. Consider adding assertions to check that the returned affiliate info matches the expected values for the test address.
| func TestQueryReferredBy(t *testing.T) { | ||
| net, ctx := setupNetwork(t) | ||
|
|
||
| testAddress := constants.AliceAccAddress.String() | ||
| out, err := clitestutil.ExecTestCLICmd(ctx, cli.GetCmdQueryReferredBy(), []string{testAddress}) | ||
| require.NoError(t, err) | ||
|
|
||
| var resp types.ReferredByResponse | ||
| require.NoError(t, net.Config.Codec.UnmarshalJSON(out.Bytes(), &resp)) | ||
| } |
There was a problem hiding this comment.
Add assertions for the returned referred-by info.
The TestQueryReferredBy function correctly sets up the network and executes the GetCmdQueryReferredBy CLI command with a test address. It also unmarshals the response into a ReferredByResponse struct.
However, it does not assert anything about the returned referred-by info. Consider adding assertions to check that the returned referred-by info matches the expected values for the test address.
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)
- protocol/x/affiliates/keeper/grpc_query_test.go (1 hunks)
- protocol/x/revshare/keeper/revshare_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/revshare/keeper/revshare_test.go
| @@ -148,7 +148,7 @@ func TestReferredBy(t *testing.T) { | |||
| }, | |||
| setup: func(ctx sdk.Context, k keeper.Keeper) {}, | |||
| expected: nil, | |||
| expectError: types.ErrAffiliateNotFound, | |||
| expectError: nil, | |||
There was a problem hiding this comment.
Inconsistency between test case name and expected behavior.
The test case "Affiliate not found" implies that the test is checking the scenario where an affiliate is not found. However, setting expectError to nil suggests that the test is no longer expecting an error in this case.
This inconsistency could lead to confusion and potentially mask any issues related to handling the "Affiliate not found" scenario.
Consider either:
- Updating the test case name to reflect the new expected behavior, or
- Reverting the change and setting
expectErrortotypes.ErrAffiliateNotFoundto align with the test case name and the scenario being tested.
For example:
-"Affiliate not found": {
+"Affiliate not registered": {
req: &types.ReferredByRequest{
Address: constants.DaveAccAddress.String(),
},
setup: func(ctx sdk.Context, k keeper.Keeper) {},
expected: nil,
expectError: nil,
},or
"Affiliate not found": {
req: &types.ReferredByRequest{
Address: constants.DaveAccAddress.String(),
},
setup: func(ctx sdk.Context, k keeper.Keeper) {},
expected: nil,
- expectError: nil,
+ expectError: types.ErrAffiliateNotFound,
},Committable suggestion was skipped due to low confidence.
affanv14
force-pushed
the
affan/affiliates-client
branch
from
c6c69e4
to
05c4163
Compare
Changelist
Adds client code for queries and transactions
Test Plan
Adds unit tests for queries
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
Chores