[OTE-692] Implement message server for affiliates #2216
Conversation
WalkthroughThe changes implement actual logic for the Changes
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: 0
Outside diff range, codebase verification and nitpick comments (2)
protocol/x/affiliates/keeper/msg_server.go (1)
28-34: Secure and efficient function for updating affiliate tiers.The
UpdateAffiliateTiersfunction properly checks for the caller's authority and handles errors effectively. It updates the affiliate tiers correctly and returns a response object upon success.Suggestion for Improvement:
Consider enhancing the error message in line 30 to provide more context about why the authority is invalid, which could help in debugging and user feedback.protocol/x/affiliates/keeper/msg_server_test.go (1)
31-94: Comprehensive test function for registering affiliates.The
TestMsgServer_RegisterAffiliatefunction is well-structured and covers a variety of scenarios, including valid registrations and various error conditions. The use of setup functions and assertions is appropriate and effective.Suggestion for Improvement:
Consider adding more scenarios if there are other edge cases not covered by the current test cases.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/affiliates/keeper/msg_server.go (2 hunks)
- protocol/x/affiliates/keeper/msg_server_test.go (2 hunks)
Additional comments not posted (2)
protocol/x/affiliates/keeper/msg_server.go (1)
18-23: Well-implemented function for registering affiliates.The
RegisterAffiliatefunction correctly unwraps the SDK context, handles errors appropriately, and returns a response object upon successful registration.protocol/x/affiliates/keeper/msg_server_test.go (1)
96-134: Effective test function for updating affiliate tiers.The
TestMsgServer_UpdateAffiliateTiersfunction correctly tests the authority checks and handles both valid and invalid update scenarios. The assertions are used effectively to ensure the expected outcomes.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/affiliates/keeper/msg_server_test.go (1)
96-134: Well-structured test cases forUpdateAffiliateTierswith a suggestion for improvement.The test function
TestMsgServer_UpdateAffiliateTierseffectively distinguishes between valid and invalid updates, ensuring system integrity. The use of constants and structured error checking enhances readability and reliability.Consider explicitly defining the expected error type in the test case for "non-gov module updates tiers" to improve clarity and ensure that the correct error is being tested.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/affiliates/keeper/msg_server_test.go (2 hunks)
Additional comments not posted (1)
protocol/x/affiliates/keeper/msg_server_test.go (1)
31-94: Well-structured and comprehensive test cases forRegisterAffiliate.The test function
TestMsgServer_RegisterAffiliateis well-organized using a table-driven approach, which is a best practice in Go testing. Each test case is clearly defined with expected errors and setup functions, ensuring comprehensive coverage and maintainability.
affanv14
force-pushed
the
affan/affiliates-msg-server
branch
from
2ab35ec to
b1ffd0c
Compare
affanv14
force-pushed
the
affan/affiliates-msg-server
branch
from
2b33570 to
f1ef02c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
protocol/x/affiliates/keeper/msg_server.go (2)
19-24: LGTM, but consider logging the error.The code changes are approved.
However, consider logging the error before returning it to the caller for better observability.
Apply this diff to log the error:
func (k msgServer) RegisterAffiliate(ctx context.Context, msg *types.MsgRegisterAffiliate) (*types.MsgRegisterAffiliateResponse, error) { sdkCtx := sdk.UnwrapSDKContext(ctx) err := k.Keeper.RegisterAffiliate(sdkCtx, msg.Referee, msg.Affiliate) if err != nil { + k.Logger(sdkCtx).Error("failed to register affiliate", "error", err) return nil, err } return &types.MsgRegisterAffiliateResponse{}, nil }
Line range hint
29-49: LGTM, but consider logging the errors and adding a comment.The code changes are approved.
However, consider the following improvements:
- Log the errors before returning them to the caller for better observability.
- Add a comment to explain the purpose of the revenue share safety validation.
Apply this diff to log the errors and add a comment:
func (k msgServer) UpdateAffiliateTiers(ctx context.Context, msg *types.MsgUpdateAffiliateTiers) (*types.MsgUpdateAffiliateTiersResponse, error) { if !k.Keeper.HasAuthority(msg.Authority) { + err := errors.New("invalid authority") + k.Logger(sdkCtx).Error("failed to update affiliate tiers", "error", err) return nil, errors.New("invalid authority") } sdkCtx := sdk.UnwrapSDKContext(ctx) unconditionalRevShareConfig, err := k.revShareKeeper.GetUnconditionalRevShareConfigParams(sdkCtx) if err != nil { + k.Logger(sdkCtx).Error("failed to get unconditional rev share config params", "error", err) return nil, err } marketMapperRevShareParams := k.revShareKeeper.GetMarketMapperRevenueShareParams(sdkCtx) + // Validate the revenue share safety to ensure that the affiliate tiers do not exceed the maximum allowed revenue share. if !k.revShareKeeper.ValidateRevShareSafety(msg.Tiers, unconditionalRevShareConfig, marketMapperRevShareParams) { + err := errorsmod.Wrapf( + types.ErrRevShareSafetyViolation, + "rev share safety violation", + ) + k.Logger(sdkCtx).Error("failed to update affiliate tiers", "error", err) return nil, errorsmod.Wrapf( types.ErrRevShareSafetyViolation, "rev share safety violation", ) } k.Keeper.UpdateAffiliateTiers(sdkCtx, msg.Tiers) return &types.MsgUpdateAffiliateTiersResponse{}, nil }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/affiliates/keeper/msg_server.go (3 hunks)
- protocol/x/affiliates/keeper/msg_server_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/affiliates/keeper/msg_server_test.go
Changelist
Implements msgserver for affililiates
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