Feat(history): Integrate payment history tracking into payment-processing contract#27
Feat(history): Integrate payment history tracking into payment-processing contract#27onelove-dev wants to merge 4 commits into
Conversation
…yment processing contract
WalkthroughAdds payment history to the payment-processing contract: new types and errors, event definitions/emission, storage keys and CRUD/indexing, expanded public API (queries, validation, reconciliation), updated processing flow to create/update records and return payment_id, plus tests and snapshots reflecting the new state and events. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ctr as PaymentProcessingContract
participant S as Storage
participant T as TokenClient
participant E as Events
C->>Ctr: process_payment_with_signature(order, sig, merchant_pk)
activate Ctr
Ctr->>S: generate_payment_id()
S-->>Ctr: payment_id
Ctr->>S: create_payment_record(Pending)
S-->>Ctr: ok
Ctr->>E: emit_payment_record_created(record)
Ctr->>E: emit_payment_status_updated(Pending → Processing)
Ctr->>S: update_payment_record(Processing)
rect rgba(200,230,255,0.3)
note over Ctr,T: Execute payment
Ctr->>T: transfer(payer → merchant, amount, token)
T-->>Ctr: ok / error
end
alt transfer ok
Ctr->>S: update_payment_record(Completed)
Ctr->>E: emit_payment_status_updated(Processing → Completed)
Ctr->>E: emit_payment_completed(record)
Ctr-->>C: payment_id
else transfer error
Ctr->>S: update_payment_record(Failed, error_message)
Ctr->>E: emit_payment_status_updated(Processing → Failed, error)
Ctr->>E: emit_payment_failed(payment_id, payer, merchant, error)
Ctr-->>C: PaymentError(...)
end
deactivate Ctr
sequenceDiagram
autonumber
participant A as Admin/Caller
participant Ctr as PaymentProcessingContract
participant S as Storage
participant E as Events
A->>Ctr: reconcile_payments(payment_ids)
activate Ctr
loop for each payment_id
Ctr->>S: validate_payment_record(payment_id)
S-->>Ctr: bool
alt inconsistent
Ctr->>S: update_payment_record(...fix...)
end
end
Ctr->>E: emit_payment_reconciliation(payment_ids, found, fixed)
Ctr-->>A: fixed_count
deactivate Ctr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Contracts/payment-processing-contract/src/test.rs (2)
455-496: LGTM! Basic validation test covers the happy path.The test verifies that a successfully completed payment validates correctly.
Consider adding a test case for validation of a non-existent payment ID to ensure proper error handling:
#[test] #[should_panic] fn test_payment_validation_not_found() { let env = Env::default(); let contract_id = env.register(PaymentProcessingContract {}, ()); let client = PaymentProcessingContractClient::new(&env, &contract_id); // Try to validate non-existent payment let fake_payment_id = String::from_str(&env, "NONEXISTENT"); client.validate_payment(&fake_payment_id); }
498-545: LGTM! Reconciliation test covers the consistent payment scenario.The test verifies that reconciling an already-consistent payment returns 0 fixes, which is the expected behavior.
Consider adding a test for the scenario where reconciliation actually fixes inconsistencies. This would require either:
- A helper function to manually corrupt a payment record's state
- A test that simulates a partial failure scenario
This would provide more comprehensive coverage of the reconciliation functionality.
Contracts/payment-processing-contract/src/types.rs (1)
25-34: Consider explicit discriminants for binary stability.The
PaymentStatusenum usesrepr(u32)but relies on implicit discriminants. For a contract type that will be stored and potentially upgraded, explicit discriminants prevent accidental reordering from breaking compatibility.Apply this diff to add explicit discriminants:
#[contracttype] #[derive(Clone, Debug, PartialEq, Eq)] #[repr(u32)] pub enum PaymentStatus { - Pending, - Processing, - Completed, - Failed, - Cancelled, + Pending = 0, + Processing = 1, + Completed = 2, + Failed = 3, + Cancelled = 4, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Contracts/payment-processing-contract/src/error.rs(2 hunks)Contracts/payment-processing-contract/src/events.rs(1 hunks)Contracts/payment-processing-contract/src/lib.rs(5 hunks)Contracts/payment-processing-contract/src/storage.rs(3 hunks)Contracts/payment-processing-contract/src/test.rs(4 hunks)Contracts/payment-processing-contract/src/types.rs(1 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_duplicate_nonce.1.json(2 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_payment_history_query_by_merchant.1.json(1 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_payment_history_query_by_payer.1.json(1 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_payment_reconciliation.1.json(1 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_payment_validation.1.json(1 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_successful_payment_with_signature.1.json(2 hunks)Contracts/payment-processing-contract/test_snapshots/test/test_unsupported_token.1.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Contracts/payment-processing-contract/src/lib.rs (2)
Contracts/payment-processing-contract/src/storage.rs (4)
get_payment_record(137-141)get_merchant_payments(252-256)get_payer_payments(258-262)new(39-41)Contracts/payment-processing-contract/src/events.rs (5)
emit_payment_record_created(61-72)emit_payment_status_updated(74-89)emit_payment_completed(91-101)emit_payment_failed(103-118)emit_payment_reconciliation(120-133)
Contracts/payment-processing-contract/src/storage.rs (1)
Contracts/payment-processing-contract/src/lib.rs (6)
get_payment_record(36-36)get_payment_record(195-198)get_merchant_payments(37-37)get_merchant_payments(200-203)get_payer_payments(38-38)get_payer_payments(205-208)
🔇 Additional comments (8)
Contracts/payment-processing-contract/src/error.rs (1)
15-18: LGTM! Clear and consistent error additions.The new error variants align well with the payment history tracking feature. The discriminants are sequential, messages are descriptive, and the Display implementation follows the existing pattern.
Also applies to: 31-34
Contracts/payment-processing-contract/src/test.rs (4)
8-11: LGTM! Necessary imports for expanded test coverage.The addition of
PaymentStatus,PaymentRecordQuery, andQueryFiltersupports the new query and validation test cases.
117-145: LGTM! Comprehensive verification of payment record creation.The test now properly captures the
payment_idand verifies:
- Payment record fields (payer, merchant, amount, status)
- Completion timestamp
- Payment appears in both merchant and payer histories
This ensures the payment history integration is working correctly.
286-368: LGTM! Thorough test of merchant query functionality.The test properly verifies that querying by merchant returns all payments for that merchant, regardless of payer. The assertion logic correctly checks for both payment IDs in the results.
370-453: LGTM! Comprehensive test of payer query functionality.The test properly verifies that querying by payer returns all payments made by that payer across different merchants. The test structure mirrors the merchant query test, providing consistent coverage.
Contracts/payment-processing-contract/src/types.rs (3)
36-51: LGTM! Well-designed payment record structure.The
PaymentRecordstruct includes all necessary fields for tracking the payment lifecycle:
- Comprehensive payment details (payer, merchant, amount, token, nonce, order_id)
- Status tracking with timestamps (created_at, updated_at, completed_at)
- Optional error_message for failure scenarios
The use of
Optionforcompleted_atanderror_messageis appropriate since these are only populated in specific scenarios.
53-58: LGTM! Clean and focused query filter design.The
QueryFilterenum supports the two primary query patterns (by merchant and by payer) as specified in the requirements. The structure is straightforward and extensible if additional filter types are needed in the future.
60-66: LGTM! Flexible query structure with timestamp filtering.The
PaymentRecordQuerystruct combines the filter criteria with optional timestamp bounds, enabling:
- Queries for all payments (no timestamp filter)
- Queries from a specific time (from_timestamp only)
- Queries within a time range (both timestamps)
This design supports the querying requirements effectively.
| // Generate payment ID | ||
| let payment_id = storage.generate_payment_id(); | ||
|
|
||
| // Create initial payment record with Pending status | ||
| let mut payment_record = PaymentRecord { | ||
| payment_id: payment_id.clone(), | ||
| payer: payer.clone(), | ||
| merchant: order.merchant_address.clone(), | ||
| amount: order.amount, | ||
| token: order.token.clone(), | ||
| nonce: order.nonce, | ||
| order_id: order.order_id.clone(), | ||
| status: PaymentStatus::Pending, | ||
| created_at: current_time, | ||
| updated_at: current_time, | ||
| completed_at: None, | ||
| error_message: None, | ||
| }; | ||
|
|
||
| // Create payment record atomically before processing | ||
| storage.create_payment_record(&payment_record) | ||
| .map_err(|_| PaymentError::PaymentRecordCreationFailed)?; | ||
|
|
||
| // Emit payment record created event | ||
| Events::emit_payment_record_created(&env, &payment_record); | ||
|
|
||
| // Update status to Processing | ||
| payment_record.status = PaymentStatus::Processing; | ||
| payment_record.updated_at = env.ledger().timestamp(); | ||
| storage.update_payment_record(&payment_id, &payment_record) | ||
| .map_err(|_| PaymentError::PaymentRecordUpdateFailed)?; | ||
| Events::emit_payment_status_updated( | ||
| &env, | ||
| &payment_id, | ||
| PaymentStatus::Pending, | ||
| PaymentStatus::Processing, | ||
| None, | ||
| ); | ||
|
|
||
| // Process payment with error handling | ||
| let processing_result = Self::execute_payment( | ||
| &env, | ||
| &payer, | ||
| &order, | ||
| &_signature, | ||
| &_merchant_public_key, | ||
| &storage, | ||
| ); | ||
|
|
||
| match processing_result { | ||
| Ok(_) => { | ||
| // Update payment record to Completed | ||
| payment_record.status = PaymentStatus::Completed; | ||
| payment_record.updated_at = env.ledger().timestamp(); | ||
| payment_record.completed_at = Some(env.ledger().timestamp()); | ||
|
|
||
| storage.update_payment_record(&payment_id, &payment_record) | ||
| .map_err(|_| PaymentError::PaymentRecordUpdateFailed)?; | ||
|
|
||
| Events::emit_payment_status_updated( | ||
| &env, | ||
| &payment_id, | ||
| PaymentStatus::Processing, | ||
| PaymentStatus::Completed, | ||
| None, | ||
| ); | ||
| Events::emit_payment_completed(&env, &payment_record); | ||
|
|
||
| Ok(payment_id) | ||
| } | ||
| Err(e) => { | ||
| // Update payment record to Failed with error message | ||
| payment_record.status = PaymentStatus::Failed; | ||
| payment_record.updated_at = env.ledger().timestamp(); | ||
| let error_msg = String::from_str(&env, "Payment processing failed"); | ||
| payment_record.error_message = Some(error_msg.clone()); | ||
|
|
||
| // Try to update record, but don't fail if update fails | ||
| let _ = storage.update_payment_record(&payment_id, &payment_record); | ||
|
|
||
| Events::emit_payment_status_updated( | ||
| &env, | ||
| &payment_id, | ||
| PaymentStatus::Processing, | ||
| PaymentStatus::Failed, | ||
| payment_record.error_message.clone(), | ||
| ); | ||
| Events::emit_payment_failed( | ||
| &env, | ||
| &payment_id, | ||
| &payer, | ||
| &order.merchant_address, | ||
| error_msg, | ||
| ); | ||
|
|
||
| Err(e) | ||
| } |
There was a problem hiding this comment.
Failed payment history rolls back on error
We create and index the payment record, emit events, then return Err(e). In Soroban, returning Err aborts the invocation and reverts all earlier state writes, so the record/index updates never persist. You can see the fallout in test_duplicate_nonce where the events mention PAY_2, but the ledger still only contains PAY_1 because the call failed and the storage changes were rolled back. This violates the requirement to atomically create/update payment history and leaves observers with events that reference non-existent records.
We need to restructure the flow so storage commits survive failures—for example, run all fallible validations before inserting the record, and once the record exists, catch execute_payment errors, update the record to Failed, emit the failure events, and return Ok(payment_id) (or another non-reverting result that callers can inspect) instead of bubbling the Err. Only unrecoverable storage errors should abort before any writes. Adjusting the API if necessary to surface the failure context without reverting will keep history consistent.
|
@onelove-dev please solve potential issues and resolve conflicts. Thanks |
2 participants
Closes #17
Integrate payment history tracking into payment-processing contract
Summary
Testing
cargo test --package payment-processing-contractstellar contract buildSummary by CodeRabbit
New Features
Improvements
Tests