Update GetAllRevshare to handle liquidations #2413
Conversation
WalkthroughThe pull request introduces enhancements to the revenue sharing logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
protocol/x/revshare/keeper/revshare.go (1)
178-182: Consider Adding Unit Tests for Zero Net Fees ScenarioWhile the new condition appropriately handles the zero
netFeescase, it's important to include unit tests to ensure this scenario is covered and to prevent future regressions.Would you like assistance in writing unit tests for this case or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- protocol/x/revshare/keeper/revshare.go (1 hunks)
- protocol/x/revshare/keeper/revshare_test.go (1 hunks)
🔇 Additional comments (1)
protocol/x/revshare/keeper/revshare.go (1)
178-182: Proper Handling of Zero Net Fees during LiquidationsThe added code correctly handles scenarios where
netFeesis zero, such as during liquidations when both maker and taker fees are zero. By returning an emptyRevSharesForFill, the function avoids unnecessary calculations and potential errors when there's no revenue to share.
| { | ||
| name: "Valid revenue share from affiliates, unconditional and market mapper", |
There was a problem hiding this comment.
Duplicate Test Case Name: Rename the test case to reflect zero fees scenario
The test case at lines 729-770 has the same name as a previous test case: "Valid revenue share from affiliates, unconditional and market mapper". To avoid confusion and improve clarity, consider renaming the test case to reflect that it's testing the scenario where both maker and taker fees are zero.
Apply this diff to rename the test case:
- name: "Valid revenue share from affiliates, unconditional and market mapper",
+ name: "Valid revenue share with zero maker and taker fees",📝 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.
| { | |
| name: "Valid revenue share from affiliates, unconditional and market mapper", | |
| { | |
| name: "Valid revenue share with zero maker and taker fees", |
affanv14
force-pushed
the
affan/liquidation-fix
branch
from
42ab021 to
9565562
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
protocol/x/affiliates/abci.go (2)
14-14: Approve the simplified error logging, with a minor suggestion.The change from using
runtime/debug.Stack()tolog.ErrorLogWithErroris a good improvement. It simplifies the error logging and likely reduces log verbosity while still capturing essential information.Consider adding more context to the error message to aid in debugging:
- log.ErrorLogWithError(ctx, "error aggregating affiliate volume for fills", err) + log.ErrorLogWithError(ctx, "EndBlocker: error aggregating affiliate volume for fills", err)This minor change would make it easier to trace the origin of the error in logs.
14-14: Consider the trade-offs of simplified error logging.While the simplified error logging is generally an improvement, it's worth considering the following points:
- Performance: This change might slightly improve performance by reducing the overhead of capturing full stack traces.
- Debugging: There's a trade-off between reduced log verbosity and potentially losing some debug information. Ensure that the
log.ErrorLogWithErrorfunction captures enough context to facilitate effective debugging, especially for issues related to the liquidation process mentioned in the PR objectives.- Consistency: If this is a new logging approach, consider applying it consistently across the codebase for uniform error handling.
Could you provide more information about the
log.ErrorLogWithErrorfunction? Specifically, what context it captures and how it differs from the previous logging mechanism? This would help in assessing whether any critical information is being lost in this transition.
| @@ -467,7 +467,7 @@ func (k Keeper) persistMatchedOrders( | |||
| revSharesForFill, err := k.revshareKeeper.GetAllRevShares(ctx, fillForProcess, affiliatesWhitelistMap) | |||
| if err != nil { | |||
| revSharesForFill = revsharetypes.RevSharesForFill{} | |||
| log.ErrorLog(ctx, "error getting rev shares for fill", "error", err) | |||
| log.ErrorLogWithError(ctx, "error getting rev shares for fill", err) | |||
There was a problem hiding this comment.
Handle error from GetAllRevShares appropriately instead of proceeding
When k.revshareKeeper.GetAllRevShares returns an error, the current implementation logs the error but continues execution with an empty revSharesForFill. This may lead to incorrect fee distributions since subsequent calls, such as DistributeFees, rely on revSharesForFill to accurately represent revenue shares.
It's important to handle this error by returning it to ensure the system does not proceed in an invalid state.
Apply this diff to return the error:
if err != nil {
revSharesForFill = revsharetypes.RevSharesForFill{}
- log.ErrorLogWithError(ctx, "error getting rev shares for fill", err)
+ return takerUpdateResult, makerUpdateResult, affiliateRevSharesQuoteQuantums, err
}📝 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.
| log.ErrorLogWithError(ctx, "error getting rev shares for fill", err) | |
| if err != nil { | |
| revSharesForFill = revsharetypes.RevSharesForFill{} | |
| return takerUpdateResult, makerUpdateResult, affiliateRevSharesQuoteQuantums, err | |
| } |
| if netFees.Sign() == 0 { | ||
| return types.RevSharesForFill{}, nil | ||
| } | ||
|
|
||
| affiliateRevShares, affiliateFeesShared, err := k.getAffiliateRevShares(ctx, fill, affiliatesWhitelistMap) | ||
| if err != nil { | ||
| return types.RevSharesForFill{}, err |
There was a problem hiding this comment.
Comment on this line below: error message affiliate fees shared exceeds net fees" is not accurate since we return error when netFeesSubAffiliateFeesShared = 0 as well. Should be sth like net fees minus affiliate fee share is not larger than zero
| @@ -175,6 +175,11 @@ func (k Keeper) GetAllRevShares( | |||
| makerFees := fill.MakerFeeQuoteQuantums | |||
| netFees := big.NewInt(0).Add(takerFees, makerFees) | |||
|
|
|||
| // net fees is 0 in case of liquidations where maker and taker fees are 0 | |||
There was a problem hiding this comment.
This comment is not general enough. Net fees can be zero if takerFee > 0 and makerFee < 0. It should just say "when net fee is zero, no rev share is generated from the fill'
| @@ -726,6 +726,48 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) { | |||
| affiliatesKeeper *affiliateskeeper.Keeper) { | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
From thread, could we also add a unit test with positive maker fees and zero taker fee?
| @@ -238,7 +238,8 @@ func (k Keeper) getAffiliateRevShares( | |||
| ) ([]types.RevShare, *big.Int, error) { | |||
| takerAddr := fill.TakerAddr | |||
| takerFee := fill.TakerFeeQuoteQuantums | |||
| if fill.MonthlyRollingTakerVolumeQuantums >= types.MaxReferee30dVolumeForAffiliateShareQuantums { | |||
| if fill.MonthlyRollingTakerVolumeQuantums >= types.MaxReferee30dVolumeForAffiliateShareQuantums || | |||
There was a problem hiding this comment.
Better to stay consistent with the 0 net fee use case here instead of return a 0 fee revshare
protocol/x/revshare/types/errors.go
Outdated
| ModuleName, | ||
| 7, | ||
| "affiliate fees shared exceeds net fees", | ||
| "net fees minus affiliate fee share is not larger than zero", |
There was a problem hiding this comment.
Nit: var name and err message is saying the same thing but differently:
AffiliateFee >= netFee
netFee - AffiliateFee <= 0
Let's make them consistent for readability
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 829b68b)
Changelist
Failure error logs when going through liquidations since maker and taker fees are 0. This handles that case
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