fix(index-dtf): bps-precise fee distribution during deployment - #1075
Open
lcamargof wants to merge 2 commits into
Open
fix(index-dtf): bps-precise fee distribution during deployment#1075lcamargof wants to merge 2 commits into
lcamargof wants to merge 2 commits into
Conversation
The deploy Fees & Distribution table truncated the DAO fee registry fee to an integer (BSC's 1/3 became 33%) and normalized shares in floating point, so precise shares could not be entered or summed. Deploy and propose-dtf-settings now share bps helpers in utils/fees. Co-Authored-By: luis.camargo@reserve.org <luis.camargo@reserve.org>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughFee calculations now use hundredths-of-a-percent precision. Deployment and governance flows share conversion and even-split helpers. Tests cover fractional fees, exact allocations, remainder handling, and deployment UI behavior. ChangesFee precision and revenue allocation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeployWizard
participant RevenueDistributionSettings
participant FeeUtilities
DeployWizard->>RevenueDistributionSettings: Open Fees & Distribution step
RevenueDistributionSettings->>FeeUtilities: splitSharesEvenly(totalPercent, participants)
RevenueDistributionSettings->>FeeUtilities: revenuePortionFromShare(sharePercent, platformFee)
FeeUtilities-->>RevenueDistributionSettings: Quantized shares and contract portions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying register-app with
|
| Latest commit: |
803fa92
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4936d71.register-app.pages.dev |
| Branch Preview URL: | https://devin-1785878219-index-deplo.register-app.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/general/deploy/index-dtf-revenue.spec.ts`:
- Around line 62-88: Extend the deploy route coverage around the existing fees
tests and run it for both desktop and `@mobile` projects. Add assertions for L0
blank, L1 skeleton, L2 independently resolving partial data, and L3 completed
content, including a check that no unexpected layout reflows occur; retain the
existing allocation behavior assertions.
In
`@src/views/index-dtf/governance/views/propose/views/propose-dtf-settings/tests/fee-recipients-atom.test.ts`:
- Around line 28-36: Update the test around makeStore and feeRecipientsAtom to
exercise dtfSettingsProposalDataAtom, configuring the store with a 33.33%
platform fee and recipient change. Decode the generated setFeeRecipients
calldata and assert that the 66.67% recipient encodes to a portion of exactly
parseEther('1'), rather than testing revenuePortionFromShare directly.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e31e992-1348-42fe-9e2a-d96c952e4782
📒 Files selected for processing (19)
docs/wiki/progress.mde2e/TEST_MAP.mde2e/tests/general/deploy/index-dtf-revenue.spec.tssrc/hooks/use-platform-fee.tssrc/utils/constants.tssrc/utils/fees.tssrc/utils/tests/fees.test.tssrc/views/index-dtf/deploy/components/basic-input.tsxsrc/views/index-dtf/deploy/components/deploy-accordion.tsxsrc/views/index-dtf/deploy/permissionless-defaults.tssrc/views/index-dtf/deploy/steps/revenue/revenue-distribution-settings.tsxsrc/views/index-dtf/deploy/tests/permissionless-defaults.test.tssrc/views/index-dtf/deploy/utils/index.tssrc/views/index-dtf/deploy/utils/tests/revenue-distribution.test.tssrc/views/index-dtf/governance/CLAUDE.mdsrc/views/index-dtf/governance/views/propose/views/propose-dtf-settings/atoms.tssrc/views/index-dtf/governance/views/propose/views/propose-dtf-settings/components/sections/propose-revenue-distribution.tsxsrc/views/index-dtf/governance/views/propose/views/propose-dtf-settings/tests/fee-recipients-atom.test.tssrc/views/index-dtf/index-dtf-container.tsx
Comment on lines
+62
to
+88
| test('deploy fees: hundredths-of-a-percent shares close the allocation @smoke @mobile', async ({ | ||
| harness, | ||
| }) => { | ||
| const page = await openFeesStep(harness) | ||
|
|
||
| await page.getByTestId('deploy-share-deployerShare').fill('0.01') | ||
| await page.getByTestId('deploy-share-governanceShare').fill('66.66') | ||
|
|
||
| await expect(page.getByTestId('deploy-remaining-allocation')).toHaveText('0%') | ||
| }) | ||
|
|
||
| test('deploy fees: even distribution allocates the whole non-platform pot', async ({ | ||
| harness, | ||
| }) => { | ||
| const page = await openFeesStep(harness) | ||
|
|
||
| await page.getByTestId('deploy-even-distribution').click() | ||
|
|
||
| // 66.67% over two participants — the last one absorbs the odd basis point. | ||
| await expect(page.getByTestId('deploy-share-deployerShare')).toHaveValue( | ||
| '33.33' | ||
| ) | ||
| await expect(page.getByTestId('deploy-share-governanceShare')).toHaveValue( | ||
| '33.34' | ||
| ) | ||
| await expect(page.getByTestId('deploy-remaining-allocation')).toHaveText('0%') | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add lifecycle assertions for the deploy route.
This spec only asserts loaded-state behavior. It does not cover L0 blank, L1 skeleton, L2 partial resolution, L3 completion, or unexpected reflows. Add these assertions for desktop and the @mobile project before treating this as route coverage. As per coding guidelines, “Test every page through loading phases L0 blank, L1 skeleton, L2 independently resolving partial data, and L3 fully loaded behavior; assert zero unexpected reflows.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/general/deploy/index-dtf-revenue.spec.ts` around lines 62 - 88,
Extend the deploy route coverage around the existing fees tests and run it for
both desktop and `@mobile` projects. Add assertions for L0 blank, L1 skeleton, L2
independently resolving partial data, and L3 completed content, including a
check that no unexpected layout reflows occur; retain the existing allocation
behavior assertions.
Source: Coding guidelines
Comment on lines
+28
to
+36
| it('round-trips a whole-pot share under a fractional platform fee', () => { | ||
| // BSC takes 1/3 of revenue: a contract 100% recipient owns 66.67% of total, | ||
| // and that share must encode back to the whole pot (1e18), never above it. | ||
| const result = makeStore(33.33, '100').get(feeRecipientsAtom) | ||
|
|
||
| expect(result?.governanceShare).toBe(66.67) | ||
| expect(revenuePortionFromShare(result!.governanceShare, 33.33)).toBe( | ||
| parseEther('1') | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exercise the proposal-calldata seam.
This test reads feeRecipientsAtom and then calls revenuePortionFromShare directly. It does not execute the changed dtfSettingsProposalDataAtom path that builds setFeeRecipients calldata.
Set a fractional platform fee and recipient changes in the store. Then decode the generated calldata and assert that the recipient portion is exactly parseEther('1') for the 66.67% share with a 33.33% platform fee.
As per coding guidelines, “Tests for bug fixes and non-trivial behavior changes must exercise the real test seam rather than replacing it with a mock.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/views/index-dtf/governance/views/propose/views/propose-dtf-settings/tests/fee-recipients-atom.test.ts`
around lines 28 - 36, Update the test around makeStore and feeRecipientsAtom to
exercise dtfSettingsProposalDataAtom, configuring the store with a 33.33%
platform fee and recipient change. Decode the generated setFeeRecipients
calldata and assert that the 66.67% recipient encodes to a portion of exactly
parseEther('1'), rather than testing revenuePortionFromShare directly.
Source: Coding guidelines
…nd absorb read-back rounding drift Writing the whole additionalRevenueRecipients array left the per-index share inputs rendering their old values, and rounding each contract portion to the 0.01% grid independently could load an untouched propose form as over-allocated. Co-Authored-By: luis.camargo@reserve.org <luis.camargo@reserve.org>
7 tasks
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
1 participant
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.
Summary
Deployment's Fees & Distribution table couldn't express precise shares, while governance proposals could. Two separate causes:
333333333333333333 / 1e18= 1/3) displayed and was used as 33% — with 33% reserved, the remaining shares could never total 100%:parseEther(float.toString())), and its "Even distribution" floored to a base share and left the pot short.Deploy and
propose-dtf-settingsnow share bps-grid helpers insrc/utils/fees.ts— every fee percentage lives on the same 0.01% grid the inputs accept, and the share→portion conversion is bigint-only:So on BSC a 66.67% governance share is now exactly
1e18(the whole pot) instead of drifting, and a 0.01% edit actually moves the encoded portion. Callers changed:usePlatformFee,calculateRevenueDistribution,dtfSettingsProposalDataAtom.calculateShare, both Even-distribution buttons,getPermissionlessDefaults, and the SDK fee written intoindexDTFFeeAtom. Calldata shape, recipient sorting, the last-recipient correction to exactly1e18, and governance'stokenJarrouting are unchanged. The BSC fallback constant moves33 → 33.33to match the registry when it can't be read.Engineer review requested on the fee math itself: the 1/3 → 33.33% quantization policy (round to the grid the inputs accept), and that both deploy and propose now go through the same helper.
Testing
utils/tests/fees.test.ts, deployrevenue-distribution.test.ts(fractional fee gives the whole pot; hundredths stay distinct; 4 decimal shares total exactly1e18), governancefee-recipients-atom.test.tsround-trip, permissionless defaults.e2e/tests/general/deploy/index-dtf-revenue.spec.ts(strict mocks, desktop + mobile): a 1/3 registry fee renders33.33 %, 0.01%-grid shares close the allocation, even distribution allocates the whole pot.33 %; restoring the float even-split gives33.33where33.34is required.pnpm typecheck,pnpm lint,pnpm test:run(864), e2e helper units (72),pnpm e2e:smoke(59),governance-propose-dtf-settings3/3,wiki-lint.Link to Devin session: https://app.devin.ai/sessions/96884b34a06547fd834c57cdf9a88933
Requested by: @lcamargof
Summary by CodeRabbit
Bug Fixes
Tests