Feat/reentrancy protection - #564
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds payment-stream reentrancy locks and coverage, updates protocol 25 contract snapshots, modernizes contract test expectations, and adjusts wallet restoration, stream timing, modal markup, and related client code. ChangesPayment-stream reentrancy protection
Protocol snapshot updates
Client and contract test updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
This PR is for issue #505 [Contract] Add Reentrancy Protection Guards Across All Payment Stream Functions |
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
2 similar comments
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
|
fixed the conflicts, please merge the PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/payment-stream/src/lib.rs (2)
1049-1060: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
withdrawreleases a lock it never acquires.This function calls
release_stream_lockat lines 1058 and 1114, but it never callsacquire_stream_lock. Withdrawal has no reentrancy guard, and a preset lock is cleared instead of rejected.test_reentrancy_guard_blocks_reentrant_withdrawcannot pass with this body.Acquire the lock as the first statement, after
assert_not_paused.🔒️ Proposed fix to acquire the lock in `withdraw`
pub fn withdraw(env: Env, stream_id: u64, amount: i128) { Self::assert_not_paused(&env); + Self::acquire_stream_lock(&env, stream_id); let mut stream: Stream = Self::get_stream(env.clone(), stream_id);🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 1049 - 1060, Update withdraw to call acquire_stream_lock as the first statement after assert_not_paused, before retrieving the stream or performing authorization checks. Keep the existing release_stream_lock calls for cleanup on failure and completion, so preset locks are rejected and reentrant withdrawals are blocked.
471-473: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefine
require_not_pausedbefore using it.
create_stream,deposit, andwithdrawcallSelf::require_not_paused(&env), but the contract only definesassert_not_paused,is_paused, and pause/resume logic. The crate will not compile until the helper exists or these call sites use an existing check.🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 471 - 473, Define the missing Self::require_not_paused helper used by create_stream, deposit, and withdraw, or replace those calls with the existing assert_not_paused check. Preserve the contract’s current paused-state validation behavior and remove the redundant or undefined helper usage so the crate compiles.
🧹 Nitpick comments (8)
contracts/campaign-funding/src/lib.rs (3)
1318-1352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test or refund every contributor.
The name states that all contributors are refunded. The body refunds only
contrib4onid2. Campaignidwithcontrib1,contrib2, andcontrib3staysActiveand is never refunded, so the multi-contributor refund path is untested. Create one failed campaign with three contributors belowmin_target, then refund each one and assert every balance.🤖 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 `@contracts/campaign-funding/src/lib.rs` around lines 1318 - 1352, Update test_refund_multiple_contributors_all_refunded to use one failed campaign with contrib1, contrib2, and contrib3 contributing below min_target; trigger expiry, refund each contributor, and assert all three restored balances, removing the separate id2/contrib4 setup.
597-611: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider recording the fee rate per campaign.
calculate_feereads the current globalFeeRateat claim time. If the admin callsset_fee_rateafter a campaign becomesSuccessful, the creator receives a different net amount than the rate in force during the campaign. Storing the rate in theCampaignrecord at creation makes the fee deterministic for contributors and creators.🤖 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 `@contracts/campaign-funding/src/lib.rs` around lines 597 - 611, Record the active fee rate in each Campaign when it is created, and update calculate_fee to use that campaign-specific value instead of the mutable global FeeRate. Ensure campaign creation reads the current configured rate and persists it, while preserving existing fee validation and claim behavior.
1161-1177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the contradictory comment.
Line 1166 calls
env.mock_all_auths(), but the comment on Line 1174 states that no auth mocking is used.trigger_expiryrequires no auth, so the test can dropmock_all_authsaftersetup_contractandcreate_campaign, or the comment must be corrected.🤖 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 `@contracts/campaign-funding/src/lib.rs` around lines 1161 - 1177, Resolve the contradiction in test_trigger_expiry_permissionless by removing the unnecessary env.mock_all_auths() call while preserving setup_contract and create_campaign behavior, or update the nearby comment to accurately describe the authentication mocking. Ensure the test still verifies trigger_expiry can be called permissionlessly.contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json (1)
155-195: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRename the zero-contribution expiry test to describe its actual outcome.
test_trigger_expiry_with_zero_contributions_failssetstotal_raisedto 0 on an active campaign past the deadline and assertsstatus=Failed. The function does not panic for this input, so the test name should not imply panicking.🤖 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 `@contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json` around lines 155 - 195, Rename test_trigger_expiry_with_zero_contributions_fails to describe the campaign becoming Failed after expiry with zero contributions, without implying that the function panics. Update any references to the test name while preserving its existing assertions and behavior.contracts/campaign-funding/test_snapshots/tests/test_refund_multiple_contributors_all_refunded.1.json (1)
435-457: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRecord refunds for every contributor in this snapshot.
test_refund_multiple_contributors_all_refundedonly callsrefund(&contrib4, &id2)after expiry, so the snapshot correctly shows one refund. Rename the test to describe the single-contributor refund, or update the test to resolve the three-contributor campaign toFailedand verify each contributor’s refund.🤖 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 `@contracts/campaign-funding/test_snapshots/tests/test_refund_multiple_contributors_all_refunded.1.json` around lines 435 - 457, Update test_refund_multiple_contributors_all_refunded to resolve the three-contributor campaign to Failed after expiry, invoke refund for each contributor, and assert the snapshot records all three refunds; alternatively, rename the test and snapshot to describe the current single-contributor refund behavior.contracts/payment-stream/src/lib.rs (3)
1512-1517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant status test.
stream.status != StreamStatus::Activealready covers thePausedcase.- if stream.status == StreamStatus::Paused || stream.status != StreamStatus::Active { + if stream.status != StreamStatus::Active { return 0; }🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 1512 - 1517, In withdrawable_amount, simplify the stream status guard by removing the redundant explicit StreamStatus::Paused comparison and retain only the StreamStatus::Active check, preserving the existing zero return for every non-active stream.
1603-1608: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn
NotInitializedinstead of panicking on a missing admin.Both setters call
.unwrap()on the admin lookup. On an uninitialised contract this produces an opaque host panic, and the global lock stays set in temporary storage because no release runs.emergency_pauseat line 377 already usesunwrap_or_else(|| panic_with_error!(&env, Error::NotInitialized)). Use the same form here.Also applies to: 1636-1641
🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 1603 - 1608, Update the admin lookup in both setter authorization blocks to replace unwrap-based failure with panic_with_error! returning Error::NotInitialized, matching emergency_pause. Preserve the existing admin.require_auth() flow when an admin is present.
606-609: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
deposittransfers before it updates state.The token transfer at line 609 runs before the balance write at line 611. Every other mutating function in this PR commits state first. Move the transfer after the state write to keep one consistent ordering.
🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 606 - 609, In the deposit flow, update the state and balance before performing the token transfer. Move the token::Client transfer call after the existing balance write, preserving the current amount and recipient arguments and the reentrancy guard.
🤖 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 `@contracts/campaign-funding/src/lib.rs`:
- Around line 1021-1032: Update test_contribute_to_nonexistent_campaign to
expect the CampaignNotFound panic produced by load_campaign for missing campaign
99, and revise the adjacent comment to describe that behavior without claiming
it is wrapped as CampaignNotActive.
- Around line 449-456: Update the `claim_funds` documentation’s `# Errors` list
to remove `Error::Unauthorized`, since `campaign.creator.require_auth()`
produces the authorization failure directly rather than that contract error.
Keep the remaining documented error variants unchanged.
- Around line 357-397: Reorder contribute so all state effects occur before the
external token_client.transfer interaction: persist the updated contributor
balance and campaign status/total through Self::save_campaign, then perform the
transfer. Preserve the existing hard-cap check and events, ensuring reentrant
calls observe the updated campaign and contribution state.
In
`@contracts/campaign-funding/test_snapshots/tests/test_create_campaign_success.1.json`:
- Around line 73-81: Regenerate the campaign-funding snapshots using the
workspace-pinned soroban-sdk version 25.3.2, then commit all updated files under
the campaign-funding test_snapshots/tests directory, including the ledger
metadata shown in test_create_campaign_success.1.json.
In `@contracts/payment-stream/src/lib.rs`:
- Around line 650-654: Remove the truncated duplicate withdraw stub and all
stale duplicate definitions of get_stream, set_delegate, revoke_delegate,
get_delegate, withdrawable_amount, calculate_protocol_fee,
default_stream_metrics, and assert_is_recipient_or_delegate in
contracts/payment-stream/src/lib.rs:650-654, retaining one valid definition of
each. Also remove the spliced set_protocol_fee_rate declaration from
set_delegate at contracts/payment-stream/src/lib.rs:1365-1378 and restore a
single coherent set_delegate body.
- Around line 326-337: Unify initialize and all related reads/writes on the
documented Symbol and tuple-key scheme instead of mixing DataKey variants. In
contracts/payment-stream/src/lib.rs lines 326-337, define/document the chosen
keys; update the AlreadyInitialized guard at lines 317-320 to read the Symbol
admin key, stream counter logic at lines 485-497 to use the Symbol stream_count
key, and protocol metrics writes at lines 530-546 and 1257-1264 to use the
Symbol protocol_metrics key. Update stream creation writes at lines 611-624 and
1083-1097 to store streams under raw stream_id and per-stream metrics under
(stream_id, "metrics").
- Around line 1257-1264: Update the protocol metrics read in resume_stream to
use the same DataKey::ProtocolMetrics key used by the subsequent write,
replacing the ad hoc Symbol::new lookup. Preserve the existing increment of
total_active_streams and TTL update so resumed streams restore the metric
correctly.
In `@contracts/payment-stream/src/test.rs`:
- Around line 2437-2444: Add an explicit assert_ne!(stream_id1, stream_id2)
after the two stream creations in both affected tests, using the existing
stream_id1 and stream_id2 symbols. Keep the cancellation and status assertions
unchanged so the tests fail immediately if stream IDs are not unique.
- Around line 2204-2227: Update withdraw to call acquire_stream_lock for the
stream before processing, and ensure the lock is released on every successful
exit. Strengthen test_reentrancy_guard_blocks_reentrant_withdraw to verify the
specific reentrancy error rather than ambiguous code `#17`, and add a
successful-withdraw assertion that the stream lock key is removed afterward.
- Around line 2223-2224: Import Symbol from soroban_sdk in the test module of
contracts/payment-stream/src/test.rs. Ensure the import covers the Symbol::new
usages for both "lock" and "g_lock" keys, while leaving the existing test logic
unchanged.
---
Outside diff comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 1049-1060: Update withdraw to call acquire_stream_lock as the
first statement after assert_not_paused, before retrieving the stream or
performing authorization checks. Keep the existing release_stream_lock calls for
cleanup on failure and completion, so preset locks are rejected and reentrant
withdrawals are blocked.
- Around line 471-473: Define the missing Self::require_not_paused helper used
by create_stream, deposit, and withdraw, or replace those calls with the
existing assert_not_paused check. Preserve the contract’s current paused-state
validation behavior and remove the redundant or undefined helper usage so the
crate compiles.
---
Nitpick comments:
In `@contracts/campaign-funding/src/lib.rs`:
- Around line 1318-1352: Update test_refund_multiple_contributors_all_refunded
to use one failed campaign with contrib1, contrib2, and contrib3 contributing
below min_target; trigger expiry, refund each contributor, and assert all three
restored balances, removing the separate id2/contrib4 setup.
- Around line 597-611: Record the active fee rate in each Campaign when it is
created, and update calculate_fee to use that campaign-specific value instead of
the mutable global FeeRate. Ensure campaign creation reads the current
configured rate and persists it, while preserving existing fee validation and
claim behavior.
- Around line 1161-1177: Resolve the contradiction in
test_trigger_expiry_permissionless by removing the unnecessary
env.mock_all_auths() call while preserving setup_contract and create_campaign
behavior, or update the nearby comment to accurately describe the authentication
mocking. Ensure the test still verifies trigger_expiry can be called
permissionlessly.
In
`@contracts/campaign-funding/test_snapshots/tests/test_refund_multiple_contributors_all_refunded.1.json`:
- Around line 435-457: Update test_refund_multiple_contributors_all_refunded to
resolve the three-contributor campaign to Failed after expiry, invoke refund for
each contributor, and assert the snapshot records all three refunds;
alternatively, rename the test and snapshot to describe the current
single-contributor refund behavior.
In
`@contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json`:
- Around line 155-195: Rename test_trigger_expiry_with_zero_contributions_fails
to describe the campaign becoming Failed after expiry with zero contributions,
without implying that the function panics. Update any references to the test
name while preserving its existing assertions and behavior.
In `@contracts/payment-stream/src/lib.rs`:
- Around line 1512-1517: In withdrawable_amount, simplify the stream status
guard by removing the redundant explicit StreamStatus::Paused comparison and
retain only the StreamStatus::Active check, preserving the existing zero return
for every non-active stream.
- Around line 1603-1608: Update the admin lookup in both setter authorization
blocks to replace unwrap-based failure with panic_with_error! returning
Error::NotInitialized, matching emergency_pause. Preserve the existing
admin.require_auth() flow when an admin is present.
- Around line 606-609: In the deposit flow, update the state and balance before
performing the token transfer. Move the token::Client transfer call after the
existing balance write, preserving the current amount and recipient arguments
and the reentrancy guard.
🪄 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 Plus
Run ID: dcab643e-33a3-458b-b918-7cf66925cba8
📒 Files selected for processing (44)
contracts/Cargo.tomlcontracts/campaign-funding/Cargo.tomlcontracts/campaign-funding/src/lib.rscontracts/campaign-funding/test_snapshots/tests/test_claim_funds_double_claim.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_on_active_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_on_failed_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_zero_fee.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_accumulates.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_after_deadline.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_auto_succeed_on_hard_cap.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_exceeds_hard_cap.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_multiple_contributors.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_to_nonexistent_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_zero_amount.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_deadline_in_past.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_ids_increment.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_min_target_exceeds_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_not_initialized.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_zero_min_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_zero_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_fee_calculation_precision.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_fee_too_high.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_twice_fails.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_double_refund_prevented.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_multiple_contributors_all_refunded.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_no_contribution.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_on_active_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_on_successful_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_collector.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_rate.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_rate_too_high.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_already_resolved.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_before_deadline.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_sets_failed_when_target_not_met.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_sets_successful_when_target_met.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_with_zero_contributions_fails.1.jsoncontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rs
| /// # Errors | ||
| /// * [`Error::CampaignNotSuccessful`] — campaign is not `Successful`. | ||
| /// * [`Error::AlreadyClaimed`] — funds were already claimed. | ||
| /// * [`Error::Unauthorized`] — caller is not the campaign creator. | ||
| pub fn claim_funds(env: Env, campaign_id: u64) { | ||
| let mut campaign = Self::load_campaign(&env, campaign_id); | ||
|
|
||
| campaign.creator.require_auth(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the # Errors list for claim_funds.
The doc lists Error::Unauthorized, but the code never returns it. A non-creator caller fails inside creator.require_auth() with an authorization error, not contract error #3. Error::Unauthorized is unused in the whole file.
📝 Proposed doc fix
/// # Errors
/// * [`Error::CampaignNotSuccessful`] — campaign is not `Successful`.
/// * [`Error::AlreadyClaimed`] — funds were already claimed.
- /// * [`Error::Unauthorized`] — caller is not the campaign creator.
+ ///
+ /// A caller other than `creator` fails the `require_auth` check with an
+ /// authorization error, not a contract error.📝 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.
| /// # Errors | |
| /// * [`Error::CampaignNotSuccessful`] — campaign is not `Successful`. | |
| /// * [`Error::AlreadyClaimed`] — funds were already claimed. | |
| /// * [`Error::Unauthorized`] — caller is not the campaign creator. | |
| pub fn claim_funds(env: Env, campaign_id: u64) { | |
| let mut campaign = Self::load_campaign(&env, campaign_id); | |
| campaign.creator.require_auth(); | |
| /// # Errors | |
| /// * [`Error::CampaignNotSuccessful`] — campaign is not `Successful`. | |
| /// * [`Error::AlreadyClaimed`] — funds were already claimed. | |
| /// | |
| /// A caller other than `creator` fails the `require_auth` check with an | |
| /// authorization error, not a contract error. | |
| pub fn claim_funds(env: Env, campaign_id: u64) { | |
| let mut campaign = Self::load_campaign(&env, campaign_id); | |
| campaign.creator.require_auth(); |
🤖 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 `@contracts/campaign-funding/src/lib.rs` around lines 449 - 456, Update the
`claim_funds` documentation’s `# Errors` list to remove `Error::Unauthorized`,
since `campaign.creator.require_auth()` produces the authorization failure
directly rather than that contract error. Keep the remaining documented error
variants unchanged.
| let lock_key = (stream_id, Symbol::new(&env, "lock")); | ||
| env.storage().temporary().set(&lock_key, &true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check how Symbol reaches the payment-stream test module.
set -euo pipefail
rg -n 'use soroban_sdk' contracts/payment-stream/srcRepository: Fundable-Protocol/stellar_client_os
Length of output: 543
🏁 Script executed:
#!/bin/bash
# Inspect the payment-stream crate root and test module imports to see what Symbol can come from.
set -euo pipefail
printf '--- lib.rs ---\n'
cat -n contracts/payment-stream/src/lib.rs | sed -n '1,120p'
printf '\n--- test.rs imports and surrounding test section ---\n'
cat -n contracts/payment-stream/src/test.rs | sed -n '1,25p;2200,2235p'
printf '\n--- all Symbol definitions/usages in payment-stream/src ---\n'
rg -n '\b(Symbol|use\s+[^;]*Symbol[^;]*|\bSymbol\b)' contracts/payment-stream/src -M 2Repository: Fundable-Protocol/stellar_client_os
Length of output: 264
🏁 Script executed:
#!/bin/bash
# Inspect the payment-stream crate root and test module imports to see how Symbol is available.
set -euo pipefail
printf '%s\n' '--- lib.rs ---'
cat -n contracts/payment-stream/src/lib.rs | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- test.rs imports and surrounding test section ---'
cat -n contracts/payment-stream/src/test.rs | sed -n '1,25p;2200,2235p'
printf '%s\n' ''
printf '%s\n' '--- Symbol symbols in payment-stream/src ---'
rg -n '\b(Symbol|use\s+[^;]*Symbol[^;]*|\bSymbol\b)' contracts/payment-stream/src -M 2 || trueRepository: Fundable-Protocol/stellar_client_os
Length of output: 13457
🏁 Script executed:
#!/bin/bash
# Check whether lib.rs re-exports Symbol or imports it from soroban_sdk.
set -euo pipefail
printf '%s\n' '--- lib.rs use block ---'
sed -n '1,3p' contracts/payment-stream/src/lib.rs
printf '%s\n' ''
printf '%s\n' '--- lib.rs pub re-exports mentioning Symbol ---'
rg -n 'pub\s+use\b|Symbol' contracts/payment-stream/src/lib.rs || true
printf '%s\n' ''
printf '%s\n' '--- complete test module imports ---'
sed -n '1,8p' contracts/payment-stream/src/test.rs
printf '%s\n' ''
printf '%s\n' '--- all Symbol::new calls in tests under a module import ---'
python3 - <<'PY'
from pathlib import Path
p = Path("contracts/payment-stream/src/test.rs")
text = p.read_text()
lines = text.splitlines()
for i,l in enumerate(lines,1):
if "Symbol::new" in l:
before = "\n".join(f"{j}:{lines[j-1]}" for j in range(max(1,i-15), i))
print(f"\n-- lines {max(1,i-15)}-{i} --")
print(before)
PYRepository: Fundable-Protocol/stellar_client_os
Length of output: 14500
🏁 Script executed:
#!/bin/bash
# Check Soroban SDK symbol availability in manifests/config without modifying or running repository code.
set -euo pipefail
printf '%s\n' '--- cargo manifests mentioning soroban-sdk ---'
rg -n 'soroban-sdk|soroban_env|symbol|Symbol' -S --glob 'Cargo.toml' --glob 'Cargo.lock' . || true
printf '%s\n' ''
printf '%s\n' '--- git diff summary and relevant symbol changes ---'
git diff --stat || true
git diff -- contracts/payment-stream/src/lib.rs contracts/payment-stream/src/test.rs | sed -n '1,220p' || trueRepository: Fundable-Protocol/stellar_client_os
Length of output: 1471
🌐 Web query:
Soroban SDK 25 Symbol import crate::Symbol from soroban_sdk
💡 Result:
To use the Symbol type in the Soroban SDK (including version 25), you import it directly from the soroban_sdk crate [1][2]: use soroban_sdk::Symbol; Usage Overview: 1. Symbol Basics: Symbol is a short string type (maximum 32 characters) with a limited character set (a-zA-Z0-9_) [1][3]. It is commonly used for symbolic identifiers like function names or field names [1]. 2. Creating Symbols: - For short strings (9 characters or less), you should use the symbol_short! macro [3][4]. This performs the conversion at compile time, which is more efficient and results in smaller Wasm binaries [3][5]. use soroban_sdk::symbol_short; let my_symbol = symbol_short!("transfer"); - For longer strings (up to 32 characters) or dynamic values, use Symbol::new [3][5]: use soroban_sdk::{Env, Symbol}; let my_symbol = Symbol::new(&env, "longsymbolname123"); 3. Recommendations: Avoid using deprecated methods like Symbol::short (except via the macro) [5]. Always prefer symbol_short! for constants to ensure compile-time evaluation [3][5].
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban%5Fsdk/struct.Symbol.html
- 2: https://docs.rs/crate/soroban-sdk/25.3.1
- 3: https://www.mintlify.com/stellar/rs-soroban-sdk/api/types/symbol
- 4: https://docs.rs/soroban-sdk/latest/soroban_sdk/macro.symbol_short.html
- 5: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/symbol.rs.html
Import Symbol in the test module.
use super::* does not bring Symbol into scope because contracts/payment-stream/src/lib.rs does not import or re-export it. This makes Symbol::new(&env, "lock"), Symbol::new(&env, "g_lock"), and the related test uses unresolved; add Symbol to the soroban_sdk import path.
🤖 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 `@contracts/payment-stream/src/test.rs` around lines 2223 - 2224, Import Symbol
from soroban_sdk in the test module of contracts/payment-stream/src/test.rs.
Ensure the import covers the Symbol::new usages for both "lock" and "g_lock"
keys, while leaving the existing test logic unchanged.
| token_admin.mint(&sender, &2000); | ||
| let stream_id1 = client.create_stream(&sender, &recipient, &token, &1000, &1000, &0, &100); | ||
| let stream_id2 = client.create_stream(&sender, &recipient, &token, &1000, &1000, &0, &100); | ||
| client.cancel_stream(&stream_id1); | ||
| client.cancel_stream(&stream_id2); | ||
| assert_eq!(client.get_stream(&stream_id1).status, StreamStatus::Canceled); | ||
| assert_eq!(client.get_stream(&stream_id2).status, StreamStatus::Canceled); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These tests assume two distinct stream ids.
Both tests create two streams and treat stream_id1 and stream_id2 as different. The counter defect in contracts/payment-stream/src/lib.rs at line 493 keeps the counter at 0, so both calls return 1, the second stream overwrites the first, and the isolation assertion no longer proves anything. Add an explicit assert_ne!(stream_id1, stream_id2); so the tests fail loudly if the counter regresses.
Also applies to: 2461-2471
🤖 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 `@contracts/payment-stream/src/test.rs` around lines 2437 - 2444, Add an
explicit assert_ne!(stream_id1, stream_id2) after the two stream creations in
both affected tests, using the existing stream_id1 and stream_id2 symbols. Keep
the cancellation and status assertions unchanged so the tests fail immediately
if stream IDs are not unique.
|
@opratem 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! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/campaign-funding/test_snapshots/tests/test_contribute_success.1.json (1)
157-165: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove obsolete protocol-22 snapshots.
The 13 protocol-22 files do not correspond to current test functions. The active snapshots use protocol 25 with
soroban-sdk = 25.3.2. Remove the orphaned files instead of regenerating them.🤖 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 `@contracts/campaign-funding/test_snapshots/tests/test_contribute_success.1.json` around lines 157 - 165, Remove the obsolete orphaned protocol-22 snapshot files from the campaign-funding test snapshots; retain the active protocol-25 snapshots, including test_contribute_success.1.json, unchanged.
🤖 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 `@apps/web/src/components/organisms/wallet-modal.tsx`:
- Around line 136-139: Move aria-modal="true" from the nested wrapper div to the
Radix DialogContent element, leaving the wrapper’s other attributes unchanged.
In
`@contracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_during_dispute.1.json`:
- Around line 4-5: Restore ReentrancyGuard and its lock helpers in
contracts/payment-stream/src/lib.rs, applying them to public methods that
perform token transfers, and restore the removed reentrancy tests. Resolve the
dependency conflict using soroban-sdk = "=25.3.2", then run cargo test --all to
regenerate snapshots while preserving protocol_version 25 and mux_id 0.
---
Nitpick comments:
In
`@contracts/campaign-funding/test_snapshots/tests/test_contribute_success.1.json`:
- Around line 157-165: Remove the obsolete orphaned protocol-22 snapshot files
from the campaign-funding test snapshots; retain the active protocol-25
snapshots, including test_contribute_success.1.json, unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f749f423-85e0-456b-b4cd-63d0f6ad1283
📒 Files selected for processing (85)
apps/web/src/components/map/FundableMapView.tsxapps/web/src/components/modules/payment-stream/CreatePaymentStream.tsxapps/web/src/components/organisms/wallet-modal.tsxapps/web/src/providers/StellarWalletProvider.tsxcontracts/campaign-funding/src/lib.rscontracts/campaign-funding/test_snapshots/tests/test_claim_funds_double_claim.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_on_active_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_on_failed_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_claim_funds_zero_fee.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_accumulates.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_after_deadline.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_auto_succeed_on_hard_cap.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_exceeds_hard_cap.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_multiple_contributors.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_to_nonexistent_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_contribute_zero_amount.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_deadline_in_past.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_ids_increment.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_min_target_exceeds_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_not_initialized.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_zero_min_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_create_campaign_zero_target.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_fee_calculation_precision.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_fee_too_high.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_initialize_twice_fails.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_double_refund_prevented.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_multiple_contributors_all_refunded.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_no_contribution.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_on_active_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_on_successful_campaign.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_refund_success.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_collector.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_rate.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_set_fee_rate_too_high.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_already_resolved.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_before_deadline.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_sets_failed_when_target_not_met.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_sets_successful_when_target_met.1.jsoncontracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_with_zero_contributions_fails.1.jsoncontracts/dispute-arbiter/src/lib.rscontracts/dispute-arbiter/test_snapshots/test/test_cast_vote_approve_consensus.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_cast_vote_reject_consensus.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_create_dispute.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_double_initialize_fails.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_double_vote_rejected.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_evidence_request_consensus.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_force_resolve_timeout.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_initialize.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_non_assigned_arbiter_rejected.1.jsoncontracts/dispute-arbiter/test_snapshots/test/test_not_enough_arbiters_rejected.1.jsoncontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_cancel_already_executed_resolution_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_queued_resolution_restores_paused_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_queued_resolution_restores_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_stream_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_delegation_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_exceeds_total.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_inactive_stream_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_provider_not_set.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_respects_actual_amount_received.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_same_asset_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_slippage_exceeded.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_slippage_exceeded_by_contract_check.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_with_swap_success.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_nonexistent_resolution_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_after_timelock_succeeds.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_before_timelock_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_partial_refunds_residual_to_sender.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_execute_resolution_twice_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_get_swap_provider_roundtrip.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_blocked_during_dispute.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_already_disputed_fails.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_invalid_amounts_exceeding_balance.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_negative_amounts_rejected.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_queues_and_pauses_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resolve_dispute_requires_admin_auth.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_swap_provider_unauthorized.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_unauthorized_non_recipient_set_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_blocked_during_dispute.1.json
💤 Files with no reviewable changes (2)
- contracts/campaign-funding/src/lib.rs
- apps/web/src/providers/StellarWalletProvider.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/payment-stream/src/test.rs (1)
3666-3703: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal-lock coverage misses
emergency_pauseandemergency_unpause.These two tests cover
set_protocol_fee_rateandset_fee_collector.emergency_pauseatcontracts/payment-stream/src/lib.rsline 402 andemergency_unpauseat line 439 also acquire the global lock, and neither has a blocking test.Add one test per function, following the same shape:
💚 Proposed test
#[test] #[should_panic(expected = "Error(Contract, `#32`)")] fn test_reentrancy_guard_blocks_global_emergency_pause() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); let fee_collector = Address::generate(&env); let contract_id = env.register(PaymentStreamContract, ()); let client = PaymentStreamContractClient::new(&env, &contract_id); client.initialize(&admin, &fee_collector, &0); env.as_contract(&contract_id, || { let key = Symbol::new(&env, "g_lock"); env.storage().temporary().set(&key, &true); }); client.emergency_pause(); }🤖 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 `@contracts/payment-stream/src/test.rs` around lines 3666 - 3703, Add blocking tests for both PaymentStreamContractClient::emergency_pause and emergency_unpause in the test module, matching the existing global-lock tests: initialize the contract, set temporary g_lock to true inside env.as_contract, invoke the target method, and assert the expected Error(Contract, `#32`) panic.contracts/payment-stream/src/lib.rs (2)
693-694: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe per-stream lock in
create_stream_internalcannot block anything.Line 689 allocates
stream_idfrom a counter that only increases. No other frame can already hold the lock for that id, so the check at line 693 always passes. The guard protects a resource that does not yet exist.The reentrancy vector during the escrow
transferat line 751 is a malicious token calling back intocreate_stream(allocating further ids and re-readingStreamCount) or intodepositon a different stream. A lock keyed by the new id stops neither.The cost is two extra temporary-storage writes per created stream.
create_batch_streamsmultiplies that by up to 50;test_create_batch_streams_50already needsdisable_resource_limits().Either drop the guard here, or guard the shared resource that is actually at risk — the
StreamCountcounter — with the global lock.♻️ Option A: remove the ineffective per-id lock
env.storage().instance().set(&DataKey::StreamCount, &stream_count); - Self::acquire_stream_lock(&env, stream_id); - let current_time = env.ledger().timestamp();- Self::release_stream_lock(&env, stream_id); stream_id }♻️ Option B: guard the counter with the global lock
- Self::acquire_stream_lock(&env, stream_id); + Self::acquire_global_lock(&env);- Self::release_stream_lock(&env, stream_id); + Self::release_global_lock(&env); stream_id }Option B serialises stream creation against itself and against fee administration. Confirm that
create_batch_streamsstill works, because it callscreate_stream_internalin a loop and each iteration must release the global lock before the next acquires it.Also applies to: 754-756
🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 693 - 694, Remove the ineffective per-stream lock acquisition and release from create_stream_internal, since the newly allocated stream_id cannot already be locked. Do not add a replacement lock unless guarding the shared StreamCount counter globally; if choosing that approach, ensure the lock is released before each create_batch_streams iteration so batch creation remains functional.
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DataKey::Lockis declared but never used.The lock helpers at lines 319-331 key temporary storage with the tuple
(stream_id, Symbol::new(env, "lock")). Nothing constructsDataKey::Lock(stream_id). This leaves two documented key schemes for one concept and adds an unused enum variant to the contract type surface.Pick one scheme.
DataKey::Lock(stream_id)is consistent with every other key in this contract.♻️ Proposed change (helpers use the enum variant)
fn acquire_stream_lock(env: &Env, stream_id: u64) { - let key = (stream_id, Symbol::new(env, "lock")); + let key = DataKey::Lock(stream_id); if env.storage().temporary().get::<_, bool>(&key).unwrap_or(false) { panic_with_error!(env, Error::ReentrancyGuard); } env.storage().temporary().set(&key, &true); } fn release_stream_lock(env: &Env, stream_id: u64) { - let key = (stream_id, Symbol::new(env, "lock")); + let key = DataKey::Lock(stream_id); env.storage().temporary().remove(&key); }If you change the key, update the tests in
contracts/payment-stream/src/test.rsand regenerate the affected snapshots, because the snapshots record the literal key shape.🤖 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 `@contracts/payment-stream/src/lib.rs` around lines 25 - 26, Update the lock helpers to use DataKey::Lock(stream_id) for temporary-storage access instead of the tuple key, then remove the now-redundant Lock enum inconsistency. Adjust the affected tests and regenerate snapshots so they reflect the enum-based key shape.
🤖 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 `@contracts/payment-stream/src/test.rs`:
- Around line 3723-3726: Update both release assertions around the lock_key in
the affected tests to verify that temporary storage no longer contains the
entry, rather than interpreting a stored false value as absence. Replace the
boolean-value check with the storage API’s direct absence check while preserving
the existing lock_key construction and contract context.
---
Nitpick comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 693-694: Remove the ineffective per-stream lock acquisition and
release from create_stream_internal, since the newly allocated stream_id cannot
already be locked. Do not add a replacement lock unless guarding the shared
StreamCount counter globally; if choosing that approach, ensure the lock is
released before each create_batch_streams iteration so batch creation remains
functional.
- Around line 25-26: Update the lock helpers to use DataKey::Lock(stream_id) for
temporary-storage access instead of the tuple key, then remove the now-redundant
Lock enum inconsistency. Adjust the affected tests and regenerate snapshots so
they reflect the enum-based key shape.
In `@contracts/payment-stream/src/test.rs`:
- Around line 3666-3703: Add blocking tests for both
PaymentStreamContractClient::emergency_pause and emergency_unpause in the test
module, matching the existing global-lock tests: initialize the contract, set
temporary g_lock to true inside env.as_contract, invoke the target method, and
assert the expected Error(Contract, `#32`) panic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47379e57-71af-4378-a6e8-a970b4132305
📒 Files selected for processing (16)
apps/web/src/components/organisms/wallet-modal.tsxcontracts/campaign-funding/src/lib.rscontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_independent_streams_use_separate_locks.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_global_set_fee_collector.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_global_set_fee_rate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_cancel.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_deposit.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_pause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_revoke_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_set_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_guard_blocks_reentrant_withdraw.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_lock_released_after_successful_cancel.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_reentrancy_lock_released_after_successful_withdraw.1.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/organisms/wallet-modal.tsx
|
fixed now, can you review and merge the PR |
2 participants
Closes #505
PR: Add reentrancy protection to
payment-streamcontract (#505)Summary
Adds per-stream and global reentrancy guards to every state-mutating function in the
payment-streamSoroban contract, closes issue #505.Motivation
The
payment-streamcontract performs token transfers (viatransfer/transfer_from) inside several functions. Without reentrancy protection a malicious token contract could re-enter any of those functions mid-execution and manipulate stream state (balances, status) before the initial call has written its effects back. This class of bug has caused significant losses in EVM-based streaming protocols; Soroban's synchronous host makes the attack surface different but not absent, especially when cross-contract calls are involved.Design
Guard mechanism
Reentrancy is detected using Soroban temporary storage boolean flags. Temporary storage is:
Lock granularity
(stream_id, Symbol("lock"))create_stream,deposit,withdraw,pause_stream,resume_stream,cancel_stream,set_delegate,revoke_delegateSymbol("g_lock")set_protocol_fee_rate,set_fee_collectorPer-stream locks are preferred over a single global lock so that operations on independent streams do not block each other.
withdraw_maxis not given its own lock; it delegates entirely towithdraw, which holds the lock for the duration.Error code
Error::ReentrancyGuard = 17is added to the existing error enum.Four private helpers
All guarded functions follow check-effects-interactions order: auth checks → state validation → state mutation → lock release → token transfer. The lock is acquired at the very top of each function and released immediately before the token transfer call.
Changes
contracts/payment-stream/src/lib.rsError::ReentrancyGuard = 17added to theErrorenum.acquire_stream_lock,release_stream_lock,acquire_global_lock,release_global_lock.create_stream,deposit,withdraw,pause_stream,resume_stream,cancel_stream,set_delegate,revoke_delegate.set_protocol_fee_rate,set_fee_collector.# Errorssections listing all possible error codes.contracts/payment-stream/src/test.rs13 new tests added (appended to the existing suite):
test_reentrancy_guard_blocks_reentrant_withdrawwithdrawpanics#17test_reentrancy_guard_blocks_reentrant_depositdepositpanics#17test_reentrancy_guard_blocks_reentrant_cancelcancel_streampanics#17test_reentrancy_guard_blocks_reentrant_pausepause_streampanics#17test_reentrancy_guard_blocks_reentrant_resumeresume_streampanics#17test_reentrancy_guard_blocks_reentrant_set_delegateset_delegatepanics#17test_reentrancy_guard_blocks_reentrant_revoke_delegaterevoke_delegatepanics#17test_reentrancy_guard_blocks_global_set_fee_rateg_lock→set_protocol_fee_ratepanics#17test_reentrancy_guard_blocks_global_set_fee_collectorg_lock→set_fee_collectorpanics#17test_reentrancy_lock_released_after_successful_withdrawtest_reentrancy_lock_released_after_successful_canceltest_independent_streams_use_separate_lockswithdrawon stream BTesting
cargo build --lib -p payment-streamis clean with zero warnings.cargo testis blocked by the upstreamsoroban-env-host v22.1.3/rand_coreversion conflict tracked separately in #506; this is not introduced by this PR.Checklist
Error::ReentrancyGuard = 17added to error enumwithdraw_maxcorrectly inherits the lock viawithdrawdelegationcargo build --lib -p payment-streampasses clean# Errorson all public functionsSummary by CodeRabbit
New Features
Bug Fixes
Tests