Add dynamic fee tiers to payment stream contract - #568
Open
opratem wants to merge 8 commits into
Open
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 157 files, which is 57 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (157)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR updates campaign-funding tests to use numeric contract errors. It adds configurable, volume-based protocol fee tiers to payment streams, tracks sender escrow volume, applies tiered withdrawal fees, adds administration APIs, and expands validation coverage. ChangesCampaign funding error assertions
Payment-stream fee tiers
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
contracts/campaign-funding/src/lib.rs (2)
334-397: 🩺 Stability & Availability | 🔵 TrivialToken transfer precedes state persistence in
contribute().Unlike
claim_funds/refundin this same file (state mutated/persisted before external token calls),contribute()callstoken_client.transfer(lines 358-359) beforecampaign.total_raisedis updated andsave_campaignis called (line 386). Soroban disallows same-contract reentrancy at the host level, so this isn't currently exploitable, but for consistency and defense-in-depth it's cleaner to persist state before making the external call, matching the pattern used elsewhere in this file.🤖 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 334 - 397, Reorder contribute() so all campaign and contributor balance updates, including save_campaign, are completed before token_client.transfer. Preserve the existing validation, totals, status transition, and event behavior while ensuring the external token call occurs only after state persistence.
279-287: 🚀 Performance & Scalability | 🔵 TrivialRedundant
extend_ttlcall.
create_campaignextends instance TTL directly (line 286) and then again insidesave_campaign(line 666) a few lines later — the first call is redundant and adds unnecessary host-function overhead.🤖 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 279 - 287, Remove the redundant env.storage().instance().extend_ttl(LEDGER_THRESHOLD, LEDGER_BUMP) call from the create_campaign flow near CampaignCount updates, relying on save_campaign to perform the instance TTL extension. Leave the CampaignCount read, increment, and storage update unchanged.contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json (1)
1-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the misleading
_failsname or make the test exercise a real failure path.
test_trigger_expiry_with_zero_contributions_failsandtest_trigger_expiry_permissionlessboth calltrigger_expiryafter the deadline without any contributions and both snapshot the same successful resolution (Campaign.status = Failed). The_failstest name implies the call should error, buttrigger_expiryonly fails for invalid campaign states likeCampaignNotActive; zero contributions result in a validFailedresolution. Pick either the zero-contribution resolution case or the permissionless caller case instead of maintaining an apparent duplicate/failing expectation.🤖 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 1 - 377, Resolve the duplicate zero-contribution expiry coverage by keeping either the valid Failed-resolution case or the permissionless-caller case, and remove or rename the misleading “fails” expectation. Update both contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json (lines 1-377) and contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_with_zero_contributions_fails.1.json (lines 1-377) consistently with the chosen test scope; the relevant test should reflect trigger_expiry’s successful Campaign.status = Failed behavior rather than implying an error.
🤖 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 877-1501: Update the #[should_panic] tests throughout the test
module, including test_create_campaign_deadline_in_past, contribution, expiry,
claim, and refund cases, so their assertions match the pinned Soroban SDK’s
actual panic output. Prefer client.try_* calls with unwrap_err() and explicit
Error assertions to avoid brittle string matching; otherwise replace each
expected string with the exact host-generated message.
In `@contracts/payment-stream/src/lib.rs`:
- Around line 467-502: The persistent sender-volume entry read by
get_applicable_fee_rate_internal must have its TTL extended on reads. After
retrieving ("sv", sender), call the contract’s existing persistent-storage TTL
extension mechanism using the configured retention window, while preserving the
current default-volume and fee-tier behavior; apply the same requirement to
get_sender_volume if it performs an equivalent read.
- Around line 243-253: Update the sender volume accounting in create_stream to
increment by initial_amount, the tokens actually transferred into escrow,
instead of total_amount. Also update deposit to increment the same sender’s
tracked volume by each deposited amount, preserving get_sender_volume as
cumulative transferred volume and the existing storage TTL behavior.
---
Nitpick comments:
In `@contracts/campaign-funding/src/lib.rs`:
- Around line 334-397: Reorder contribute() so all campaign and contributor
balance updates, including save_campaign, are completed before
token_client.transfer. Preserve the existing validation, totals, status
transition, and event behavior while ensuring the external token call occurs
only after state persistence.
- Around line 279-287: Remove the redundant
env.storage().instance().extend_ttl(LEDGER_THRESHOLD, LEDGER_BUMP) call from the
create_campaign flow near CampaignCount updates, relying on save_campaign to
perform the instance TTL extension. Leave the CampaignCount read, increment, and
storage update unchanged.
In
`@contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json`:
- Around line 1-377: Resolve the duplicate zero-contribution expiry coverage by
keeping either the valid Failed-resolution case or the permissionless-caller
case, and remove or rename the misleading “fails” expectation. Update both
contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_permissionless.1.json
(lines 1-377) and
contracts/campaign-funding/test_snapshots/tests/test_trigger_expiry_with_zero_contributions_fails.1.json
(lines 1-377) consistently with the chosen test scope; the relevant test should
reflect trigger_expiry’s successful Campaign.status = Failed behavior rather
than implying an error.
🪄 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: c3a45ab9-865f-4857-ab3b-d9594c89349d
📒 Files selected for processing (45)
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/Cargo.tomlcontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payment-stream/src/lib.rs (1)
860-871: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce that higher-volume tiers never increase fees.
Threshold ordering alone accepts tiers whose
fee_rateexceeds the preceding effective rate; a laterset_protocol_fee_ratecan also lower the base below existing tier rates. That makes high-volume senders pay more, contradicting the fee-discount contract. Require non-increasing tier rates from the general rate and preserve that invariant when changing the base rate.Proposed validation direction
+ let general_rate: u32 = env.storage().instance() + .get(&Symbol::new(&env, "general_protocol_fee_rate")) + .unwrap_or(0); let mut prev_min_volume: i128 = -1_i128; + let mut prev_fee_rate = general_rate; let len = tiers.len(); for i in 0..len { let tier = tiers.get(i).unwrap(); if tier.fee_rate > MAX_FEE { panic_with_error!(&env, Error::InvalidTier); } + if tier.fee_rate > prev_fee_rate { + panic_with_error!(&env, Error::InvalidTier); + } if tier.min_volume <= prev_min_volume { panic_with_error!(&env, Error::InvalidTier); } prev_min_volume = tier.min_volume; + prev_fee_rate = tier.fee_rate; }Also reject base-rate updates below the first configured tier rate.
🤖 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 860 - 871, Update tier validation to require each tier’s fee_rate to be no greater than the preceding effective rate, starting with the general protocol fee rate, while preserving strictly ascending min_volume validation. In set_protocol_fee_rate, reject base-rate changes below the first configured tier’s fee_rate so the non-increasing fee invariant remains valid.
🤖 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.
Outside diff comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 860-871: Update tier validation to require each tier’s fee_rate to
be no greater than the preceding effective rate, starting with the general
protocol fee rate, while preserving strictly ascending min_volume validation. In
set_protocol_fee_rate, reject base-rate changes below the first configured
tier’s fee_rate so the non-increasing fee invariant remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a22d2454-d7b4-4b6a-af96-2abfcee4038b
📒 Files selected for processing (3)
contracts/campaign-funding/src/lib.rscontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/payment-stream/src/test.rs
- contracts/campaign-funding/src/lib.rs
Contributor
Author
|
fixed now |
opratem
changed the title
Contributor
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
2 similar comments
Contributor
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
Contributor
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
Contributor
Author
|
conflicts resolved |
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payment-stream/src/lib.rs (1)
1185-1199: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore one complete
calculate_protocol_feefunction.The nested declaration at
contracts/payment-stream/src/lib.rs:1188makes the firstcalculate_protocol_feeinvalid. Keep the method that acceptssenderand move the existing fee calculation body into it.🤖 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 1185 - 1199, Remove the duplicate nested calculate_protocol_fee declaration and retain the sender-aware calculate_protocol_fee(env, amount, sender) method. Move the existing fee-rate lookup and fee calculation body into that method, using get_applicable_fee_rate_internal with sender, so one complete valid function remains.
🤖 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/lib.rs`:
- Around line 1542-1559: Update set_fee_tiers to read the initialized admin and
base fee values through DataKey::Admin and DataKey::FeeRate instead of
Symbol-based keys. Update get_applicable_fee_rate_internal to use
DataKey::FeeRate as well, and remove the parallel Symbol write from
set_protocol_fee_rate so all fee-rate access uses the same storage key.
- Around line 769-779: Extract the sender volume update currently in deposit()
into a shared helper, preserving saturating addition and TTL handling. Invoke
that helper in deposit_with_swap only after actual_received passes validation
and is credited successfully. Add coverage proving a sender qualifies for the
expected fee tier through a successful swap deposit.
- Around line 696-706: The sender volume counter must not combine raw units from
different assets. Update the volume key and related accounting to use both
sender and token, then update tier lookup in the stream withdrawal path to
resolve the counter using stream.token, preserving existing fee-tier behavior
within each asset.
- Around line 263-266: Update the error discriminants for InvalidTier and
TooManyTiers to unused numeric values, preserving ContractPaused = 17 and
AlreadyPaused = 18 and ensuring every variant in the enum has a unique
discriminant.
---
Outside diff comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 1185-1199: Remove the duplicate nested calculate_protocol_fee
declaration and retain the sender-aware calculate_protocol_fee(env, amount,
sender) method. Move the existing fee-rate lookup and fee calculation body into
that method, using get_applicable_fee_rate_internal with sender, so one complete
valid function remains.
🪄 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: 86409953-32c7-46a4-bcfd-75ddbd79b4ab
📒 Files selected for processing (5)
contracts/Cargo.tomlcontracts/campaign-funding/src/lib.rscontracts/payment-stream/Cargo.tomlcontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/payment-stream/Cargo.toml
- contracts/payment-stream/src/test.rs
- contracts/campaign-funding/src/lib.rs
Comment on lines
+263
to
+266
| /// Tier fee_rate exceeds MAX_FEE or tiers not sorted strictly ascending | ||
| InvalidTier = 17, | ||
| /// Exceeded the maximum number of configurable fee tiers | ||
| TooManyTiers = 18, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="contracts/payment-stream/src/lib.rs"
awk '
/^[[:space:]]*[A-Za-z_][A-Za-z0-9_]*[[:space:]]*=[[:space:]]*[0-9]+,/ {
line=$0
sub(/.*=[[:space:]]*/, "", line)
sub(/,.*/, "", line)
if (seen[line]++) print "duplicate discriminant " line ": " $0
}
' "$file"Repository: Fundable-Protocol/stellar_client_os
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== enum and related contract/error code references in payment-stream =="
rg -n "enum .*|ContractPaused =|AlreadyPaused =|InvalidTier =|TooManyTiers =|ERROR_InvalidTier|ERROR_TooManyTiers|TooManyTiers|InvalidTier|ContractPaused|AlreadyPaused" contracts/payment-stream -g '*.rs' -g '*.toml' -g '*.md' || true
echo
echo "== contract package files =="
fd . contracts/payment-stream -d 3 -t fRepository: Fundable-Protocol/stellar_client_os
Length of output: 2911
Move the duplicate error variants to unused discriminants.
InvalidTier = 17 and TooManyTiers = 18 duplicate ContractPaused = 17 and AlreadyPaused = 18 in contracts/payment-stream/src/lib.rs, which makes the Rust enum reject duplicate enum discriminants. Use new values such as 32 and 33 or other unused codes.
🤖 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 263 - 266, Update the error
discriminants for InvalidTier and TooManyTiers to unused numeric values,
preserving ContractPaused = 17 and AlreadyPaused = 18 and ensuring every variant
in the enum has a unique discriminant.
Comment on lines
+696
to
+706
| // Track sender's cumulative stream volume for fee tier eligibility. | ||
| // Only funds actually escrowed (initial_amount) are counted here to | ||
| // prevent tier-gaming via zero-deposit streams. Additional deposits | ||
| // via deposit() also increment this counter. | ||
| let sender_vol_key = (Symbol::new(&env, "sv"), sender.clone()); | ||
| let current_vol: i128 = env.storage().persistent() | ||
| .get(&sender_vol_key) | ||
| .unwrap_or(0); | ||
| let new_vol = current_vol.saturating_add(initial_amount); | ||
| env.storage().persistent().set(&sender_vol_key, &new_vol); | ||
| env.storage().persistent().extend_ttl(&sender_vol_key, LEDGER_THRESHOLD, LEDGER_BUMP); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not aggregate raw token units across assets.
Line 700 stores one volume counter per sender. Line 1262 applies its tier to every stream from that sender. A sender can escrow a large amount of a low-value or high-decimal asset, then receive a reduced fee on withdrawals from a valuable-asset stream.
Key volume by sender and token, and resolve the tier with stream.token. If tiers must span assets, normalize volume through a trusted value source.
🤖 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 696 - 706, The sender
volume counter must not combine raw units from different assets. Update the
volume key and related accounting to use both sender and token, then update tier
lookup in the stream withdrawal path to resolve the counter using stream.token,
preserving existing fee-tier behavior within each asset.
Comment on lines
+769
to
+779
| // Update sender's cumulative volume for fee tier eligibility. | ||
| // deposit() actually moves tokens into escrow, so each deposited | ||
| // amount counts toward the sender's volume just as initial_amount | ||
| // does at stream creation. | ||
| let sender_vol_key = (Symbol::new(&env, "sv"), stream.sender.clone()); | ||
| let current_vol: i128 = env.storage().persistent() | ||
| .get(&sender_vol_key) | ||
| .unwrap_or(0); | ||
| let new_vol = current_vol.saturating_add(amount); | ||
| env.storage().persistent().set(&sender_vol_key, &new_vol); | ||
| env.storage().persistent().extend_ttl(&sender_vol_key, LEDGER_THRESHOLD, LEDGER_BUMP); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count successful swap deposits in sender volume.
This path updates volume for deposit(), but deposit_with_swap credits actual_received to stream escrow without updating the same counter. A sender who funds streams through swaps cannot qualify for the corresponding tier.
Extract the volume update into one helper. Call it after actual_received passes validation in deposit_with_swap. Add a test for tier qualification through a swap deposit.
🤖 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 769 - 779, Extract the
sender volume update currently in deposit() into a shared helper, preserving
saturating addition and TTL handling. Invoke that helper in deposit_with_swap
only after actual_received passes validation and is credited successfully. Add
coverage proving a sender qualifies for the expected fee tier through a
successful swap deposit.
Comment on lines
+1542
to
+1559
| let admin: Address = env.storage().instance() | ||
| .get(&Symbol::new(&env, "admin")) | ||
| .unwrap(); | ||
| admin.require_auth(); | ||
|
|
||
| if tiers.len() > MAX_TIERS { | ||
| panic_with_error!(&env, Error::TooManyTiers); | ||
| } | ||
|
|
||
| // Validate each tier: | ||
| // - fee_rate must not exceed MAX_FEE | ||
| // - fee_rate must be non-increasing (each tier must be <= the preceding | ||
| // effective rate, starting from general_protocol_fee_rate) so that | ||
| // higher-volume senders are never charged more than lower-volume ones | ||
| // - min_volume must be strictly ascending | ||
| let general_rate: u32 = env.storage().instance() | ||
| .get(&Symbol::new(&env, "general_protocol_fee_rate")) | ||
| .unwrap_or(0); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the initialized storage keys for tier administration.
initialize stores the admin and base fee rate under DataKey::Admin and DataKey::FeeRate. This method reads Symbol("admin") and Symbol("general_protocol_fee_rate") instead. The admin lookup panics before authorization, so set_fee_tiers cannot work after normal initialization. The fee-rate lookup defaults to zero, so positive tier rates also fail validation.
Use DataKey::Admin and DataKey::FeeRate here. Also update get_applicable_fee_rate_internal to read DataKey::FeeRate and remove the parallel symbol write in set_protocol_fee_rate.
🤖 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 1542 - 1559, Update
set_fee_tiers to read the initialized admin and base fee values through
DataKey::Admin and DataKey::FeeRate instead of Symbol-based keys. Update
get_applicable_fee_rate_internal to use DataKey::FeeRate as well, and remove the
parallel Symbol write from set_protocol_fee_rate so all fee-rate access uses the
same storage key.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements a dynamic protocol fee tier system for the Soroban payment stream contracts. The protocol now supports configurable fee tiers based on stream volume, allowing high-volume contributors to benefit from reduced protocol fees while keeping fee calculations deterministic and efficient.
Related Issue
Closes #504
Type of Change
Changes Made
Errorenum with typed errors for invalid fee configuration and calculation failures where applicable.Testing
cargo test cargo build --target wasm32-unknown-unknownAll tests pass successfully with zero compiler warnings.
Checklist
require_auth())Screenshots
N/A — Smart contract changes only.
Additional Notes
The dynamic fee tier system is designed to provide predictable and efficient fee calculations while rewarding higher-volume contributors with reduced protocol fees. The implementation integrates cleanly with the existing payment stream contracts without introducing breaking changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests