Fix/burn fee thresholds cleanup#111
Merged
Junman140 merged 5 commits intoPi-Defi-world:mainfrom Mar 30, 2026
Merged
Conversation
|
@sudo-robi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCorrected reserve-threshold comparisons in the burn-fee calculation, added explicit 85%/115% threshold constants, added Jest tests for below/at/within/above-threshold cases, reordered a RabbitMQ queue entry, and expanded test environment fallback variables for CI/local runs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/feePolicy/__tests__/feePolicyService.test.ts (1)
24-95: Consider adding edge case tests for completeness.The threshold tests are well-designed, but the suite could benefit from additional edge cases that exist in
getBurnFeeBps:
- Currency not found in reserve status
targetWeight <= 0guard📝 Optional: Additional edge case tests
it("returns base burn fee when currency is not found", async () => { mockGetReserveStatus.mockResolvedValueOnce({ currencies: [ { currency: "USD", targetWeight: 40, actualWeight: 40 }, ], }); await expect(getBurnFeeBps("NGN")).resolves.toBe(10); }); it("returns base burn fee when targetWeight is zero", async () => { mockGetReserveStatus.mockResolvedValueOnce({ currencies: [ { currency: "NGN", targetWeight: 0, actualWeight: 40 }, ], }); await expect(getBurnFeeBps("NGN")).resolves.toBe(10); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/feePolicy/__tests__/feePolicyService.test.ts` around lines 24 - 95, Tests for getBurnFeeBps are missing edge cases for when the requested currency is absent from reserve status and when targetWeight is non-positive; add two unit tests in src/services/feePolicy/__tests__/feePolicyService.test.ts that mock mockGetReserveStatus to return a currencies list without the requested currency and another where the matching currency has targetWeight <= 0, then assert getBurnFeeBps("NGN") resolves to the base burn fee (10) in both cases; reference the existing test patterns and the getBurnFeeBps function to place and structure these new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/feePolicy/__tests__/feePolicyService.test.ts`:
- Around line 24-95: Tests for getBurnFeeBps are missing edge cases for when the
requested currency is absent from reserve status and when targetWeight is
non-positive; add two unit tests in
src/services/feePolicy/__tests__/feePolicyService.test.ts that mock
mockGetReserveStatus to return a currencies list without the requested currency
and another where the matching currency has targetWeight <= 0, then assert
getBurnFeeBps("NGN") resolves to the base burn fee (10) in both cases; reference
the existing test patterns and the getBurnFeeBps function to place and structure
these new tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f3187e6-e173-469c-8091-44254383afda
📒 Files selected for processing (2)
src/services/feePolicy/__tests__/feePolicyService.test.tssrc/services/feePolicy/feePolicyService.ts
Member
|
@sudo-robi test failed |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/jest.setup.ts (1)
3-11:⚠️ Potential issue | 🟡 MinorDead code: Lines 8-11 are unreachable.
The
||=operator only assigns when the left-hand side is falsy. Since lines 3-6 already assign values toDATABASE_URL,MONGODB_URI,RABBITMQ_URL, andJWT_SECRET, lines 8-11 will never execute—they are dead code.Remove the duplicate assignments to avoid confusion:
🧹 Proposed fix to remove dead code
// Defaults for CI and local `pnpm test` when .env is absent. // Must match .github/workflows/ci.yml postgres service (POSTGRES_USER/PASSWORD). process.env.DATABASE_URL ||= "postgresql://postgres:postgres@localhost:5432/acbu_test"; process.env.MONGODB_URI ||= "mongodb://localhost:27017/acbu_test"; process.env.RABBITMQ_URL ||= "amqp://guest:guest@localhost:5672"; process.env.JWT_SECRET ||= "test-jwt-secret-for-ci"; process.env.API_KEY_SALT ||= "test-api-key-salt"; -process.env.DATABASE_URL ||= "postgresql://test:test@localhost:5432/acbu_test"; -process.env.MONGODB_URI ||= "mongodb://localhost:27017/acbu_test"; -process.env.RABBITMQ_URL ||= "amqp://localhost:5672"; -process.env.JWT_SECRET ||= "test-secret";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/jest.setup.ts` around lines 3 - 11, Remove the duplicate fallback assignments that never run: delete the second set of process.env assignments for DATABASE_URL, MONGODB_URI, RABBITMQ_URL, and JWT_SECRET and keep only the initial ||= fallbacks (ensure API_KEY_SALT remains set once); this removes dead code and preserves the intended conditional environment variable initialization for DATABASE_URL, MONGODB_URI, RABBITMQ_URL, JWT_SECRET, and API_KEY_SALT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/rabbitmq.ts`:
- Around line 114-116: The QUEUES object contains a duplicated key AUDIT_LOGS
(declared twice with the same value); remove the redundant entry so AUDIT_LOGS
appears only once in QUEUES to avoid silent overwrites and the Biome lint
violation—locate the QUEUES object and delete the duplicate AUDIT_LOGS line
(keep a single AUDIT_LOGS: "audit_logs" entry).
In `@tests/jest.setup.ts`:
- Line 3: The failing Prettier formatting is on the assignment to
process.env.DATABASE_URL in tests/jest.setup.ts; run your project's formatter
(e.g., Prettier) on this file or the repo to apply the expected style so the
line 'process.env.DATABASE_URL ||=
"postgresql://postgres:postgres@localhost:5432/acbu_test";' is reformatted to
match project rules, then stage the updated file and push the change; ensure you
target the statement that references process.env.DATABASE_URL so the same token
names remain unchanged.
---
Outside diff comments:
In `@tests/jest.setup.ts`:
- Around line 3-11: Remove the duplicate fallback assignments that never run:
delete the second set of process.env assignments for DATABASE_URL, MONGODB_URI,
RABBITMQ_URL, and JWT_SECRET and keep only the initial ||= fallbacks (ensure
API_KEY_SALT remains set once); this removes dead code and preserves the
intended conditional environment variable initialization for DATABASE_URL,
MONGODB_URI, RABBITMQ_URL, JWT_SECRET, and API_KEY_SALT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afb87fe4-bc06-4c17-b9d3-7e499fa12dfd
📒 Files selected for processing (2)
src/config/rabbitmq.tstests/jest.setup.ts
| @@ -1,3 +1,10 @@ | |||
| // Defaults for CI and local `pnpm test` when .env is absent. | |||
| // Must match .github/workflows/ci.yml postgres service (POSTGRES_USER/PASSWORD). | |||
| process.env.DATABASE_URL ||= "postgresql://postgres:postgres@localhost:5432/acbu_test"; | |||
There was a problem hiding this comment.
Fix formatting issue flagged by Prettier.
Static analysis indicates a formatting issue on this line. Run your formatter to resolve it.
🧰 Tools
🪛 ESLint
[error] 3-3: Insert ⏎·
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/jest.setup.ts` at line 3, The failing Prettier formatting is on the
assignment to process.env.DATABASE_URL in tests/jest.setup.ts; run your
project's formatter (e.g., Prettier) on this file or the repo to apply the
expected style so the line 'process.env.DATABASE_URL ||=
"postgresql://postgres:postgres@localhost:5432/acbu_test";' is reformatted to
match project rules, then stage the updated file and push the change; ensure you
target the statement that references process.env.DATABASE_URL so the same token
names remain unchanged.
Member
|
@sudo-robi cl failed |
Contributor
Author
|
@Junman140 ci has been resolved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #33
This PR corrects the burn fee threshold logic for reserve weights. The previous implementation used values 15 and 21, which were not consistent with the intended percentage-of-target calculation. The new logic applies a high burn fee below 85% of target, a lower burn fee above 115%, and the base fee in between. Named constants are introduced for clarity, and focused unit tests are added to cover all threshold scenarios. All tests pass.
Summary by CodeRabbit
Tests
Refactor
Chores