fix(web): resolve contract addresses to friendly ticker symbols in PaymentStreamSummary - #540
Conversation
…ymentStreamSummary Add getTokenSymbol() utility that maps both token values (e.g. "USDC") and contract addresses (e.g. "CBIELTK...") to display-friendly ticker symbols. Update PaymentStreamSummary.tsx to use resolved symbols in all token displays. Write 16 unit tests covering value lookup, address lookup, unknown tokens, edge cases, and consistency properties.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds token symbol resolution, migrates Soroban contracts to typed events, updates contract storage snapshots, and applies frontend, SDK, and type-safety changes. ChangesWeb application and SDK updates
Soroban contract behavior
Snapshot updates
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)apps/web/src/components/organisms/wallet-modal.tsxFile contains syntax errors that prevent linting: Line 138: Unexpected token. Did you mean apps/web/src/providers/StellarWalletProvider.tsxFile contains syntax errors that prevent linting: Line 131: Expected a statement but instead found ')'.; Line 462: expected 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 |
|
@Cerome360 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! 🚀 |
|
Greetings chief @pragmaticAweds please confirm this is okay and merge sir |
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
contracts/payment-stream/src/lib.rs (1)
119-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding the revoked delegate address to
DelegationRevokedEvent.
DelegationGrantedEventcarriesdelegate.DelegationRevokedEventcarries onlystream_idandrecipient. A consumer that tracks delegate lifecycle cannot determine which delegate lost access from the revocation event alone.Both publication sites already hold the value: Line 751 binds
old_delegate, andrevoke_delegatecan read the key before removal.♻️ Proposed change to the event struct
#[contractevent(topics = ["DelegationRevoked"])] #[derive(Clone)] pub struct DelegationRevokedEvent { pub stream_id: u64, pub recipient: Address, + pub delegate: Address, }Update the publication site at Lines 753-757 to pass
old_delegate.clone(), and read the stored delegate inrevoke_delegatebeforeremovefor the site at Lines 821-825.🤖 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 119 - 124, Update DelegationRevokedEvent to include the revoked delegate address, matching DelegationGrantedEvent. Populate the new field at both publication sites: pass old_delegate.clone() where it is already bound, and read the stored delegate before removal in revoke_delegate before constructing the event.
🤖 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`:
- Line 1918: Update the event assertions in the emergency pause/unpause test
around setup_paused_contract and test_emergency_unpause_emits_event: record the
event stream before each emergency_pause or emergency_unpause operation, then
assert the newly generated event is specifically EmergencyPausedEvent or
EmergencyUnpausedEvent with the expected topic and payload, rather than only
checking that any events exist.
In
`@contracts/payment-stream/test_snapshots/test/test/test_stream_paused_event_emitted.1.json`:
- Around line 293-484: Update the pause snapshot for
test_stream_paused_event_emitted to include the expected StreamPaused event in
its events array, matching the event emitted by pause_stream. Also update the
corresponding resume_stream snapshot to include StreamResumed, preserving the
existing event snapshot format and values.
In `@contracts/soulbound-badge/src/lib.rs`:
- Around line 683-684: Update the comment in test_is_eligible_for_badge above
the Silver eligibility assertion to state that the contributor is not eligible
for the next level, matching the false assertion and the 1,000 contribution
versus the 10,000 SILVER_THRESHOLD.
In
`@contracts/soulbound-badge/test_snapshots/test/test_record_contribution_bronze_badge.1.json`:
- Around line 336-337: Regenerate the listed snapshots to reflect the
ContributionRecordedEvent emitted by record_contribution: update
contracts/soulbound-badge/test_snapshots/test/test_record_contribution_bronze_badge.1.json
(336-337) and
contracts/soulbound-badge/test_snapshots/test/test_is_eligible_for_badge.1.json
(338-339) with amount 1000 and total 1000; update
contracts/soulbound-badge/test_snapshots/test/test_multiple_users.1.json
(608-609) with the final event for amount 15000 and total 15000; update
contracts/soulbound-badge/test_snapshots/test/test_record_contribution_multiple_badges.1.json
(484-485) with amount 100000 and total 100000; and update
contracts/soulbound-badge/test_snapshots/test/test_record_contribution_progressive_badges.1.json
(452-453) with the final event for amount 9000 and total 10000.
---
Nitpick comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 119-124: Update DelegationRevokedEvent to include the revoked
delegate address, matching DelegationGrantedEvent. Populate the new field at
both publication sites: pass old_delegate.clone() where it is already bound, and
read the stored delegate before removal in revoke_delegate before constructing
the event.
🪄 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: 521e012e-cf5e-4c51-a7f4-4f4fe2e51e26
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (75)
contracts/distributor/src/lib.rscontracts/nft-stream/src/lib.rscontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_cancel_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_delegate_withdraw.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_after_withdrawal.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_exceeds_total.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_invalid_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_multiple.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_negative_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_updates_last_activity.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_pause_already_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_pause_emits_event.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_pause_sets_flag.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_unpause_clears_flag.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_unpause_emits_event.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_emergency_unpause_when_not_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_get_nonexistent_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_is_paused_default_false.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_multiple_streams_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_multiple_withdrawals_accumulate_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_non_admin_cannot_emergency_pause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_non_admin_cannot_emergency_unpause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_only_sender_can_pause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_only_sender_can_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_operations_resume_after_unpause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_overwrite_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_and_resume_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_unpause_cycle.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pausing_stops_token_vesting.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_protocol_metrics_initialization.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_read_operations_work_while_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_recipient_can_still_withdraw_after_delegate_set.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resume_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resuming_continues_from_where_it_left_off.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_delegate_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_nonexistent_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_self_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_stream_paused_event_emitted.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_stream_resumed_event_emitted.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_unauthorized_delegate_withdraw_after_revoke.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_after_pause_and_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount_zero_for_paused_streams.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawal_updates_metrics.1.jsoncontracts/soulbound-badge/src/lib.rscontracts/soulbound-badge/test_snapshots/test/test_all_milestone_levels.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_badge_data_integrity.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_get_badge.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_get_badge_not_found.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_get_user_badges.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_get_user_contribution.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_initialize.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_is_eligible_for_badge.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_multiple_users.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_re_initialize_fails.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_bronze_badge.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_multiple_badges.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_negative_amount.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_no_duplicate_badges.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_progressive_badges.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_zero_amount.1.json
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| "ext": "v0" | ||
| }, | ||
| "live_until": 535680 | ||
| }, | ||
| { | ||
| "entry": { | ||
| "last_modified_ledger_seq": 0, | ||
| "data": { | ||
| "contract_data": { | ||
| "ext": "v0", | ||
| "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4", | ||
| "key": { | ||
| "vec": [ | ||
| { | ||
| "u64": "1" | ||
| }, | ||
| { | ||
| "symbol": "metrics" | ||
| } | ||
| ] | ||
| }, | ||
| "durability": "persistent", | ||
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "symbol": "current_delegate" | ||
| "symbol": "sender" | ||
| }, | ||
| "val": "void" | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "last_activity" | ||
| "symbol": "start_time" | ||
| }, | ||
| "val": { | ||
| "u64": "0" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "last_delegation_time" | ||
| "symbol": "status" | ||
| }, | ||
| "val": { | ||
| "u64": "0" | ||
| "vec": [ | ||
| { | ||
| "symbol": "Paused" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "pause_count" | ||
| "symbol": "token" | ||
| }, | ||
| "val": { | ||
| "u32": 1 | ||
| "address": "CDLDVFKHEZ2RVB3NG4UQA4VPD3TSHV6XMHXMHP2BSGCJ2IIWVTOHGDSG" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "total_delegations" | ||
| "symbol": "total_amount" | ||
| }, | ||
| "val": { | ||
| "u32": 0 | ||
| "i128": "1000" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "total_withdrawn" | ||
| "symbol": "total_paused_duration" | ||
| }, | ||
| "val": { | ||
| "i128": "0" | ||
| "u64": "0" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "withdrawal_count" | ||
| "symbol": "withdrawn_amount" | ||
| }, | ||
| "val": { | ||
| "u32": 0 | ||
| "i128": "0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the events array in the pause/resume event snapshots.
set -euo pipefail
for f in \
contracts/payment-stream/test_snapshots/test/test/test_stream_paused_event_emitted.1.json \
contracts/payment-stream/test_snapshots/test/test/test_stream_resumed_event_emitted.1.json \
contracts/payment-stream/test_snapshots/test/test/test_emergency_pause_emits_event.1.json
do
echo "=== $f ==="
jq '.events' "$f"
done
# Locate the pause/resume event struct definitions and their publish sites.
rg -nP -C4 '(StreamPaused|StreamResumed|EmergencyPaused)' contracts/payment-stream/src/lib.rsRepository: Fundable-Protocol/stellar_client_os
Length of output: 3197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== events section in test_stream_paused_event_emitted.1.json ==="
jq '.events' contracts/payment-stream/test_snapshots/test/test/test_stream_paused_event_emitted.1.json
echo
echo "=== events section in test_stream_resumed_event_emitted.1.json ==="
jq '.events' contracts/payment-stream/test_snapshots/test/test/test_stream_resumed_event_emitted.1.json
echo
echo "=== relevant StreamPaused/StreamResumed publish call sites ==="
rg -n -C8 '(StreamPausedEvent|StreamResumedEvent|\.publish\(\&env\)|fn .*pause|fn .*resume|pause_stream|resume_stream)' contracts/payment-stream/src/lib.rsRepository: Fundable-Protocol/stellar_client_os
Length of output: 11261
Add the expected StreamPaused event to the pause snapshot.
test_stream_paused_event_emitted.1.json has an empty events array, while emergency_pause does emit EmergencyPaused in its snapshot. The StreamPausedEvent exists and is published from pause_stream, so include that event when the test name asserts event emission. Also update the resume_stream snapshot so it contains StreamResumed.
🤖 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/test_snapshots/test/test/test_stream_paused_event_emitted.1.json`
around lines 293 - 484, Update the pause snapshot for
test_stream_paused_event_emitted to include the expected StreamPaused event in
its events array, matching the event emitted by pause_stream. Also update the
corresponding resume_stream snapshot to include StreamResumed, preserving the
existing event snapshot format and values.
| // But eligible for next level | ||
| assert!(!client.is_eligible_for_badge(&contributor, MilestoneLevel::Silver)); | ||
| assert!(!client.is_eligible_for_badge(&contributor, &MilestoneLevel::Silver)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the misleading comment in test_is_eligible_for_badge.
The comment says the contributor is eligible for the next level. The assertion correctly expects false, because the total contribution is 1_000 and SILVER_THRESHOLD is 10_000. Correct the comment so it describes the asserted behavior.
📝 Proposed comment fix
- // But eligible for next level
+ // Not yet eligible for Silver: total is 1_000, threshold is 10_000
assert!(!client.is_eligible_for_badge(&contributor, &MilestoneLevel::Silver));📝 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.
| // But eligible for next level | |
| assert!(!client.is_eligible_for_badge(&contributor, MilestoneLevel::Silver)); | |
| assert!(!client.is_eligible_for_badge(&contributor, &MilestoneLevel::Silver)); | |
| // Not yet eligible for Silver: total is 1_000, threshold is 10_000 | |
| assert!(!client.is_eligible_for_badge(&contributor, &MilestoneLevel::Silver)); |
🤖 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/soulbound-badge/src/lib.rs` around lines 683 - 684, Update the
comment in test_is_eligible_for_badge above the Silver eligibility assertion to
state that the contributor is not eligible for the next level, matching the
false assertion and the 1,000 contribution versus the 10,000 SILVER_THRESHOLD.
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 (3)
contracts/payment-stream/src/lib.rs (2)
529-577: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCall
validate_stream_paramsfromcreate_stream_internalto remove the duplicated checks.Lines 537-548 and lines 566-577 contain the same four validations. The two copies can drift apart.
create_batch_streamsalso runs the checks twice per entry.Note that the up-front loop in
create_batch_streamsis not required for atomicity. A Soroban panic reverts the whole invocation, socreate_stream_internalalone already guarantees no partial batch. Keep the loop only if you want the failure to occur before any token transfer.♻️ Proposed refactor
fn create_stream_internal( env: Env, sender: Address, recipient: Address, token: Address, total_amount: i128, initial_amount: i128, start_time: u64, end_time: u64, cliff_duration: u64, ) -> u64 { Self::assert_not_paused(&env); - // Validate inputs - if total_amount <= 0 { - panic_with_error!(&env, Error::InvalidAmount); - } - if initial_amount < 0 || initial_amount > total_amount { - panic_with_error!(&env, Error::InvalidAmount); - } - if end_time <= start_time { - panic_with_error!(&env, Error::InvalidTimeRange); - } - if cliff_duration >= end_time - start_time { - panic_with_error!(&env, Error::InvalidCliff); - } + Self::validate_stream_params( + &env, + total_amount, + initial_amount, + start_time, + end_time, + cliff_duration, + );🤖 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 529 - 577, Update create_stream_internal to call validate_stream_params with the stream arguments after asserting the contract is not paused, then remove its duplicated amount, time-range, and cliff validations. Update create_batch_streams to avoid repeating the same validation unless intentionally retaining the upfront checks for earlier failure before token transfers.
25-28: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a migration for existing payment-stream storage.
DataKey::StreamCount,DataKey::Paused, andDataKey::DexRouteruse typedcontracttypeenum keys, while the persistedStreamstructs now includecliff_duration,paused_at, andtotal_paused_duration. The repo has noupgrade/migratepath, so any deployed instance with pre-change storage cannot decode streams correctly after this version is installed. Add an upgrade migration or document this as a breaking redeployment.🤖 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 - 28, Address storage compatibility for existing payment-stream deployments by adding an explicit upgrade/migration path that converts legacy DataKey and Stream storage to the current typed keys and fields, including defaults for cliff_duration, paused_at, and total_paused_duration; alternatively, document and enforce that this release requires a breaking redeployment. Implement the migration in the contract’s upgrade/deployment flow and preserve correct decoding of existing streams.contracts/payment-stream/src/test.rs (1)
2755-2762: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThe 50-entry batch limit may not be executable on-chain.
This test must call
env.budget().reset_unlimited()andenv.cost_estimate().disable_resource_limits()to pass, and the comment states that a full 50-recipient batch exceeds the emulated mainnet write-entry budget.
MAX_STREAMS_PER_BATCHis therefore advertised as 50 but a 50-entry call would likely fail on mainnet with a resource error rather thanBatchLimitExceeded. Callers get an opaque failure instead of a clear contract error.Consider one of the following:
- Lower
MAX_STREAMS_PER_BATCHto a value that fits the real write-entry budget, and keep a test at that limit without disabling resource limits.- Keep 50 but add a test at the largest batch size that passes under default limits, so the practical ceiling is recorded and protected against regression.
Document the practical ceiling in the
create_batch_streamsdoc comment either way.🤖 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 2755 - 2762, Align the advertised MAX_STREAMS_PER_BATCH limit with the executable on-chain resource ceiling: either lower it to the largest batch size that passes under default limits, or retain 50 while adding a default-limits test covering the largest practical batch size. Update test_create_batch_streams_50 and the create_batch_streams documentation so the practical ceiling is explicitly documented and protected without relying on unlimited budgets.
🧹 Nitpick comments (6)
contracts/soulbound-badge/src/lib.rs (3)
176-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNote that
user_contributionis stale after this call.Persisting before minting is correct:
mint_badgere-readsUserContributionat lines 413-419 and appends tobadges_minted, so it now sees the fresh total.The consequence is that the local
user_contributionno longer matches storage oncecheck_and_mint_badgesreturns. It is missing the badges thatmint_badgeappended. The current code never writes it back, so the behavior is correct today. A future edit that persists the local here would silently drop the newly minted badges.Add a short comment so the constraint survives.
🛡️ Proposed comment
// Check for new milestone achievements let new_badges = Self::check_and_mint_badges( env.clone(), contributor.clone(), user_contribution.total_contributed, &user_contribution.badges_minted, ); + // NOTE: `mint_badge` has now appended to the stored `badges_minted`. + // The local `user_contribution` is stale for that field. Do not persist + // it again here or the newly minted badges are lost. env.storage().persistent() .extend_ttl(&DataKey::UserContribution(contributor.clone()), LEDGER_THRESHOLD, LEDGER_BUMP);🤖 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/soulbound-badge/src/lib.rs` around lines 176 - 184, Add a concise comment immediately after `check_and_mint_badges` in the surrounding function explaining that `user_contribution` is stale because `mint_badge` updates the persisted `UserContribution`, and must not be written back or persisted afterward.
427-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning the event topic naming across contracts.
The snapshots show this contract emits
badge_mintedandcontribution_recorded, whilecontracts/payment-stream/src/lib.rsemitsStreamPaused,EmergencyPaused, and similar PascalCase topics. Both surfaces are being defined in this PR, so this is the cheapest point to pick one convention. Indexers that consume both contracts otherwise need two topic conventions.🤖 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/soulbound-badge/src/lib.rs` around lines 427 - 434, Align the event topic naming in the soulbound-badge contract with the PascalCase convention used by the payment-stream contract. Update the event definitions corresponding to BadgeMintedEvent and ContributionRecordedEvent so their emitted topics use consistent PascalCase names, while preserving their existing payloads and behavior.
727-747: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test duplicates the event assertion from
test_record_contribution_bronze_badge.Lines 736-747 assert the same
ContributionRecordedEventfor the same 1_000 contribution thattest_record_contribution_bronze_badgealready asserts at lines 527-537. The added value here is the pre-contribution eligibility check at line 728.The test is named
test_is_eligible_for_badgebut now asserts no eligibility outcome after the contribution. Consider dropping the duplicated event block and keeping this test focused on eligibility.🤖 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/soulbound-badge/src/lib.rs` around lines 727 - 747, Update test_is_eligible_for_badge in soulbound-badge/src/lib.rs to keep it focused on eligibility by removing the duplicated ContributionRecordedEvent assertion block after record_contribution. Preserve the pre-contribution is_eligible_for_badge check as the unique purpose of this test, and rely on test_record_contribution_bronze_badge for validating the emitted event.contracts/payment-stream/src/test.rs (2)
1288-1293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert a literal
paused_durationinstead of re-deriving it.
10u64.saturating_sub(paused_at)repeats the arithmetic thatresume_streamperforms. If that arithmetic changes, the expectation moves with it and the test still passes. The clock is 0 at pause and 10 at resume, so the value is known.💚 Proposed test change
let expected = StreamResumedEvent { stream_id, - resumed_at: env.ledger().timestamp(), - paused_duration: 10u64.saturating_sub(paused_at), + resumed_at: 10, + paused_duration: 10, } .to_xdr(&env, &contract_id);
paused_atthen becomes unused; assert it instead of dropping it:client.pause_stream(&stream_id); let paused_at = env.ledger().timestamp(); + assert_eq!(paused_at, 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/test.rs` around lines 1288 - 1293, Update the expected StreamResumedEvent in the resume test to use the known literal paused_duration value rather than recalculating it from paused_at. Since paused_at becomes unused, assert its expected pause-time value instead of leaving it unused, while preserving the existing timestamp and event assertions.
738-740: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the cross-reference and add coverage for
DelegationRevokedEvent.Two issues at this comment:
- This test covers revocation, but the comment points the reader to the
DelegationGrantedEventassertion. The relevant event here isDelegationRevokedEvent. The text appears to be copied fromtest_set_delegateat lines 635-637.- No test asserts a
DelegationRevokedEventpayload. The contract emits it from two changed sites incontracts/payment-stream/src/lib.rs: delegate replacement at lines 945-949 and revocation at lines 1013-1017. Both are uncovered.Restructure this test so
revoke_delegateis the last contract invocation, then assert the event payload.💚 Proposed test change
// Set delegate client.set_delegate(&stream_id, &delegate); // Check delegate is set let retrieved_delegate = client.get_delegate(&stream_id); assert_eq!(retrieved_delegate, Some(delegate.clone())); - // Revoke delegate + // Revoke delegate (last contract invocation so the event buffer is intact) client.revoke_delegate(&stream_id); - // Check delegate is removed - let retrieved_delegate = client.get_delegate(&stream_id); - assert_eq!(retrieved_delegate, None); - - // (No event assertion here: the test ends with a get_delegate read, which - // clears the event buffer, so this test's snapshot holds no events. The - // DelegationGrantedEvent payload is asserted in test_delegate_withdraw.) + // Verify the DelegationRevokedEvent was emitted with the correct payload. + let expected = DelegationRevokedEvent { + stream_id, + recipient: recipient.clone(), + } + .to_xdr(&env, &contract_id); + let events = env.events().all(); + assert!( + events.events().iter().any(|e| *e == expected), + "expected DelegationRevokedEvent to be emitted" + );Add the import:
use crate::{ - DelegationGrantedEvent, EmergencyPausedEvent, EmergencyUnpausedEvent, + DelegationGrantedEvent, DelegationRevokedEvent, EmergencyPausedEvent, EmergencyUnpausedEvent, PaymentStreamContract, PaymentStreamContractClient, StreamPausedEvent, StreamResumedEvent, StreamParams, StreamStatus, };🤖 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 738 - 740, Update the revocation test in contracts/payment-stream/src/test.rs so revoke_delegate is the final contract invocation, preserving any required setup reads beforehand; then import and assert the emitted DelegationRevokedEvent payload. Correct the nearby comment to reference DelegationRevokedEvent and the appropriate test, and add coverage for both delegate-replacement and explicit-revocation emission paths where the existing tests cover those operations.contracts/payment-stream/src/lib.rs (1)
430-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the Clippy
too_many_argumentswarnings on these two functions.Clippy reports 9/7 arguments at both sites.
contracts/nft-stream/src/lib.rsalready annotates itscreate_streamwith#[allow(clippy::too_many_arguments)]for the same pattern. Apply the same annotation so the build stays clean under-D warnings.🔧 Proposed fix
+ #[allow(clippy::too_many_arguments)] pub fn create_stream_with_cliff(+ #[allow(clippy::too_many_arguments)] fn create_stream_internal(Also applies to: 552-562
🤖 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 430 - 440, Add #[allow(clippy::too_many_arguments)] immediately before both create_stream_with_cliff and the other affected stream-creation function, matching the existing annotation pattern in the NFT stream contract, so Clippy warnings are suppressed under -D warnings.Source: Linters/SAST tools
🤖 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 529-577: Update create_stream_internal to call
validate_stream_params with the stream arguments after asserting the contract is
not paused, then remove its duplicated amount, time-range, and cliff
validations. Update create_batch_streams to avoid repeating the same validation
unless intentionally retaining the upfront checks for earlier failure before
token transfers.
- Around line 25-28: Address storage compatibility for existing payment-stream
deployments by adding an explicit upgrade/migration path that converts legacy
DataKey and Stream storage to the current typed keys and fields, including
defaults for cliff_duration, paused_at, and total_paused_duration;
alternatively, document and enforce that this release requires a breaking
redeployment. Implement the migration in the contract’s upgrade/deployment flow
and preserve correct decoding of existing streams.
In `@contracts/payment-stream/src/test.rs`:
- Around line 2755-2762: Align the advertised MAX_STREAMS_PER_BATCH limit with
the executable on-chain resource ceiling: either lower it to the largest batch
size that passes under default limits, or retain 50 while adding a
default-limits test covering the largest practical batch size. Update
test_create_batch_streams_50 and the create_batch_streams documentation so the
practical ceiling is explicitly documented and protected without relying on
unlimited budgets.
---
Nitpick comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 430-440: Add #[allow(clippy::too_many_arguments)] immediately
before both create_stream_with_cliff and the other affected stream-creation
function, matching the existing annotation pattern in the NFT stream contract,
so Clippy warnings are suppressed under -D warnings.
In `@contracts/payment-stream/src/test.rs`:
- Around line 1288-1293: Update the expected StreamResumedEvent in the resume
test to use the known literal paused_duration value rather than recalculating it
from paused_at. Since paused_at becomes unused, assert its expected pause-time
value instead of leaving it unused, while preserving the existing timestamp and
event assertions.
- Around line 738-740: Update the revocation test in
contracts/payment-stream/src/test.rs so revoke_delegate is the final contract
invocation, preserving any required setup reads beforehand; then import and
assert the emitted DelegationRevokedEvent payload. Correct the nearby comment to
reference DelegationRevokedEvent and the appropriate test, and add coverage for
both delegate-replacement and explicit-revocation emission paths where the
existing tests cover those operations.
In `@contracts/soulbound-badge/src/lib.rs`:
- Around line 176-184: Add a concise comment immediately after
`check_and_mint_badges` in the surrounding function explaining that
`user_contribution` is stale because `mint_badge` updates the persisted
`UserContribution`, and must not be written back or persisted afterward.
- Around line 427-434: Align the event topic naming in the soulbound-badge
contract with the PascalCase convention used by the payment-stream contract.
Update the event definitions corresponding to BadgeMintedEvent and
ContributionRecordedEvent so their emitted topics use consistent PascalCase
names, while preserving their existing payloads and behavior.
- Around line 727-747: Update test_is_eligible_for_badge in
soulbound-badge/src/lib.rs to keep it focused on eligibility by removing the
duplicated ContributionRecordedEvent assertion block after record_contribution.
Preserve the pre-contribution is_eligible_for_badge check as the unique purpose
of this test, and rely on test_record_contribution_bronze_badge for validating
the emitted event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a65be089-1ddd-4ad3-b276-4b366f7ea790
📒 Files selected for processing (68)
contracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_cancel_before_cliff_refunds_full_balance.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cancel_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cliff_blocks_withdrawal_before_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cliff_linear_vesting_after_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cliff_release_at_boundary.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_cliff_with_pause_and_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_50.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_empty.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_invalid_entry_leaves_no_state.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_invalid_entry_panics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_over_limit.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_unauthorized.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_batch_streams_with_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream_cliff_duration_zero.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream_with_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_create_stream_with_cliff_invalid.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_delegate_withdraw.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_after_withdrawal.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_exceeds_total.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_invalid_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_multiple.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_negative_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_deposit_updates_last_activity.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_multiple_streams_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_multiple_withdrawals_accumulate_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_only_sender_can_pause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_only_sender_can_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_operations_resume_after_unpause.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_overwrite_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_and_resume_stream.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pause_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_pausing_stops_token_vesting.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_read_operations_work_while_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_recipient_can_still_withdraw_after_delegate_set.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resume_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_resuming_continues_from_where_it_left_off.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_delegate_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_revoke_nonexistent_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_set_self_delegate.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_stream_paused_event_emitted.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_stream_resumed_event_emitted.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_after_pause_and_resume.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_before_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max_before_cliff.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max_blocked_when_paused.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdraw_max_updates_metrics.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount_zero_for_paused_streams.1.jsoncontracts/payment-stream/test_snapshots/test/test/test_withdrawal_updates_metrics.1.jsoncontracts/soulbound-badge/src/lib.rscontracts/soulbound-badge/test_snapshots/test/test_is_eligible_for_badge.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_multiple_users.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_not_eligible_for_next_milestone.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_bronze_badge.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_multiple_badges.1.jsoncontracts/soulbound-badge/test_snapshots/test/test_record_contribution_progressive_badges.1.json
🚧 Files skipped from review as they are similar to previous changes (38)
- contracts/payment-stream/test_snapshots/test/test/test_deposit_invalid_amount.1.json
- contracts/payment-stream/test_snapshots/test/test/test_resuming_continues_from_where_it_left_off.1.json
- contracts/payment-stream/test_snapshots/test/test/test_recipient_can_still_withdraw_after_delegate_set.1.json
- contracts/payment-stream/test_snapshots/test/test/test_set_delegate.1.json
- contracts/payment-stream/test_snapshots/test/test/test_pause_updates_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_create_stream.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw_blocked_when_paused.1.json
- contracts/payment-stream/test_snapshots/test/test/test_multiple_withdrawals_accumulate_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit_blocked_when_paused.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw_after_pause_and_resume.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw_max.1.json
- contracts/payment-stream/test_snapshots/test/test/test_pause_and_resume_stream.1.json
- contracts/payment-stream/test_snapshots/test/test/test_operations_resume_after_unpause.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount_zero_for_paused_streams.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit_negative_amount.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit_multiple.1.json
- contracts/payment-stream/test_snapshots/test/test/test_pausing_stops_token_vesting.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw.1.json
- contracts/payment-stream/test_snapshots/test/test/test_revoke_delegate_updates_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_cancel_stream.1.json
- contracts/payment-stream/test_snapshots/test/test/test_delegate_withdraw.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit_after_withdrawal.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw_max_updates_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_only_sender_can_pause.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdrawable_amount.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdrawal_updates_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_only_sender_can_resume.1.json
- contracts/payment-stream/test_snapshots/test/test/test_overwrite_delegate.1.json
- contracts/payment-stream/test_snapshots/test/test/test_read_operations_work_while_paused.1.json
- contracts/payment-stream/test_snapshots/test/test/test_revoke_nonexistent_delegate.1.json
- contracts/soulbound-badge/test_snapshots/test/test_record_contribution_bronze_badge.1.json
- contracts/payment-stream/test_snapshots/test/test/test_resume_updates_metrics.1.json
- contracts/payment-stream/test_snapshots/test/test/test_withdraw_max_blocked_when_paused.1.json
- contracts/payment-stream/test_snapshots/test/test/test_revoke_delegate.1.json
- contracts/payment-stream/test_snapshots/test/test/test_deposit_updates_last_activity.1.json
- contracts/payment-stream/test_snapshots/test/test/test_set_self_delegate.1.json
- contracts/payment-stream/test_snapshots/test/test/test_multiple_streams_metrics.1.json
- Remove @ts-nocheck, parsing errors, setState-in-effect violations, and forbidden require() imports - Clean up unused vars/imports and no-explicit-any across source and tests - Disable react-hooks/incompatible-library (React Compiler not enabled) - Rebuild OfframpSummary countdown with interval pattern; type contract.deployer - CI: configure frontend env vars from deployments/testnet.json - Delete stale type shim files - SDK: tighten types in clients and utils
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sdk/src/utils/streamHistory.ts (1)
72-78: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate
response.cursorand use the raw event count for pagination.
getEventsreturns a top-level response cursor; using the last matching event’sidmakes pagination depend on filtered results instead of the RPC page token. Storeresponse.cursorwith the parsed result, and compare an exported raw RPC event count againstpageLimitso unrelated stream events cannot finish the loop before more matching events are fetched.🤖 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 `@packages/sdk/src/utils/streamHistory.ts` around lines 72 - 78, Update getEvents to return response.cursor directly alongside parsedEvents, rather than deriving the cursor from the last matching event. Use the exported raw RPC event count for the pageLimit pagination condition so filtering unrelated stream events does not prematurely stop fetching matching events.packages/sdk/src/utils/errors.ts (1)
88-110: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRead the operation union before accessing
tr, and read trapped contract errors from the failure path.
firstOp.tr()only exists forOperationResultCode.opInner(). ForopBAD_AUTH,opNO_ACCOUNT, andopNOT_SUPPORTED, this code reaches an inaccessible union arm before theswitch(firstOp.switch())handler can return the operation-specific message.
InvokeHostFunctionResultCode.invokeHostFunctionSuccess()carries aHash, not anScValreturn value. Contract parsing should target operation-level error return values or trapped invoke results, not the success arm.Guard the
tr()access withoperationCode === xdr.OperationResultCode.opInner()and move theswitch(operationCode)handler accordingly. Add fixtures for a trapped contract call and for non-inner operation errors.🤖 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 `@packages/sdk/src/utils/errors.ts` around lines 88 - 110, Update the operation-result handling around firstOp so it reads firstOp.switch() before any tr() access, and only accesses the transaction result when the operation code is xdr.OperationResultCode.opInner(). Move contract-error parsing to the appropriate operation-level failure or trapped invoke-result path instead of treating InvokeHostFunctionResultCode.invokeHostFunctionSuccess() as an ScVal-bearing arm. Preserve operation-specific messages for non-inner codes such as opBAD_AUTH, opNO_ACCOUNT, and opNOT_SUPPORTED, and add fixtures covering trapped contract calls and non-inner operation errors.
🧹 Nitpick comments (1)
apps/web/src/graphql/analytics.service.ts (1)
118-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep
networkin the concrete data-source signature.
StreamDataSource.getStreams(network)andAnalyticsServicestill pass a network at Lines 220, 252, 294, and 336. The RPC sketch at Lines 128-130 also needs it to select the endpoint. RetaingetStreams(network: string), or update the interface and every caller together.🤖 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 `@apps/web/src/graphql/analytics.service.ts` at line 118, Restore the network: string parameter on AnalyticsService.getStreams and the concrete StreamDataSource.getStreams implementation, then propagate the network argument through the RPC call and all existing callers so endpoint selection remains network-specific.
🤖 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/app/`(overview)/distribution/page.tsx:
- Line 203: Update the useBalanceValidation call that produces
distInsufficientBalance to pass the actual distribution amount and currently
selected token instead of an empty amount and undefined token, preserving the
existing validation flow so over-balance distributions are rejected.
In `@apps/web/src/app/api/webhooks/subscriptions/route.ts`:
- Around line 28-31: Stop returning raw Error.message values from the webhook
API catch handlers. In apps/web/src/app/api/webhooks/subscriptions/route.ts
lines 28-31 and 59-62, apps/web/src/app/api/webhooks/subscriptions/[id]/route.ts
lines 30-33, and apps/web/src/app/api/webhooks/trigger/route.ts lines 34-37, log
sanitized server-side diagnostics and return a generic 500 error response
instead.
In `@apps/web/src/components/offramp/OfframpQuoteModal.tsx`:
- Around line 45-57: Update the expiry timer useEffect so it resets timeLeft to
null before returning when offrampData?.expiresAt is absent. Keep the existing
countdown and interval behavior unchanged for quotes with an expiration.
In `@apps/web/src/providers/StellarWalletProvider.tsx`:
- Line 80: Update StellarWalletProvider’s persisted wallet-ID handling to read
`@fundable/web`:selected_wallet first and fall back to the legacy
stellar_wallet_id key, while continuing to write only the new key. Ensure every
cleanup path removes both storage keys, and add a regression test covering
auto-reconnect from a session stored under stellar_wallet_id.
In `@apps/web/src/services/webhook.service.ts`:
- Around line 124-125: Update the dead-letter file read error handling in the
surrounding service method so only an ENOENT error produces an empty deadLetters
array; rethrow every other read or parse failure after the existing logging,
preventing the later write from overwriting preserved entries.
In `@packages/sdk/src/PaymentStreamClient.ts`:
- Line 11: Remove the unused StreamStatus import from the import declarations in
PaymentStreamClient.ts, leaving all other imports unchanged.
In `@packages/sdk/src/utils/streamHistory.ts`:
- Around line 46-54: Update the initial request construction in getStreamHistory
so an absent cursor and omitted startLedger do not default to ledger 1; require
startLedger or resolve the node’s retained oldestLedger via getHealth before
creating the request. Preserve cursor precedence for continuation pages, ensure
getAllStreamHistory follows the same behavior, and add coverage for omitted
startLedger.
---
Outside diff comments:
In `@packages/sdk/src/utils/errors.ts`:
- Around line 88-110: Update the operation-result handling around firstOp so it
reads firstOp.switch() before any tr() access, and only accesses the transaction
result when the operation code is xdr.OperationResultCode.opInner(). Move
contract-error parsing to the appropriate operation-level failure or trapped
invoke-result path instead of treating
InvokeHostFunctionResultCode.invokeHostFunctionSuccess() as an ScVal-bearing
arm. Preserve operation-specific messages for non-inner codes such as
opBAD_AUTH, opNO_ACCOUNT, and opNOT_SUPPORTED, and add fixtures covering trapped
contract calls and non-inner operation errors.
In `@packages/sdk/src/utils/streamHistory.ts`:
- Around line 72-78: Update getEvents to return response.cursor directly
alongside parsedEvents, rather than deriving the cursor from the last matching
event. Use the exported raw RPC event count for the pageLimit pagination
condition so filtering unrelated stream events does not prematurely stop
fetching matching events.
---
Nitpick comments:
In `@apps/web/src/graphql/analytics.service.ts`:
- Line 118: Restore the network: string parameter on AnalyticsService.getStreams
and the concrete StreamDataSource.getStreams implementation, then propagate the
network argument through the RPC call and all existing callers so endpoint
selection remains network-specific.
🪄 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: 699f034e-d658-447c-abc9-3e4f2f55f45c
📒 Files selected for processing (92)
.github/workflows/frontend.ymlapps/web/eslint.config.mjsapps/web/src/app/(overview)/distribution/page.tsxapps/web/src/app/(overview)/history/page.tsxapps/web/src/app/(overview)/offramp/page.tsxapps/web/src/app/api/certificates/route.tsapps/web/src/app/api/compile/route.tsapps/web/src/app/api/milestone-photos/route.tsapps/web/src/app/api/streams/map/route.tsapps/web/src/app/api/webhooks/subscriptions/[id]/route.tsapps/web/src/app/api/webhooks/subscriptions/route.tsapps/web/src/app/api/webhooks/trigger/route.tsapps/web/src/app/error.test.tsxapps/web/src/components/modules/history/DistributionDetailsModal.tsxapps/web/src/components/modules/history/HistoryTable.tsxapps/web/src/components/modules/payment-stream/CreatePaymentStream.tsxapps/web/src/components/modules/payment-stream/PaymentStreamSummary.tsxapps/web/src/components/modules/payment-stream/StreamActionsCell.tsxapps/web/src/components/modules/payment-stream/StreamCountdown.tsxapps/web/src/components/modules/payment-stream/StreamsHistory.tsxapps/web/src/components/modules/payment-stream/StreamsTable.tsxapps/web/src/components/modules/payment-stream/streamColumns.tsxapps/web/src/components/molecules/RecipientRow.tsxapps/web/src/components/offramp/BankSelector.tsxapps/web/src/components/offramp/OfframpQuoteModal.tsxapps/web/src/components/offramp/OfframpSuccessModal.tsxapps/web/src/components/offramp/OfframpSummary.tsxapps/web/src/components/organisms/DistributionForm.tsxapps/web/src/components/organisms/ImpactMap.tsxapps/web/src/components/organisms/OfframpSwapWidget.test.tsxapps/web/src/components/organisms/RecipientTable.tsxapps/web/src/components/organisms/SyncQueueBanner.test.tsxapps/web/src/components/organisms/SyncQueueDrawer.tsxapps/web/src/components/organisms/connect-button.tsxapps/web/src/components/organisms/navbar.tsxapps/web/src/components/organisms/wallet-modal.tsxapps/web/src/components/token-balance/TokenBalance.tsxapps/web/src/components/token-balance/TokenBalanceList.tsxapps/web/src/components/ui/error-boundary.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/graphql/analytics.service.tsapps/web/src/hooks/use-balance-validation.tsapps/web/src/hooks/use-create-stream.test.tsapps/web/src/hooks/use-create-stream.tsapps/web/src/hooks/use-debounce-callback.tsapps/web/src/hooks/use-distribute.tsapps/web/src/hooks/use-distribution-state.test.tsapps/web/src/hooks/use-distribution-transaction.tsapps/web/src/hooks/use-mobile.d.tsapps/web/src/hooks/use-mobile.jsapps/web/src/hooks/use-stream-delegation.tsapps/web/src/hooks/use-stream-progress.tsapps/web/src/hooks/use-tilt.tsapps/web/src/hooks/use-withdraw.tsapps/web/src/hooks/useOfframpBridge.test.tsapps/web/src/hooks/useOfframpBridge.tsapps/web/src/lib/__tests__/sanitize-error.test.tsapps/web/src/lib/__tests__/stream-validation.test.tsapps/web/src/lib/api.tsapps/web/src/lib/geo-cache.test.tsapps/web/src/lib/geo-cache.tsapps/web/src/lib/rate-limit.test.tsapps/web/src/lib/sanitize-error.tsapps/web/src/lib/stellar.tsapps/web/src/providers/StellarWalletProvider.test.tsxapps/web/src/providers/StellarWalletProvider.tsxapps/web/src/services/__tests__/webhook.service.test.tsapps/web/src/services/certificate.service.test.tsapps/web/src/services/certificate.service.tsapps/web/src/services/contract.deployer.tsapps/web/src/services/offramp.mock.tsapps/web/src/services/offramp.service.tsapps/web/src/services/stellar.service.retry.test.tsapps/web/src/services/stellar.service.test.tsapps/web/src/services/stellar.service.tsapps/web/src/services/transform-balances.test.tsapps/web/src/services/webhook.service.tsapps/web/src/types/class-variance-authority.d.tsapps/web/src/types/lucide-react.d.tsapps/web/src/types/radix-slot.d.tsapps/web/src/types/react-namespace.d.tsapps/web/src/types/react-shim.d.tsapps/web/src/types/webhook.tsapps/web/src/utils/amount-validation.test.tsapps/web/src/utils/csv-processing.tsapps/web/src/utils/safe-storage.tsapps/web/src/utils/stellar-validation.test.tspackages/sdk/src/DistributorClient.tspackages/sdk/src/PaymentStreamClient.tspackages/sdk/src/utils/batchDistribution.tspackages/sdk/src/utils/errors.tspackages/sdk/src/utils/streamHistory.ts
💤 Files with no reviewable changes (22)
- apps/web/src/components/organisms/SyncQueueDrawer.tsx
- apps/web/src/components/organisms/DistributionForm.tsx
- apps/web/src/hooks/use-mobile.js
- apps/web/src/components/token-balance/TokenBalance.tsx
- apps/web/src/components/organisms/SyncQueueBanner.test.tsx
- apps/web/src/components/modules/payment-stream/StreamsHistory.tsx
- apps/web/src/components/modules/payment-stream/StreamActionsCell.tsx
- apps/web/src/types/radix-slot.d.ts
- apps/web/src/hooks/use-mobile.d.ts
- apps/web/src/lib/tests/stream-validation.test.ts
- apps/web/src/types/class-variance-authority.d.ts
- apps/web/src/hooks/use-create-stream.test.ts
- apps/web/src/components/organisms/navbar.tsx
- apps/web/src/types/lucide-react.d.ts
- apps/web/src/components/organisms/RecipientTable.tsx
- apps/web/src/types/react-shim.d.ts
- apps/web/src/hooks/use-stream-delegation.ts
- apps/web/src/hooks/useOfframpBridge.test.ts
- apps/web/src/components/modules/history/DistributionDetailsModal.tsx
- apps/web/src/components/modules/payment-stream/StreamCountdown.tsx
- apps/web/src/components/modules/payment-stream/streamColumns.tsx
- apps/web/src/types/react-namespace.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/modules/payment-stream/PaymentStreamSummary.tsx
| }; | ||
|
|
||
| const { distInsufficientBalance } = useBalanceValidation(); | ||
| const { insufficientBalance: distInsufficientBalance } = useBalanceValidation("", undefined); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the actual distribution amount and selected token.
Line 203 always passes an empty amount. useBalanceValidation returns no error when amount is empty. Therefore, distInsufficientBalance is always false and cannot prevent an over-balance distribution.
🤖 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 `@apps/web/src/app/`(overview)/distribution/page.tsx at line 203, Update the
useBalanceValidation call that produces distInsufficientBalance to pass the
actual distribution amount and currently selected token instead of an empty
amount and undefined token, preserving the existing validation flow so
over-balance distributions are rejected.
| } catch (err: unknown) { | ||
| const message = err instanceof Error ? err.message : "Unknown error"; | ||
| return NextResponse.json( | ||
| { error: err.message || "Failed to retrieve subscriptions" }, | ||
| { error: message || "Failed to retrieve subscriptions" }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose raw internal error messages from webhook API routes.
These public handlers return Error.message directly. WebhookService can produce filesystem and implementation details. Return a generic 500 response and log sanitized diagnostics server-side.
apps/web/src/app/api/webhooks/subscriptions/route.ts#L28-L31: Return a generic error for GET failures.apps/web/src/app/api/webhooks/subscriptions/route.ts#L59-L62: Return a generic error for POST failures.apps/web/src/app/api/webhooks/subscriptions/[id]/route.ts#L30-L33: Return a generic error for DELETE failures.apps/web/src/app/api/webhooks/trigger/route.ts#L34-L37: Return a generic error for trigger failures.
📍 Affects 3 files
apps/web/src/app/api/webhooks/subscriptions/route.ts#L28-L31(this comment)apps/web/src/app/api/webhooks/subscriptions/route.ts#L59-L62apps/web/src/app/api/webhooks/subscriptions/[id]/route.ts#L30-L33apps/web/src/app/api/webhooks/trigger/route.ts#L34-L37
🤖 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 `@apps/web/src/app/api/webhooks/subscriptions/route.ts` around lines 28 - 31,
Stop returning raw Error.message values from the webhook API catch handlers. In
apps/web/src/app/api/webhooks/subscriptions/route.ts lines 28-31 and 59-62,
apps/web/src/app/api/webhooks/subscriptions/[id]/route.ts lines 30-33, and
apps/web/src/app/api/webhooks/trigger/route.ts lines 34-37, log sanitized
server-side diagnostics and return a generic 500 error response instead.
| useEffect(() => { | ||
| if (!offrampData?.expiresAt) { | ||
| setTimeLeft(null); | ||
| setIsExpired(false); | ||
| return; | ||
| } | ||
| if (!offrampData?.expiresAt) return; | ||
|
|
||
| const expiresAt = new Date(offrampData.expiresAt).getTime(); | ||
| const tick = () => { | ||
| const remaining = Math.floor( | ||
| (new Date(offrampData.expiresAt!).getTime() - Date.now()) / 1000, | ||
| ); | ||
| if (remaining <= 0) { | ||
| setTimeLeft(0); | ||
| setIsExpired(true); | ||
| return; | ||
| } | ||
| setTimeLeft(remaining); | ||
| setIsExpired(false); | ||
| const remaining = Math.floor((expiresAt - Date.now()) / 1000); | ||
| setTimeLeft(Math.max(0, remaining)); | ||
| }; | ||
|
|
||
| tick(); | ||
| const intervalId = setInterval(tick, 1000); | ||
| return () => clearInterval(intervalId); | ||
| }, [offrampData?.expiresAt]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear stale expiry state when the quote has no expiration.
If a prior quote expired, timeLeft remains 0 when the next offrampData has no expiresAt. The modal then shows “Quote Expired” for a quote that has no expiry. Reset timeLeft to null before the early return.
Proposed fix
useEffect(() => {
+ setTimeLeft(null);
if (!offrampData?.expiresAt) return;📝 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.
| useEffect(() => { | |
| if (!offrampData?.expiresAt) { | |
| setTimeLeft(null); | |
| setIsExpired(false); | |
| return; | |
| } | |
| if (!offrampData?.expiresAt) return; | |
| const expiresAt = new Date(offrampData.expiresAt).getTime(); | |
| const tick = () => { | |
| const remaining = Math.floor( | |
| (new Date(offrampData.expiresAt!).getTime() - Date.now()) / 1000, | |
| ); | |
| if (remaining <= 0) { | |
| setTimeLeft(0); | |
| setIsExpired(true); | |
| return; | |
| } | |
| setTimeLeft(remaining); | |
| setIsExpired(false); | |
| const remaining = Math.floor((expiresAt - Date.now()) / 1000); | |
| setTimeLeft(Math.max(0, remaining)); | |
| }; | |
| tick(); | |
| const intervalId = setInterval(tick, 1000); | |
| return () => clearInterval(intervalId); | |
| }, [offrampData?.expiresAt]); | |
| useEffect(() => { | |
| setTimeLeft(null); | |
| if (!offrampData?.expiresAt) return; | |
| const expiresAt = new Date(offrampData.expiresAt).getTime(); | |
| const tick = () => { | |
| const remaining = Math.floor((expiresAt - Date.now()) / 1000); | |
| setTimeLeft(Math.max(0, remaining)); | |
| }; | |
| tick(); | |
| const intervalId = setInterval(tick, 1000); | |
| return () => clearInterval(intervalId); | |
| }, [offrampData?.expiresAt]); |
🤖 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 `@apps/web/src/components/offramp/OfframpQuoteModal.tsx` around lines 45 - 57,
Update the expiry timer useEffect so it resets timeLeft to null before returning
when offrampData?.expiresAt is absent. Keep the existing countdown and interval
behavior unchanged for quotes with an expiration.
| } | ||
| const savedAddress = safeGetItem("stellar_wallet_address"); | ||
| const savedWalletId = safeGetItem("stellar_wallet_id"); | ||
| const savedWalletId = safeGetItem("@fundable/web:selected_wallet"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Migrate the previous wallet-ID storage key.
The provider now reads only @fundable/web:selected_wallet, while previous sessions used stellar_wallet_id. Existing users will fail the persisted-session check and lose auto-reconnect after upgrade. Read the old key as a fallback, write the new key, and remove both keys in every cleanup path. Add a regression test for an old-key session.
Also applies to: 184-184
🤖 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 `@apps/web/src/providers/StellarWalletProvider.tsx` at line 80, Update
StellarWalletProvider’s persisted wallet-ID handling to read
`@fundable/web`:selected_wallet first and fall back to the legacy
stellar_wallet_id key, while continuing to write only the new key. Ensure every
cleanup path removes both storage keys, and add a regression test covering
auto-reconnect from a session stored under stellar_wallet_id.
| } catch (err: unknown) { | ||
| if ((err as { code?: string }).code !== 'ENOENT') { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve existing dead-letter entries on read failures.
The catch logs every non-ENOENT error but then continues with an empty deadLetters array. A malformed file or transient read failure can overwrite all previous entries at Line 131. Treat only ENOENT as an empty log and rethrow every other error.
Proposed fix
} catch (err: unknown) {
- if ((err as { code?: string }).code !== 'ENOENT') {
- console.error('Failed to read dead-letter log:', err);
+ const code =
+ typeof err === 'object' && err !== null && 'code' in err
+ ? (err as { code?: unknown }).code
+ : undefined;
+ if (code !== 'ENOENT') {
+ throw err;
}
}📝 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.
| } catch (err: unknown) { | |
| if ((err as { code?: string }).code !== 'ENOENT') { | |
| } catch (err: unknown) { | |
| const code = | |
| typeof err === 'object' && err !== null && 'code' in err | |
| ? (err as { code?: unknown }).code | |
| : undefined; | |
| if (code !== 'ENOENT') { | |
| throw err; | |
| } | |
| } |
🤖 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 `@apps/web/src/services/webhook.service.ts` around lines 124 - 125, Update the
dead-letter file read error handling in the surrounding service method so only
an ENOENT error produces an empty deadLetters array; rethrow every other read or
parse failure after the existing logging, preventing the later write from
overwriting preserved entries.
| } from "@stellar/stellar-sdk/contract"; | ||
| import { Address } from "@stellar/stellar-sdk"; | ||
| import { | ||
| Stream, | ||
| StreamMetrics, | ||
| ProtocolMetrics, | ||
| StreamStatus, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused StreamStatus import.
The lint check reports StreamStatus as unused at Line 11. Remove it before merge.
Proposed fix
- StreamStatus,📝 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.
| StreamStatus, |
🧰 Tools
🪛 GitHub Actions: SDK CI / 1_lint-and-format.txt
[warning] 11-11: ESLint: 'StreamStatus' is defined but never used. (@typescript-eslint/no-unused-vars)
🪛 GitHub Actions: SDK CI / lint-and-format
[warning] 11-11: ESLint: 'StreamStatus' is defined but never used (@typescript-eslint/no-unused-vars). Command: pnpm --filter @fundable/sdk lint
🪛 GitHub Check: lint-and-format
[warning] 11-11:
'StreamStatus' is defined but never used
🤖 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 `@packages/sdk/src/PaymentStreamClient.ts` at line 11, Remove the unused
StreamStatus import from the import declarations in PaymentStreamClient.ts,
leaving all other imports unchanged.
Source: Linters/SAST tools
| const filter: StellarSdk.rpc.Api.EventFilter = { | ||
| type: "contract", | ||
| contractIds: [contractId], | ||
| }; | ||
|
|
||
| // cursor takes precedence over startLedger for continuation pages | ||
| const requestParams: StellarSdk.rpc.GetEventsRequest = cursor | ||
| const requestParams: StellarSdk.rpc.Api.GetEventsRequest = cursor | ||
| ? { filters: [filter], cursor, limit } | ||
| : { filters: [filter], startLedger, limit }; | ||
| : { filters: [filter], startLedger: startLedger ?? 1, limit }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'getStreamHistory|getAllStreamHistory|startLedger|cursor' \
packages/sdk apps/web \
--glob '*.ts' --glob '*.tsx' || trueRepository: Fundable-Protocol/stellar_client_os
Length of output: 50392
🌐 Web query:
Stellar RPC getEvents startLedger rejected if startLedger is outside retained ledger range documentation
💡 Result:
In Stellar RPC, the getEvents method requires that the startLedger parameter falls within the node's currently retained ledger range [1][2]. If startLedger is outside this range—meaning it is either older than the oldest stored ledger or newer than the latest ledger known to that specific RPC node—the request will be rejected with an error [1][2]. Key details regarding this behavior include: Ledger Retention: Stellar RPC instances maintain a bounded history of events and transactions, defined by the HISTORY_RETENTION_WINDOW configuration (defaulting to 120,960 ledgers, or approximately 7 days) [1][3]. Error Handling: When a request is rejected due to an out-of-bounds startLedger, the server typically returns a JSON-RPC error code -32600 [4][5][6]. The error message explicitly states that the startLedger must be within the current ledger range (e.g., "startLedger must be within the ledger range: [oldest] - [latest]") [4][6]. Validation: Developers are advised to use the getHealth RPC method to inspect the node's current oldestLedger and latestLedger values before submitting a getEvents request, ensuring the startLedger is within the valid bounds [1][5]. Transient vs. Permanent Errors: An error indicating that startLedger is "newer than the latest ledger" may be a transient condition caused by synchronization lag between the network head and the specific RPC node being queried; the ledger may become available shortly thereafter [6]. In contrast, an error indicating that startLedger is "older than the oldest ledger" typically implies the data has been pruned from the node's history [1][7].
Citations:
- 1: https://developers.stellar.org/docs/data/apis/rpc/api-reference/methods/getEvents
- 2: https://www.alchemy.com/docs/chains/stellar/stellar-api-endpoints/get-events
- 3: https://developers.stellar.org/docs/data/apis/rpc/admin-guide/configuring
- 4: Inconsistent out-of-range error for RPC methods stellar/stellar-rpc#454
- 5: Shall getEvent min startLedger depth be equal to earliest accessible ledger entry? stellar/stellar-rpc#260
- 6: Wait for soroban ledger ingestion at chain head instead of failing fatally on -32600 subquery/subql-stellar#167
- 7: https://developers.stellar.org/docs/build/guides/events/ingest
Do not default the first RPC page to ledger 1.
When cursor is absent and startLedger is omitted, this sends startLedger: 1. Stellar RPC rejects getEvents requests whose ledger is outside the node’s retained range. If a node’s oldestLedger is greater than 1, the first call fails for both getStreamHistory() and getAllStreamHistory() calls. Require startLedger, or fetch getHealth oldestLedger before creating the first request. Add a test for omitted startLedger.
🤖 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 `@packages/sdk/src/utils/streamHistory.ts` around lines 46 - 54, Update the
initial request construction in getStreamHistory so an absent cursor and omitted
startLedger do not default to ledger 1; require startLedger or resolve the
node’s retained oldestLedger via getHealth before creating the request. Preserve
cursor precedence for continuation pages, ensure getAllStreamHistory follows the
same behavior, and add coverage for omitted startLedger.
2 participants
Closes #426
Summary
Displays raw contract ID string instead of token ticker symbol in the PaymentStreamSummary component.
What Changed
apps/web/src/lib/validations.ts: AddedgetTokenSymbol()function that resolves both token values ("USDC") and contract addresses ("CBIELTK6...") to friendly ticker symbols by looking up againstSUPPORTED_TOKENS.apps/web/src/components/modules/payment-stream/PaymentStreamSummary.tsx: Replaced all rawtokenstring references withgetTokenSymbol(token)— applies to the Token label, Total Amount, Rate per Hour, and Rate per Day displays.apps/web/src/lib/__tests__/validations.test.ts: 16 tests covering success paths (by value and by address), failure paths (unrecognised tokens), and edge cases (empty string, whitespace, case sensitivity, consistency).Key Design Decisions
getTokenSymbol()function is pure and backwards-compatible — any valid token value or address produces the correct symbol, and anything unrecognised passes through unchanged.Acceptance Criteria Checklist
PaymentStreamSummary.tsx:80-95— contract addresses now display as friendly symbolsTest Output
Security Note
No sensitive data is handled or exposed. The function only maps public contract addresses to public ticker symbols. No API keys, secrets, or private data involved.
Summary by CodeRabbit
New Features
Bug Fixes
Tests