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
🧹 Nitpick comments (3)
.github/workflows/indexer-build-and-push-dev-staging.yml (3)
9-9: Confirm intent: feature branch pushes will also build/push STAGING images.Adding
revshares_loggingunderon.push.branchestriggers all four jobs (dev, dev2, dev4, and staging) on each push to that branch. If your staging ECR images are auto-deployed, this could promote feature-branch builds into staging unintentionally. If staging images should only come frommainorrelease/*, gate the staging job with a branch condition.Proposed job-level guard (outside the changed line range):
jobs: call-build-and-push-ecs-services-staging: if: github.ref_name == 'main' || startsWith(github.ref_name, 'release/indexer/') name: (Staging) Build and Push ECS Services uses: ./.github/workflows/indexer-build-and-push-all-ecr-images.yml with: ENVIRONMENT: staging secrets: inherit
9-9: Consistent quoting style.Other branch patterns use single quotes. Consider matching for consistency.
- - "revshares_logging" + - 'revshares_logging'
3-11: Optional: add manual trigger to avoid keeping a per-feature branch in triggers.If this branch inclusion is temporary to exercise builds, consider removing it and adding
workflow_dispatchso you can manually run the workflow from the branch without modifying triggers.Proposed event block (outside the changed line range):
on: push: branches: - main - 'release/indexer/v[0-9]+.[0-9]+.x' - 'release/indexer/v[0-9]+.x' workflow_dispatch: inputs: environment: description: 'Target environment (dev|dev2|dev4|staging)' required: false default: 'dev'Then guard jobs accordingly, e.g.,
if: inputs.environment == 'staging'or map it viaenv.
📜 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)
.github/workflows/indexer-build-and-push-dev-staging.yml(1 hunks)
⏰ 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). (61)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: benchmark
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: container-tests
- GitHub Check: test-coverage-upload
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: golangci-lint
- GitHub Check: build
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: Summary
- GitHub Check: build-and-push-snapshot-dev4
- GitHub Check: build-and-push-staging
- GitHub Check: build-and-push-snapshot-staging
- GitHub Check: build-and-push-dev4
- GitHub Check: build-and-push-snapshot-dev
- GitHub Check: build-and-push-dev
- GitHub Check: build-and-push-snapshot-dev2
- GitHub Check: build-and-push-dev2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protocol/x/clob/keeper/process_single_match.go (1)
539-546: Use lower-volume or debug‐level logging for revSharesForFill on the hot pathLogging the entire
revSharesForFillobject atInfoon every fill can flood node logs and impact I/O. Instead, align with existing helpers and either:• Summarize at
Infowith just key counts and flags usinglog.InfoLog
• Or move this to debug using the availableDebugLoghelperAffected location:
protocol/x/clob/keeper/process_single_match.go:543Suggested diff for summarized Info‐level logging:
- k.Logger(ctx).Info("rev shares for fill", "revSharesForFill", revSharesForFill) + log.InfoLog( + ctx, + "rev shares for fill", + "height", ctx.BlockHeight(), + "productId", perpetualId, + "taker", matchWithOrders.TakerOrder.GetSubaccountId().Owner, + "maker", matchWithOrders.MakerOrder.GetSubaccountId().Owner, + "allRevShares", len(revSharesForFill.AllRevShares), + "affiliateSet", revSharesForFill.AffiliateRevShare != nil, + )Or, if you don’t need Info volume at all, switch to debug:
- k.Logger(ctx).Info("rev shares for fill", "revSharesForFill", revSharesForFill) + log.DebugLog( + ctx, + "rev shares for fill", + "allRevShares", len(revSharesForFill.AllRevShares), + "affiliateSet", revSharesForFill.AffiliateRevShare != nil, + )Optionally, you can sample the Info log (e.g. once every N blocks) if periodic high-volume insights are still desirable:
if ctx.BlockHeight()%100 == 0 { log.InfoLog(ctx, "sampled rev shares for fill", "height", ctx.BlockHeight(), "allRevShares", len(revSharesForFill.AllRevShares), "affiliateSet", revSharesForFill.AffiliateRevShare != nil, ) }
📜 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/clob/keeper/process_single_match.go(1 hunks)
⏰ 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). (43)
- GitHub Check: test-coverage-upload
- GitHub Check: benchmark
- GitHub Check: golangci-lint
- GitHub Check: container-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: Summary
- GitHub Check: build-and-push-snapshot-dev
- GitHub Check: build-and-push-snapshot-staging
- GitHub Check: build-and-push-snapshot-dev2
- GitHub Check: build-and-push-snapshot-dev4
- GitHub Check: build-and-push-dev4
- GitHub Check: build-and-push-dev
- GitHub Check: build-and-push-staging
- GitHub Check: build-and-push-dev2
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 (1)
protocol/x/revshare/keeper/revshare.go (1)
178-190: Critical: Fix incorrect uint64 cast in revshare safety checkThe net‐fee comparison in
ValidateRevshareSafetycurrently uses an unsigned cast that overflows iflowestMakerFeePpmis negative, allowing unsafe configurations to pass the check. Replace theSetUint64approach with signedbig.Intaddition.• Location
protocol/x/revshare/keeper/revshare.go, lines 178–190• Proposed change
- bigNetFee := new(big.Int).SetUint64( - // Casting is safe since both variables are int32. - uint64(lowestTakerFeePpm) + uint64(lowestMakerFeePpm), - ) + // Use signed big.Int math; maker fee can be negative (rebate). + bigNetFee := new(big.Int).Add( + lib.BigI(lowestTakerFeePpm), + lib.BigI(lowestMakerFeePpm), + )• Update the preceding comment—remove “casting is safe” and note that both fees are handled as signed integers.
• Scan result
- A quick
rgscan acrossprotocol/shows no other instances ofuint64(...)on fee variables; this fix is isolated to the revshare safety check.
🧹 Nitpick comments (10)
protocol/x/revshare/keeper/revshare.go (5)
236-238: Return a generic error when remaining net fees ≤ 0 (not affiliate-specific).If remaining net fees become non-positive due to ORRS subtraction (the no-affiliate path), returning
ErrAffiliateFeesSharedGreaterThanOrEqualToNetFeesis misleading. Prefer a generic error (or reuseErrTotalFeesSharedExceedsNetFees) so callers/logs aren’t confused about the cause.Apply one of these diffs:
Option A (reuse existing error):
- if netFeesSubRevenueShare.Sign() <= 0 { - return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees - } + if netFeesSubRevenueShare.Sign() <= 0 { + return types.RevSharesForFill{}, types.ErrTotalFeesSharedExceedsNetFees + }Option B (introduce a new error, if you prefer finer granularity):
- if netFeesSubRevenueShare.Sign() <= 0 { - return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedGreaterThanOrEqualToNetFees - } + if netFeesSubRevenueShare.Sign() <= 0 { + return types.RevSharesForFill{}, types.ErrRemainingNetFeesNonPositive + }(You’d add
ErrRemainingNetFeesNonPositiveinprotocol/x/revshare/types/errors.go.)
273-279: Tighten structured logging: use canonical keys and avoid message-embedded labels.Cosmos logger expects a message plus key/value pairs. Put labels into keys; stringify big.Ints to avoid ambiguous output.
- 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(), + "revshares", revShares, + )
345-346: Minor: makeBigMulPpmargument order consistent with other call sites.Everywhere else you do
BigMulPpm(amount, ppm). Here it’s reversed; not wrong, just inconsistent.- takerRevShare := lib.BigMulPpm(lib.BigU(takerOrderRouterRevSharePpm), takerFeesSide, false) + takerRevShare := lib.BigMulPpm(takerFeesSide, lib.BigU(takerOrderRouterRevSharePpm), false)
142-149: Guard against missing unconditional config to avoid MustUnmarshal panic.If this key isn’t set in genesis/migrations,
MustUnmarshal(nil, &unconditionalRevShareConfig)could panic. Returning an empty config when missing is safer.func (k Keeper) GetUnconditionalRevShareConfigParams(ctx sdk.Context) (types.UnconditionalRevShareConfig, error) { store := ctx.KVStore(k.storeKey) - unconditionalRevShareConfigBytes := store.Get( - []byte(types.UnconditionalRevShareConfigKey), - ) + unconditionalRevShareConfigBytes := store.Get([]byte(types.UnconditionalRevShareConfigKey)) var unconditionalRevShareConfig types.UnconditionalRevShareConfig - k.cdc.MustUnmarshal(unconditionalRevShareConfigBytes, &unconditionalRevShareConfig) + if unconditionalRevShareConfigBytes == nil { + return types.UnconditionalRevShareConfig{}, nil + } + k.cdc.MustUnmarshal(unconditionalRevShareConfigBytes, &unconditionalRevShareConfig) return unconditionalRevShareConfig, nil }
394-404: Rename shadowed variable inside loop for clarity.The loop variable
revShareis re-declared as a struct in the body, shadowing the iteration var and harming readability.- for _, revShare := range unconditionalRevShareConfig.Configs { - feeShared := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(revShare.SharePpm), false) - revShare := types.RevShare{ - Recipient: revShare.Address, + for _, cfg := range unconditionalRevShareConfig.Configs { + feeShared := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(cfg.SharePpm), false) + revShare := types.RevShare{ + Recipient: cfg.Address, RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE, RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL, QuoteQuantums: feeShared, - RevSharePpm: revShare.SharePpm, + RevSharePpm: cfg.SharePpm, } revShares = append(revShares, revShare) }protocol/x/revshare/keeper/revshare_test.go (5)
674-679: Fix comment: 100_000 PPM is 10%, not 20%.The PPM comment for Carl’s unconditional share is incorrect.
- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%
707-716: Fix comments: 500_000 PPM is 50% (not 20%).Minor doc accuracy in setup block.
- SharePpm: 500_000, // 20% + SharePpm: 500_000, // 50%
724-732: Rename test: maker fee is positive and there’s no maker ORRS address in the fill.Test name mentions “maker fee rebate and maker order router rev share”, but
MakerFeeQuoteQuantumsis positive andMakerOrderRouterAddrisn’t set. Rename to avoid confusion.- name: "Valid taker order router rev share with unconditional rev share and maker fee rebate and maker order router rev share", + name: "Valid taker ORRS with unconditional rev share; maker has positive fee; no maker ORRS",
818-824: Fix comment: 100_000 PPM is 10%, not 20%.Same PPM comment issue in this test’s expected block.
- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%
794-802: Rename test: “maker fee rebate” wording is inaccurate (maker fee is positive).Maker pays a positive fee (100_000), so it’s not a rebate. The test itself is correct—just the name is misleading.
- name: "Valid taker order router rev share with unconditional rev share and maker fee rebate and maker order router rev share", + name: "Valid taker & maker ORRS with unconditional rev share; maker has positive fee",
📜 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 (2)
protocol/x/revshare/keeper/revshare.go(2 hunks)protocol/x/revshare/keeper/revshare_test.go(1 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
🧬 Code graph analysis (1)
protocol/x/revshare/keeper/revshare_test.go (4)
protocol/x/revshare/types/types.go (4)
RevSharesForFill(59-64)RevShare(32-38)RevShareFeeSource(40-40)RevShareType(49-49)protocol/testutil/constants/addresses.go (4)
DaveAccAddress(13-13)BobAccAddress(11-11)CarlAccAddress(12-12)AliceAccAddress(10-10)protocol/x/clob/types/types.go (1)
FillForProcess(5-23)protocol/x/revshare/types/revshare.pb.go (6)
UnconditionalRevShareConfig(76-79)UnconditionalRevShareConfig(83-83)UnconditionalRevShareConfig(84-86)UnconditionalRevShareConfig_RecipientConfig(122-127)UnconditionalRevShareConfig_RecipientConfig(135-135)UnconditionalRevShareConfig_RecipientConfig(136-138)
⏰ 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). (17)
- GitHub Check: test-coverage-upload
- GitHub Check: build
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: container-tests
- GitHub Check: golangci-lint
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: benchmark
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (1)
protocol/x/revshare/keeper/revshare.go (1)
223-234: Good fix: use a copy of netFees and subtract ORRS/affiliate from the copy (no in-place mutation).
new(big.Int).Set(netFees)prevents side-effects and keeps later validations correct. The split between the “no affiliate → subtract ORRS” and “has affiliate → subtract affiliate fees” branches matches the intended precedence in tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
protocol/x/revshare/keeper/revshare_test.go (3)
679-679: Fix ppm-to-percent comments (avoid confusion during future edits)A few inline comments misstate the percent equivalent of the ppm values. Suggest correcting to match the values used in assertions.
- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%- SharePpm: 500_000, // 20% + SharePpm: 500_000, // 50%- err := keeper.SetOrderRouterRevShare(ctx, constants.DaveAccAddress.String(), 500_000) // 10% + err := keeper.SetOrderRouterRevShare(ctx, constants.DaveAccAddress.String(), 500_000) // 50%- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%Also applies to: 710-710, 719-719, 825-825
724-726: Rename test to reflect there is no maker order-router address setThis case configures a maker ORRS ppm but does not set MakerOrderRouterAddr in the fill; no maker ORRS is expected (and none is asserted). The current title mentions “maker order router rev share”, which can mislead readers.
- name: "Valid taker order router rev share with unconditional rev share and maker fee " + - "rebate and maker order router rev share", + name: "Valid taker order router rev share with unconditional rev share and positive maker fee (no maker router addr)",
698-698: Keep FillQuoteQuantums scale consistent across tests (pure readability)Most tests in this file use 100_000_000_000; these three use 100_000_000. It doesn’t affect logic here, but aligning the magnitude reduces cognitive overhead when eyeballing scenarios.
- FillQuoteQuantums: big.NewInt(100_000_000), + FillQuoteQuantums: big.NewInt(100_000_000_000),- FillQuoteQuantums: big.NewInt(100_000_000), + FillQuoteQuantums: big.NewInt(100_000_000_000),- FillQuoteQuantums: big.NewInt(100_000_000), + FillQuoteQuantums: big.NewInt(100_000_000_000),Also applies to: 767-767, 844-844
📜 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_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
protocol/x/revshare/keeper/revshare_test.go (4)
protocol/x/revshare/types/types.go (9)
RevSharesForFill(59-64)RevShare(32-38)RevShareFeeSource(40-40)REV_SHARE_FEE_SOURCE_TAKER_FEE(45-45)RevShareType(49-49)REV_SHARE_TYPE_ORDER_ROUTER(56-56)REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE(44-44)REV_SHARE_TYPE_UNCONDITIONAL(54-54)REV_SHARE_FEE_SOURCE_MAKER_FEE(46-46)protocol/testutil/constants/addresses.go (4)
DaveAccAddress(13-13)BobAccAddress(11-11)CarlAccAddress(12-12)AliceAccAddress(10-10)protocol/x/clob/types/types.go (1)
FillForProcess(5-23)protocol/x/revshare/types/revshare.pb.go (6)
UnconditionalRevShareConfig(76-79)UnconditionalRevShareConfig(83-83)UnconditionalRevShareConfig(84-86)UnconditionalRevShareConfig_RecipientConfig(122-127)UnconditionalRevShareConfig_RecipientConfig(135-135)UnconditionalRevShareConfig_RecipientConfig(136-138)
⏰ 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). (28)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: golangci-lint
- GitHub Check: container-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: liveness-test
- GitHub Check: build-and-push-testnet
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-race
- GitHub Check: test-coverage-upload
- GitHub Check: benchmark
- GitHub Check: build
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
protocol/x/revshare/keeper/revshare_test.go (1)
681-691: Order-router first, unconditional after leftover: math checks outThe expected quantums and ppm aggregates align with the intended flow: apply taker ORRS against min(takerFee, netFees), then apply unconditional shares to the remaining net (after taker ORRS), and finally include maker ORRS if present. Nice coverage of the negative-maker-fee case as well.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
protocol/x/revshare/keeper/revshare_test.go (2)
678-678: Minor comment inconsistency in the test case.The comment states "20%" but the actual value is 100,000 ppm, which equals 10%. The calculation is correct (17,500 is indeed 10% of 175,000), but the comment should be updated for clarity.
- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%
825-825: Minor comment inconsistency in the test case.Similar to the previous case, the comment shows "20%" but the value is 100,000 ppm (10%). The calculation is correct but the comment should match.
- RevSharePpm: 100_000, // 20% + RevSharePpm: 100_000, // 10%
📜 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_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
protocol/x/revshare/keeper/revshare_test.go (4)
protocol/x/revshare/types/types.go (9)
RevSharesForFill(59-64)RevShare(32-38)RevShareFeeSource(40-40)REV_SHARE_FEE_SOURCE_TAKER_FEE(45-45)RevShareType(49-49)REV_SHARE_TYPE_ORDER_ROUTER(56-56)REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE(44-44)REV_SHARE_TYPE_UNCONDITIONAL(54-54)REV_SHARE_FEE_SOURCE_MAKER_FEE(46-46)protocol/testutil/constants/addresses.go (4)
DaveAccAddress(13-13)BobAccAddress(11-11)CarlAccAddress(12-12)AliceAccAddress(10-10)protocol/x/clob/types/types.go (1)
FillForProcess(5-23)protocol/x/revshare/types/revshare.pb.go (6)
UnconditionalRevShareConfig(76-79)UnconditionalRevShareConfig(83-83)UnconditionalRevShareConfig(84-86)UnconditionalRevShareConfig_RecipientConfig(122-127)UnconditionalRevShareConfig_RecipientConfig(135-135)UnconditionalRevShareConfig_RecipientConfig(136-138)
⏰ 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). (28)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: test-coverage-upload
- GitHub Check: build
- GitHub Check: container-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-mainnet
- GitHub Check: golangci-lint
- GitHub Check: benchmark
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (3)
protocol/x/revshare/keeper/revshare_test.go (3)
655-722: LGTM! New test case for taker order router rev share correctly validates complex fee distribution.This test case properly validates the taker order-router revenue sharing (ORRS) scenario with unconditional rev shares. The calculations are accurate:
- Dave receives 50% of taker fees (175,000 from 350,000 total)
- Remaining net protocol revenue (175,000) is split between Bob (50% = 87,500) and Carl (20% = 17,500)
- FeeSourceToRevSharePpm totals 1,100,000 (110%), which is valid as it reflects different fee sources
723-793: LGTM! Test case properly validates combined taker and maker order router scenarios.The test correctly handles the more complex scenario where:
- Dave gets taker ORRS: 50% of 450,000 = 225,000
- Alice gets maker ORRS: 50% of 100,000 = 50,000 (but no maker ORRS is actually allocated in this test)
- Unconditional rev shares are calculated on remaining net protocol revenue: (450,000 + 100,000) - 225,000 = 325,000
- Bob: 50% of 325,000 = 162,500
- Carl: 10% of 325,000 = 32,500
The calculations align with the expected behavior for combined order router and unconditional rev shares.
794-872: LGTM! Test case correctly validates both taker and maker order router rev shares.This test case properly exercises the scenario where both taker and maker have order router rev shares:
- Dave (taker ORRS): 50% of 450,000 = 225,000
- Alice (maker ORRS): 50% of 100,000 = 50,000
- Remaining net protocol revenue: (450,000 + 100,000) - (225,000 + 50,000) = 275,000
- Unconditional shares: Bob (50% of 275,000 = 137,500) + Carl (10% of 275,000 = 27,500)
The test correctly includes the
MakerOrderRouterAddrfield and validates the proper distribution across all fee sources.
|
|
||
| var orderRouterRevShares []types.RevShare | ||
| netFeesSubRevenueShare := netFees | ||
| netFeesSubRevenueShare := new(big.Int).Set(netFees) |
There was a problem hiding this comment.
nit: line 233 in the else - lets make it netFeesSubRevenueShare.Sub(netFeesSubRevenueShare, affiliateFeesShared)?
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
|
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