feat: Implement payment history query and management functions (#15)#32
feat: Implement payment history query and management functions (#15)#32ritik4ever wants to merge 4 commits into
Conversation
…ell#15) - Added comprehensive payment record data structures * PaymentQueryParams for flexible filtering * PaymentStats for global analytics * MerchantPaymentSummary and PayerPaymentSummary * PaymentBucket for time-based aggregation * CompressedPaymentRecord for archival - Implemented efficient storage indexing * Merchant-based payment index * Payer-based payment index * Time-based bucketing for temporal queries * Token-based indexing * Composite indexes for complex queries - Added payment history query functions * get_merchant_payment_history with pagination * get_payer_payment_history with pagination * query_payments with flexible filtering * get_payments_by_time_range * get_payments_by_token - Implemented statistics and analytics * Merchant payment statistics * Payer payment statistics * Global payment statistics * Real-time stats updates on payment processing - Added data management features * Payment record validation * Compression for old records * Archive functionality for historical data * Storage optimization patterns - Comprehensive test suite * Payment history recording tests * Query and pagination tests * Statistics calculation tests * Time-based and token-based query tests * Archive functionality tests * Edge case and error handling tests - Gas-optimized implementation * Efficient bitmap-based indexing * Batch operations support * Minimal storage reads/writes * Optimized data structures All acceptance criteria met: ✓ Well-defined and documented data structures ✓ Efficient storage operations ✓ Data integrity validation ✓ Storage optimization for large datasets ✓ Efficient indexing for queries ✓ Thoroughly tested ✓ Optimized for contract limitations ✓ Extensible for future features
WalkthroughAdds paginated payment history, query/filter APIs, time/token indexing, statistics aggregation, archival/compression, new contract types, two pagination-related error variants, and comprehensive tests for history, queries, stats, time bucketing, token grouping, pagination boundaries, and archival. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Contract as PaymentProcessingTrait
participant Storage as Storage
participant Ledger as Ledger/State
rect rgb(235,245,255)
note right of Client: Retrieve paginated merchant/payer history
Client->>Contract: get_merchant_payment_history(merchant, limit, offset)
Contract->>Contract: validate pagination -> may return InvalidPaginationParams/QueryLimitExceeded
Contract->>Storage: get_merchant_payments(merchant, limit, offset)
Storage->>Ledger: Read MerchantPaymentIndex + PaymentMetadata
Ledger-->>Storage: Indexed entries
Storage-->>Contract: Vec<PaymentRecord>
Contract-->>Client: Result<Vec<PaymentRecord>, PaymentError>
end
sequenceDiagram
autonumber
actor Client
participant Contract as PaymentProcessingTrait
participant Storage as Storage
participant Ledger as Ledger/State
rect rgb(235,255,235)
note right of Client: Record payment and index for queries
Client->>Contract: process_payment(...)
Contract->>Storage: index_payment(record)
Storage->>Ledger: Update merchant/payer/time/token indices & stats
Ledger-->>Storage: OK
Storage-->>Contract: OK
Contract-->>Client: Result<(), PaymentError>
end
rect rgb(255,245,235)
note right of Client: Archive old payments (admin)
Client->>Contract: archive_old_payments(admin, cutoff_time)
Contract->>Contract: verify admin -> may return error
Contract->>Storage: compress_old_payments(cutoff_time, batch_size)
Storage->>Ledger: Create CompressedArchive entries, update indices
Ledger-->>Storage: OK
Storage-->>Contract: OK
Contract-->>Client: Result<(), PaymentError>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
Please review my PR @MPSxDev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Contracts/payment-processing-contract/src/storage.rs (1)
5-7: Missing imports for newly used types (compile blocker).
PaymentIndexEntry,PaymentBucket,MerchantPaymentSummary,PayerPaymentSummary,PaymentStats,CompressedPaymentRecordare referenced but not imported.Apply this diff:
-use crate::{ - types::{Merchant, NonceTracker, Fee, MultiSigPayment, PaymentRecord, RefundRequest, MultiSigPaymentRecord}, - error::PaymentError, -}; +use crate::{ + types::{ + Merchant, NonceTracker, Fee, MultiSigPayment, PaymentRecord, RefundRequest, MultiSigPaymentRecord, + PaymentIndexEntry, PaymentBucket, MerchantPaymentSummary, PayerPaymentSummary, PaymentStats, + CompressedPaymentRecord, + }, + error::PaymentError, +};
🧹 Nitpick comments (4)
Contracts/payment-processing-contract/src/types.rs (1)
288-371: Types look aligned; consider typingCompressedPaymentRecord.status.Using raw
u8with magic numbers hurts readability/safety. Prefer a compact enum:#[contracttype] #[derive(Clone, Debug, PartialEq)] pub enum CompressedStatus { Paid = 0, PartiallyRefunded = 1, FullyRefunded = 2 }Then change
status: CompressedStatus.Contracts/payment-processing-contract/src/storage.rs (3)
682-704: No pagination validation here; ensure contract layer enforces limits.These helpers return
Vecand don’t validatelimit/offset. Given new errors, confirmlib.rsvalidates (e.g.,1 <= limit <= MAX_LIMIT, saneoffset) before calling storage.Also applies to: 706-728
595-624: Stats fields aren’t fully maintained (uniques).
PayerPaymentSummary.unique_merchantsnever updates.PaymentStats.unique_payersnever updates.Consider maintaining small per-merchant/payer sets or counting via maps to compute uniques; or document as “not tracked.”
Also applies to: 626-652, 654-678
785-823: Archival doesn’t prune or mark originals.
compress_old_paymentswrites compressed copies but doesn’t remove or flag originals fromPayments. If intentional, consider:
- Removing archived records from hot map,
- Or marking them to route queries to archive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Contracts/payment-processing-contract/src/error.rs(1 hunks)Contracts/payment-processing-contract/src/lib.rs(2 hunks)Contracts/payment-processing-contract/src/storage.rs(3 hunks)Contracts/payment-processing-contract/src/test.rs(1 hunks)Contracts/payment-processing-contract/src/test_payment_history.rs(1 hunks)Contracts/payment-processing-contract/src/types.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Contracts/payment-processing-contract/src/storage.rs (1)
Contracts/payment-processing-contract/src/lib.rs (1)
get_payments_by_time_range(1393-1397)
Contracts/payment-processing-contract/src/test_payment_history.rs (1)
Contracts/payment-processing-contract/src/test.rs (1)
create_token_contract(18-27)
Contracts/payment-processing-contract/src/lib.rs (1)
Contracts/payment-processing-contract/src/storage.rs (5)
env(421-424)env(430-433)env(438-441)env(656-659)get_payments_by_time_range(759-783)
🔇 Additional comments (1)
Contracts/payment-processing-contract/src/test.rs (1)
3-4: OK: test module inclusion looks correct.
mod test_payment_history;properly gates under#![cfg(test)].
- Changed CompressedPaymentRecord.status from u8 to u32 (Soroban compatible) - Fixed import statement formatting (added missing comma) - Added missing type imports in storage.rs - Made get_payments_map public for query functions - Added Display implementations for new error variants - Moved trait method implementations from trait to impl block
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Contracts/payment-processing-contract/src/types.rs (1)
288-370: New types support payment history and analytics.The seven new types (PaymentQueryParams, PaymentStats, MerchantPaymentSummary, PayerPaymentSummary, PaymentIndexEntry, PaymentBucket, CompressedPaymentRecord) are well-structured and align with the PR objectives.
Consider refactoring
CompressedPaymentRecord.status(line 369) to use an enum instead of a u32 with magic numbers:+#[contracttype] +#[derive(Clone, Debug, PartialEq)] +pub enum CompressedPaymentStatus { + Paid = 0, + PartiallyRefunded = 1, + FullyRefunded = 2, +} + #[contracttype] #[derive(Clone, Debug, PartialEq)] pub struct CompressedPaymentRecord { pub order_id: String, pub merchant_address: Address, pub payer_address: Address, pub token: Address, pub amount: i128, pub paid_at: u64, - pub status: u32, // 0: paid, 1: partially refunded, 2: fully refunded + pub status: CompressedPaymentStatus, }This improves type safety and eliminates the need for inline documentation of magic numbers.
Contracts/payment-processing-contract/src/storage.rs (1)
476-504: Optimize storage writes in index_payment; timestamp unit confirmed
env.ledger().timestamp()returns seconds, so daily bucket calculation is correct.- Each payment triggers 7+ persistent writes (merchant index, payer index, time bucket, token index, 3 stats maps); consider batching, pruning old entries, or consolidating indexes to reduce gas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Contracts/payment-processing-contract/src/error.rs(3 hunks)Contracts/payment-processing-contract/src/lib.rs(4 hunks)Contracts/payment-processing-contract/src/storage.rs(5 hunks)Contracts/payment-processing-contract/src/types.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Contracts/payment-processing-contract/src/storage.rs (1)
Contracts/payment-processing-contract/src/lib.rs (2)
get_payments_by_time_range(200-204)get_payments_by_time_range(1425-1438)
Contracts/payment-processing-contract/src/lib.rs (1)
Contracts/payment-processing-contract/src/storage.rs (6)
env(426-429)env(435-438)env(443-446)env(661-664)get_payments_by_time_range(764-788)new(79-81)
🔇 Additional comments (2)
Contracts/payment-processing-contract/src/error.rs (1)
53-54: LGTM! Display implementations added.The new error variants and their Display implementations are now present and resolve the previous review concern.
Also applies to: 106-107
Contracts/payment-processing-contract/src/lib.rs (1)
580-582: Payment indexing integrated successfully.The integration of
storage.index_paymentafter saving the payment record ensures that payment history, stats, and indices are updated consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
Contracts/payment-processing-contract/src/lib.rs (4)
1323-1393: Optimizequery_paymentsto leverage indices instead of full scan.
query_paymentsiterates over all payments in storage (line 1339), which is inefficient and defeats the purpose of the indexing layer. For large datasets, this will exceed gas limits.Leverage the time-based and token-based indices built in storage.rs:
- For time-range queries, use
get_payments_by_time_rangeto retrieve relevant buckets, then fetch payments by order_id- For token queries, use the token index (DataKey::TokenBasedIndex) to retrieve order_ids, then fetch payments
- Consider building additional indices for amount-based filtering if needed frequently
1404-1417: Use semantically correct error for missing stats.Lines 1410 and 1416 return
PaymentNotFoundfor missing payer/global stats, which is misleading.Consider returning a more appropriate error or a default/empty stats object. For example:
- storage.get_payer_stats(&payer) - .ok_or(PaymentError::PaymentNotFound) + Ok(storage.get_payer_stats(&payer).unwrap_or(PayerPaymentSummary { + payer_address: payer, + total_spent: 0, + payment_count: 0, + unique_merchants: 0, + first_payment: None, + last_payment: None, + }))Similarly for global stats at line 1416.
1419-1432: Use semantically correct error for time range validation.Line 1426 uses
InvalidAmountfor time range validation, which is semantically incorrect.Consider a more appropriate error. For example:
- if start_time >= end_time { - return Err(PaymentError::InvalidAmount); - } + if start_time >= end_time { + return Err(PaymentError::InvalidPaginationParams); + }Alternatively, add a new error variant like
InvalidTimeRangeifInvalidPaginationParamsdoesn't fit semantically.
1495-1497: Use semantically correct error for cutoff time validation.Line 1497 uses
InvalidAmountfor cutoff time validation, which is semantically incorrect.Consider a more appropriate error. For example:
- if cutoff_time >= env.ledger().timestamp() { - return Err(PaymentError::InvalidAmount); - } + if cutoff_time >= env.ledger().timestamp() { + return Err(PaymentError::InvalidPaginationParams); + }Alternatively, add a new error variant like
InvalidCutoffTimeifInvalidPaginationParamsdoesn't fit semantically.
🧹 Nitpick comments (1)
Contracts/payment-processing-contract/src/storage.rs (1)
476-504: Consider batching index updates to reduce storage writes.
index_paymentperforms 5+ separate storage writes (merchant index, payer index, time bucket, token index, merchant stats, payer stats, global stats). Each write incurs gas costs.Consider accumulating index updates in memory and writing them in a single batch operation, similar to how
batch_mark_nonces_used(lines 224-240) optimizes nonce updates. This would reduce gas costs for high-throughput scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Contracts/payment-processing-contract/src/lib.rs(4 hunks)Contracts/payment-processing-contract/src/storage.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Contracts/payment-processing-contract/src/storage.rs (1)
Contracts/payment-processing-contract/src/lib.rs (2)
get_payments_by_time_range(200-204)get_payments_by_time_range(1419-1432)
Contracts/payment-processing-contract/src/lib.rs (1)
Contracts/payment-processing-contract/src/storage.rs (6)
env(426-429)env(435-438)env(443-446)env(661-664)get_payments_by_time_range(764-788)new(79-81)
🔇 Additional comments (10)
Contracts/payment-processing-contract/src/storage.rs (3)
687-733: LGTM with pagination validation.The merchant and payer payment query methods correctly implement pagination with offset/limit logic and fetch payments by order_id from the index.
735-761: LGTM.Statistics retrieval methods correctly fetch from persistent storage and return Optional results.
790-859: LGTM with batching and cursor tracking.The compression function now addresses the previous review feedback by:
- Accepting a
batch_sizeparameter (line 792)- Tracking progress with a persistent cursor (lines 801-806)
- Processing only up to
batch_sizerecords per invocation (lines 817-820)- Clearing the cursor when done (lines 852-856)
This allows callers to repeatedly invoke the function to complete archival in chunks without gas exhaustion.
Contracts/payment-processing-contract/src/lib.rs (7)
22-26: LGTM.New type imports are correctly added and properly formatted with commas.
163-218: LGTM.Payment history query and management functions are correctly declared in the trait with appropriate parameter types and return signatures.
581-582: LGTM.Payment indexing is correctly invoked after saving the payment record, enabling efficient queries.
1285-1313: LGTM.Merchant and payer payment history methods correctly validate pagination parameters using
InvalidPaginationParamsand delegate to the storage layer.
1315-1321: LGTM.Direct order_id lookup correctly delegates to storage.
1395-1402: LGTM.Merchant stats retrieval correctly uses
MerchantNotFounderror when stats don't exist, which is semantically appropriate.
1434-1475: LGTM.Token-based query now correctly uses the token index (lines 1446-1453) to retrieve order_ids and then fetches payments, avoiding the full scan. Pagination is properly implemented with offset/limit logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
Contracts/payment-processing-contract/src/lib.rs (5)
1323-1393: Avoid full scan in query_payments; leverage indices and validate time rangeUse time/token indices to avoid O(N) scans and add start/end validation. See diff sketch:
fn query_payments( env: Env, params: PaymentQueryParams, ) -> Result<Vec<PaymentRecord>, PaymentError> { let storage = Storage::new(&env); // Validate pagination parameters if params.limit == 0 || params.limit > 100 { return Err(PaymentError::InvalidPaginationParams); } + // Validate time range if both provided + if let (Some(start), Some(end)) = (params.start_time, params.end_time) { + if start >= end { + return Err(PaymentError::InvalidPaginationParams); + } + } - // Get all payments from storage - let all_payments = storage.get_payments_map(); - let mut filtered_payments = Vec::new(&env); + let mut filtered_payments = Vec::new(&env); let mut count = 0u32; let mut skipped = 0u32; - for (_order_id, payment) in all_payments.iter() { + // Prefer narrow sources: token index, then time buckets, else full map + // 1) If token specified, iterate order_ids from token index. + if let Some(ref token) = params.token { + let token_index: Map<Address, soroban_sdk::Vec<soroban_sdk::String>> = env + .storage() + .persistent() + .get(&crate::storage::DataKey::TokenBasedIndex.as_symbol(&env)) + .unwrap_or_else(|| Map::new(&env)); + let order_ids = token_index.get(token.clone()).unwrap_or_else(|| soroban_sdk::Vec::new(&env)); + for i in 0..order_ids.len() { + if let Some(order_id) = order_ids.get(i) { + if let Ok(payment) = storage.get_payment(&order_id) { + // Apply time/amount filters + let mut matches = true; + if let Some(start) = params.start_time { if payment.paid_at < start { matches = false; } } + if let Some(end) = params.end_time { if payment.paid_at > end { matches = false; } } + if let Some(mina) = params.min_amount { if payment.amount < mina { matches = false; } } + if let Some(maxa) = params.max_amount { if payment.amount > maxa { matches = false; } } + if matches { + if skipped < params.offset { skipped += 1; continue; } + filtered_payments.push_back(payment); + count += 1; + if count >= params.limit { break; } + } + } + } + } + return Ok(filtered_payments); + } + + // 2) If time range specified, iterate buckets + if let (Some(start), Some(end)) = (params.start_time, params.end_time) { + let buckets = storage.get_payments_by_time_range(start, end); + for bi in 0..buckets.len() { + if let Some(bucket) = buckets.get(bi) { + let ids = bucket.order_ids; // assuming bucket carries order_ids + for i in 0..ids.len() { + if let Some(order_id) = ids.get(i) { + if let Ok(payment) = storage.get_payment(&order_id) { + // Apply amount filters + let mut matches = true; + if let Some(mina) = params.min_amount { if payment.amount < mina { matches = false; } } + if let Some(maxa) = params.max_amount { if payment.amount > maxa { matches = false; } } + if matches { + if skipped < params.offset { skipped += 1; continue; } + filtered_payments.push_back(payment); + count += 1; + if count >= params.limit { break; } + } + } + } + } + if count >= params.limit { break; } + } + } + return Ok(filtered_payments); + } + + // 3) Fallback (no filters narrowing the set): iterate all payments (as before) + let all_payments = storage.get_payments_map(); for (_order_id, payment) in all_payments.iter() { // Apply filters let mut matches = true; // Time range filter if let Some(start_time) = params.start_time { if payment.paid_at < start_time { matches = false; } } if let Some(end_time) = params.end_time { if payment.paid_at > end_time { matches = false; } } // Amount range filter if let Some(min_amount) = params.min_amount { if payment.amount < min_amount { matches = false; } } if let Some(max_amount) = params.max_amount { if payment.amount > max_amount { matches = false; } } - // Token filter - if let Some(ref token) = params.token { - if payment.token != *token { - matches = false; - } - } + // Token filter (unlikely path when token is None) + if let Some(ref token) = params.token { + if payment.token != *token { matches = false; } + } if matches { // Handle offset if skipped < params.offset { skipped += 1; continue; } // Add to results filtered_payments.push_back(payment); count += 1; // Check limit if count >= params.limit { break; } } } Ok(filtered_payments) }
1404-1412: Return sensible default for missing payer stats (or use stats-specific error)Mapping to PaymentNotFound is misleading. Prefer default summary or a stats-specific error.
- storage.get_payer_stats(&payer) - .ok_or(PaymentError::PaymentNotFound) + Ok(storage.get_payer_stats(&payer).unwrap_or(PayerPaymentSummary { + payer_address: payer, + total_spent: 0, + payment_count: 0, + unique_merchants: 0, + first_payment: None, + last_payment: None, + }))
1413-1418: Global stats error semanticsAvoid PaymentNotFound; return default stats or introduce a StatsNotFound variant.
Options:
- If PaymentStats: Default exists: Ok(storage.get_global_stats().unwrap_or_default())
- Else: add PaymentError::StatsNotFound and use .ok_or(PaymentError::StatsNotFound)
1419-1433: Use appropriate error for invalid time rangeUse InvalidPaginationParams (or a dedicated InvalidTimeRange) instead of InvalidAmount.
- if start_time >= end_time { - return Err(PaymentError::InvalidAmount); - } + if start_time >= end_time { + return Err(PaymentError::InvalidPaginationParams); + }
1477-1508: Archive cutoff validation error + consider returning processed count
- Use a semantically correct error for cutoff validation.
- Optionally bubble up processed count from compression to let callers loop.
- if cutoff_time >= env.ledger().timestamp() { - return Err(PaymentError::InvalidAmount); - } + if cutoff_time >= env.ledger().timestamp() { + return Err(PaymentError::InvalidPaginationParams); + }Optional API improvement:
- Capture returned count and return Result<u32, PaymentError>.
- Emit the count in the event payload for observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Contracts/payment-processing-contract/src/lib.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Contracts/payment-processing-contract/src/lib.rs (1)
Contracts/payment-processing-contract/src/storage.rs (6)
env(426-429)env(435-438)env(443-446)env(661-664)get_payments_by_time_range(764-788)new(79-81)
🔇 Additional comments (6)
Contracts/payment-processing-contract/src/lib.rs (6)
580-583: Indexing after saving payment — LGTMPayment is saved then indexed; aligns with query patterns and stats updates.
1278-1283: get_refund_status — LGTMStraightforward lookup and return of status.
1284-1298: Merchant payment history — LGTMValidates pagination and delegates to storage-backed index.
1300-1313: Payer payment history — LGTMCorrect pagination validation and indexed retrieval.
1315-1322: Fetch by order_id — LGTMDirect storage lookup with correct error propagation.
1395-1403: Merchant stats — LGTMError mapping to MerchantNotFound is acceptable if merchant must exist to have stats.
1 participant
Description
This PR implements comprehensive payment history tracking, querying, and management functionality as specified in issue #15.
Changes Made
Data Structures
PaymentQueryParamsfor flexible payment filteringPaymentStatsfor global payment analyticsMerchantPaymentSummaryandPayerPaymentSummaryfor user-specific statisticsPaymentBucketfor time-based aggregationCompressedPaymentRecordfor efficient archivalPaymentIndexEntryfor optimized indexingStorage Layer
Query Functions
get_merchant_payment_history: Paginated merchant payment historyget_payer_payment_history: Paginated payer payment historyget_payment_by_order_id: Single payment lookupquery_payments: Flexible filtering with multiple criteriaget_merchant_payment_stats: Merchant-specific statisticsget_payer_payment_stats: Payer-specific statisticsget_global_payment_stats: Platform-wide analyticsget_payments_by_time_range: Time-based payment aggregationget_payments_by_token: Token-specific payment queriesarchive_old_payments: Admin function for data archivalIntegration
process_payment_with_signatureTesting
test_payment_history.rs)Performance Optimizations
Gas Efficiency
Acceptance Criteria Met
Summary by CodeRabbit
New Features
Bug Fixes
Tests