Fix the contract issue and test it#109
Conversation
|
@mubarak23 so sorry for the late push the issue has been fixed now |
|
@Abidoyesimze please format the codes and push |
|
also there is no proper test for the function |
|
""" WalkthroughThis update introduces a batch donation feature, enabling donors to contribute to multiple campaigns in a single atomic transaction with input validation, optimized aggregation, auto-capping, and detailed event emission. It adds new structs, events, and methods to support this functionality, refactors internal donation processing for return values, updates campaign validation logic, and improves error message constants. The test suite is reorganized and expanded with batch donation and edge case tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Donor
participant Contract
participant ERC20
Donor->>Contract: batch_donate([(campaign_id, amount), ...])
Contract->>Contract: Validate input size and campaign states
Contract->>Contract: Aggregate donations per campaign with auto-capping
Contract->>ERC20: Transfer total capped amount from Donor
loop For each donation in batch
Contract->>Contract: Process internal donation (returns actual amount, donation ID)
Contract-->>Donor: Emit DonationResultEvent
end
Contract-->>Donor: Emit BatchDonationProcessed event
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (1)
src/campaign_donation.cairo (1)
681-683: Consider renaming method to clarify it returns capped total.The method
_validate_and_calculate_total_optimizedreturns the total after auto-capping, not the raw requested total. Consider renaming to_validate_and_calculate_capped_total_optimizedfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/campaign_donation.cairo(7 hunks)src/interfaces/ICampaignDonation.cairo(2 hunks)src/lib.cairo(1 hunks)tests/test_campaign_donation.cairo(5 hunks)
🔇 Additional comments (8)
src/lib.cairo (1)
8-8: No issues with this change.Adding a blank line at the end of the file is a minor formatting change.
tests/test_campaign_donation.cairo (2)
475-475: LGTM!Prefixing with underscore to silence unused variable warning is appropriate.
59-59: ```shell
#!/bin/bashCheck if the tests
test_create_campaign_invalid_zero_amountandtest_get_campaignsexist (commented or active)rg "fn test_create_campaign_invalid_zero_amount" -n tests/test_campaign_donation.cairo || echo "Not found: test_create_campaign_invalid_zero_amount"
rg "fn test_get_campaigns" -n tests/test_campaign_donation.cairo || echo "Not found: test_get_campaigns"</details> <details> <summary>src/interfaces/ICampaignDonation.cairo (2)</summary> `10-28`: **Well-structured batch donation types.** The `DonationResult` and `BatchDonationProcessed` structs are appropriately designed for tracking batch donation outcomes and events. --- `90-125`: **Excellent documentation for the batch donation feature.** The `batch_donate` method is well-documented with clear requirements, behavior, and effects. The auto-capping feature and atomic processing approach are good design decisions. </details> <details> <summary>src/campaign_donation.cairo (3)</summary> `397-401`: **Good fix for deprecated API.** Replacing `contract_address_const` with `try_into()` properly addresses the deprecated API usage. --- `617-641`: **Consistent fix for deprecated contract address API.** All `contract_address_const` usages properly updated to use `try_into()`. --- `362-367`: **Critical refund logic correctly implemented.** The refund mechanism properly handles excess amounts when donations are auto-capped, ensuring donors don't lose funds. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Emit batch event (use actual amount, not pre-calculated amount) | ||
| self | ||
| .emit( | ||
| Event::BatchDonationProcessed( | ||
| BatchDonationProcessed { | ||
| donor, | ||
| total_campaigns: campaign_amounts.len(), | ||
| successful_donations, | ||
| total_amount: actual_total_amount // Use actual amount, not pre-calculated | ||
| }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Event is missing the results array.
The BatchDonationProcessed event struct includes a results field (line 27 in ICampaignDonation.cairo), but the event emission here doesn't include the results array that was built up during processing.
Apply this fix:
self
.emit(
Event::BatchDonationProcessed(
BatchDonationProcessed {
donor,
total_campaigns: campaign_amounts.len(),
successful_donations,
- total_amount: actual_total_amount // Use actual amount, not pre-calculated
+ total_amount: actual_total_amount, // Use actual amount, not pre-calculated
+ results, // Include the results array
},
),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Emit batch event (use actual amount, not pre-calculated amount) | |
| self | |
| .emit( | |
| Event::BatchDonationProcessed( | |
| BatchDonationProcessed { | |
| donor, | |
| total_campaigns: campaign_amounts.len(), | |
| successful_donations, | |
| total_amount: actual_total_amount // Use actual amount, not pre-calculated | |
| }, | |
| ), | |
| ); | |
| // Emit batch event (use actual amount, not pre-calculated amount) | |
| self | |
| .emit( | |
| Event::BatchDonationProcessed( | |
| BatchDonationProcessed { | |
| donor, | |
| total_campaigns: campaign_amounts.len(), | |
| successful_donations, | |
| total_amount: actual_total_amount, // Use actual amount, not pre-calculated | |
| results, // Include the results array | |
| }, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In src/campaign_donation.cairo around lines 369 to 380, the
BatchDonationProcessed event emission is missing the results array that should
be included as per the event struct definition. To fix this, add the results
array variable to the BatchDonationProcessed event parameters when emitting the
event, ensuring it matches the expected structure defined in
ICampaignDonation.cairo.
|
so this works but i cannot merge, because there is not test for it, @Abidoyesimze write a unit that show the implementation actually works |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/campaign_donation.cairo (1)
140-147: Event struct missing the results field.The
BatchDonationProcessedevent is missing theresultsfield that should be included according to the interface definition.Add the missing field to match the interface:
#[derive(Drop, starknet::Event)] pub struct BatchDonationProcessed { #[key] pub donor: ContractAddress, pub total_campaigns: u32, pub successful_donations: u32, pub total_amount: u256, + pub results: Array<DonationResult>, }
🧹 Nitpick comments (2)
src/base/errors.cairo (1)
91-93: Consider using "donate" instead of "denote" for clarity.The current message uses "denote" which might be a typo. Consider this alternative that's clearer and still within the 31-character limit:
- pub const CANNOT_DENOTE_ZERO_AMOUNT: felt252 = 'Error: Cannot denote zero amt'; + pub const CANNOT_DONATE_ZERO_AMOUNT: felt252 = 'Error: Cannot donate zero amt';src/campaign_donation.cairo (1)
698-783: Consider using a more efficient data structure for campaign totals.While the current implementation works and avoids Copy trait issues, rebuilding the entire array when updating totals (lines 727-744) could be inefficient. For the current MAX_BATCH_SIZE of 20, this is acceptable, but consider documenting this performance characteristic.
Add a comment explaining the trade-off:
+ // Note: We rebuild the array to update totals due to Cairo's immutability constraints. + // This is O(n²) worst case but acceptable for MAX_BATCH_SIZE = 20. if found { // Update existing total (simplified approach - rebuild array)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/base/errors.cairo(4 hunks)src/campaign_donation.cairo(8 hunks)tests/test_campaign_donation.cairo(15 hunks)
🔇 Additional comments (10)
src/base/errors.cairo (2)
20-22: LGTM! Error message properly shortened.The error message has been correctly shortened to fit within the felt252 31-character limit while maintaining clarity.
136-184: Well-structured error constants for new features.The new error constants for batch donations and NFT operations are properly organized, concise, and within the felt252 character limit. The section headers improve code organization.
tests/test_campaign_donation.cairo (6)
137-143: Good improvement to donation verification.The addition of checking
donation_id != 0ensures the donation was actually found before verifying other properties. This prevents false positives when a default/empty donation struct is returned.
283-314: Excellent edge case coverage for closed campaigns.This test properly verifies that donations are rejected when a campaign has already reached its goal, ensuring the contract's business logic is correctly enforced.
358-402: Well-structured batch donation test.The test properly validates the batch donation feature by:
- Creating multiple campaigns
- Executing a batch donation to all campaigns
- Verifying both the total transfer and individual campaign balances
423-427: Good type safety practice.Using explicit
u32type for the loop counter improves code clarity and prevents potential type inference issues.
434-470: Excellent test for batch donation auto-capping.This test validates a key feature of batch donations - automatic capping of amounts that exceed campaign targets. This ensures donors aren't overcharged and campaigns don't receive more than needed.
1422-1492: Comprehensive integration test coverage.This integration test excellently validates the complete campaign lifecycle including:
- Token transfers to multiple donors
- Campaign creation
- Multiple donations from different donors
- Campaign state verification
- Donor tracking functionality
- Fund withdrawal by owner
This provides confidence in the contract's end-to-end functionality.
src/campaign_donation.cairo (2)
416-418: Good handling of not-found case.Using a zero-valued Donations struct as a sentinel value is a clean approach. The use of
try_into()properly handles the felt252 to ContractAddress conversion.
802-813: Well-implemented auto-capping logic.The auto-capping feature ensures campaigns don't receive more than their target amount, which is essential for batch donations. The early return when
actual_amount == 0prevents unnecessary processing.
| self | ||
| .emit( | ||
| Event::BatchDonationProcessed( | ||
| BatchDonationProcessed { | ||
| donor, | ||
| total_campaigns: campaign_amounts.len(), | ||
| successful_donations, | ||
| total_amount: actual_total_amount, | ||
| }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Event emission logic issues in batch donation.
There are two issues with the event emission:
- The
BatchDonationProcessedevent is emitted inside the loop for each donation, but it should be emitted once after all donations are processed - The event doesn't include the results array as mentioned in the past review comment
Move the BatchDonationProcessed event outside the loop and include results:
);
- self
- .emit(
- Event::BatchDonationProcessed(
- BatchDonationProcessed {
- donor,
- total_campaigns: campaign_amounts.len(),
- successful_donations,
- total_amount: actual_total_amount,
- },
- ),
- );
}
i += 1;
}
+
+ // Emit batch summary event once after processing all donations
+ self
+ .emit(
+ Event::BatchDonationProcessed(
+ BatchDonationProcessed {
+ donor,
+ total_campaigns: campaign_amounts.len(),
+ successful_donations,
+ total_amount: actual_total_amount,
+ results,
+ },
+ ),
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self | |
| .emit( | |
| Event::BatchDonationProcessed( | |
| BatchDonationProcessed { | |
| donor, | |
| total_campaigns: campaign_amounts.len(), | |
| successful_donations, | |
| total_amount: actual_total_amount, | |
| }, | |
| ), | |
| ); | |
| ); | |
| } | |
| i += 1; | |
| } | |
| // Emit batch summary event once after processing all donations | |
| self | |
| .emit( | |
| Event::BatchDonationProcessed( | |
| BatchDonationProcessed { | |
| donor, | |
| total_campaigns: campaign_amounts.len(), | |
| successful_donations, | |
| total_amount: actual_total_amount, | |
| results, | |
| }, | |
| ), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/campaign_donation.cairo around lines 384 to 394, the
BatchDonationProcessed event is currently emitted inside the loop for each
donation, but it should be emitted once after processing all donations. Move the
event emission outside the loop and modify the event to include the results
array as part of the BatchDonationProcessed data structure before emitting it.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/campaign_donation.cairo (1)
392-402: Critical issue: Event emitted inside loop instead of once after completion.This duplicates the past review comment that hasn't been addressed. The
BatchDonationProcessedevent is being emitted on every iteration, but should be emitted once after processing all donations.Move the event emission outside the loop:
); - self - .emit( - Event::BatchDonationProcessed( - BatchDonationProcessed { - donor, - total_campaigns: campaign_amounts.len(), - successful_donations, - total_amount: actual_total_amount, - }, - ), - ); } i += 1; } + + // Emit batch summary event once after processing all donations + self + .emit( + Event::BatchDonationProcessed( + BatchDonationProcessed { + donor, + total_campaigns: campaign_amounts.len(), + successful_donations, + total_amount: actual_total_amount, + results, + }, + ), + );
🧹 Nitpick comments (1)
src/campaign_donation.cairo (1)
706-791: Complex optimization may have readability trade-offs.The optimized validation function rebuilds arrays to update campaign totals, which adds significant complexity. Consider if the performance gain justifies the reduced readability and maintainability.
For future iterations, consider using a more efficient data structure if the complexity becomes a maintenance burden:
// Alternative approach using simpler logic at cost of some performance fn _validate_and_calculate_total_simple( self: @ContractState, campaign_amounts: @Array<(u256, u256)> ) -> u256 { // Simpler O(n²) approach that's more readable // Could be acceptable for small batch sizes (≤20) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/base/errors.cairo(3 hunks)src/campaign_donation.cairo(8 hunks)tests/test_campaign_donation.cairo(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/base/errors.cairo
🔇 Additional comments (15)
src/campaign_donation.cairo (7)
64-67: LGTM - New events properly defined.The new
BatchDonationProcessedandDonationResultEventevents are well-structured and will provide good tracking for batch donation operations.
122-131: LGTM - Event struct properly defined.The
DonationResultEventstruct includes all necessary fields for tracking individual donation results within batch operations.
325-337: LGTM - Input validation is comprehensive.The batch size limit, empty array check, and total amount validation provide good protection against misuse and ensure reasonable gas costs.
349-352: LGTM - Single token transfer optimization.Transferring the total amount once instead of per donation is an excellent gas optimization that maintains atomicity.
422-426: LGTM - Proper migration from deprecated constants.Using
try_into().unwrap()instead of deprecatedcontract_address_constis the correct approach for modern Cairo code.
624-637: LGTM - Well-designed helper structs.Both
CampaignBatchTotalandDonationResultstructs are properly designed with appropriateDropandCopytraits for efficient batch processing.
804-859: LGTM - Auto-capping logic is well-implemented.The donation processing correctly handles auto-capping when amounts exceed remaining campaign targets, which prevents over-donations and maintains campaign integrity.
tests/test_campaign_donation.cairo (8)
57-99: LGTM - Excellent test organization.The section headers make the test suite much more navigable and maintainable. This is a significant improvement in code organization.
137-142: LGTM - Improved assertion robustness.Checking that
donation_id != 0ensures the donation was actually found and not returning a default empty value. This makes the tests more reliable.
358-402: LGTM - Comprehensive batch donation test coverage.The batch donation tests cover the happy path with multiple campaigns and verify both total transfer amounts and individual campaign balances. This provides good confidence in the feature.
404-432: LGTM - Good edge case coverage.Testing empty arrays and oversized batches ensures the input validation works correctly and prevents misuse of the batch donation feature.
434-470: LGTM - Auto-capping test validates critical behavior.This test confirms that the auto-capping feature works correctly when donation amounts exceed campaign targets, which is essential for preventing over-donations.
698-779: LGTM - Comprehensive withdrawal test coverage.The new withdrawal tests cover important error conditions like unauthorized access, double withdrawals, and attempts to withdraw from unclosed campaigns. This significantly improves test coverage.
1422-1492: LGTM - Excellent integration test.This end-to-end test validates the complete campaign lifecycle including multiple donors, tracking, and withdrawal. It provides valuable confidence that all components work together correctly.
1128-1128: LGTM - Fixed missing stop call.Adding the missing
stop_cheat_caller_addressensures proper test cleanup and prevents side effects on subsequent tests.
| #[derive(Drop, starknet::Event)] | ||
| pub struct BatchDonationProcessed { | ||
| #[key] | ||
| pub donor: ContractAddress, | ||
| pub total_campaigns: u32, | ||
| pub successful_donations: u32, | ||
| pub total_amount: u256, | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing results field in BatchDonationProcessed event.
The event struct is missing the results field that should contain the array of DonationResult items, which is referenced in the batch donation logic but not included in the event definition.
Add the missing results field:
#[derive(Drop, starknet::Event)]
pub struct BatchDonationProcessed {
#[key]
pub donor: ContractAddress,
pub total_campaigns: u32,
pub successful_donations: u32,
pub total_amount: u256,
+ pub results: Array<DonationResult>,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Drop, starknet::Event)] | |
| pub struct BatchDonationProcessed { | |
| #[key] | |
| pub donor: ContractAddress, | |
| pub total_campaigns: u32, | |
| pub successful_donations: u32, | |
| pub total_amount: u256, | |
| } | |
| #[derive(Drop, starknet::Event)] | |
| pub struct BatchDonationProcessed { | |
| #[key] | |
| pub donor: ContractAddress, | |
| pub total_campaigns: u32, | |
| pub successful_donations: u32, | |
| pub total_amount: u256, | |
| + pub results: Array<DonationResult>, | |
| } |
🤖 Prompt for AI Agents
In src/campaign_donation.cairo around lines 141 to 149, the
BatchDonationProcessed event struct is missing the results field that should
hold an array of DonationResult items. Add a new field named results with the
appropriate type (likely an array or vector of DonationResult) to the struct
definition to match the batch donation logic and ensure all relevant data is
emitted in the event.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores