Open
Conversation
Transplant the reviewed transfer restriction, time transfer, and lockup modules plus their example crates onto upstream/main as an independent PR.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThree new RWA compliance module examples are added to the workspace: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/rwa-transfer-restrict/src/lib.rs (1)
3-3: Remove unused importString.The
Stringtype is imported but not used anywhere in this file.🧹 Proposed fix
-use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, String, Vec}; +use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, Vec};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/rwa-transfer-restrict/src/lib.rs` at line 3, The import list includes an unused symbol `String` in the `use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, String, Vec};` declaration; remove `String` from that use statement so it becomes `use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, Vec};` to clear the unused-import warning.examples/rwa-time-transfers-limits/src/lib.rs (1)
50-71: Avoid maintaining a second copy of the transfer-limit engine here.Most of this file mirrors
packages/tokens/src/rwa/compliance/modules/time_transfers_limits/mod.rs. Keeping two copies of the counter/reset/update logic makes it easy for the example path to miss future compliance fixes. Export shared helpers fromstellar_tokensand keep only the example-specific auth/constructor surface here.Also applies to: 80-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/rwa-time-transfers-limits/src/lib.rs` around lines 50 - 71, This file duplicates the transfer-limit engine (functions is_counter_finished, reset_counter_if_needed, increase_counters and their use of get_counter/set_counter/get_limits/TransferCounter); instead of maintaining this copy, remove these duplicate functions and call the shared helpers exported from the stellar_tokens time_transfers_limits module (import the common reset/update/counter helpers and types) and wire them into the example’s auth/constructor surface only, keeping example-specific glue but delegating all counter/reset/update logic to the centralized helpers.examples/rwa-initial-lockup-period/src/lib.rs (1)
47-88: Prefer a thin example wrapper over a forked lockup engine.These helpers and hook bodies are effectively a second implementation of
packages/tokens/src/rwa/compliance/modules/initial_lockup_period/mod.rs, just with different auth gating. That makes future compliance fixes easy to miss on the example path. Since this crate already depends onstellar_tokens, consider exporting shared auth-agnostic helpers and keeping only the example-specific auth wrapper here.Also applies to: 97-227
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tokens/src/rwa/compliance/modules/initial_lockup_period/mod.rs`:
- Around line 183-204: The on_transfer path currently mutates balances without
re-checking that the transfer is allowed; fix by enforcing the unlocked-balance
predicate immediately before state updates: after computing/consuming matured
locks (use get_total_locked, get_internal_balance and update_locked_tokens as
already done), recompute the effective free balance (e.g., let post_balance =
get_internal_balance(e, &token, &from); let post_total_locked =
get_total_locked(e, &token, &from); let post_free = post_balance -
post_total_locked) and require that amount <= post_free (or call the existing
transfer-check helper if one exists) and abort otherwise; only then call
sub_i128_or_panic and add_i128_or_panic to mutate balances.
In `@packages/tokens/src/rwa/compliance/modules/time_transfers_limits/mod.rs`:
- Around line 217-223: The on_transfer hook currently updates counters without
re-checking the transfer limit predicate, so make it fail-closed by calling the
same limit-check used in the CanTransfer path before mutating state: after
require_non_negative_amount and resolving irs/from_id (in on_transfer), invoke
the predicate function used by CanTransfer (the limit-check function used to
allow/reject transfers) with the same params (e.g., token, from_id, amount) and
use a require-style assertion to abort if it fails, then only call
increase_counters; reference on_transfer, get_irs_client, stored_identity, and
increase_counters to locate where to insert the check.
---
Nitpick comments:
In `@examples/rwa-time-transfers-limits/src/lib.rs`:
- Around line 50-71: This file duplicates the transfer-limit engine (functions
is_counter_finished, reset_counter_if_needed, increase_counters and their use of
get_counter/set_counter/get_limits/TransferCounter); instead of maintaining this
copy, remove these duplicate functions and call the shared helpers exported from
the stellar_tokens time_transfers_limits module (import the common
reset/update/counter helpers and types) and wire them into the example’s
auth/constructor surface only, keeping example-specific glue but delegating all
counter/reset/update logic to the centralized helpers.
In `@examples/rwa-transfer-restrict/src/lib.rs`:
- Line 3: The import list includes an unused symbol `String` in the `use
soroban_sdk::{contract, contractimpl, contracttype, Address, Env, String, Vec};`
declaration; remove `String` from that use statement so it becomes `use
soroban_sdk::{contract, contractimpl, contracttype, Address, Env, Vec};` to
clear the unused-import warning.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7a7bda9-48a0-4635-97fb-70ef341afb19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlexamples/rwa-initial-lockup-period/Cargo.tomlexamples/rwa-initial-lockup-period/README.mdexamples/rwa-initial-lockup-period/src/lib.rsexamples/rwa-time-transfers-limits/Cargo.tomlexamples/rwa-time-transfers-limits/README.mdexamples/rwa-time-transfers-limits/src/lib.rsexamples/rwa-transfer-restrict/Cargo.tomlexamples/rwa-transfer-restrict/README.mdexamples/rwa-transfer-restrict/src/lib.rspackages/tokens/src/rwa/compliance/modules/initial_lockup_period/mod.rspackages/tokens/src/rwa/compliance/modules/initial_lockup_period/storage.rspackages/tokens/src/rwa/compliance/modules/initial_lockup_period/test.rspackages/tokens/src/rwa/compliance/modules/mod.rspackages/tokens/src/rwa/compliance/modules/time_transfers_limits/mod.rspackages/tokens/src/rwa/compliance/modules/time_transfers_limits/storage.rspackages/tokens/src/rwa/compliance/modules/time_transfers_limits/test.rspackages/tokens/src/rwa/compliance/modules/transfer_restrict/mod.rspackages/tokens/src/rwa/compliance/modules/transfer_restrict/storage.rspackages/tokens/src/rwa/compliance/modules/transfer_restrict/test.rs
Comment on lines
+183
to
+204
| fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) { | ||
| get_compliance_address(e).require_auth(); | ||
| require_non_negative_amount(e, amount); | ||
|
|
||
| let total_locked = get_total_locked(e, &token, &from); | ||
|
|
||
| if total_locked > 0 { | ||
| let pre_balance = get_internal_balance(e, &token, &from); | ||
| let pre_free = pre_balance - total_locked; | ||
|
|
||
| if amount > pre_free.max(0) { | ||
| let to_consume = amount - pre_free.max(0); | ||
| update_locked_tokens(e, &token, &from, to_consume); | ||
| } | ||
| } | ||
|
|
||
| let from_bal = get_internal_balance(e, &token, &from); | ||
| set_internal_balance(e, &token, &from, sub_i128_or_panic(e, from_bal, amount)); | ||
|
|
||
| let to_bal = get_internal_balance(e, &token, &to); | ||
| set_internal_balance(e, &token, &to, add_i128_or_panic(e, to_bal, amount)); | ||
| } |
Contributor
There was a problem hiding this comment.
on_transfer needs its own unlocked-balance guard.
Unlike on_destroyed, this path never proves that the amount is actually unlocked. If CanTransfer is miswired or skipped, it will subtract the full amount after consuming only matured locks, which can move still-locked value and violate the total_locked <= internal_balance invariant. Re-check the transfer predicate before mutating state.
🛠️ Proposed fix
fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) {
get_compliance_address(e).require_auth();
require_non_negative_amount(e, amount);
+ assert!(
+ Self::can_transfer(e, from.clone(), to.clone(), amount, token.clone()),
+ "InitialLockupPeriodModule: insufficient unlocked balance for transfer"
+ );
let total_locked = get_total_locked(e, &token, &from);📝 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
| fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) { | |
| get_compliance_address(e).require_auth(); | |
| require_non_negative_amount(e, amount); | |
| let total_locked = get_total_locked(e, &token, &from); | |
| if total_locked > 0 { | |
| let pre_balance = get_internal_balance(e, &token, &from); | |
| let pre_free = pre_balance - total_locked; | |
| if amount > pre_free.max(0) { | |
| let to_consume = amount - pre_free.max(0); | |
| update_locked_tokens(e, &token, &from, to_consume); | |
| } | |
| } | |
| let from_bal = get_internal_balance(e, &token, &from); | |
| set_internal_balance(e, &token, &from, sub_i128_or_panic(e, from_bal, amount)); | |
| let to_bal = get_internal_balance(e, &token, &to); | |
| set_internal_balance(e, &token, &to, add_i128_or_panic(e, to_bal, amount)); | |
| } | |
| fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) { | |
| get_compliance_address(e).require_auth(); | |
| require_non_negative_amount(e, amount); | |
| assert!( | |
| Self::can_transfer(e, from.clone(), to.clone(), amount, token.clone()), | |
| "InitialLockupPeriodModule: insufficient unlocked balance for transfer" | |
| ); | |
| let total_locked = get_total_locked(e, &token, &from); | |
| if total_locked > 0 { | |
| let pre_balance = get_internal_balance(e, &token, &from); | |
| let pre_free = pre_balance - total_locked; | |
| if amount > pre_free.max(0) { | |
| let to_consume = amount - pre_free.max(0); | |
| update_locked_tokens(e, &token, &from, to_consume); | |
| } | |
| } | |
| let from_bal = get_internal_balance(e, &token, &from); | |
| set_internal_balance(e, &token, &from, sub_i128_or_panic(e, from_bal, amount)); | |
| let to_bal = get_internal_balance(e, &token, &to); | |
| set_internal_balance(e, &token, &to, add_i128_or_panic(e, to_bal, amount)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tokens/src/rwa/compliance/modules/initial_lockup_period/mod.rs`
around lines 183 - 204, The on_transfer path currently mutates balances without
re-checking that the transfer is allowed; fix by enforcing the unlocked-balance
predicate immediately before state updates: after computing/consuming matured
locks (use get_total_locked, get_internal_balance and update_locked_tokens as
already done), recompute the effective free balance (e.g., let post_balance =
get_internal_balance(e, &token, &from); let post_total_locked =
get_total_locked(e, &token, &from); let post_free = post_balance -
post_total_locked) and require that amount <= post_free (or call the existing
transfer-check helper if one exists) and abort otherwise; only then call
sub_i128_or_panic and add_i128_or_panic to mutate balances.
Comment on lines
+217
to
+223
| fn on_transfer(e: &Env, from: Address, _to: Address, amount: i128, token: Address) { | ||
| get_compliance_address(e).require_auth(); | ||
| require_non_negative_amount(e, amount); | ||
| let irs = get_irs_client(e, &token); | ||
| let from_id = irs.stored_identity(&from); | ||
| increase_counters(e, &token, &from_id, amount); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep on_transfer fail-closed.
This hook trusts that CanTransfer executed first. If Transferred is wired without CanTransfer, or this entrypoint is invoked directly, the transfer is accepted and the counter is updated after the fact. Re-check the limit predicate here before mutating counters.
🛠️ Proposed fix
- fn on_transfer(e: &Env, from: Address, _to: Address, amount: i128, token: Address) {
+ fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) {
get_compliance_address(e).require_auth();
require_non_negative_amount(e, amount);
+ assert!(
+ Self::can_transfer(e, from.clone(), to.clone(), amount, token.clone()),
+ "TimeTransfersLimitsModule: transfer exceeds configured limits"
+ );
let irs = get_irs_client(e, &token);
let from_id = irs.stored_identity(&from);
increase_counters(e, &token, &from_id, amount);📝 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
| fn on_transfer(e: &Env, from: Address, _to: Address, amount: i128, token: Address) { | |
| get_compliance_address(e).require_auth(); | |
| require_non_negative_amount(e, amount); | |
| let irs = get_irs_client(e, &token); | |
| let from_id = irs.stored_identity(&from); | |
| increase_counters(e, &token, &from_id, amount); | |
| } | |
| fn on_transfer(e: &Env, from: Address, to: Address, amount: i128, token: Address) { | |
| get_compliance_address(e).require_auth(); | |
| require_non_negative_amount(e, amount); | |
| assert!( | |
| Self::can_transfer(e, from.clone(), to.clone(), amount, token.clone()), | |
| "TimeTransfersLimitsModule: transfer exceeds configured limits" | |
| ); | |
| let irs = get_irs_client(e, &token); | |
| let from_id = irs.stored_identity(&from); | |
| increase_counters(e, &token, &from_id, amount); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tokens/src/rwa/compliance/modules/time_transfers_limits/mod.rs`
around lines 217 - 223, The on_transfer hook currently updates counters without
re-checking the transfer limit predicate, so make it fail-closed by calling the
same limit-check used in the CanTransfer path before mutating state: after
require_non_negative_amount and resolving irs/from_id (in on_transfer), invoke
the predicate function used by CanTransfer (the limit-check function used to
allow/reject transfers) with the same params (e.g., token, from_id, amount) and
use a require-style assertion to abort if it fails, then only call
increase_counters; reference on_transfer, get_irs_client, stored_identity, and
increase_counters to locate where to insert the check.
Cover the shared compliance storage helpers directly so Codecov reflects the real exercised behavior without touching production logic.
Apply rustfmt to the new shared-helper coverage assertions so the transfer PR passes the workspace formatting check.
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
1 participant
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.
Summary
Adds three transfer-related compliance modules for RWA tokens: allowlist-based transfer restriction, rolling transfer limits, and an initial post-mint lockup period.
Changes
transfer_restrict,time_transfers_limits, andinitial_lockup_periodcompliance modules with their storage and hook logic.rwa-transfer-restrict,rwa-time-transfers-limits, andrwa-initial-lockup-periodexample crates.Test plan
cargo +nightly fmt --all -- --checkcargo test -p stellar-tokens --lib transfercargo test -p stellar-tokens --lib lockupcargo test -p rwa-transfer-restrict --libcargo test -p rwa-time-transfers-limits --libcargo test -p rwa-initial-lockup-period --lib