-
Notifications
You must be signed in to change notification settings - Fork 19
Show network and admin fees (values) in Donation flow, collective and wallet screens #243 #305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show network and admin fees (values) in Donation flow, collective and wallet screens #243 #305
Conversation
- Added useCollectiveFees hook to fetch protocol and manager fees. - Updated fee calculations and display in the DonorCollectiveCard. - Enhanced tooltips to show dynamic fee information based on fetched data. - Introduced calculateFeeAmounts and formatFlowRateToDaily utility functions for fee calculations.
- Enhanced the useCollectiveFees hook to provide better error messages and handle loading states. - Updated fee calculations in DonorCollectiveCard to account for error states and loading indicators. - Added visual indicators for live fee usage and improved tooltip content for clarity. - Refactored fee fetching logic to ensure fallback values are only used when necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The abundant console.log and console.warn statements in both the SDK’s getCollectiveFees and the useCollectiveFees hook should be gated behind a debug flag or removed to avoid leaking internal logs in production.
- Fee rendering logic is duplicated between DonateComponent (with hardcoded 5%/3%) and DonorCollectiveCard (dynamic); consider extracting a shared FeeDisplay component to centralize formatting and ensure consistency.
- The inline Tooltip and fee UI sections are lengthy—abstracting them into smaller, reusable subcomponents can help reduce file complexity and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The abundant console.log and console.warn statements in both the SDK’s getCollectiveFees and the useCollectiveFees hook should be gated behind a debug flag or removed to avoid leaking internal logs in production.
- Fee rendering logic is duplicated between DonateComponent (with hardcoded 5%/3%) and DonorCollectiveCard (dynamic); consider extracting a shared FeeDisplay component to centralize formatting and ensure consistency.
- The inline Tooltip and fee UI sections are lengthy—abstracting them into smaller, reusable subcomponents can help reduce file complexity and improve maintainability.
## Individual Comments
### Comment 1
<location> `packages/app/src/components/WalletCards/styles.ts:114` </location>
<code_context>
+ supportingSection: {
+ gap: 2,
+ },
+ supportingLabel: {
+ fontSize: 14,
+ font: 'bold',
+ fontWeight: '600',
+ color: Colors.black,
+ ...InterSemiBold,
+ },
+ supportingNumber: {
</code_context>
<issue_to_address>
The 'font' property in supportingLabel is not a valid React Native style property.
React Native will ignore 'font'. Use 'fontWeight', 'fontStyle', or 'fontFamily' instead. Please remove or replace 'font: "bold"'.
</issue_to_address>
### Comment 2
<location> `packages/app/src/components/DonateComponent.tsx:638` </location>
<code_context>
+ <VStack space={2} backgroundColor="gray.50" p={2} borderRadius="lg">
</code_context>
<issue_to_address>
Fee information is hardcoded and not dynamic.
Consider sourcing the fee percentages from the same hook or SDK used elsewhere to maintain consistency if the actual values change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Invalid Font Property ▹ view | ✅ Fix detected | |
| Redundant Float Parsing ▹ view | ✅ Fix detected | |
| Insufficient Input Validation for BigNumber Conversion ▹ view | ✅ Fix detected | |
| Inefficient Fee Data Fetching ▹ view | ||
| Debug statements in production code ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/app/src/hooks/index.ts | ✅ |
| packages/app/src/lib/calculateFeeAmounts.ts | ✅ |
| packages/app/src/components/WalletCards/styles.ts | ✅ |
| packages/sdk-js/scripts/testFees.ts | ✅ |
| packages/app/src/hooks/useCollectiveFees.ts | ✅ |
| packages/app/src/components/WalletCards/DonorCollectiveCard.tsx | ✅ |
| packages/app/src/components/DonateComponent.tsx | ✅ |
| packages/sdk-js/src/goodcollective/goodcollective.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| ); | ||
| }; | ||
|
|
||
| const { fees, loading: feesLoading, error: feesError } = useCollectiveFees(donorCollective.collective); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inefficient Fee Data Fetching 
Tell me more
What is the issue?
The fees data is fetched individually for each DonorCollectiveCard instance, potentially causing multiple unnecessary network requests if multiple cards are rendered.
Why this matters
This can lead to increased network traffic and slower page load times, especially when displaying multiple collective cards simultaneously.
Suggested change ∙ Feature Preview
Move the fee fetching logic to a parent component and pass the fees data down as props, or implement a caching mechanism in useCollectiveFees hook using React Query or similar state management solution:
const queryClient = useQueryClient();
const { data: fees } = useQuery(['collectiveFees', collective],
() => fetchFees(collective),
{
staleTime: 5 * 60 * 1000, // Cache for 5 minutes
cacheTime: 30 * 60 * 1000 // Keep unused data for 30 minutes
}
);Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
- Improved error handling for fetching protocol fees from UBI and DirectPayments factories. - Added fallback values for protocol fees to ensure stability in case of RPC errors or factory failures. - Enhanced logging for better debugging and visibility of fee retrieval processes.
…veFees hook" This reverts commit 9866d72.
- Removed the deprecated 'font' property and retained 'fontWeight' for better consistency in styling.
- Refactored the fee calculation logic to use a parsed float value for daily amounts, improving readability and consistency in the return format. - Ensured that both G$ and USD amounts are calculated using the same parsed value for accuracy.
- Added try-catch blocks to handle invalid flowRate formats in calculateFeeAmounts and formatFlowRateToDaily functions. - Ensured that fallback values are returned in case of errors, enhancing stability and user experience.
- Integrated useCollectiveFees hook to fetch and display protocol and manager fees dynamically. - Improved loading and error handling states for fee information, providing users with real-time updates. - Updated the UI to reflect fetched fee values or fallback defaults, ensuring clarity in donation fee structures.
- Added useRealtimeStats hook to fetch live pool statistics for the donor collective. - Updated fee calculation logic to consider realtime stats, enhancing accuracy in fee display. - Improved the overall user experience by providing dynamic updates based on fetched statistics.
|
|
||
| // Debug: Show if we're using actual fees or fallback values | ||
| const isUsingActualFees = | ||
| Boolean(realtimeStats) || (fees && fees.protocolFeeBps !== 500 && fees.managerFeeBps !== 300); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand what is happening here.
why use realtimestats here, if all you want to show is per-user fee's amount?
- getRealTimestats should be used to show the collective accumulated fees, on the goodcollective page
- your calculateFeeAmount per user, is what you are showing here.
- Integrated useRealtimeStats hook to fetch and display accumulated fees for the collective. - Updated UI to include a new RowItem for displaying total fees, enhancing user visibility of financial metrics. - Improved fee calculation logic to utilize real-time statistics for accuracy in fee representation.
- Replaced manual fee calculation and network determination with a direct call to GoodCollectiveSDK's getCollectiveFees method. - Improved error handling for unsupported networks and fee fetching, enhancing stability and user experience. - Removed redundant code related to network name determination and pool type checks, streamlining the hook's functionality.
…ovider for fee retrieval - Replaced useEthersSigner with useEthersProvider to improve provider handling. - Enhanced error handling for missing pool address and RPC provider availability. - Streamlined dependency arrays in hooks for better performance and clarity.
packages/app/src/components/WalletCards/DonorCollectiveCard.tsx
Outdated
Show resolved
Hide resolved
packages/app/src/components/WalletCards/DonorCollectiveCard.tsx
Outdated
Show resolved
Hide resolved
packages/app/src/components/WalletCards/DonorCollectiveCard.tsx
Outdated
Show resolved
Hide resolved
…aManuel/GoodCollective into Show-network-and-admin-fees
…for child components



Description
This PR implements dynamic fee fetching and display for donor collective cards, replacing hardcoded fee values with real-time data from smart contracts. The implementation provides users with transparent, accurate fee information for both protocol and manager fees.
About
Fixes #243 - Network and Manager fee transparency and accuracy in donor collective cards
Key Changes
1. Dynamic Fee Fetching (useCollectiveFees hook)
2. Enhanced UI Components
3. SDK Integration
factory.feeBps()directlypool.getManagerFee()Technical Implementation
DirectPaymentsFactory.feeBps()orUBIPoolFactory.feeBps()How Has This Been Tested?
Manual Testing
✅ Development Environment: Verified 10% protocol fees display correctly
✅ Production Environment: Confirmed 5% protocol fees via network detection
✅ Testnet: Validated 10% protocol fees on Alfajores
✅ No Active Flow: Confirmed percentage display (10%, 5%) instead of "G$ 0/day"
✅ Active Flow: Verified daily amount calculations with actual flow rates
✅ Error States: Tested loading and error message displays
✅ Live Indicators: Confirmed "✓ Live" badges appear when actual fees are used
Automated Testing
✅ SDK Test Script: Verified factory fee fetching across all networks
✅ Contract Integration: Tested fee fetching from actual deployed contracts
✅ Error Handling: Validated graceful degradation when contracts unavailable
Test Results
✓ All tests passing
Impact
Description by Korbit AI
What change is being made?
Integrate the display of network and administration fees in the donation flow, collective, and wallet screens.
Why are these changes being made?
This update provides greater transparency for users by showing the exact network and manager fees incurred during donations, enhancing user trust and decision-making. Additionally, the integration of tooltip elements and detailed fee calculations from the "GoodCollectiveSDK" ensures users receive live and complete fee data in real-time while using the application. This approach is necessary to align with user experience improvements and the accurate reflection of the donation process costs.