[OTE-776] Implement GetAllRevshares #2228
Conversation
tests for revshare changes
WalkthroughThis pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Poem
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 (
|
bbdd776 to
5d22979
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/x/clob/types/types.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (2 hunks)
- protocol/x/revshare/keeper/revshare_test.go (2 hunks)
- protocol/x/revshare/types/constants.go (1 hunks)
- protocol/x/revshare/types/errors.go (1 hunks)
- protocol/x/revshare/types/types.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/revshare/types/constants.go
Additional comments not posted (22)
protocol/x/revshare/types/errors.go (1)
34-38: LGTM!The new error constant
ErrTotalFeesSharedExceedsNetFeesis appropriately named, has a unique error code, and a clear error message. The code changes are approved.protocol/x/revshare/types/types.go (6)
3-7: LGTM!The imports are necessary for the new types and constants added in this file. The code changes are approved.
32-37: LGTM!The
RevSharestruct is well-defined with appropriately named and typed fields. The code changes are approved.
39-40: LGTM!The
RevShareFeeSourcetype is correctly defined as an integer. The code changes are approved.
41-45: LGTM!The
RevShareFeeSourceconstants are correctly defined using theiotakeyword and are appropriately named. The code changes are approved.
47-48: LGTM!The
RevShareTypetype is correctly defined as an integer. The code changes are approved.
49-54: LGTM!The
RevShareTypeconstants are correctly defined using theiotakeyword and are appropriately named. The code changes are approved.protocol/x/clob/types/types.go (9)
2-3: LGTM!The import is necessary for the new types and functions added in this file. The code changes are approved.
5-12: LGTM!The
FillForProcessinterface is well-defined with appropriately named and typed methods. The code changes are approved.
14-21: LGTM!The
PerpetualFillForProcessstruct is well-defined with appropriately named and typed fields. The code changes are approved.
23-25: LGTM!The
TakerAddrmethod is correctly implemented and follows theFillForProcessinterface. The code changes are approved.
27-29: LGTM!The
TakerFeeQuoteQuantumsmethod is correctly implemented and follows theFillForProcessinterface. The code changes are approved.
31-33: LGTM!The
MakerAddrmethod is correctly implemented and follows theFillForProcessinterface. The code changes are approved.
35-37: LGTM!The
MakerFeeQuoteQuantumsmethod is correctly implemented and follows theFillForProcessinterface. The code changes are approved.
39-41: LGTM!The
FillQuoteQuantumsmethod is correctly implemented and follows theFillForProcessinterface. The code changes are approved.
43-63: LGTM!The
PerpetualIdmethod and theCreatePerpetualFillForProcessfactory function are correctly implemented and follow theFillForProcessinterface. The code changes are approved.protocol/x/revshare/keeper/revshare.go (4)
150-191: LGTM!The code changes are approved. The function is well-structured, modular, and correctly aggregates revenue shares from various sources while performing the necessary checks.
193-221: LGTM!The code changes are approved. The function correctly handles the logic for calculating affiliate revenue shares.
223-245: LGTM!The code changes are approved. The function correctly handles the logic for calculating unconditional revenue shares.
247-272: LGTM!The code changes are approved. The function correctly handles the logic for calculating market mapper revenue shares.
protocol/x/revshare/keeper/revshare_test.go (2)
270-444: LGTM!The code changes are approved. The test function is comprehensive and covers various scenarios for valid revenue share retrieval, ensuring the correctness of the
GetAllRevSharesfunction.
446-583: LGTM!The code changes are approved. The test function thoroughly tests the error scenarios for revenue share retrieval, ensuring that the
GetAllRevSharesfunction correctly handles invalid configurations.
| totalFeesShared.Add(totalFeesShared, feeShared) | ||
| revShare := types.RevShare{ | ||
| Recipient: revShare.Address, | ||
| RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE, |
There was a problem hiding this comment.
On second thought, do we need this in any downstream logic, besides metrics?
There was a problem hiding this comment.
Dont we need revshare Fee source in trading rewards formula?
There was a problem hiding this comment.
And revshare type to retrieve affiliate revshares later for sending indexer events
There was a problem hiding this comment.
Dont we need revshare Fee source in trading rewards formula?
Ah yes we need so that we can separate reward logic for maker and taker fee
| name string | ||
| revenueSharePpmNetFees uint32 | ||
| revenueSharePpmTakerFees uint32 | ||
| expectedRevShares int |
There was a problem hiding this comment.
Should have more granular expected value than just # of rev shares. e.g. []types.RevShare
b922893 to
b060f38
Compare
| }, | ||
| fill: clobtypes.CreatePerpetualFillForProcess( |
There was a problem hiding this comment.
This function actually makes the code less readable, could we just replace this with a struct literal whereever it's used? For example it's hard for reader to tell what these fields are. For sensitive object like fills, we want no room for accidentally setting the wrong fields
There was a problem hiding this comment.
If we want to get rid of the function and update the struct directly, we have to rename the fields to be capitalized(same as the interface - causing errors)
two options here:
- Remove interface entirely and stick with structs
- Change the anme of underlying stuct vars(like PerpetualMarketAddr or MarketAddr_) which might be a little more confusing
| big.NewInt(10_000_000), | ||
| constants.BobAccAddress.String(), | ||
| big.NewInt(2_000_000), | ||
| big.NewInt(100000), |
There was a problem hiding this comment.
Fees are usually in the magnitude of ~0.01% of fill quote quantums, so let's use more realistic values here
| expectedNetFeeRevSharesPpmByAddress: map[string]uint32{ | ||
| constants.AliceAccAddress.String(): 400_000, // 40% of net fees | ||
| constants.BobAccAddress.String(): 200_000, // 20% of net fees | ||
| name: "Valid revenue share with 30d volume greater than max 30d referral volume", |
There was a problem hiding this comment.
Can you add a test where maker fee is negative and taker fee is positive, and check that market mapper and unconditonal rev share are based net fee
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/revshare/keeper/revshare_test.go (1)
411-411: Consider adding a test case for negative maker fee and positive taker fee.To ensure the robustness of the
GetAllRevSharesfunction, it would be beneficial to add a test case where the maker fee is negative and the taker fee is positive. This test case should verify that the market mapper and unconditional revenue shares are based on the net fee.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/clob/types/types.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (2 hunks)
- protocol/x/revshare/keeper/revshare_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/revshare/keeper/revshare.go
Additional comments not posted (3)
protocol/x/clob/types/types.go (1)
5-19: LGTM!The
FillForProcessstruct is well-defined and serves its purpose of encapsulating fill-related data. The decision to keep the struct in thetypespackage is justified based on the past review comments. The use ofProductIdinstead ofPerpetualIdaligns with the suggestion for generalization. The addition of theMonthlyRollingTakerVolumeQuantumsfield enhances the ability to analyze trading activity over time.The code changes are consistent with the AI-generated summary and address the concerns raised in the past review comments.
protocol/x/revshare/keeper/revshare_test.go (2)
279-279: Great job on the descriptive test case name!The test case name clearly conveys the scenario being tested.
591-728: Excellent test coverage for invalid scenarios!The test cases thoroughly cover scenarios where the total fees shared exceed the net fees from different sources. This helps ensure that the
GetAllRevSharesfunction correctly handles and returns the expected errors in such cases.
Changelist
Implement functions to get all revshares for all market types
Test Plan
Adds unit tests
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