chore(stellar): Adjust fee according to CAP-0015#680
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request enhances fee-bump transaction handling in the Stellar domain by implementing CAP-0015-aware fee scaling. The change introduces a new helper function to correctly compute scaled fees based on operation counts and updates the fee-bump calculation logic to handle the scaling semantics, including changes to error handling and expanded test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #680 +/- ##
==========================================
+ Coverage 90.91% 90.96% +0.04%
==========================================
Files 288 288
Lines 117665 118548 +883
==========================================
+ Hits 106981 107843 +862
- Misses 10684 10705 +21
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/domain/transaction/stellar/prepare/common.rs (1)
219-223:⚠️ Potential issue | 🟡 MinorValidate negative
max_feeconsistently before branching.
max_fee < 0is currently accepted on the existing-resource-fee path (Line 272 viarequired_fee.max(max_fee)), but rejected on other paths viascale_fee_for_fee_bump. Add a single guard near function entry for consistent behavior.Suggested patch
pub async fn calculate_fee_bump_required_fee<P>( inner_envelope: &TransactionEnvelope, max_fee: i64, provider: &P, ) -> Result<u32, TransactionError> where P: StellarProviderTrait + Send + Sync, { + if max_fee < 0 { + return Err(TransactionError::ValidationError(format!( + "Invalid max_fee: {max_fee} (must be non-negative)" + ))); + } + // CAP-0015: fee-bump effective num_ops = inner_num_ops + 1 let inner_num_ops = extract_operations(inner_envelope)Also applies to: 263-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/transaction/stellar/prepare/common.rs` around lines 219 - 223, In calculate_fee_bump_required_fee, add an early guard that rejects negative max_fee values before any branching so behavior is consistent across paths: if max_fee < 0 return an appropriate TransactionError immediately; this ensures both the path that uses required_fee.max(max_fee) and the path that calls scale_fee_for_fee_bump see the same validation. Reference the function calculate_fee_bump_required_fee and the helpers/use-sites scale_fee_for_fee_bump and required_fee.max(max_fee) when making the change.
🧹 Nitpick comments (2)
src/domain/transaction/stellar/prepare/common.rs (2)
848-850: Update the stale test comment to match behavior.The comment says “return required_fee” but the assertion expects
max_feeviamax(required_fee, max_fee).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/transaction/stellar/prepare/common.rs` around lines 848 - 850, Update the stale test comment in src/domain/transaction/stellar/prepare/common.rs to reflect actual behavior: it should state that the function returns the larger of required_fee and max_fee (i.e., max(required_fee, max_fee) = 200000) because the assertion checks result.unwrap() == max_fee as u32; mention required_fee (100200), max_fee (200000), and that max(...) yields 200000 to match the assertion using result.unwrap().
322-330: Deduplicate repeated non-simulation scaling/conversion logic.The same
scale_fee_for_fee_bump(max_fee, ...)+u32::try_fromblock appears twice. Extracting a small local helper reduces drift risk.Refactor sketch
+ let scale_and_convert_max_fee = || -> Result<u32, TransactionError> { + let required_fee = scale_fee_for_fee_bump(max_fee, inner_num_ops, fee_bump_num_ops)?; + u32::try_from(required_fee).map_err(|_| { + TransactionError::ValidationError(format!( + "Fee conversion overflow: required fee {required_fee} exceeds u32::MAX" + )) + }) + }; + if xdr_needs_simulation(inner_envelope).unwrap_or(false) { ... match simulate_if_needed(inner_envelope, provider).await? { Some(sim_resp) => { ... } - None => { - // No simulation needed, scale max_fee for fee-bump - let required_fee = - scale_fee_for_fee_bump(max_fee, inner_num_ops, fee_bump_num_ops)?; - u32::try_from(required_fee).map_err(|_| { - TransactionError::ValidationError(format!( - "Fee conversion overflow: required fee {required_fee} exceeds u32::MAX" - )) - }) - } + None => scale_and_convert_max_fee(), } } else { - // No simulation needed, scale max_fee for fee-bump - let required_fee = scale_fee_for_fee_bump(max_fee, inner_num_ops, fee_bump_num_ops)?; - u32::try_from(required_fee).map_err(|_| { - TransactionError::ValidationError(format!( - "Fee conversion overflow: required fee {required_fee} exceeds u32::MAX" - )) - }) + scale_and_convert_max_fee() }Also applies to: 333-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/transaction/stellar/prepare/common.rs` around lines 322 - 330, The code repeats the sequence scale_fee_for_fee_bump(max_fee, ..., ...) followed by u32::try_from(required_fee).map_err(|_| TransactionError::ValidationError(...)), so extract a small helper (e.g., fn scale_and_convert_fee(max_fee: i64, num_ops: u32, desc: &str) -> Result<u32, TransactionError>) inside the same module that calls scale_fee_for_fee_bump, performs u32::try_from and maps the overflow to the existing TransactionError::ValidationError message (preserving the "Fee conversion overflow: required fee {required_fee} exceeds u32::MAX" wording), then replace both occurrences with calls to that helper to remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/domain/transaction/stellar/prepare/common.rs`:
- Around line 219-223: In calculate_fee_bump_required_fee, add an early guard
that rejects negative max_fee values before any branching so behavior is
consistent across paths: if max_fee < 0 return an appropriate TransactionError
immediately; this ensures both the path that uses required_fee.max(max_fee) and
the path that calls scale_fee_for_fee_bump see the same validation. Reference
the function calculate_fee_bump_required_fee and the helpers/use-sites
scale_fee_for_fee_bump and required_fee.max(max_fee) when making the change.
---
Nitpick comments:
In `@src/domain/transaction/stellar/prepare/common.rs`:
- Around line 848-850: Update the stale test comment in
src/domain/transaction/stellar/prepare/common.rs to reflect actual behavior: it
should state that the function returns the larger of required_fee and max_fee
(i.e., max(required_fee, max_fee) = 200000) because the assertion checks
result.unwrap() == max_fee as u32; mention required_fee (100200), max_fee
(200000), and that max(...) yields 200000 to match the assertion using
result.unwrap().
- Around line 322-330: The code repeats the sequence
scale_fee_for_fee_bump(max_fee, ..., ...) followed by
u32::try_from(required_fee).map_err(|_| TransactionError::ValidationError(...)),
so extract a small helper (e.g., fn scale_and_convert_fee(max_fee: i64, num_ops:
u32, desc: &str) -> Result<u32, TransactionError>) inside the same module that
calls scale_fee_for_fee_bump, performs u32::try_from and maps the overflow to
the existing TransactionError::ValidationError message (preserving the "Fee
conversion overflow: required fee {required_fee} exceeds u32::MAX" wording),
then replace both occurrences with calls to that helper to remove duplication.
NicoMolinaOZ
left a comment
NicoMolinaOZ
left a comment
There was a problem hiding this comment.
LGTM! thanks
I have been testing sending parallel txs and I haven't seen errors, looks good
|
Good catch on this! One small issue was how this code was calculating the new fee, we were scaling the inners inclusion fee AND resource fee, when it should just be the inclusion fee, per the fee bump spec. I pushed some changes to resolve that. Also I reverted the change that would override the max_fee that the client sets. This would break assumptions for all stellar transactions not just channel accounts txs |
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
dylankilkenny
force-pushed
the
fix/fee-bump-num-ops
branch
from
dae1b1c to
438798d
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts Stellar fee-bump fee bidding logic to comply with CAP-0015, fixing incorrect fee scaling and improving handling of Soroban resource fees during fee-bump preparation.
Changes:
- Updates fee-bump required-fee calculation to scale inclusion-fee rate by
(inner_num_ops + 1)and carry Soroban resource fee once. - Adds helper functions for extracting inner fees, computing required fee-bump fee, and validating
max_feeconversions/limits. - Expands unit test coverage for Soroban/classic fee-bump scenarios and simulation behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[cfg(test)] | ||
| mod calculate_fee_bump_required_fee_tests { | ||
| use super::*; | ||
| use crate::models::TransactionError; |
There was a problem hiding this comment.
Redundant import: use super::*; already brings TransactionError into scope from the parent module, so use crate::models::TransactionError; is unnecessary here. Removing it avoids duplication and keeps the test module imports minimal.
| use crate::models::TransactionError; |
| let inner_num_ops = extract_operations(inner_envelope) | ||
| .map(|ops| ops.len() as i64) | ||
| .unwrap_or(1) | ||
| .max(1); | ||
| let fee_bump_num_ops = inner_num_ops + 1; // CAP-0015 | ||
|
|
There was a problem hiding this comment.
inner_num_ops silently falls back to 1 if extract_operations(inner_envelope) returns an error (unwrap_or(1)). This can mask XDR parsing / envelope-shape issues and lead to an incorrect required fee. Prefer propagating the extraction error (mapping it into TransactionError) and only defaulting/clamping based on the actual length when extraction succeeds.
| let inner_num_ops = extract_operations(inner_envelope) | |
| .map(|ops| ops.len() as i64) | |
| .unwrap_or(1) | |
| .max(1); | |
| let fee_bump_num_ops = inner_num_ops + 1; // CAP-0015 | |
| let inner_num_ops = extract_operations(inner_envelope)? | |
| .len() as i64; | |
| let inner_num_ops = inner_num_ops.max(1); | |
| let fee_bump_num_ops = inner_num_ops + 1; // CAP-0015 |
| let inner_inclusion_total = inner_tx_fee.saturating_sub(resource_fee); | ||
| let inner_fee_rate = | ||
| ceil_div(inner_inclusion_total, inner_num_ops).max(STELLAR_DEFAULT_TRANSACTION_FEE as i64); | ||
|
|
||
| debug!( | ||
| "Simulation complete. Inclusion fee: {}, Resource fee: {}, Total: {}", | ||
| inclusion_fee, resource_fee, required_fee | ||
| ); | ||
| inner_fee_rate | ||
| .checked_mul(fee_bump_num_ops) | ||
| .ok_or_else(|| { |
There was a problem hiding this comment.
calculate_fee_bump_fee is documented as computing the minimum fee-bump fee, but the current math rounds up the per-op rate first (ceil_div(inner_inclusion_total, inner_num_ops)) and then multiplies by fee_bump_num_ops. This can overestimate the minimum required fee (e.g., inner inclusion total 201 over 2 ops only needs 302 for 3 ops, but the function returns 303), which may incorrectly reject valid max_fee bids. Consider computing the scaled inclusion total with a single ceiling at the end (e.g., ceil_div(inner_inclusion_total * fee_bump_num_ops, inner_num_ops) with overflow checks) and then applying the base-fee minimum constraint.
|
LGTM, I have reviewed using some AI tools, and it seems to be ok, let me know @tirumerla if we need some manual testing before merging |
Thanks should be ok to merge |
Summary
Summary by CodeRabbit
Bug Fixes
New Features