fix: add missing extend_ttl for persistent storage TTL management - #162
Conversation
|
@Myart352 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! 🚀 |
📝 WalkthroughWalkthroughPersistent storage accessors and writes now refresh TTLs across the event, factory, payments, and ticket contracts. Named ledger-based TTL constants replace inline durations. Existing missing-value errors, defaults, and optional-value behavior remain unchanged. ChangesPersistent storage TTL management
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
🤖 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/ticket/src/storage.rs`:
- Around line 29-31: Replace the seconds-based TTL values used by
storage::extend_ttl with ledger-count constants, and route every ticket TTL
extension through those constants. Update contracts/ticket/src/storage.rs at
lines 29-31 and contracts/ticket/src/lib.rs at lines 42-46, including the inline
literals in lib.rs, while preserving the existing threshold and bump semantics.
🪄 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 Plus
Run ID: 8a60147c-6a87-4fe9-bdd6-94be9fafa73f
📒 Files selected for processing (3)
contracts/event/src/storage.rscontracts/ticket/src/lib.rscontracts/ticket/src/storage.rs
b288e25 to
e008796
Compare
|
@Myart352 pls ensure to run |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/ticket/src/storage.rs (1)
93-105: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRefresh count entries before relying on stored index bounds.
get_event_tickets_countandget_owner_tickets_countread the count asOption<u64>, while non-zero entries are extended withstorage().persistent().extend(&key, env.storage().get_ttl(&DataKey::TicketCount)...). If the count entry is deleted or unavailable, the index array bound no longer reflects existing data.remove_owner_ticketalso deletes the membership before this read and can return with a zero count, potentially leaving stale index entries that later adds can overwrite. Read the count asOption<u64>, extend only existing counts, and handle missing counts with a fallback or corrective rebuild before using the indexes. Also applies to: 181-189, 193-205, 211-223🤖 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/ticket/src/storage.rs` around lines 93 - 105, Update get_event_tickets_count, get_owner_tickets_count, and the related removal paths to read count entries as Option<u64>, refresh TTL only when a count exists, and recover missing counts by rebuilding or otherwise correcting the index metadata before using bounds. In remove_owner_ticket, avoid returning solely because the count is missing or zero after deleting membership; ensure stale index entries are removed and subsequent additions cannot overwrite them.Source: MCP tools
🧹 Nitpick comments (1)
contracts/ticket/src/storage.rs (1)
193-223: 🚀 Performance & Scalability | 🔵 TrivialBound per-entry TTL refreshes in ticket-list queries.
Each returned index entry can trigger an
extend_ttlcall. The loop has no page or batch limit. Large owner or event lists can therefore consume significant ledger-write resources and refundable fees. Soroban transactions can fail when TTL extensions exceed resource or fee limits. (developers.stellar.org)Add pagination or bounded batches, or validate the maximum list size with Soroban simulation.
🤖 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/ticket/src/storage.rs` around lines 193 - 223, Bound the number of entries processed and TTL-refreshed in get_tickets_by_owner and get_tickets_by_event by adding pagination or a fixed maximum batch size. Ensure each query returns only the bounded page and does not call extend_ttl for unbounded lists; use the existing count/index storage behavior for offsets and preserve returned ticket ordering.Source: MCP 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.
Inline comments:
In `@contracts/event/src/storage.rs`:
- Around line 52-54: Replace the time-based TTL values passed to
Persistent::extend_ttl in contracts/event/src/storage.rs lines 52-54 and
234-236, contracts/factory/src/storage.rs lines 35-37, and
contracts/payments/src/storage.rs lines 120-122 with shared, tested ledger-based
TTL constants; define or reuse those constants consistently across all affected
storage accessors while preserving the existing threshold and bump behavior.
- Around line 275-283: Update get_event_privacy so it only calls
persistent().extend_ttl for the EventPrivacy key when persistent().get(&key)
returns Some; preserve PrivacyLevel::Standard as the fallback for missing
entries and return the retrieved or default privacy level.
In `@contracts/ticket/src/lib.rs`:
- Around line 377-384: Update the NextTicketId handling in mint_ticket to read
the persistent value as Option<u64>, call extend_ttl only when the key exists,
and use 1 as the fallback when it is absent. Preserve the existing counter
behavior for present values while preventing extend_ttl from running on a
missing key.
In `@contracts/ticket/src/storage.rs`:
- Around line 6-7: Update the TTL_THRESHOLD and TTL_BUMP constants in the
storage configuration to use ledger counts rather than seconds, after verifying
the repository’s target network and its maximum supported TTL. Ensure both
values remain within that network maximum, with TTL_BUMP not exceeding the
documented 3,110,400-ledger limit.
---
Outside diff comments:
In `@contracts/ticket/src/storage.rs`:
- Around line 93-105: Update get_event_tickets_count, get_owner_tickets_count,
and the related removal paths to read count entries as Option<u64>, refresh TTL
only when a count exists, and recover missing counts by rebuilding or otherwise
correcting the index metadata before using bounds. In remove_owner_ticket, avoid
returning solely because the count is missing or zero after deleting membership;
ensure stale index entries are removed and subsequent additions cannot overwrite
them.
---
Nitpick comments:
In `@contracts/ticket/src/storage.rs`:
- Around line 193-223: Bound the number of entries processed and TTL-refreshed
in get_tickets_by_owner and get_tickets_by_event by adding pagination or a fixed
maximum batch size. Ensure each query returns only the bounded page and does not
call extend_ttl for unbounded lists; use the existing count/index storage
behavior for offsets and preserve returned ticket ordering.
🪄 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 Plus
Run ID: aff8ef78-52a0-45ca-bace-c5bb79e2a4c3
📒 Files selected for processing (5)
contracts/event/src/storage.rscontracts/factory/src/storage.rscontracts/payments/src/storage.rscontracts/ticket/src/lib.rscontracts/ticket/src/storage.rs
- Replace seconds-style TTL constants with ledger counts (518,400 threshold / 1,036,800 bump) that stay within the 3,110,400-ledger network maximum; previously TTL_BUMP (5,184,000) exceeded the max and would panic on mainnet. - Route all inline extend_ttl literals through the shared constants in event, factory, payments, and ticket storage. - get_event_privacy / read_next_ticket_id only extend TTL when the key exists (extend_ttl on a missing key panics with Error(Storage, MissingValue)). - get_event_tickets_count / get_owner_tickets_count read Option<u64> and refresh TTL only when a count exists. - remove_owner_ticket defensively cleans up its index slot when the count entry is missing so later adds cannot collide with stale entries.
…ocol max - mint_ticket, transfer_ticket, use_ticket now extend the Ticket key TTL immediately after writing it, so a fresh ticket is not left at the default low TTL until its first read (matches the PR's write-path intent). - Route ProcessedNonce/ProcessedNonceHash entries through 7/14-day ledger constants (120,960/241,920) instead of seconds-style literals. - mark_nullifier_spent previously used 365-day seconds-style literals (31,536,000/63,072,000) that exceed the 3,110,400-ledger network maximum and would panic on mainnet; cap it at 60-day threshold / protocol-maximum bump.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/payments/src/storage.rs`:
- Around line 7-19: Add Soroban simulation test cases that verify ledger-TTL
behavior for the replay-protection storage constants now defined in the file.
Create test scenarios that advance the simulated ledger before and after each
threshold value (TTL_THRESHOLD, TTL_BUMP, NONCE_TTL_THRESHOLD, NONCE_TTL_BUMP,
NULLIFIER_TTL_THRESHOLD, NULLIFIER_TTL_BUMP). For each constant, include test
paths that verify existing entries remain valid, entries can be renewed before
expiry, and entries correctly fail verification after expiry to enforce replay
protection. Ensure the simulation ledger advancement covers the full lifecycle
of each TTL constant.
In `@contracts/ticket/src/storage.rs`:
- Around line 191-209: Update get_tickets_by_owner so each OwnerTicket(owner,
ticket_id) membership entry has its persistent TTL extended while reading the
owner’s tickets, alongside the existing count and index refreshes. Use the same
TTL_THRESHOLD and TTL_BUMP values and ensure the membership key remains
available for later cleanup in remove_owner_ticket.
- Around line 6-10: Add TTL expiry simulations in the ticket storage tests
around the existing ledger_sequence and storage-key test helpers. Advance
ledger_sequence() beyond TTL_THRESHOLD, verify each key type—Ticket,
OwnerTicket, OwnerTicketIndex, OwnerTicketsCount, and RecoveryKey—expires as
expected, then perform reads/writes to confirm the access refreshes its TTL and
prevents expiry; cover all listed key types before completion.
🪄 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 Plus
Run ID: a1fe660d-f419-400a-b1b3-be28bd7ee9c3
📒 Files selected for processing (5)
contracts/event/src/storage.rscontracts/factory/src/storage.rscontracts/payments/src/storage.rscontracts/ticket/src/lib.rscontracts/ticket/src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/factory/src/storage.rs
- contracts/event/src/storage.rs
| /// TTL refresh threshold in ledgers (~30 days at 5s/ledger). | ||
| const TTL_THRESHOLD: u32 = 518_400; | ||
| /// TTL extension target in ledgers (~60 days at 5s/ledger), well within the | ||
| /// network maximum of 3,110,400 ledgers. | ||
| const TTL_BUMP: u32 = 1_036_800; | ||
| const CURRENT_VERSION: u32 = 1; | ||
| /// Processed-nonce replay-protection entries: ~7-day/14-day ledger schedule. | ||
| const NONCE_TTL_THRESHOLD: u32 = 120_960; // ~7 days at 5s/ledger | ||
| const NONCE_TTL_BUMP: u32 = 241_920; // ~14 days at 5s/ledger | ||
| /// Spent-nullifier replay-protection entries: extend to the protocol maximum | ||
| /// (3,110,400 ledgers, ~180 days) since a replayed nullifier must never pass. | ||
| const NULLIFIER_TTL_THRESHOLD: u32 = 1_036_800; // ~60 days | ||
| const NULLIFIER_TTL_BUMP: u32 = 3_110_400; // protocol maximum TTL |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate TTL-focused Rust tests and their ledger-advance logic.
fd -t f -e rs . | rg '(^|/)(tests?|.*_test)\.rs$' || true
rg -n -C 5 'extend_ttl|TTL_THRESHOLD|TTL_BUMP|NONCE_TTL|NULLIFIER_TTL|ledger.*sequence|jump' --glob '*.rs' .Repository: BuidlZone-Labs/zicket-contract
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== payments storage outline =="
ast-grep outline contracts/payments/src/storage.rs || true
echo "== payments storage relevant constants/functions =="
sed -n '1,260p' contracts/payments/src/storage.rs
echo "== tests mentioning replay/nullifier/ttl in payments =="
rg -n -C 6 'mark_nullifier_spent|has_nullifier|nonce|replay|TTL_|ledger.*sequence|simulate|simulation' contracts/payments/src --glob '*.rs' || trueRepository: BuidlZone-Labs/zicket-contract
Length of output: 50387
Add ledger-TTL simulation coverage for replay-protection storage.
The current ledger advances cover dispute timeouts, not the storage TTLs. Add Soroban simulation cases that advance ledgers before and after TTL_THRESHOLD, TTL_BUMP, NONCE_TTL_THRESHOLD, NONCE_TTL_BUMP, NULLIFIER_TTL_THRESHOLD, and NULLIFIER_TTL_BUMP. Include existing entries, renewal before expiry, and replay after expiry.
🤖 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/payments/src/storage.rs` around lines 7 - 19, Add Soroban
simulation test cases that verify ledger-TTL behavior for the replay-protection
storage constants now defined in the file. Create test scenarios that advance
the simulated ledger before and after each threshold value (TTL_THRESHOLD,
TTL_BUMP, NONCE_TTL_THRESHOLD, NONCE_TTL_BUMP, NULLIFIER_TTL_THRESHOLD,
NULLIFIER_TTL_BUMP). For each constant, include test paths that verify existing
entries remain valid, entries can be renewed before expiry, and entries
correctly fail verification after expiry to enforce replay protection. Ensure
the simulation ledger advancement covers the full lifecycle of each TTL
constant.
| /// TTL refresh threshold in ledgers (~30 days at 5s/ledger). | ||
| pub const TTL_THRESHOLD: u32 = 518_400; | ||
| /// TTL extension target in ledgers (~60 days at 5s/ledger), well within the | ||
| /// network maximum of 3,110,400 ledgers. | ||
| pub const TTL_BUMP: u32 = 1_036_800; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -e rs . | rg '(^|/)(tests?|.*_test)\.rs$' || true
rg -n -C 5 'TTL_THRESHOLD|TTL_BUMP|extend_ttl|NextTicketId|OwnerTicketIndex|RecoveryKey' --glob '*.rs' .Repository: BuidlZone-Labs/zicket-contract
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## ticket test files"
fd -t f -e rs . contracts/ticket | sed -n '1,120p'
echo
echo "## ticket test references to TTL/expiry/expire"
rg -n -C 4 'TTL_THRESHOLD|TTL_BUMP|TTL|expire|expiration|ledger_sequence|sequence\(|extend_ttl|recovery' contracts/ticket -g '*.rs' | sed -n '1,240p'
echo
echo "## storage renewal call sites in contracts/ticket"
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('contracts/ticket/src').glob('*.rs'):
txt=p.read_text()
if 'extend_ttl' in txt:
print(f"\n# {p}")
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if 'extend_ttl' in l:
start=max(1,i-3); end=min(len(lines),i+3)
print(f"\n--- around {p}:{i} ---")
for j in range(start,end+1):
print(f"{j}: {lines[j-1]}")
PYRepository: BuidlZone-Labs/zicket-contract
Length of output: 25235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## contracts/ticket/src/test.rs outline/context"
wc -l contracts/ticket/src/test.rs
sed -n '1,260p' contracts/ticket/src/test.rs
echo
echo "## contracts/ticket/src/test.rs TTL/expiry/sequence references only"
rg -n -C 3 'TTL_THRESHOLD|TTL_BUMP|TTL|expire|expiration|ledger_sequence|sequence\(|extend_ttl|RecoveryKey|transfer|remove_owner_ticket|get_owner_ticket|get_tickets_by_owner|get_recovery_key' contracts/ticket/src/test.rs || true
echo
echo "## contracts/ticket/src/lib.rs relevant sections"
sed -n '360,420p' contracts/ticket/src/lib.rs
sed -n '480,560p' contracts/ticket/src/lib.rs
sed -n '680,770p' contracts/ticket/src/lib.rsRepository: BuidlZone-Labs/zicket-contract
Length of output: 14163
Add TTL expiry simulations for ticket storage keys.
contracts/ticket/src/test.rs does not advance ledger_sequence() past TTL_THRESHOLD. Add coverage that reads/writes refresh Ticket, OwnerTicket, OwnerTicketIndex, OwnerTicketsCount, and RecoveryKey so the expiry objective is satisfied before merge.
🤖 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/ticket/src/storage.rs` around lines 6 - 10, Add TTL expiry
simulations in the ticket storage tests around the existing ledger_sequence and
storage-key test helpers. Advance ledger_sequence() beyond TTL_THRESHOLD, verify
each key type—Ticket, OwnerTicket, OwnerTicketIndex, OwnerTicketsCount, and
RecoveryKey—expires as expected, then perform reads/writes to confirm the access
refreshes its TTL and prevents expiry; cover all listed key types before
completion.
| let count: Option<u64> = env.storage().persistent().get(&key); | ||
| if count.is_some() { | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, TTL_THRESHOLD, TTL_BUMP); | ||
| } | ||
| count.unwrap_or(0) | ||
| } | ||
|
|
||
| /// Get the count of tickets owned by an address | ||
| pub fn get_owner_tickets_count(env: &Env, owner: &Address) -> u64 { | ||
| let key = DataKey::OwnerTicketsCount(owner.clone()); | ||
| env.storage().persistent().get(&key).unwrap_or(0) | ||
| let count: Option<u64> = env.storage().persistent().get(&key); | ||
| if count.is_some() { | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, TTL_THRESHOLD, TTL_BUMP); | ||
| } | ||
| count.unwrap_or(0) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full owner-list and removal paths.
ast-grep outline contracts/ticket/src/storage.rs --items all --type function --match 'get_tickets_by_owner|remove_owner_ticket'
sed -n '95,269p' contracts/ticket/src/storage.rs
rg -n -C 8 'OwnerTicketIndex|OwnerTicketsCount|OwnerTicket\(' contracts/ticket/src/storage.rsRepository: BuidlZone-Labs/zicket-contract
Length of output: 12958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where get_tickets_by_owner and remove_owner_ticket are called from.
rg -n -C 12 'get_tickets_by_owner|remove_owner_ticket|transfer|Transfer|transfer_contract' contracts/ticket/src -g '*.rs'
# Read the surrounding contract API section if not included above.
sed -n '260,460p' contracts/ticket/src/storage.rs 2>/dev/null || true
rg -n 'pub fn|fn ' contracts/ticket/src/storage.rsRepository: BuidlZone-Labs/zicket-contract
Length of output: 50386
Extend the owner-list read TTL to OwnerTicket(owner, ticket_id).
get_tickets_by_owner refreshes the list count and index before reading each ticket_id, but not the membership entry that remove_owner_ticket later looks up in transfer_ticket, recovery, and admin transfers. If OwnerTicket(owner, ticket_id) expires, the index and count can stay alive and cleanup will skip removing OwnerTicketIndex(owner, idx), leaving the old owner with stale ownership metadata.
Also applies to: 220-222
🤖 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/ticket/src/storage.rs` around lines 191 - 209, Update
get_tickets_by_owner so each OwnerTicket(owner, ticket_id) membership entry has
its persistent TTL extended while reading the owner’s tickets, alongside the
existing count and index refreshes. Use the same TTL_THRESHOLD and TTL_BUMP
values and ensure the membership key remains available for later cleanup in
remove_owner_ticket.
2 participants
Closes #142
Summary
Contracts across the codebase write data to Soroban persistent storage but several critical storage operations were missing
extend_ttlcalls. Without periodic TTL extension, persistent entries become archived/expired, bricking contract reads and user payouts over time.Changes
Ticket Contract (
contracts/ticket/src/storage.rs)extend_ttltoget_ticket,update_ticket,get_tickets_by_owner,get_tickets_by_event,get_recovery_key,set_recovery_key, andget_payments_contractTicket Contract (
contracts/ticket/src/lib.rs)extend_ttlto all persistent storage writes inmint_ticket(Ticket, OwnerTickets, EventTickets keys)extend_ttltowrite_next_ticket_idextend_ttltotransfer_ticket,use_ticket,recover_ticket, andadmin_transfer_ticketowner ticket storage updatesEvent Contract (
contracts/event/src/storage.rs)extend_ttltoset_ticket_contractandset_payments_contractextend_ttlon reads forget_ticket_contractandget_payments_contractTTL Configuration
Consistent with existing TTL constants (
TTL_THRESHOLD,TTL_BUMP) already defined across all contracts.Summary by CodeRabbit
Reliability
Bug Fixes