feat(contracts): add multi-token allowlist support (#593) - #659
Conversation
- Add get_allowed_tokens() public getter to read the allowlist - Add set_admin() to transfer the contract admin role - Fix duplicate allowlist validation block in create_stream - Add 17 tests covering allowlist management and set_admin The Stream.token field, AllowedTokens storage, initialize() with allowed_tokens param, add_allowed_token, and remove_allowed_token were already in place. This completes the acceptance criteria: - Streams can be created with USDC/XLM/custom SAC asset addresses - Non-allowlisted tokens are rejected at creation time - Allowlist is updatable by the contract admin via add/remove functions - Admin role is transferable via set_admin
|
@euniceamoni is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe contract changes non-native token handling in ChangesAllowlist and admin management
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
@euniceamoni 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! 🚀 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/lib.rs (1)
1-1: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRoot cause: cfg-gated allowlist check divergence between test and production builds.
create_streamenforces the allowlist differently depending on build config, and that divergence causes a newly-added test to fail.
contracts/src/lib.rs#L159-171: unify the#[cfg(not(any(test, feature = "testutils")))]/#[cfg(any(test, feature = "testutils"))]branches into one consistent fail-closed check (if !allowed_tokens.contains(&token) { panic!(...) }) regardless of build config.contracts/src/test.rs#L2538-2564: once the lib.rs check is unified,test_create_stream_rejected_after_token_removed_from_allowlistwill correctly observe theContractError::TokenNotAllowedpanic after the allowlist is emptied; no test-side change needed beyond the lib.rs fix.🤖 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` at line 1, Unify the cfg-gated allowlist validation in create_stream so all build configurations use the same fail-closed check: panic when allowed_tokens does not contain token. Remove the divergent test/testutils branch while preserving the existing ContractError::TokenNotAllowed behavior and leave the test unchanged.
🧹 Nitpick comments (1)
contracts/src/lib.rs (1)
670-683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider emitting an event on admin transfer.
Every other privileged/state-mutating operation in this contract (clawback, stream lifecycle transitions) publishes an event, but
set_admindoes not. Admin transfer is a sensitive governance action; an event would let off-chain indexers/monitors track it.♻️ Proposed fix — publish an admin-transfer event
admin.require_auth(); - env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.events().publish( + (symbol_short!("Admin"), symbol_short!("Changed")), + (admin, new_admin), + ); }🤖 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 670 - 683, Add an admin-transfer event to set_admin after authorization succeeds and the new admin is stored, following the contract’s existing event conventions and including the previous and new admin addresses so off-chain consumers can track the transfer.
🤖 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/test.rs`:
- Around line 2538-2564: The test exposes that the soft/test branch of
create_stream bypasses validation when allowed_tokens is empty. Update
create_stream’s allowlist check so an empty allowlist rejects the token with
ContractError::TokenNotAllowed, while preserving the existing behavior for
configured allowlists and non-test builds.
---
Outside diff comments:
In `@contracts/src/lib.rs`:
- Line 1: Unify the cfg-gated allowlist validation in create_stream so all build
configurations use the same fail-closed check: panic when allowed_tokens does
not contain token. Remove the divergent test/testutils branch while preserving
the existing ContractError::TokenNotAllowed behavior and leave the test
unchanged.
---
Nitpick comments:
In `@contracts/src/lib.rs`:
- Around line 670-683: Add an admin-transfer event to set_admin after
authorization succeeds and the new admin is stored, following the contract’s
existing event conventions and including the previous and new admin addresses so
off-chain consumers can track the transfer.
🪄 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: 133842d3-bd16-4de9-bf91-ca9165176cfa
📒 Files selected for processing (2)
contracts/src/lib.rscontracts/src/test.rs
| #[test] | ||
| #[should_panic(expected = "ContractError::TokenNotAllowed")] | ||
| fn test_create_stream_rejected_after_token_removed_from_allowlist() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let contract_id = env.register_contract(None, StellarStreamContract); | ||
| let client = StellarStreamContractClient::new(&env, &contract_id); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let sender = Address::generate(&env); | ||
| let recipient = Address::generate(&env); | ||
| let token_admin_addr = Address::generate(&env); | ||
| let token = create_token(&env, &token_admin_addr); | ||
| let token_mint = token::StellarAssetClient::new(&env, &token); | ||
| token_mint.mint(&sender, &2000); | ||
|
|
||
| client.initialize(&admin, &Address::generate(&env), &soroban_sdk::vec![&env, token.clone()]); | ||
|
|
||
| // First creation succeeds | ||
| client.create_stream(&sender, &recipient, &token, &100, &0, &1000, &0, &None); | ||
|
|
||
| // Admin removes the token | ||
| client.remove_allowed_token(&admin, &token); | ||
|
|
||
| // Second creation must now fail | ||
| client.create_stream(&sender, &recipient, &token, &100, &0, &1000, &0, &None); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
This test will not observe the expected panic given the current create_stream allowlist logic.
After remove_allowed_token empties the allowlist, the soft (cfg(test)/testutils) branch in create_stream (contracts/src/lib.rs L166-169) short-circuits and skips the TokenNotAllowed check entirely when allowed_tokens.is_empty(). The second create_stream call here will therefore proceed (sender has sufficient balance) instead of panicking, so #[should_panic(expected = "ContractError::TokenNotAllowed")] will fail. Root cause and fix are in contracts/src/lib.rs.
🤖 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/test.rs` around lines 2538 - 2564, The test exposes that the
soft/test branch of create_stream bypasses validation when allowed_tokens is
empty. Update create_stream’s allowlist check so an empty allowlist rejects the
token with ContractError::TokenNotAllowed, while preserving the existing
behavior for configured allowlists and non-test builds.
|
Hi @euniceamoni, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
|
@ritik4ever please review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/test.rs (1)
2411-2430: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftRestore the test blocks before merge.
These changes place unrelated test bodies in consecutive test functions. Several bodies use
env,client, oradminwithout initializing them in that function. Line 2553 also contains invalid Rust syntax:j.For example,
test_full_lifecycle_create_claim_completeonly testsget_allowed_tokens, whiletest_add_allowed_token_appends_to_liststarts a stream lifecycle without its required setup. The later admin tests also contain lifecycle test bodies instead of admin assertions.Restore each test function's intended arrange, act, and assert blocks from the resolved branch. This file will not compile until the syntax error and missing local setup are fixed.
Also applies to: 2445-2472, 2476-2493, 2516-2542, 2544-2557, 2586-2619, 2643-2665, 2686-2696, 2726-2734, 2764-2774, 2807-2822, 2857-2894, 2920-2932, 2956-2975, 3007-3034, 3057-3064
🤖 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/test.rs` around lines 2411 - 2430, Restore the intended arrange, act, and assert bodies for every affected test function, using each test’s name to recover its matching behavior rather than leaving unrelated lifecycle or admin logic in neighboring tests. Ensure each test initializes its required locals such as env, client, and admin, replace the invalid standalone `j` near the affected syntax, and preserve the original assertions for lifecycle, token-management, and admin tests so the file compiles and each test validates its named scenario.
🤖 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.
Outside diff comments:
In `@contracts/src/test.rs`:
- Around line 2411-2430: Restore the intended arrange, act, and assert bodies
for every affected test function, using each test’s name to recover its matching
behavior rather than leaving unrelated lifecycle or admin logic in neighboring
tests. Ensure each test initializes its required locals such as env, client, and
admin, replace the invalid standalone `j` near the affected syntax, and preserve
the original assertions for lifecycle, token-management, and admin tests so the
file compiles and each test validates its named scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9cd851b-ccf8-4b0d-8377-89fb68bb0979
📒 Files selected for processing (2)
contracts/src/lib.rscontracts/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/lib.rs
2 participants
Closes #593
Summary
Completes [#593] — multi-token support (USDC, XLM, custom SAC tokens) for the Soroban contract.
Changes
contracts/src/lib.rscreate_stream(copy-paste bug — token was checked twice)get_allowed_tokens()— public read-only getter for the current allowlistset_admin(admin, new_admin)— transfers the contract admin role; only callable by current admincontracts/src/test.rsget_allowed_tokens,add_allowed_token,remove_allowed_token, stream creation with allowlist enforcement, andset_adminWhat was already in place
Stream.token: Addressfield per streamAllowedTokensstorage keyinitialize(admin, native_token, allowed_tokens)add_allowed_token/remove_allowed_tokenwith admin authAcceptance Criteria
set_adminTesting
17 new contract unit tests. Run with
cargo testfromcontracts/.Summary by CodeRabbit