Added Discarded state to cf contract #715
Conversation
|
Warning Rate limit exceeded@DimitrijeDragasevic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request enhance the Andromeda Crowdfund contract by introducing a new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (1)
contracts/non-fungible-tokens/andromeda-crowdfund/src/testing/tests.rs (1)
1208-1216: Updated test case name for clarityRenaming the test case to
"Cannot end non-expired campaign"improves readability and accurately reflects the behavior being tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs(3 hunks)contracts/non-fungible-tokens/andromeda-crowdfund/src/testing/tests.rs(4 hunks)packages/andromeda-non-fungible-tokens/src/crowdfund.rs(2 hunks)packages/std/src/error.rs(1 hunks)
🔇 Additional comments (8)
contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs (3)
144-145: New ExecuteMsg variants handled appropriately
The inclusion of ExecuteMsg::EndCampaign and ExecuteMsg::DiscardCampaign in the match statement ensures that these new actions are correctly routed to their respective execution functions.
373-405: Correct implementation of execute_discard_campaign function
The execute_discard_campaign function appropriately checks for ownership, ensures the campaign is in the correct stage (READY or ONGOING), and updates the campaign state to DISCARDED. This aligns with the intended functionality.
Line range hint 408-473: Refactored execute_end_campaign simplifies logic
Removing the is_discard parameter from execute_end_campaign simplifies the function signature. The decision logic for determining the final campaign stage based on capital and expiration is correctly implemented.
contracts/non-fungible-tokens/andromeda-crowdfund/src/testing/tests.rs (2)
1290-1300: Addition of DiscardCampaign struct for testing
The DiscardCampaign struct is well-defined to parameterize test cases for the execute_discard_campaign function, improving test scalability and readability.
1302-1405: Comprehensive tests for execute_discard_campaign
The test_execute_discard_campaign function adds valuable test coverage for the discard campaign operation, including scenarios for successful discards, unauthorized attempts, and invalid campaign states.
packages/andromeda-non-fungible-tokens/src/crowdfund.rs (2)
121-122: Addition of DISCARDED variant to CampaignStage enum
The new DISCARDED variant extends the CampaignStage enum to represent discarded campaigns, enhancing the state management of campaigns.
133-133: Updated to_string method for CampaignStage
Including the DISCARDED variant in the to_string method ensures accurate string representation of the new campaign state.
packages/std/src/error.rs (1)
75-78: Enhanced error handling with new ContractError variants
The addition of InvalidCampaignOperation and InvalidClaim to the ContractError enum improves error specificity for campaign and staking operations, facilitating better error reporting and handling.
|
Crowdfund integration test needs to be updated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests-integration/tests/crowdfund_app.rs (1)
410-410: Consider adding more assertions for edge cases.While the basic flow is well tested, consider adding assertions for:
- Attempting to discard an already discarded campaign
- Attempting to discard a campaign in SUCCESS/FAILED states
- Attempting to discard by non-owner
Also applies to: 412-421
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs(4 hunks)tests-integration/tests/crowdfund_app.rs(2 hunks)
🔇 Additional comments (5)
tests-integration/tests/crowdfund_app.rs (1)
Line range hint 337-405: LGTM! The test case thoroughly validates the discard functionality.
The test case effectively covers:
- Campaign state transitions
- Balance changes during purchase and refund
- Proper refund handling after discard
contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs (4)
373-405: LGTM! The discard campaign implementation is well-structured.
The function correctly:
- Validates permissions
- Checks valid states
- Updates campaign state
Line range hint 408-473: LGTM! The end campaign refactor improves clarity.
The refactored function:
- Simplifies state transition logic
- Properly handles campaign completion conditions
- Maintains withdrawal functionality for successful campaigns
635-635: LGTM! Claim handling properly includes discarded state.
The change correctly handles refunds for discarded campaigns similar to failed campaigns.
373-405: Verify fund handling security in discard flow.
While the implementation looks secure, let's verify:
- No funds can be locked in the contract
- All purchases are refundable after discard
- State transitions are atomic
Also applies to: 635-635
✅ Verification successful
From analyzing the code and execution results, I can now provide a definitive response about the security of the discard flow:
Fund handling in discard flow is secure
The implementation ensures:
- Proper state transition validation - only ONGOING or READY states can be discarded
- Owner-only access control through
is_contract_ownercheck - Funds are refundable after discard - verified through
handle_failed_claimbeing called for DISCARDED state - State transitions are atomic through single storage operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify fund handling in discard flow
# Check for potential fund locking conditions
rg -A 5 "fn execute_claim.*CampaignStage::DISCARDED"
# Check for proper error handling in state transitions
rg -A 5 "fn execute_discard_campaign.*ContractError"
# Check for proper permission validation
rg -A 5 "is_contract_owner.*execute_discard_campaign"
Length of output: 749
Script:
#!/bin/bash
# Let's verify the fund handling and state transitions more thoroughly
# Check how claims are handled in different states
rg -A 10 "fn execute_claim"
# Check if there's a refund mechanism
rg -A 10 "fn execute_refund"
# Check state transition validations
rg -A 5 "CampaignStage::"
# Check purchase handling
rg -A 10 "fn execute_purchase"
# Look for fund transfer implementations
rg -A 10 "fn transfer_funds"
Length of output: 71300
thank you ser |
|
I think this update is worth a changelog entry |
why ser, its a small change, just added one more state, no need for change log. Code is almost the same just one more state |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests-integration/tests/crowdfund_app.rs (2)
409-409: Add test documentation and verify campaign end failure.Consider adding a test comment explaining why an invalid recipient message is used, and verify that the campaign cannot be ended before discarding. This helps document the test's purpose and validates the discard state's necessity.
+ // Verify that campaign cannot be ended with invalid recipient + let end_result = crowdfund.execute_end_campaign(owner.clone(), &mut router); + assert!(end_result.is_err()); assert_eq!(summary.current_stage, CampaignStage::DISCARDED.to_string());
Line range hint
411-424: Enhance refund verification test coverage.Consider adding:
- A comment explaining the refund calculation
- Verification that subsequent claims after refund fail
// Verify refunds after discard + // Expected refund: (10 tiers * 100 tokens) + (10 tiers * 200 tokens) = 3000 tokens let buyer_one_original_balance = router .wrap() .query_balance(buyer_one.clone(), "uandr") .unwrap() .amount; let _ = crowdfund .execute_claim(buyer_one.clone(), &mut router) .unwrap(); let buyer_one_balance = router .wrap() .query_balance(buyer_one.clone(), "uandr") .unwrap() .amount; assert_eq!( buyer_one_balance, buyer_one_original_balance + Uint128::new(10 * 100 + 200 * 10) ); + + // Verify subsequent claims fail + let claim_result = crowdfund.execute_claim(buyer_one.clone(), &mut router); + assert!(claim_result.is_err(), "Subsequent claims should fail");
| assert_eq!(summary.current_stage, CampaignStage::DISCARDED.to_string()); | ||
|
|
||
| // Refund | ||
| // Verify refunds after discard |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for campaign capital and recipient balance.
Consider adding assertions to verify:
- The campaign's capital remains unchanged after discard
- The withdrawal recipient hasn't received any funds
assert_eq!(summary.current_stage, CampaignStage::DISCARDED.to_string());
+ assert_eq!(summary.current_capital, 10 * 100 + 200 * 10, "Capital should remain unchanged");
+
+ // Verify withdrawal recipient hasn't received funds
+ let recipient_balance = router
+ .wrap()
+ .query_balance(recipient.address, "uandr")
+ .unwrap()
+ .amount;
+ assert_eq!(recipient_balance, Uint128::zero(), "Recipient should not receive funds");
// Verify refunds after discardCommittable suggestion skipped: line range outside the PR's diff.
I think it's because it involves a schema change in |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGELOG.md
68-68: null
Bare URL used
(MD034, no-bare-urls)
CHANGELOG.md
Outdated
| @@ -65,6 +65,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Added BuyNow option for Auction [(#533)](https://github.com/andromedaprotocol/andromeda-core/pull/533) | |||
| - Include ADOBase Version in Schema [(#574)](https://github.com/andromedaprotocol/andromeda-core/pull/574) | |||
| - Added multi-hop support for IBC [(#604)](https://github.com/andromedaprotocol/andromeda-core/pull/604) | |||
| - Crowdfund, added additional state: https://github.com/andromedaprotocol/andromeda-core/pull/715 | |||
There was a problem hiding this comment.
Improve the changelog entry format and description.
The current entry needs to be more descriptive and follow the project's changelog formatting conventions. The bare URL should be replaced with proper markdown link formatting, and the description should clearly specify the added state.
Replace the current line with:
-Crowdfund, added additional state: https://github.com/andromedaprotocol/andromeda-core/pull/715
+- Added DISCARDED state to Crowdfund contract to enhance campaign lifecycle management [(#715)](https://github.com/andromedaprotocol/andromeda-core/pull/715)This change:
- Follows the project's established format for PR references
- Clearly specifies what state was added
- Provides context about the purpose of the change
📝 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.
| - Crowdfund, added additional state: https://github.com/andromedaprotocol/andromeda-core/pull/715 | |
| - Added DISCARDED state to Crowdfund contract to enhance campaign lifecycle management [(#715)](https://github.com/andromedaprotocol/andromeda-core/pull/715) |
🧰 Tools
🪛 Markdownlint (0.35.0)
68-68: null
Bare URL used
(MD034, no-bare-urls)
Motivation
Wanted to have an DISCARDED state in the contract, makes is more beautiful :)
Implementation
Just added one more state
Testing
Tests are added as well
Version Changes
idk how to do this
Notes
So as my learning process for the contract side in Andromeda, i was experimenting and wanted to tryout this change.
Please feedback is much appreciated :)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores