Skip to content

Fee Discount - Remove max duration restriction#3294

Open
davidli1997 wants to merge 4 commits intomainfrom
davidli/fee-discount
Open

Fee Discount - Remove max duration restriction#3294
davidli1997 wants to merge 4 commits intomainfrom
davidli/fee-discount

Conversation

@davidli1997
Copy link
Contributor

@davidli1997 davidli1997 commented Dec 22, 2025

Changelist

Remove 90-day maximum duration restriction for market fee discount campaigns. This allows governance to set fee discounts for periods longer than 90 days without needing multiple proposals.
Context: Proposal 330 failed validation with a 92-day campaign (Nov 1 - Jan 31). The max duration limit was overly restrictive for legitimate governance use cases.

Test Plan

  • Updated unit tests to verify fee discount params with durations exceeding 90 days pass validation
  • Existing validation still enforced: start_time < end_time, end_time > currentTime, charge_ppm <= 1_000_000

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • Improvements

    • Market fee discount periods may now extend beyond the previous 90-day maximum
  • Tests

    • Updated tests to allow and validate longer fee discount periods (examples extended to 365 days)
    • Removed tests that enforced the former 90-day boundary constraint

✏️ Tip: You can customize this high-level summary in your review settings.

@davidli1997 davidli1997 requested a review from a team as a code owner December 22, 2025 18:18
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Walkthrough

The changes remove the 90-day maximum duration constraint from fee discount parameters. The MaxFeeDiscountDuration constant is deleted, validation logic enforcing the limit is removed from the Validate function, and test cases are updated to permit durations of 365 days and beyond.

Changes

Cohort / File(s) Summary
Fee Discount Type Updates
protocol/x/feetiers/types/per_market_fee_discount.go
Removed exported constant MaxFeeDiscountDuration and corresponding validation logic that enforced a 90-day maximum duration limit. Validate function now only checks start/end ordering, future end time, and charge_ppm range.
Keeper Test Updates
protocol/x/feetiers/keeper/msg_server_test.go
Removed test case that validated rejection of invalid parameters with durations exceeding 90 days.
Type Test Updates
protocol/x/feetiers/types/per_market_fee_discount_test.go
Replaced "maximum duration" test case with "no maximum duration" test allowing 365-day periods. Removed test cases validating rejection of durations exceeding the maximum and removed edge cases testing boundaries near the old 90-day limit.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Areas to spot-check:
    • Validate logic in per_market_fee_discount.go to confirm only the intended duration cap check was removed.
    • Updated test cases in per_market_fee_discount_test.go to ensure new durations and expectations are consistent.
    • msg_server_test.go to verify no remaining tests assume the 90-day cap.

Poem

🐰 I hopped through code and nudged the line,
No ninety days to fence the vine,
A year (or more) can dance and play,
Discounts linger, bright as day,
Carrots for devs — hooray! 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: removing the 90-day maximum duration restriction for fee discounts, which is the primary objective of the PR.
Description check ✅ Passed The PR description includes all required template sections: Changelist describes the removal of the 90-day limit with context, Test Plan details validation updates, and Author/Reviewer Checklist is present with appropriate label reminders.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch davidli/fee-discount

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d52da74 and 18422ab.

📒 Files selected for processing (1)
  • protocol/x/feetiers/types/per_market_fee_discount_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • protocol/x/feetiers/types/per_market_fee_discount_test.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). (28)
  • GitHub Check: build
  • GitHub Check: liveness-test
  • GitHub Check: unit-end-to-end-and-integration
  • GitHub Check: test-coverage-upload
  • GitHub Check: test-race
  • GitHub Check: golangci-lint
  • GitHub Check: benchmark
  • GitHub Check: container-tests
  • 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-ecs-service-roundtable / (roundtable) 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: (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: (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-auxo-lambda / (auxo) Build and Push Lambda
  • 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-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-ecs-service-socks / (socks) Build and Push
  • GitHub Check: build-and-push-testnet
  • GitHub Check: check-sample-pregenesis-up-to-date
  • GitHub Check: build-and-push-mainnet
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Summary

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/feetiers/types/per_market_fee_discount_test.go (1)

243-243: Update stale comment referencing removed constant.

The comment references MaxFeeDiscountDuration constant, which was removed as part of this PR. Update the comment to reflect that this test now validates duration edge cases without an upper bound.

🔎 Proposed fix
-// Test for edge cases around the MaxFeeDiscountDuration constant
+// Test for edge cases around different fee discount durations
🧹 Nitpick comments (1)
protocol/x/feetiers/types/per_market_fee_discount_test.go (1)

268-271: Good coverage for standard long-duration campaigns.

The 365-day test case adequately validates that the duration restriction has been removed for typical long-term campaigns.

Optionally, consider adding a test case for very long durations (e.g., 5-10 years) to ensure there are no unforeseen issues with extremely long campaigns, though this is not critical given the current coverage.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1dd02c and d52da74.

📒 Files selected for processing (3)
  • protocol/x/feetiers/keeper/msg_server_test.go
  • protocol/x/feetiers/types/per_market_fee_discount.go
  • protocol/x/feetiers/types/per_market_fee_discount_test.go
💤 Files with no reviewable changes (2)
  • protocol/x/feetiers/keeper/msg_server_test.go
  • protocol/x/feetiers/types/per_market_fee_discount.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-15T16:58:46.335Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: protocol/x/clob/keeper/twap_order_state_test.go:191-209
Timestamp: 2025-04-15T16:58:46.335Z
Learning: For TWAP orders, message validation in message_place_order.go handles edge cases with comprehensive checks for parameters like durations and intervals, making it unnecessary to test invalid parameters in twap_order_state_test.go which only receives pre-validated inputs.

Applied to files:

  • protocol/x/feetiers/types/per_market_fee_discount_test.go
🧬 Code graph analysis (1)
protocol/x/feetiers/types/per_market_fee_discount_test.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/feetiers/per_market_fee_discount.ts (2)
  • PerMarketFeeDiscountParams (9-25)
  • PerMarketFeeDiscountParams (58-122)
protocol/x/feetiers/types/per_market_fee_discount.pb.go (3)
  • PerMarketFeeDiscountParams (32-43)
  • PerMarketFeeDiscountParams (47-47)
  • PerMarketFeeDiscountParams (48-50)
⏰ 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: (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-vulcan / (vulcan) 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: (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-roundtable / (roundtable) Build and Push
  • 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-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: (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: (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-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: golangci-lint
  • GitHub Check: test-coverage-upload
  • GitHub Check: check-sample-pregenesis-up-to-date
  • GitHub Check: container-tests
  • GitHub Check: liveness-test
  • GitHub Check: unit-end-to-end-and-integration
  • GitHub Check: test-race
  • GitHub Check: build-and-push-testnet
  • GitHub Check: build
  • GitHub Check: build-and-push-mainnet
  • GitHub Check: benchmark
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Summary
🔇 Additional comments (1)
protocol/x/feetiers/types/per_market_fee_discount_test.go (1)

61-69: LGTM! Validates removal of duration restriction.

The test case correctly validates that fee discount campaigns can now span 1 year, demonstrating the removal of the 90-day maximum duration restriction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants