RWA: country compliance modules#650
Conversation
Transplant the reviewed country compliance modules and example crates onto upstream/main so they can ship as an independent PR without the old stack.
|
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:
WalkthroughThis PR introduces two new RWA compliance modules— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/tokens/src/rwa/compliance/modules/country_restrict/storage.rs (1)
19-28: Optional: Consider extracting a shared storage helper.The
country_allowandcountry_restrictstorage modules share nearly identical logic (read with TTL extension, set with TTL, remove). If more country-based modules are anticipated, a generic helper could reduce duplication. However, keeping them separate is reasonable for clarity and independent evolution.Also applies to: 37-41, 50-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tokens/src/rwa/compliance/modules/country_restrict/storage.rs` around lines 19 - 28, Extract the repeated TTL-aware storage operations into a shared helper to avoid duplication across the country_allow and country_restrict modules: create a small utility (e.g., functions like get_with_ttl_extend, set_with_ttl, remove_with_ttl) that accepts the Env reference, a storage key type (used by CountryRestrictStorageKey and the country_allow key), TTL threshold and extend amounts, and a closure/type param for the value; then replace the body of is_country_restricted (and the analogous functions at the other locations) to call get_with_ttl_extend instead of duplicating e.storage().persistent().get(...).inspect(...).unwrap_or_default(), and use set_with_ttl/remove_with_ttl for writes and deletes so all TTL logic is centralized and consistent.examples/rwa-country-restrict/src/lib.rs (1)
3-3: Remove unusedStringimport.The
Stringtype is imported but never used in this file.Suggested 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-country-restrict/src/lib.rs` at line 3, The import list from soroban_sdk includes an unused symbol `String`; remove `String` from the `use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, String, Vec};` statement so the import becomes only the actually used symbols (`contract`, `contractimpl`, `contracttype`, `Address`, `Env`, `Vec`), eliminating the unused `String` import and any related compiler warnings.examples/rwa-country-allow/src/lib.rs (1)
3-3: Remove unusedStringimport.The
Stringtype is imported but never used in this file.Suggested 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-country-allow/src/lib.rs` at line 3, Remove the unused String import from the module-level use statement: in the use declaration that currently lists contract, contractimpl, contracttype, Address, Env, String, Vec remove the String identifier so only used symbols remain; ensure no other code references String in this file before committing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/rwa-country-allow/src/lib.rs`:
- Line 3: Remove the unused String import from the module-level use statement:
in the use declaration that currently lists contract, contractimpl,
contracttype, Address, Env, String, Vec remove the String identifier so only
used symbols remain; ensure no other code references String in this file before
committing the change.
In `@examples/rwa-country-restrict/src/lib.rs`:
- Line 3: The import list from soroban_sdk includes an unused symbol `String`;
remove `String` from the `use soroban_sdk::{contract, contractimpl,
contracttype, Address, Env, String, Vec};` statement so the import becomes only
the actually used symbols (`contract`, `contractimpl`, `contracttype`,
`Address`, `Env`, `Vec`), eliminating the unused `String` import and any related
compiler warnings.
In `@packages/tokens/src/rwa/compliance/modules/country_restrict/storage.rs`:
- Around line 19-28: Extract the repeated TTL-aware storage operations into a
shared helper to avoid duplication across the country_allow and country_restrict
modules: create a small utility (e.g., functions like get_with_ttl_extend,
set_with_ttl, remove_with_ttl) that accepts the Env reference, a storage key
type (used by CountryRestrictStorageKey and the country_allow key), TTL
threshold and extend amounts, and a closure/type param for the value; then
replace the body of is_country_restricted (and the analogous functions at the
other locations) to call get_with_ttl_extend instead of duplicating
e.storage().persistent().get(...).inspect(...).unwrap_or_default(), and use
set_with_ttl/remove_with_ttl for writes and deletes so all TTL logic is
centralized and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a63ad065-ba2e-4f2b-b4b6-cecc0c0c99a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlexamples/rwa-country-allow/Cargo.tomlexamples/rwa-country-allow/README.mdexamples/rwa-country-allow/src/lib.rsexamples/rwa-country-restrict/Cargo.tomlexamples/rwa-country-restrict/README.mdexamples/rwa-country-restrict/src/lib.rspackages/tokens/src/rwa/compliance/modules/country_allow/mod.rspackages/tokens/src/rwa/compliance/modules/country_allow/storage.rspackages/tokens/src/rwa/compliance/modules/country_restrict/mod.rspackages/tokens/src/rwa/compliance/modules/country_restrict/storage.rspackages/tokens/src/rwa/compliance/modules/mod.rs
Align the country compliance modules with the repository's preferred test layout by extracting inline test modules into dedicated test.rs files.
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
examples are not done in the light of our other examples. The structure is missing, there is only lib file, and an unwanted readme.
There isn't contract, nor tests.
Move country_allow and country_restrict module logic into public free functions in storage.rs. Remove per-module contracttrait definitions from mod.rs. Restructure examples into canonical lib.rs / contract.rs layout. Delete example READMEs.
Thanks, I updated this to be closer to the structure of the other examples. The example now has the usual split, the extra README is gone, and I added local tests for both country examples so they’re easier to review and a bit more complete. I left the repeated methods as-is for now instead of trying to refactor that part in this PR. That felt like a bigger cleanup than I wanted to mix into what was meant to stay a structural-only pass. If you want, I can take that on next in a separate follow-up. |
Align the country standalone examples and module docs with the repo's compliance-module conventions. Add example-local coverage for the admin/compliance auth boundary so the reviewed structure is verified without changing module behavior.
|
Why there is no trait on I think this doesn't match with the design of the library. |
Restore module-specific country traits and move the example contract API onto those trait implementations.
Use set-style storage semantics and emit country add/remove events only when the stored state changes.
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
Much better!
I found 2-3 small things, that I expected code-quality skill to catch tbh. If it missed them, can you please let me know, so I can tweak the skill further for emphasizing these points?
Thanks!
Thanks again for the careful review. I addressed the latest comments and aligned the country modules more closely with the code-quality rules and nearby library patterns. Specifically, I updated the storage sections/docs, removed TTL extension after writes, routed events through module-level emit helpers, and changed the country traits to compose with I hope this is up to par now :) |
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
LGTM!
I'll review with a fresh set of eyes tomorrow morning just to double check everything.
In the meantime, there is only one small issue remaining:
- in mod.rs files, the trait should come first, then the event related things. It is just re-ordering
Feel free to adjust other PRs based on the feedback for this PR.
And, in the meantime, if you think the code-quality skill missed some important corrections that I've made in here, please let me know, I'll tweak it :)
Thanks again!
Thanks for your patience, it has been a great learning curve! So far I would say the skill was a great addition. Maybe the most useful addition from my learning would be: But it's up to you! |
| fn set_admin(e: &Env, admin: &Address) { | ||
| e.storage().instance().set(&DataKey::Admin, admin); | ||
| } | ||
|
|
||
| fn get_admin(e: &Env) -> Address { | ||
| e.storage().instance().get(&DataKey::Admin).expect("admin must be set") | ||
| } |
There was a problem hiding this comment.
access_control module exposes set_admin and get_admin, let's use those instead
| /// No default implementation is provided because this is a privileged | ||
| /// operation that requires custom access control. Access control should be | ||
| /// enforced before calling | ||
| /// [`super::storage::set_irs_address`]. |
There was a problem hiding this comment.
super usage is frowned upon in this library. Let's use the full path
There was a problem hiding this comment.
also, let's search for super keyword for this PR and change all the instances of it with the full path
| /// default implementation because each contract must enforce its own access | ||
| /// control before delegating to storage helpers. | ||
| #[contracttrait] | ||
| pub trait CountryAllow: ComplianceModule { |
There was a problem hiding this comment.
the trait methods inline documentation is not matching with this library's style:
- error section is missing
- event section style is not matching
can you fix these for all the trait methods in this PR?
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * [`crate::rwa::compliance::modules::ComplianceModuleError::IdentityRegistryNotSet`] |
There was a problem hiding this comment.
ComplianceModuleError should have been imported above. No need to provide the superfluous path here, it is way too long. Again, apply this comment to everywhere possible please
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
some styling changes are required, code itself looks good
2 participants
Country Compliance Modules
This PR introduces two country-based compliance modules for RWA token management:
Country Allow
Only recipients whose identity has at least one country code present in the allowlist may receive tokens. Administrators can configure which countries are allowed per token, either individually or in batch.
Country Restrict
Recipients whose identity has a country code on the restriction list are blocked from receiving tokens. Administrators can configure which countries are restricted per token, either individually or in batch.