all 4 fixed#562
Merged
Merged
Conversation
|
@MadakiElisha is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements delegated manager roles for artist-allowlist with TTL hardening, adds holder indexing and top-fan query capabilities to fan-token, and centralizes event publishing in tip-time-lock. Changes include new storage abstractions, authorization layers, and index maintenance logic across multiple contracts. Changes
Sequence DiagramssequenceDiagram
actor Caller
participant Contract
participant Storage
Caller->>Contract: set_allowlist_mode(artist, caller, mode)
Contract->>Contract: require_artist_or_manager(artist, caller)
Contract->>Storage: is_manager(artist, caller)
Storage-->>Contract: bool (manager check result)
alt Caller is artist or authorized manager
Contract->>Storage: set_config(artist, config)
Storage->>Storage: bump TTL
Contract->>Contract: emit event with caller
Contract-->>Caller: Ok(())
else Unauthorized
Contract-->>Caller: Error::Unauthorized
end
sequenceDiagram
actor Caller
participant Contract
participant Storage
Caller->>Contract: mint_for_tip(artist, holder, amount)
Contract->>Contract: update balance
Contract->>Storage: sync_holder(artist, holder, new_balance)
alt Balance > 0 and holder not in list
Storage->>Storage: append holder to list
else Balance <= 0 and holder in list
Storage->>Storage: remove holder (swap & pop)
end
Storage->>Storage: persist updated list
Storage->>Storage: bump HolderIndex TTL
Contract-->>Caller: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/tip-time-lock/src/test.rs (1)
484-485:⚠️ Potential issue | 🟡 MinorReuse the returned
lock_idinstead of hard-coding"1".These tests assume the first generated id is always
1, which makes them fail for the wrong reason if the id scheme ever changes. Capture the value returned fromcreate_time_lock_tipand use it in the later claim/refund calls.Also applies to: 530-530, 587-587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/tip-time-lock/src/test.rs` around lines 484 - 485, The test is hard-coding "1" for the lock id; change it to capture and reuse the id returned by create_time_lock_tip (e.g., let lock_id = client.create_time_lock_tip(...).unwrap() or the actual return binding used in tests) and pass that variable to client.claim_tip and client.refund_tip instead of String::from_str("1"); update all occurrences (the claim/refund calls) to use the captured lock_id (converting to the expected type/string only if necessary) so tests no longer assume the id scheme.contracts/artist-allowlist/src/lib.rs (1)
132-145:⚠️ Potential issue | 🟠 MajorMove token validation to
set_token_gatewith proper error handling.The
Client::new()call on line 134 does not validate thattoken_addressis a real token contract—it only creates a client wrapper. The misleading comment on lines 132–133 should be removed. When an invalid token address is set,set_token_gatereturns success, butcheck_can_tipwill panic later when it callsclient.balance()becausecheck_can_tipreturns aboolwith no error handling.Validate the token address during setup by calling an actual token method (e.g.,
balance()ortransfer_from()) and map any failures to returnInvalidTokenConfig. This prevents silent acceptance of invalid tokens and provides proper error feedback to the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/artist-allowlist/src/lib.rs` around lines 132 - 145, Remove the misleading comment and the pointless token::Client::new() wrapper call from the setup path and instead perform real validation inside storage::set_token_gate (or the public function that calls it) by creating token::Client and invoking a token method such as client.balance(artist) (or another read method) and map any client errors into the InvalidTokenConfig error; update set_token_gate's signature to return a Result (propagate that Result to the public setter) so failures are returned to the caller and ensure check_can_tip continues to use token::Client but no longer needs to guard against an unvalidated token address because invalid tokens will be rejected at setup.
🧹 Nitpick comments (1)
contracts/tip-time-lock/src/test.rs (1)
30-38: Tighten the event assertion helper.Searching the
Debugoutput of the full event log is brittle: unrelated events can satisfy these substring checks, and any change in the SDK's formatting will break the tests. Prefer asserting the latest contract event and decoding its payload directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/tip-time-lock/src/test.rs` around lines 30 - 38, The helper assert_event_snapshot_contains currently searches the Debug string of env.events().all(), which is brittle; change assert_event_snapshot_contains to fetch the most recent event (e.g., let last = env.events().all().last().expect(...)) and assert against its decoded payload/fields instead of substring matching the Debug output. Decode the event payload (from the event's data/topics as provided by the SDK) and compare specific fields or expected strings directly so the test inspects the latest contract event value rather than scanning the entire debug dump.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/artist-allowlist/src/indexes.rs`:
- Line 27: The code is extending IndexKey::ArtistEntries TTL via bump(env, &key)
without touching the underlying DataKey::Entry TTLs, causing
list_allowlist/get_allowlist_count to become stale; either remove the index-only
bump calls (references: bump, IndexKey::ArtistEntries) from the affected
functions or, if you must refresh the index, also iterate the referenced
DataKey::Entry items and call storage::get_entry/has_entry (or explicitly
refresh/prune those entry keys) so entry TTLs stay in sync; update the logic in
the functions that call bump (and corresponding blocks at the other locations
mentioned) to use one consistent approach.
In `@contracts/artist-allowlist/src/lib.rs`:
- Around line 169-171: The allowlist mutation paths (e.g., after
storage::set_entry and indexes::add_to_index) must refresh the config TTL so
DataKey::Config does not expire and cause check_can_tip to silently fall back to
true; add a call to a new helper (e.g., config::refresh_config_ttl(&env) or
config::touch_config(&env)) immediately after each mutation to update the
DataKey::Config entry's expiration while preserving its contents, implement that
helper to read the existing DataKey::Config, re-save it with the same payload
and a renewed TTL, and apply this change at all mutation sites mentioned
(including the other occurrences around 192-194, 206-209, 278-280, 314-316).
In `@contracts/fan-token/src/queries.rs`:
- Around line 21-61: The top_fans function currently builds and fully
selection-sorts all holder balances (ranked) with O(n²); change it to compute
only the top `limit` items by maintaining a bounded min-heap or a fixed-size
top-k buffer while iterating holders from storage::get_holders: for each
FanBalance (from storage::get_balance) insert into a min-heap of size at most
`limit` (evict the smallest when capacity exceeded) or keep a sorted Vec of up
to `limit` and insert/evict appropriately, then drain the heap/buffer into `top`
in descending order; update references to ranked, len, and the final take logic
so you never perform the full selection sort and only track up to `limit`
FanBalance entries.
- Around line 11-12: The computation of end uses plain addition which can
overflow even though start uses saturating_mul; change the end calculation to
use a saturating add (e.g., replace `start + page_size` with
`start.saturating_add(page_size)`) so the expression in the function that
computes pagination (the `start`/`end` logic in queries.rs) becomes `let end =
start.saturating_add(page_size).min(holders.len());` ensuring extreme `page`
values cannot overflow or wrap.
In `@contracts/fan-token/src/storage.rs`:
- Around line 84-115: sync_holder currently writes an empty holder Vec back to
storage and extends its TTL when the last holder is removed, leaving a dead
DataKey::HolderIndex entry; update sync_holder so that after removing the last
holder (when holders.is_empty()), you remove the key from storage instead of
calling env.storage().persistent().set and do not call extend_ttl for that
key—use env.storage().persistent().remove(&DataKey::HolderIndex(artist.clone()))
and only call extend_ttl when you set a non-empty holders Vec.
---
Outside diff comments:
In `@contracts/artist-allowlist/src/lib.rs`:
- Around line 132-145: Remove the misleading comment and the pointless
token::Client::new() wrapper call from the setup path and instead perform real
validation inside storage::set_token_gate (or the public function that calls it)
by creating token::Client and invoking a token method such as
client.balance(artist) (or another read method) and map any client errors into
the InvalidTokenConfig error; update set_token_gate's signature to return a
Result (propagate that Result to the public setter) so failures are returned to
the caller and ensure check_can_tip continues to use token::Client but no longer
needs to guard against an unvalidated token address because invalid tokens will
be rejected at setup.
In `@contracts/tip-time-lock/src/test.rs`:
- Around line 484-485: The test is hard-coding "1" for the lock id; change it to
capture and reuse the id returned by create_time_lock_tip (e.g., let lock_id =
client.create_time_lock_tip(...).unwrap() or the actual return binding used in
tests) and pass that variable to client.claim_tip and client.refund_tip instead
of String::from_str("1"); update all occurrences (the claim/refund calls) to use
the captured lock_id (converting to the expected type/string only if necessary)
so tests no longer assume the id scheme.
---
Nitpick comments:
In `@contracts/tip-time-lock/src/test.rs`:
- Around line 30-38: The helper assert_event_snapshot_contains currently
searches the Debug string of env.events().all(), which is brittle; change
assert_event_snapshot_contains to fetch the most recent event (e.g., let last =
env.events().all().last().expect(...)) and assert against its decoded
payload/fields instead of substring matching the Debug output. Decode the event
payload (from the event's data/topics as provided by the SDK) and compare
specific fields or expected strings directly so the test inspects the latest
contract event value rather than scanning the entire debug dump.
🪄 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: 1184f7dc-0eeb-4fc6-b454-710f00ed4a40
📒 Files selected for processing (12)
contracts/artist-allowlist/src/access.rscontracts/artist-allowlist/src/indexes.rscontracts/artist-allowlist/src/lib.rscontracts/artist-allowlist/src/storage.rscontracts/artist-allowlist/src/test.rscontracts/fan-token/src/lib.rscontracts/fan-token/src/queries.rscontracts/fan-token/src/storage.rscontracts/fan-token/src/test.rscontracts/tip-time-lock/src/events.rscontracts/tip-time-lock/src/lib.rscontracts/tip-time-lock/src/test.rs
| .unwrap_or_else(|| Vec::new(env)); | ||
| entries.push_back(address.clone()); | ||
| env.storage().persistent().set(&key, &entries); | ||
| bump(env, &key); |
There was a problem hiding this comment.
Don't extend the index TTL independently from the entry TTLs.
These paths now keep IndexKey::ArtistEntries alive, but the underlying DataKey::Entry records are only refreshed through storage::get_entry/has_entry. That means list_allowlist and get_allowlist_count can drift stale after entry keys expire, while is_on_allowlist correctly returns false. Please either refresh/prune the entry keys here as well, or stop bumping the index on its own.
Also applies to: 48-48, 62-64, 77-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/artist-allowlist/src/indexes.rs` at line 27, The code is extending
IndexKey::ArtistEntries TTL via bump(env, &key) without touching the underlying
DataKey::Entry TTLs, causing list_allowlist/get_allowlist_count to become stale;
either remove the index-only bump calls (references: bump,
IndexKey::ArtistEntries) from the affected functions or, if you must refresh the
index, also iterate the referenced DataKey::Entry items and call
storage::get_entry/has_entry (or explicitly refresh/prune those entry keys) so
entry TTLs stay in sync; update the logic in the functions that call bump (and
corresponding blocks at the other locations mentioned) to use one consistent
approach.
Comment on lines
+169
to
171
| storage::set_entry(&env, &artist, &address, &entry); | ||
|
|
||
| indexes::add_to_index(&env, &artist, &address); |
There was a problem hiding this comment.
Refresh the config TTL when the allowlist is actively mutated.
These write paths only touch entry/index state. Since check_can_tip falls back to true when DataKey::Config is missing at Lines 206-209, an artist can keep actively managing an AllowlistOnly setup and still silently fail open once the config key expires. Please bump the config key alongside these mutations, or centralize a helper that touches all state needed to enforce the current mode.
Also applies to: 192-194, 206-209, 278-280, 314-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/artist-allowlist/src/lib.rs` around lines 169 - 171, The allowlist
mutation paths (e.g., after storage::set_entry and indexes::add_to_index) must
refresh the config TTL so DataKey::Config does not expire and cause
check_can_tip to silently fall back to true; add a call to a new helper (e.g.,
config::refresh_config_ttl(&env) or config::touch_config(&env)) immediately
after each mutation to update the DataKey::Config entry's expiration while
preserving its contents, implement that helper to read the existing
DataKey::Config, re-save it with the same payload and a renewed TTL, and apply
this change at all mutation sites mentioned (including the other occurrences
around 192-194, 206-209, 278-280, 314-316).
Comment on lines
+11
to
+12
| let start = page.saturating_mul(page_size); | ||
| let end = (start + page_size).min(holders.len()); |
There was a problem hiding this comment.
Guard the end-index arithmetic.
start + page_size can overflow for large page values even though the start offset is saturated. Use a saturating add here so extreme inputs don't turn the query into a panic or wraparound.
♻️ Suggested fix
- let end = (start + page_size).min(holders.len());
+ let end = start.saturating_add(page_size).min(holders.len());📝 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 start = page.saturating_mul(page_size); | |
| let end = (start + page_size).min(holders.len()); | |
| let start = page.saturating_mul(page_size); | |
| let end = start.saturating_add(page_size).min(holders.len()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/fan-token/src/queries.rs` around lines 11 - 12, The computation of
end uses plain addition which can overflow even though start uses
saturating_mul; change the end calculation to use a saturating add (e.g.,
replace `start + page_size` with `start.saturating_add(page_size)`) so the
expression in the function that computes pagination (the `start`/`end` logic in
queries.rs) becomes `let end =
start.saturating_add(page_size).min(holders.len());` ensuring extreme `page`
values cannot overflow or wrap.
Comment on lines
+21
to
+61
| pub fn top_fans(env: &Env, artist: &Address, limit: u32) -> Vec<FanBalance> { | ||
| if limit == 0 { | ||
| return Vec::new(env); | ||
| } | ||
|
|
||
| let holders = storage::get_holders(env, artist); | ||
| let mut ranked = Vec::new(env); | ||
|
|
||
| for holder in holders.iter() { | ||
| if let Some(balance) = storage::get_balance(env, artist, &holder) { | ||
| if balance.balance > 0 { | ||
| ranked.push_back(balance); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let len = ranked.len(); | ||
| for i in 0..len { | ||
| let mut max_idx = i; | ||
| for j in (i + 1)..len { | ||
| let left = ranked.get(max_idx).unwrap(); | ||
| let right = ranked.get(j).unwrap(); | ||
| if right.balance > left.balance { | ||
| max_idx = j; | ||
| } | ||
| } | ||
| if max_idx != i { | ||
| let vi = ranked.get(i).unwrap(); | ||
| let vmax = ranked.get(max_idx).unwrap(); | ||
| ranked.set(i, vmax); | ||
| ranked.set(max_idx, vi); | ||
| } | ||
| } | ||
|
|
||
| let take = limit.min(ranked.len()); | ||
| let mut top = Vec::new(env); | ||
| for i in 0..take { | ||
| top.push_back(ranked.get(i).unwrap()); | ||
| } | ||
| top | ||
| } |
There was a problem hiding this comment.
Avoid ranking all holders with selection sort.
This walks the entire holder set with O(n²) work even though callers only need the top limit. That will get expensive quickly as the holder index grows; a bounded top-k pass would scale much better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/fan-token/src/queries.rs` around lines 21 - 61, The top_fans
function currently builds and fully selection-sorts all holder balances (ranked)
with O(n²); change it to compute only the top `limit` items by maintaining a
bounded min-heap or a fixed-size top-k buffer while iterating holders from
storage::get_holders: for each FanBalance (from storage::get_balance) insert
into a min-heap of size at most `limit` (evict the smallest when capacity
exceeded) or keep a sorted Vec of up to `limit` and insert/evict appropriately,
then drain the heap/buffer into `top` in descending order; update references to
ranked, len, and the final take logic so you never perform the full selection
sort and only track up to `limit` FanBalance entries.
Comment on lines
+84
to
+115
| pub fn sync_holder(env: &Env, artist: &Address, holder: &Address, balance: i128) { | ||
| let key = DataKey::HolderIndex(artist.clone()); | ||
| let mut holders: soroban_sdk::Vec<Address> = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&key) | ||
| .unwrap_or_else(|| soroban_sdk::Vec::new(env)); | ||
|
|
||
| let mut idx: Option<u32> = None; | ||
| for i in 0..holders.len() { | ||
| if holders.get(i).unwrap() == *holder { | ||
| idx = Some(i); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if balance > 0 { | ||
| if idx.is_none() { | ||
| holders.push_back(holder.clone()); | ||
| } | ||
| } else if let Some(i) = idx { | ||
| let last = holders.pop_back().unwrap(); | ||
| if i < holders.len() { | ||
| holders.set(i, last); | ||
| } | ||
| } | ||
|
|
||
| env.storage().persistent().set(&key, &holders); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, LIFETIME_THRESHOLD, EXTEND_TO); | ||
| } |
There was a problem hiding this comment.
Drop the holder-index key once it becomes empty.
sync_holder still writes an empty vector and renews its TTL after the last holder is removed. That leaves a dead index entry around and works against the rent-hardening goal for artists whose holder list is empty.
🧹 Suggested fix
- env.storage().persistent().set(&key, &holders);
- env.storage().persistent().extend_ttl(&key, LIFETIME_THRESHOLD, EXTEND_TO);
+ if holders.is_empty() {
+ env.storage().persistent().remove(&key);
+ } else {
+ env.storage().persistent().set(&key, &holders);
+ env.storage().persistent().extend_ttl(&key, LIFETIME_THRESHOLD, EXTEND_TO);
+ }📝 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
| pub fn sync_holder(env: &Env, artist: &Address, holder: &Address, balance: i128) { | |
| let key = DataKey::HolderIndex(artist.clone()); | |
| let mut holders: soroban_sdk::Vec<Address> = env | |
| .storage() | |
| .persistent() | |
| .get(&key) | |
| .unwrap_or_else(|| soroban_sdk::Vec::new(env)); | |
| let mut idx: Option<u32> = None; | |
| for i in 0..holders.len() { | |
| if holders.get(i).unwrap() == *holder { | |
| idx = Some(i); | |
| break; | |
| } | |
| } | |
| if balance > 0 { | |
| if idx.is_none() { | |
| holders.push_back(holder.clone()); | |
| } | |
| } else if let Some(i) = idx { | |
| let last = holders.pop_back().unwrap(); | |
| if i < holders.len() { | |
| holders.set(i, last); | |
| } | |
| } | |
| env.storage().persistent().set(&key, &holders); | |
| env.storage() | |
| .persistent() | |
| .extend_ttl(&key, LIFETIME_THRESHOLD, EXTEND_TO); | |
| } | |
| pub fn sync_holder(env: &Env, artist: &Address, holder: &Address, balance: i128) { | |
| let key = DataKey::HolderIndex(artist.clone()); | |
| let mut holders: soroban_sdk::Vec<Address> = env | |
| .storage() | |
| .persistent() | |
| .get(&key) | |
| .unwrap_or_else(|| soroban_sdk::Vec::new(env)); | |
| let mut idx: Option<u32> = None; | |
| for i in 0..holders.len() { | |
| if holders.get(i).unwrap() == *holder { | |
| idx = Some(i); | |
| break; | |
| } | |
| } | |
| if balance > 0 { | |
| if idx.is_none() { | |
| holders.push_back(holder.clone()); | |
| } | |
| } else if let Some(i) = idx { | |
| let last = holders.pop_back().unwrap(); | |
| if i < holders.len() { | |
| holders.set(i, last); | |
| } | |
| } | |
| if holders.is_empty() { | |
| env.storage().persistent().remove(&key); | |
| } else { | |
| env.storage().persistent().set(&key, &holders); | |
| env.storage() | |
| .persistent() | |
| .extend_ttl(&key, LIFETIME_THRESHOLD, EXTEND_TO); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/fan-token/src/storage.rs` around lines 84 - 115, sync_holder
currently writes an empty holder Vec back to storage and extends its TTL when
the last holder is removed, leaving a dead DataKey::HolderIndex entry; update
sync_holder so that after removing the last holder (when holders.is_empty()),
you remove the key from storage instead of calling
env.storage().persistent().set and do not call extend_ttl for that key—use
env.storage().persistent().remove(&DataKey::HolderIndex(artist.clone())) and
only call extend_ttl when you set a non-empty holders Vec.
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.
close #513
close #518
close #519
close #520
Summary by CodeRabbit
New Features
Improvements