feat(contracts): implement issues #271, #272, #273, #274#279
Merged
Conversation
LightForgeHub#273, LightForgeHub#274 Closes LightForgeHub#271 — Session Recording Consent Flag Closes LightForgeHub#272 — Data Deletion Request (Right to Be Forgotten) Closes LightForgeHub#273 — Dead Letter Queue for Failed Token Transfers Closes LightForgeHub#274 — Contract Event Replay Index ═══════════════════════════════════════════════════════════════ ISSUE LightForgeHub#271 — Session Recording Consent Flag ═══════════════════════════════════════════════════════════════ Problem: SkillSphere may offer session recording. Without explicit on-chain consent from both parties, recording could create legal liability. What was done: • Added a new RecordingConsent enum to lib.rs with three states: - None (default, no consent given) - SeekerApproved (first party has consented) - BothApproved (both seeker and expert have consented) • Added recording_consent: RecordingConsent field to the Session struct. All four Session instantiation sites (start_session via create_active_session, reserve_session, start_session_with_voucher, and the DEX-swap session path) are initialised to RecordingConsent::None. • Added grant_recording_consent(caller, session_id) public function on SkillSphereContract. Either the seeker or expert may call it. The state machine transitions None → SeekerApproved → BothApproved. Recording is only considered approved when BothApproved is reached. • Consent state is persisted in the Session struct and therefore queryable via the existing get_session() function. • Emits a RecordingConsentUpdated event (symbol: recCons) via the standardised publish_event webhook envelope, carrying (caller, new_consent_state). • Rejects calls on sessions that are not Active or Paused (InvalidSessionState) and rejects non-participants (Unauthorized). Files changed: contracts/src/lib.rs — RecordingConsent enum, Session field, grant_recording_consent() function. ═══════════════════════════════════════════════════════════════ ISSUE LightForgeHub#272 — Data Deletion Request (Right to Be Forgotten) ═══════════════════════════════════════════════════════════════ Problem: GDPR-compliant platforms must allow users to delete personal data. On-chain data is immutable, but metadata CIDs can be replaced with a tombstone so the content is no longer reachable. What was done: • Added a data_deletion sub-module to contracts/src/identity.rs. It exposes two helpers used by the main contract: - tombstone(env) → String::from_str(env, "DELETED") - emit_deletion_event(env, address) — publishes a raw DataDeletionRequested event (topic: dataDel) carrying (address, timestamp). • Added request_data_deletion(caller, address) public function on SkillSphereContract in lib.rs: - Requires auth from caller. - Authorisation check: caller must be the address owner OR hold the SuperAdmin role (via roles::has_role). - Tombstones ExpertProfile.metadata_cid for the address if a profile exists in persistent storage. - Iterates all sessions (0..SessionCounter) and tombstones metadata_cid and encrypted_notes_hash for any session where seeker == address || expert == address. - Returns InvalidSessionState immediately if any matching session is still Active or Paused (cannot delete from live sessions). - Emits DataDeletionRequested event via identity::data_deletion:: emit_deletion_event. • Declared identity as pub mod identity in lib.rs so the data_deletion sub-module is accessible from the contract impl. Files changed: contracts/src/identity.rs — data_deletion pub mod added. contracts/src/lib.rs — pub mod identity declaration, request_data_deletion() function. ═══════════════════════════════════════════════════════════════ ISSUE LightForgeHub#273 — Dead Letter Queue for Failed Token Transfers ═══════════════════════════════════════════════════════════════ Problem: If a token transfer in settle_session fails (e.g. the recipient's trustline is missing on Stellar), the session becomes permanently stuck and the expert's funds are unrecoverable. What was done: • Created contracts/src/recovery.rs — a new module implementing the Dead Letter Queue (DLQ): - enqueue_failed_transfer(env, recipient, amount): stores the failed amount under DataKey::FailedTransfer(recipient) in persistent storage. Amounts are summed if multiple failures occur for the same recipient. Records the enqueue timestamp under a ("dlq_ts", recipient) key for expiry tracking. Emits a TransferQueued event (symbol: dlqQueue). - claim_failed_transfer(env, recipient, token): requires auth from recipient. Checks the 180-day expiry window. Clears the DLQ entry (checks-effects-interactions pattern) then retries the token transfer. Emits a TransferClaimed event (dlqClaim). Returns InsufficientBalance if nothing is queued, SessionExpired if the entry has expired. - pending_failed_transfer(env, recipient): read-only query. - DLQ_EXPIRY_SECS = 180 * 24 * 60 * 60 (180 days). • Added DataKey::FailedTransfer(Address) variant to the DataKey enum in lib.rs. • Modified internal_settle() in lib.rs: the expert payout transfer now uses token_client.try_transfer() instead of transfer(). On failure, recovery::enqueue_failed_transfer() is called and expert_payout is set to 0 so settlement still completes cleanly. • Added two public entry points on SkillSphereContract: - claim_failed_transfer(recipient, token) → Result<i128, Error> - get_failed_transfer_amount(recipient) → i128 • Declared recovery as pub mod recovery in lib.rs. Files changed: contracts/src/recovery.rs — new file, full DLQ implementation. contracts/src/lib.rs — DataKey::FailedTransfer, pub mod recovery, try_transfer in internal_settle, claim_failed_transfer() and get_failed_transfer_amount() functions. ═══════════════════════════════════════════════════════════════ ISSUE LightForgeHub#274 — Contract Event Replay Index (Ring Buffer) ═══════════════════════════════════════════════════════════════ Problem: If the off-chain indexer misses events (downtime, chain reorg), it needs a way to re-fetch historical events without re-scanning the entire chain from genesis. What was done: • Extended contracts/src/events.rs with a ring-buffer implementation: - EVENT_LOG_CAPACITY = 1000 (max entries retained). - EventLogEntry contracttype struct: { index, event_type, session_id, timestamp }. - append_to_ring(env, event_type, session_id): writes an entry to DataKey::EventLog(slot) in Temporary storage (low rent cost) and advances the head pointer stored under symbol "evtHead". Slot = head % 1000, so the buffer wraps automatically. - get_event_log(env, from_index, limit): returns up to limit entries starting from from_index. Validates that each slot's stored index matches the requested index to detect overwritten entries. Returns a Vec<EventLogEntry>. - event_log_head(env): returns the current head pointer. • publish_event() now calls append_to_ring() after publishing the webhook event, so every state-change event is automatically captured in the ring buffer. No call sites needed updating. • Added DataKey::EventLog(u32) variant to the DataKey enum in lib.rs. • Added two public entry points on SkillSphereContract: - get_event_log(from_index, limit) → Vec<EventLogEntry> - event_log_head() → u32 • Temporary storage is used for ring-buffer slots to keep rent costs low, as specified in the acceptance criteria. Files changed: contracts/src/events.rs — EventLogEntry struct, ring-buffer logic, publish_event updated to write to buffer. contracts/src/lib.rs — DataKey::EventLog, get_event_log() and event_log_head() functions.
📝 WalkthroughWalkthroughThis PR implements four linked features for SkillSphere sessions: recording-consent state tracking, GDPR-compliant data deletion, a dead-letter queue for failed token transfers, and an on-chain event replay ring buffer. The changes extend the Session struct, modify settlement logic to safely handle transfer failures, and add new public APIs for consent management, data deletion, transfer recovery, and event log queries. ChangesSession Management Enhancements
Sequence Diagram(s)sequenceDiagram
participant Caller
participant publish_event
participant RingBuffer
participant Webhook
Caller->>publish_event: event_type, session_id, payload
publish_event->>Webhook: publish envelope with timestamp
publish_event->>RingBuffer: append EventLogEntry (index, event_type, session_id, timestamp)
RingBuffer->>RingBuffer: write to modulo slot, advance head
sequenceDiagram
participant Recipient
participant claim_failed_transfer
participant DLQ Storage
participant TokenTransfer
Recipient->>claim_failed_transfer: recipient, token (authorized)
claim_failed_transfer->>DLQ Storage: check amount exists and not expired
claim_failed_transfer->>DLQ Storage: clear amount and timestamp
claim_failed_transfer->>TokenTransfer: execute transfer
TokenTransfer->>Recipient: token sent
sequenceDiagram
participant Settlement
participant try_transfer
participant DLQ
participant ErrorHandler
Settlement->>try_transfer: expert_payout, token
alt Transfer succeeds
try_transfer->>Settlement: Ok(amount)
else Transfer fails
try_transfer->>DLQ: enqueue_failed_transfer(expert, amount)
try_transfer->>ErrorHandler: set expert_payout to 0
try_transfer->>Settlement: Ok(0)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🤖 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/src/events.rs`:
- Around line 43-64: In append_to_ring, after writing the slot entry
(DataKey::EventLog(slot)) and the replay head (head_key()) to
env.storage().temporary(), check each key's remaining TTL and call the temporary
storage extend_ttl API to refresh them when below a threshold; specifically, for
both the slot key and head_key() in append_to_ring, call
env.storage().temporary().extend_ttl(&key, threshold, extend_to) (or equivalent)
only when TTL is low, using extend_to sized to cover the expected replay window
so the event log and head do not expire prematurely.
In `@contracts/src/identity.rs`:
- Around line 140-145: The emit_deletion_event function is publishing directly
to env.events(), bypassing the shared publisher and preventing the
DataDeletionRequested event from reaching the replay ring and webhook envelope;
update emit_deletion_event to call the centralized publish_event helper (from
contracts::events, e.g. publish_event or the shared event publisher function)
and pass the DataDeletionRequested event payload (address and timestamp as the
standardized event struct/enum) so the event goes through the replay ring and
standardized webhook envelope rather than calling env.events().publish directly.
In `@contracts/src/lib.rs`:
- Around line 3406-3424: The deletion pass only visits persistent Session
entries and skips archived sessions and the session-normalization logic; update
the loop to use the existing accessor that normalizes Reserved to Active by
calling get_session_or_error (instead of direct persistent().get::<DataKey,
Session>(&DataKey::Session(id))) so the Active/Paused guard behaves correctly,
and additionally tombstone archived sessions by also iterating persistent
archived entries (DataKey::ArchivedSession) or using get_archived_session to
load and overwrite archived Session records' metadata_cid and
encrypted_notes_hash with the tombstone; ensure you still set the storage via
persistent().set for both DataKey::Session(id) and DataKey::ArchivedSession(id)
and reference SessionStatus::Active, SessionStatus::Paused and
Error::InvalidSessionState unchanged.
- Around line 238-239: The DLQ key is too coarse: change the FailedTransfer enum
variant and all storage/lookup sites to include the token so failures are keyed
by (recipient, token) instead of only Address. Update the FailedTransfer variant
signature (replace FailedTransfer(Address) with FailedTransfer(Address, TokenId)
or a small struct), update any storage map or set used to track failed transfers
(and their key types), and modify claim_failed_transfer plus the call sites
noted (around the existing claim_failed_transfer implementation and the code at
the other referenced locations) to pass and match the token parameter when
inserting, querying, or removing failed transfers; ensure related
serialization/serde, pattern matches, and tests are updated accordingly.
- Around line 362-369: The RecordingConsent enum currently allows a single
caller to transition None->SeekerApproved->BothApproved and thus bypass
two-party consent; replace SeekerApproved with two distinct "first-approver"
states and add an explicit Expert-only state so the machine encodes who approved
first (e.g., rename SeekerApproved -> SeekerOnly and add ExpertOnly, keep
BothApproved for final state), and update every approval-handling function that
checks or sets RecordingConsent so transitions require the other party to call
before moving to BothApproved (i.e., None -> SeekerOnly or ExpertOnly depending
on caller, and SeekerOnly -> BothApproved only if caller is expert, ExpertOnly
-> BothApproved only if caller is seeker); change any matching/serialization
code that referenced SeekerApproved/None to use the new variants.
- Around line 3400-3405: The scrub loop is using the wrong counter and a 0-based
range: replace the use of DataKey::SessionCounter and the range "for id in
0..counter" with the same storage key the session creation uses
(symbol_short!("next_sid")) and iterate 1-based (e.g., get next_sid as a u64 and
loop for id in 1..=next_sid) so the scrub actually visits every real session id
referenced by the session creation logic.
- Around line 396-397: The migration for Session is incomplete: update
contracts/src/migrations.rs so run() handles v1->v2 correctly (and include a
default/no-op for later versions) and change migrate_v1_to_v2 to reconstruct
Session including the new non-optional recording_consent and the expires_at
field; specifically, in migrate_v1_to_v2 populate recording_consent using a safe
default (e.g., RecordingConsent::default() or RecordingConsent::OptOut) and set
expires_at to the original value if present or None/appropriate default,
ensuring DataKey::Session records (and archived sessions read by
get_archived_session / storage::write_archive) are migrated to the new shape to
prevent deserialization panics in get_session_or_error, archive_session,
batch_archive_sessions, etc.
In `@contracts/src/recovery.rs`:
- Around line 32-38: The DLQ timestamp is only set when prev == 0, causing new
entries appended to old buckets to inherit stale expiry; change the logic in the
enqueue path so dlq_ts_key(recipient) is updated to env.ledger().timestamp()
whenever you append to the bucket (i.e., on any enqueue, not just when prev ==
0). Locate the code that calls env.storage().persistent().set(&ts_key,
&env.ledger().timestamp()) and remove or broaden the prev == 0 guard so the
timestamp is refreshed on append.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 365a8dc2-bc46-496f-b323-adaf98e5666e
📒 Files selected for processing (4)
contracts/src/events.rscontracts/src/identity.rscontracts/src/lib.rscontracts/src/recovery.rs
Comment on lines
+43
to
+64
| fn append_to_ring(env: &Env, event_type: Symbol, session_id: u64) { | ||
| let head: u32 = env | ||
| .storage() | ||
| .temporary() | ||
| .get(&head_key()) | ||
| .unwrap_or(0u32); | ||
|
|
||
| let slot = head % EVENT_LOG_CAPACITY; | ||
| let entry = EventLogEntry { | ||
| index: head, | ||
| event_type, | ||
| session_id, | ||
| timestamp: env.ledger().timestamp(), | ||
| }; | ||
|
|
||
| let key = crate::DataKey::EventLog(slot); | ||
| env.storage().temporary().set(&key, &entry); | ||
|
|
||
| use soroban_sdk::{symbol_short, Env, IntoVal, Symbol, Val}; | ||
| env.storage() | ||
| .temporary() | ||
| .set(&head_key(), &head.saturating_add(1)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For soroban-sdk 21.0.0, when using env.storage().temporary().set(...), do those entries require explicit TTL extension to remain queryable later, and what is the supported API for extending TTL on temporary-storage keys?
💡 Result:
In soroban-sdk 21.0.0, entries created via env.storage.temporary.set(...) do have a limited lifetime (Time-To-Live, or TTL) and will be permanently deleted once that TTL expires [1]. To ensure such entries remain queryable beyond their initial expiration, you must proactively extend their TTL [2][3]. The supported API for extending the TTL of a temporary storage key is the extend_ttl method, accessed through the temporary storage interface [4][5]: env.storage.temporary.extend_ttl(&key, threshold, extend_to); This method works as follows: 1. &key: The identifier for the data entry you wish to extend [4]. 2. threshold: A safeguard parameter; the extension only triggers if the current TTL is lower than this value (measured in ledgers) [4][3]. 3. extend_ttl: The new desired TTL value (measured in ledgers) if the extension is triggered [4][2]. This design allows developers to distribute the cost of TTL management by checking and extending entries during normal contract interactions, rather than performing unnecessary extensions on every call [3]. While temporary entries are ephemeral and generally intended for short-lived data, explicit management is required if you need to maintain access to them over a longer duration [3][1].
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Storage.html
- 2: https://developers.stellar.org/docs/build/guides/archival/test-ttl-extension
- 3: https://www.certik.com/blog/soroban-contract-state-management
- 4: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Temporary.html
- 5: https://www.sorobanexamples.xyz/temporary-storage
Extend TTL for replay buffer keys (temporary storage).
append_to_ring writes both the ring-slot entry (DataKey::EventLog(slot)) and the replay head (head_key()) to env.storage().temporary() but never refreshes their TTL. Temporary entries in Soroban are deleted when their TTL expires, so get_event_log() / event_log_head() can silently reset and break the replay contract.
Add TTL extension for both keys (slot entry and head) using the temporary-storage API, e.g. env.storage().temporary().extend_ttl(&key, threshold, extend_to—only extend when TTL is low, and size extend_to to cover the expected replay window).
🤖 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/src/events.rs` around lines 43 - 64, In append_to_ring, after
writing the slot entry (DataKey::EventLog(slot)) and the replay head
(head_key()) to env.storage().temporary(), check each key's remaining TTL and
call the temporary storage extend_ttl API to refresh them when below a
threshold; specifically, for both the slot key and head_key() in append_to_ring,
call env.storage().temporary().extend_ttl(&key, threshold, extend_to) (or
equivalent) only when TTL is low, using extend_to sized to cover the expected
replay window so the event log and head do not expire prematurely.
Comment on lines
+140
to
+145
| /// Emit the `DataDeletionRequested` event. | ||
| pub fn emit_deletion_event(env: &Env, address: &Address) { | ||
| env.events().publish( | ||
| (symbol_short!("dataDel"),), | ||
| (address.clone(), env.ledger().timestamp()), | ||
| ); |
There was a problem hiding this comment.
Route deletion events through the shared event publisher.
Publishing directly here bypasses contracts/src/events.rs::publish_event, so DataDeletionRequested never reaches the replay ring and also skips the standardized webhook envelope. That leaves this new state-change event invisible to get_event_log().
🤖 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/src/identity.rs` around lines 140 - 145, The emit_deletion_event
function is publishing directly to env.events(), bypassing the shared publisher
and preventing the DataDeletionRequested event from reaching the replay ring and
webhook envelope; update emit_deletion_event to call the centralized
publish_event helper (from contracts::events, e.g. publish_event or the shared
event publisher function) and pass the DataDeletionRequested event payload
(address and timestamp as the standardized event struct/enum) so the event goes
through the replay ring and standardized webhook envelope rather than calling
env.events().publish directly.
Comment on lines
+238
to
+239
| // Issue #273 - Dead Letter Queue | ||
| FailedTransfer(Address), |
There was a problem hiding this comment.
DLQ state is keyed too coarsely for a multi-token contract.
Failed payouts are stored by recipient only, but settlement failures happen per (recipient, token) pair and claim_failed_transfer already asks for a token. If the same expert has failed payouts in two assets, those balances collide and the later claim can pay the wrong token or amount.
Key failed transfers by both recipient and token
- FailedTransfer(Address),
+ FailedTransfer(Address, Address),
@@
- recovery::enqueue_failed_transfer(env, &expert, expert_payout);
+ recovery::enqueue_failed_transfer(env, &expert, &token, expert_payout);
@@
- pub fn get_failed_transfer_amount(env: Env, recipient: Address) -> i128 {
- recovery::pending_failed_transfer(&env, &recipient)
+ pub fn get_failed_transfer_amount(env: Env, recipient: Address, token: Address) -> i128 {
+ recovery::pending_failed_transfer(&env, &recipient, &token)
}Also applies to: 3439-3449, 4297-4300
🤖 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/src/lib.rs` around lines 238 - 239, The DLQ key is too coarse:
change the FailedTransfer enum variant and all storage/lookup sites to include
the token so failures are keyed by (recipient, token) instead of only Address.
Update the FailedTransfer variant signature (replace FailedTransfer(Address)
with FailedTransfer(Address, TokenId) or a small struct), update any storage map
or set used to track failed transfers (and their key types), and modify
claim_failed_transfer plus the call sites noted (around the existing
claim_failed_transfer implementation and the code at the other referenced
locations) to pass and match the token parameter when inserting, querying, or
removing failed transfers; ensure related serialization/serde, pattern matches,
and tests are updated accordingly.
Comment on lines
+362
to
+369
| /// Recording consent states for session recording (Issue #271). | ||
| #[contracttype] | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub enum RecordingConsent { | ||
| None, | ||
| SeekerApproved, | ||
| BothApproved, | ||
| } |
There was a problem hiding this comment.
A single participant can unilaterally reach BothApproved.
SeekerApproved does not encode who approved first, and the None branch stores the same state for seeker-first and expert-first. That means the same caller can invoke this twice and flip the session to BothApproved, which breaks the two-party consent guarantee.
Suggested shape for the state machine
pub enum RecordingConsent {
None,
SeekerApproved,
+ ExpertApproved,
BothApproved,
}
@@
- let new_consent = match session.recording_consent {
- RecordingConsent::None => {
- if caller == session.seeker {
- RecordingConsent::SeekerApproved
- } else {
- // Expert consented first — treat as SeekerApproved placeholder
- // (expert consent is implicit once seeker approves).
- RecordingConsent::SeekerApproved
- }
- }
- RecordingConsent::SeekerApproved => RecordingConsent::BothApproved,
- RecordingConsent::BothApproved => RecordingConsent::BothApproved,
+ let new_consent = match (&session.recording_consent, caller == session.seeker, caller == session.expert) {
+ (RecordingConsent::None, true, false) => RecordingConsent::SeekerApproved,
+ (RecordingConsent::None, false, true) => RecordingConsent::ExpertApproved,
+ (RecordingConsent::SeekerApproved, false, true) => RecordingConsent::BothApproved,
+ (RecordingConsent::ExpertApproved, true, false) => RecordingConsent::BothApproved,
+ (state, _, _) => state.clone(),
};Also applies to: 3317-3344
🤖 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/src/lib.rs` around lines 362 - 369, The RecordingConsent enum
currently allows a single caller to transition
None->SeekerApproved->BothApproved and thus bypass two-party consent; replace
SeekerApproved with two distinct "first-approver" states and add an explicit
Expert-only state so the machine encodes who approved first (e.g., rename
SeekerApproved -> SeekerOnly and add ExpertOnly, keep BothApproved for final
state), and update every approval-handling function that checks or sets
RecordingConsent so transitions require the other party to call before moving to
BothApproved (i.e., None -> SeekerOnly or ExpertOnly depending on caller, and
SeekerOnly -> BothApproved only if caller is expert, ExpertOnly -> BothApproved
only if caller is seeker); change any matching/serialization code that
referenced SeekerApproved/None to use the new variants.
Comment on lines
+396
to
+397
| /// Recording consent state (Issue #271). | ||
| pub recording_consent: RecordingConsent, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema touch points =="
rg -n -C2 'recording_consent|pub struct Session|DataKey::Session\(|write_archive|read_archive|migrations::run' contracts/src
for f in contracts/src/migrations.rs contracts/src/storage.rs; do
if [ -f "$f" ]; then
printf '\n== %s ==\n' "$f"
sed -n '1,260p' "$f"
fi
doneRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 17163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Session/recording_consent types =="
rg -n "enum RecordingConsent|contracttype.*RecordingConsent|struct Session\b|recording_consent" contracts/src/lib.rs contracts/src/*.rs
echo
echo "== migration dispatch/version =="
rg -n "fn migrate\b|migrations::run|get_contract_version|contract_version|current|new_version" contracts/src/lib.rs contracts/src/migrations.rs
echo
echo "== where Session is read from persistent/temporary =="
rg -n "\.get::<DataKey, Session>\(|get_archived_session\(|read_archive\(|temporary\(\)\.get\(&DataKey::Session" contracts/src/lib.rs contracts/src/storage.rs contracts/src/*.rs
echo
echo "== archived session write/remove =="
rg -n "write_archive\(|remove\(&DataKey::Session" contracts/src/lib.rs contracts/src/*.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 13827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== recording_consent in migrations.rs =="
rg -n "recording_consent|migrate_v" contracts/src/migrations.rs
echo
echo "== Session struct fields (lib.rs around definition) =="
sed -n '340,460p' contracts/src/lib.rs
echo
echo "== migrate() + contract versioning (lib.rs) =="
sed -n '1760,2060p' contracts/src/lib.rs
echo
echo "== DataKey::Session definition (lib.rs) =="
rg -n "enum DataKey|DataKey::Session" contracts/src/lib.rs
sed -n '1,220p' contracts/src/lib.rs
echo
echo "== migrate_v1_to_v2 struct literal (lib.rs snippet already) -- expand around construction end =="
rg -n "fn migrate_v1_to_v2" -n contracts/src/migrations.rs
# print around that function more
sed -n '60,150p' contracts/src/migrations.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 26700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== recording_consent in migrations.rs =="
rg -n "recording_consent|migrate_v" contracts/src/migrations.rs
echo
echo "== Session struct fields (lib.rs around definition) =="
sed -n '340,460p' contracts/src/lib.rs
echo
echo "== migrate() + contract versioning (lib.rs) =="
sed -n '1800,2060p' contracts/src/lib.rs
echo
echo "== DataKey::Session definition (lib.rs top) =="
rg -n "enum DataKey|DataKey::Session" contracts/src/lib.rs
sed -n '1,240p' contracts/src/lib.rs
echo
echo "== migrate_v1_to_v2 struct literal (fuller window) =="
sed -n '80,140p' contracts/src/migrations.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 24615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== recording_consent in migrations.rs =="
rg -n "recording_consent|migrate_v" contracts/src/migrations.rs
echo
echo "== Session struct fields (lib.rs around definition) =="
sed -n '340,460p' contracts/src/lib.rs
echo
echo "== migrate() + contract versioning (lib.rs) =="
sed -n '1800,2060p' contracts/src/lib.rs
echo
echo "== DataKey::Session definition (lib.rs top) =="
rg -n "enum DataKey|DataKey::Session" contracts/src/lib.rs
sed -n '1,240p' contracts/src/lib.rs
echo
echo "== migrate_v1_to_v2 struct literal (fuller window) =="
sed -n '80,140p' contracts/src/migrations.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 24615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrations.rs (migrate dispatch + migrate_v1_to_v2 with line numbers) =="
# show migrate/run + migrate_v1_to_v2 body fully
nl -ba contracts/src/migrations.rs | sed -n '1,220p'
echo
echo "== find any migrate_v2_to_* (none expected) =="
rg -n "migrate_v2_to|migrate_v3_to|migrate_v[0-9]+_to_v[0-9]+" contracts/src/migrations.rs
echo
echo "== where get_session_or_error reads persistent Session =="
rg -n "get_session_or_error|get_session_or_error\(" contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrations.rs full (cat -n) =="
cat -n contracts/src/migrations.rs | sed -n '1,220p'
echo
echo "== find any migrate_v2_to_* (none expected) =="
rg -n "migrate_v[0-9]+_to_v[0-9]+" contracts/src/migrations.rs
echo
echo "== get_session_or_error implementation (lib.rs) =="
rg -n "fn get_session_or_error" -n contracts/src/lib.rs
start=$(rg -n "fn get_session_or_error" contracts/src/lib.rs | head -n1 | cut -d: -f1)
# print around it
sed -n "$((start-30)),$((start+220))p" contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 17884
Fix the Session schema migration for non-optional recording_consent
contracts/src/migrations.rs only supports v1 -> v2 (run() has no later migration arms), and migrate_v1_to_v2 reconstructs Session without the new non-optional recording_consent field (and doesn’t set expires_at either). This means existing DataKey::Session(..) records (and archived sessions stored via storage::write_archive and later read as Session in get_archived_session) won’t be migrated to the new shape, so the first persistent read (get_session_or_error, archive_session, batch_archive_sessions, etc.) can fail/deserialization can panic.
🤖 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/src/lib.rs` around lines 396 - 397, The migration for Session is
incomplete: update contracts/src/migrations.rs so run() handles v1->v2 correctly
(and include a default/no-op for later versions) and change migrate_v1_to_v2 to
reconstruct Session including the new non-optional recording_consent and the
expires_at field; specifically, in migrate_v1_to_v2 populate recording_consent
using a safe default (e.g., RecordingConsent::default() or
RecordingConsent::OptOut) and set expires_at to the original value if present or
None/appropriate default, ensuring DataKey::Session records (and archived
sessions read by get_archived_session / storage::write_archive) are migrated to
the new shape to prevent deserialization panics in get_session_or_error,
archive_session, batch_archive_sessions, etc.
Comment on lines
+3400
to
+3405
| let counter: u64 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DataKey::SessionCounter) | ||
| .unwrap_or(0u64); | ||
| for id in 0..counter { |
There was a problem hiding this comment.
This deletion loop never reaches real session ids.
Session creation advances symbol_short!("next_sid"), not DataKey::SessionCounter, and session ids are 1-based. In the current form the scrub pass skips every real session on a deployed contract.
Use the same counter that session creation uses
- let counter: u64 = env
- .storage()
- .instance()
- .get(&DataKey::SessionCounter)
- .unwrap_or(0u64);
- for id in 0..counter {
+ let next_session_id: u64 = env
+ .storage()
+ .instance()
+ .get(&symbol_short!("next_sid"))
+ .unwrap_or(1u64);
+ for id in 1..next_session_id {📝 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.
Suggested change
| let counter: u64 = env | |
| .storage() | |
| .instance() | |
| .get(&DataKey::SessionCounter) | |
| .unwrap_or(0u64); | |
| for id in 0..counter { | |
| let next_session_id: u64 = env | |
| .storage() | |
| .instance() | |
| .get(&symbol_short!("next_sid")) | |
| .unwrap_or(1u64); | |
| for id in 1..next_session_id { |
🤖 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/src/lib.rs` around lines 3400 - 3405, The scrub loop is using the
wrong counter and a 0-based range: replace the use of DataKey::SessionCounter
and the range "for id in 0..counter" with the same storage key the session
creation uses (symbol_short!("next_sid")) and iterate 1-based (e.g., get
next_sid as a u64 and loop for id in 1..=next_sid) so the scrub actually visits
every real session id referenced by the session creation logic.
Comment on lines
+3406
to
+3424
| if let Some(mut session) = env | ||
| .storage() | ||
| .persistent() | ||
| .get::<DataKey, Session>(&DataKey::Session(id)) | ||
| { | ||
| if session.seeker != address && session.expert != address { | ||
| continue; | ||
| } | ||
| // Block deletion from active sessions. | ||
| if matches!(session.status, SessionStatus::Active | SessionStatus::Paused) { | ||
| return Err(Error::InvalidSessionState); | ||
| } | ||
| session.metadata_cid = tombstone.clone(); | ||
| session.encrypted_notes_hash = Some(tombstone.clone()); | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::Session(id), &session); | ||
| } | ||
| } |
There was a problem hiding this comment.
The deletion pass misses data that is still reachable.
This code only walks persistent Session entries, so archived sessions remain readable via get_archived_session. It also bypasses get_session_or_error, which means an overdue Reserved session is not normalized to Active before the Active/Paused guard runs.
🤖 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/src/lib.rs` around lines 3406 - 3424, The deletion pass only visits
persistent Session entries and skips archived sessions and the
session-normalization logic; update the loop to use the existing accessor that
normalizes Reserved to Active by calling get_session_or_error (instead of direct
persistent().get::<DataKey, Session>(&DataKey::Session(id))) so the
Active/Paused guard behaves correctly, and additionally tombstone archived
sessions by also iterating persistent archived entries
(DataKey::ArchivedSession) or using get_archived_session to load and overwrite
archived Session records' metadata_cid and encrypted_notes_hash with the
tombstone; ensure you still set the storage via persistent().set for both
DataKey::Session(id) and DataKey::ArchivedSession(id) and reference
SessionStatus::Active, SessionStatus::Paused and Error::InvalidSessionState
unchanged.
Comment on lines
+32
to
+38
| // Record timestamp for expiry (only set on first enqueue). | ||
| let ts_key = dlq_ts_key(recipient); | ||
| if prev == 0 { | ||
| env.storage() | ||
| .persistent() | ||
| .set(&ts_key, &env.ledger().timestamp()); | ||
| } |
There was a problem hiding this comment.
Refresh the DLQ expiry when appending to an existing bucket.
Right now a transfer added on day 179 inherits the bucket's original timestamp and can be expired/cleared almost immediately on the next claim. That causes newly queued funds to age out with old ones.
Suggested fix
- // Record timestamp for expiry (only set on first enqueue).
+ // Refresh expiry from the latest failed transfer attempt.
let ts_key = dlq_ts_key(recipient);
- if prev == 0 {
- env.storage()
- .persistent()
- .set(&ts_key, &env.ledger().timestamp());
- }
+ env.storage()
+ .persistent()
+ .set(&ts_key, &env.ledger().timestamp());📝 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.
Suggested change
| // Record timestamp for expiry (only set on first enqueue). | |
| let ts_key = dlq_ts_key(recipient); | |
| if prev == 0 { | |
| env.storage() | |
| .persistent() | |
| .set(&ts_key, &env.ledger().timestamp()); | |
| } | |
| // Refresh expiry from the latest failed transfer attempt. | |
| let ts_key = dlq_ts_key(recipient); | |
| env.storage() | |
| .persistent() | |
| .set(&ts_key, &env.ledger().timestamp()); |
🤖 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/src/recovery.rs` around lines 32 - 38, The DLQ timestamp is only
set when prev == 0, causing new entries appended to old buckets to inherit stale
expiry; change the logic in the enqueue path so dlq_ts_key(recipient) is updated
to env.ledger().timestamp() whenever you append to the bucket (i.e., on any
enqueue, not just when prev == 0). Locate the code that calls
env.storage().persistent().set(&ts_key, &env.ledger().timestamp()) and remove or
broaden the prev == 0 guard so the timestamp is refreshed on append.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #271 — Session Recording Consent Flag
Closes #272 — Data Deletion Request (Right to Be Forgotten)
Closes #273 — Dead Letter Queue for Failed Token Transfers
Closes #274 — Contract Event Replay Index
═══════════════════════════════════════════════════════════════ ISSUE #271 — Session Recording Consent Flag
═══════════════════════════════════════════════════════════════ Problem:
SkillSphere may offer session recording. Without explicit on-chain
consent from both parties, recording could create legal liability.
What was done:
• Added a new RecordingConsent enum to lib.rs with three states:
- None (default, no consent given)
- SeekerApproved (first party has consented)
- BothApproved (both seeker and expert have consented)
• Added recording_consent: RecordingConsent field to the Session
struct. All four Session instantiation sites (start_session via
create_active_session, reserve_session, start_session_with_voucher,
and the DEX-swap session path) are initialised to RecordingConsent::None.
• Added grant_recording_consent(caller, session_id) public function
on SkillSphereContract. Either the seeker or expert may call it.
The state machine transitions None → SeekerApproved → BothApproved.
Recording is only considered approved when BothApproved is reached.
• Consent state is persisted in the Session struct and therefore
queryable via the existing get_session() function.
• Emits a RecordingConsentUpdated event (symbol: recCons) via the
standardised publish_event webhook envelope, carrying
(caller, new_consent_state).
• Rejects calls on sessions that are not Active or Paused
(InvalidSessionState) and rejects non-participants (Unauthorized).
Files changed:
contracts/src/lib.rs — RecordingConsent enum, Session field,
grant_recording_consent() function.
═══════════════════════════════════════════════════════════════ ISSUE #272 — Data Deletion Request (Right to Be Forgotten) ═══════════════════════════════════════════════════════════════ Problem:
GDPR-compliant platforms must allow users to delete personal data.
On-chain data is immutable, but metadata CIDs can be replaced with
a tombstone so the content is no longer reachable.
What was done:
• Added a data_deletion sub-module to contracts/src/identity.rs.
It exposes two helpers used by the main contract:
- tombstone(env) → String::from_str(env, "DELETED")
- emit_deletion_event(env, address) — publishes a raw DataDeletionRequested event (topic: dataDel) carrying (address, timestamp). • Added request_data_deletion(caller, address) public function on SkillSphereContract in lib.rs: - Requires auth from caller. - Authorisation check: caller must be the address owner OR hold the SuperAdmin role (via roles::has_role). - Tombstones ExpertProfile.metadata_cid for the address if a profile exists in persistent storage. - Iterates all sessions (0..SessionCounter) and tombstones metadata_cid and encrypted_notes_hash for any session where seeker == address || expert == address. - Returns InvalidSessionState immediately if any matching session is still Active or Paused (cannot delete from live sessions). - Emits DataDeletionRequested event via identity::data_deletion:: emit_deletion_event. • Declared identity as pub mod identity in lib.rs so the data_deletion sub-module is accessible from the contract impl.
Files changed:
contracts/src/identity.rs — data_deletion pub mod added.
contracts/src/lib.rs — pub mod identity declaration,
request_data_deletion() function.
═══════════════════════════════════════════════════════════════ ISSUE #273 — Dead Letter Queue for Failed Token Transfers ═══════════════════════════════════════════════════════════════ Problem:
If a token transfer in settle_session fails (e.g. the recipient's
trustline is missing on Stellar), the session becomes permanently
stuck and the expert's funds are unrecoverable.
What was done:
• Created contracts/src/recovery.rs — a new module implementing the
Dead Letter Queue (DLQ):
- enqueue_failed_transfer(env, recipient, amount): stores the failed amount under DataKey::FailedTransfer(recipient) in persistent storage. Amounts are summed if multiple failures occur for the same recipient. Records the enqueue timestamp under a ("dlq_ts", recipient) key for expiry tracking. Emits a TransferQueued event (symbol: dlqQueue).
- claim_failed_transfer(env, recipient, token): requires auth from recipient. Checks the 180-day expiry window. Clears the DLQ entry (checks-effects-interactions pattern) then retries the token transfer. Emits a TransferClaimed event (dlqClaim). Returns InsufficientBalance if nothing is queued, SessionExpired if the entry has expired. - pending_failed_transfer(env, recipient): read-only query. - DLQ_EXPIRY_SECS = 180 * 24 * 60 * 60 (180 days). • Added DataKey::FailedTransfer(Address) variant to the DataKey enum in lib.rs. • Modified internal_settle() in lib.rs: the expert payout transfer now uses token_client.try_transfer() instead of transfer(). On failure, recovery::enqueue_failed_transfer() is called and expert_payout is set to 0 so settlement still completes cleanly. • Added two public entry points on SkillSphereContract: - claim_failed_transfer(recipient, token) → Result<i128, Error> - get_failed_transfer_amount(recipient) → i128 • Declared recovery as pub mod recovery in lib.rs.
Files changed:
contracts/src/recovery.rs — new file, full DLQ implementation.
contracts/src/lib.rs — DataKey::FailedTransfer, pub mod recovery,
try_transfer in internal_settle,
claim_failed_transfer() and
get_failed_transfer_amount() functions.
═══════════════════════════════════════════════════════════════ ISSUE #274 — Contract Event Replay Index (Ring Buffer) ═══════════════════════════════════════════════════════════════ Problem:
If the off-chain indexer misses events (downtime, chain reorg), it
needs a way to re-fetch historical events without re-scanning the
entire chain from genesis.
What was done:
• Extended contracts/src/events.rs with a ring-buffer implementation:
- EVENT_LOG_CAPACITY = 1000 (max entries retained).
- EventLogEntry contracttype struct: { index, event_type, session_id, timestamp }. - append_to_ring(env, event_type, session_id): writes an entry to DataKey::EventLog(slot) in Temporary storage (low rent cost) and advances the head pointer stored under symbol "evtHead". Slot = head % 1000, so the buffer wraps automatically. - get_event_log(env, from_index, limit): returns up to limit entries starting from from_index. Validates that each slot's stored index matches the requested index to detect overwritten entries. Returns a Vec. - event_log_head(env): returns the current head pointer. • publish_event() now calls append_to_ring() after publishing the webhook event, so every state-change event is automatically captured in the ring buffer. No call sites needed updating. • Added DataKey::EventLog(u32) variant to the DataKey enum in lib.rs. • Added two public entry points on SkillSphereContract: - get_event_log(from_index, limit) → Vec - event_log_head() → u32 • Temporary storage is used for ring-buffer slots to keep rent costs low, as specified in the acceptance criteria.
Files changed:
contracts/src/events.rs — EventLogEntry struct, ring-buffer logic,
publish_event updated to write to buffer.
contracts/src/lib.rs — DataKey::EventLog, get_event_log() and
event_log_head() functions.
Summary by CodeRabbit
Release Notes