Create new instance for fee calculation#3051
Conversation
WalkthroughAvoid in-place mutation of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant K as Revshare Keeper
participant AFF as Affiliate calc
participant OR as Order-router config
participant LOG as Logger
participant C as Caller
rect rgba(200,230,255,0.18)
note over K: GetAllRevShares start — use copy of netFees
K->>K: netFeesSub := new(big.Int).Set(netFees)
end
alt Affiliate present
K->>AFF: compute affiliateFeesShared
AFF-->>K: affiliateFeesShared
K->>K: netFeesSub -= affiliateFeesShared
else No affiliate
note right of K: no affiliate subtraction
end
rect rgba(220,255,220,0.12)
K->>OR: compute order-router rev shares (taker/maker)
OR-->>K: routerShares
K->>K: netFeesSub -= routerShares
K->>K: aggregate unconditional & maker shares
end
K->>K: totalFeesShared := sum(all shares)
alt totalFeesShared > netFees
K->>LOG: error(totalFeesShared, netFees, RevShares)
K-->>C: ErrTotalFeesSharedExceedsNetFees
else OK
K-->>C: RevSharesForFill
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
protocol/x/revshare/keeper/revshare.go (3)
343-346: Prevent negative taker ORRS when maker rebate > taker fee.takerFeesSide = min(taker, taker+maker) can become negative when the maker rebate exceeds the taker fee, yielding a negative QuoteQuantums for the taker’s ORRS. That inflates the “remainder” (subtracting a negative), misallocates unconditional/MM shares, and can mask the final invariant check.
Clamp the base at zero and keep BigMulPpm argument order consistent with other sites.
- // This is taker ppm * min(taker, taker - maker_rebate) - takerFeesSide := lib.BigMin(takerFees, new(big.Int).Add(takerFees, makerFees)) - takerRevShare := lib.BigMulPpm(lib.BigU(takerOrderRouterRevSharePpm), takerFeesSide, false) + // taker ppm * max(0, min(taker, taker - maker_rebate)) + takerFeesSide := lib.BigMax( + lib.BigI(0), + lib.BigMin(takerFees, new(big.Int).Add(takerFees, makerFees)), + ) + takerRevShare := lib.BigMulPpm(takerFeesSide, lib.BigU(takerOrderRouterRevSharePpm), false)I recommend adding/expanding tests where maker rebate > taker fee to verify taker ORRS is zero, not negative. I can draft them if helpful.
178-181: Unsigned cast of possibly negative fees corrupts safety check.Casting int32 fees to uint64 before addition converts negatives into huge positives, breaking ValidateRevShareSafety for maker rebates (negative maker fee). Use signed big.Int addition.
- bigNetFee := new(big.Int).SetUint64( - // Casting is safe since both variables are int32. - uint64(lowestTakerFeePpm) + uint64(lowestMakerFeePpm), - ) + bigNetFee := new(big.Int).Add( + lib.BigI(lowestTakerFeePpm), + lib.BigI(lowestMakerFeePpm), + )
141-149: Ensure safe handling of uninitializedUnconditionalRevShareConfig
GetUnconditionalRevShareConfigParamscurrently callsMustUnmarshalon the bytes returned from the KV store without checking for a nil slice. Because this module does not perform any explicit initialization ofUnconditionalRevShareConfigin itsInitGenesis, default genesis, or upgrade handlers—and only tests invokeSetUnconditionalRevShareConfigParams—there exists a real risk of a panic when the key is unset on chain startup or after an upgrade.Please apply the following mandatory guard:
• File:
protocol/x/revshare/keeper/revshare.go
• Lines: 141–149func (k Keeper) GetUnconditionalRevShareConfigParams(ctx sdk.Context) (types.UnconditionalRevShareConfig, error) { store := ctx.KVStore(k.storeKey) unconditionalRevShareConfigBytes := store.Get( []byte(types.UnconditionalRevShareConfigKey), ) var unconditionalRevShareConfig types.UnconditionalRevShareConfig - k.cdc.MustUnmarshal(unconditionalRevShareConfigBytes, &unconditionalRevShareConfig) + if unconditionalRevShareConfigBytes == nil { + // Return empty config if not yet set to avoid panics on MustUnmarshal + return unconditionalRevShareConfig, nil + } + k.cdc.MustUnmarshal(unconditionalRevShareConfigBytes, &unconditionalRevShareConfig) return unconditionalRevShareConfig, nil }Applying this change will ensure the keeper safely returns a zero-value config instead of panicking when the store key is absent.
♻️ Duplicate comments (1)
protocol/x/revshare/keeper/revshare.go (1)
233-234: Subtract affiliate fees from the net-fee copy (matches earlier suggestion).This resolves the prior nit about subtracting into the local copy rather than mutating netFees.
🧹 Nitpick comments (4)
protocol/x/revshare/keeper/revshare.go (4)
236-238: Misleading error when ORRS consume the remainder — split the check or use a neutral error.The current error type ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees fires even in the ORRS-only path (when there’s no affiliate share) if ORRS fully consume the remainder. That’s confusing for ops/debugging.
Consider branching by path (affiliate vs ORRS) or using a neutral error for “non-positive remainder.”
Apply one of the following minimal diffs:
Option A (branch-specific checks):
- if netFeesSubRevenueShare.Sign() <= 0 { - return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees - } + if len(affiliateRevShares) == 0 { + if netFeesSubRevenueShare.Sign() < 0 { + return types.RevSharesForFill{}, types.ErrTotalFeesSharedExceedsNetFees + } + } else { + if netFeesSubRevenueShare.Sign() <= 0 { + return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees + } + }Option B (neutral check; allow exact-zero remainder to proceed with zero unconditional/MM):
- if netFeesSubRevenueShare.Sign() <= 0 { - return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees - } + if netFeesSubRevenueShare.Sign() < 0 { + return types.RevSharesForFill{}, types.ErrTotalFeesSharedExceedsNetFees + }
273-279: Improve logging: structured keys, more context.Use structured keys without spaces/colons and include identifiers (marketId, taker/maker) to aid incident triage.
- k.Logger(ctx).Error( - "Total fees exceed net fees. Total fees: ", totalFeesShared, - "Net fees: ", netFees, - "Revshares generated: ", revShares) + k.Logger(ctx).Error( + "total fees exceed net fees", + "total_fees_shared", totalFeesShared.String(), + "net_fees", netFees.String(), + "market_id", fill.MarketId, + "taker_addr", fill.TakerAddr, + "maker_addr", fill.MakerAddr, + "revshares_generated", revShares, + )
240-245: Naming consistency: parameters still say “SubAffiliateFeesShared” but now also reflect ORRS.Now that ORRS also reduce the base, the parameter names are misleading. Renaming improves readability without behavior changes.
- func (k Keeper) getUnconditionalRevShares( + func (k Keeper) getUnconditionalRevShares( ctx sdk.Context, - netFeesSubAffiliateFeesShared *big.Int, + netProtocolRevenueBase *big.Int, ) ([]types.RevShare, error) { @@ - feeShared := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(revShare.SharePpm), false) + feeShared := lib.BigMulPpm(netProtocolRevenueBase, lib.BigU(revShare.SharePpm), false)- func (k Keeper) getMarketMapperRevShare( + func (k Keeper) getMarketMapperRevShare( ctx sdk.Context, marketId uint32, - netFeesSubAffiliateFeesShared *big.Int, + netProtocolRevenueBase *big.Int, ) ([]types.RevShare, error) { @@ - marketMapperRevshareAmount := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(revenueSharePpm), false) + marketMapperRevshareAmount := lib.BigMulPpm(netProtocolRevenueBase, lib.BigU(revenueSharePpm), false)- unconditionalRevShares, err := k.getUnconditionalRevShares(ctx, netFeesSubRevenueShare) + unconditionalRevShares, err := k.getUnconditionalRevShares(ctx, netFeesSubRevenueShare) @@ - marketMapperRevShares, err := k.getMarketMapperRevShare(ctx, fill.MarketId, netFeesSubRevenueShare) + marketMapperRevShares, err := k.getMarketMapperRevShare(ctx, fill.MarketId, netFeesSubRevenueShare)Also applies to: 385-406, 408-432
332-353: Optional: unify BigMulPpm argument order for readability.Elsewhere you consistently pass (amount, ppm). The taker branch currently passes (ppm, amount). While mathematically equivalent, consistent ordering reduces cognitive load.
The change in the “negative taker ORRS” fix above already standardizes it; no further action needed if you adopt that diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
protocol/x/revshare/keeper/revshare.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/revshare/keeper/revshare.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/revshare/keeper/revshare.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
protocol/x/revshare/keeper/revshare.go (1)
223-223: Use a fresh big.Int to avoid aliasing/mutation — correct fix.Cloning netFees into netFeesSubRevenueShare prevents in-place mutation by subsequent Sub calls. This was the right move to keep the original netFees intact for later validation.
|
@Mergifyio backport release/indexer/v9.x |
|
@Mergifyio backport release/protocol/v9.x |
✅ Backports have been createdDetails
|
✅ Backports have been createdDetails
|
(cherry picked from commit 143470d)
(cherry picked from commit 143470d)
Co-authored-by: Justin Barnett <[email protected]>
Co-authored-by: Justin Barnett <[email protected]>
2 participants
Changelist
Test Plan
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
Bug Fixes
Tests