feat(rwa): add compliance module base architecture#607
feat(rwa): add compliance module base architecture#607pasevin wants to merge 27 commits intoOpenZeppelin:mainfrom
Conversation
|
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 a new compliance modules subsystem for T-REX in Soroban, featuring shared utilities for compliance address management, hook verification, safe arithmetic operations, and identity registry integration, alongside new error types and TTL constants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 Coding Plan
Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/common.rs`:
- Around line 70-77: set_compliance_address currently allows a first-write
takeover because require_compliance_auth does no auth before initialization; fix
by introducing and enforcing an explicit trusted-initializer/admin authorization
path before the first write—modify set_compliance_address to require a verified
admin (e.g., check a stored admin key or invoke a new
require_admin/require_trusted_initializer helper) before calling
e.storage().persistent().set(&compliance_key(e), compliance) and extending TTL
(MODULE_TTL_THRESHOLD, MODULE_EXTEND_AMOUNT); also update the related auth
helper require_compliance_auth to delegate to the admin check for the initial
unset case and apply the same change to the analogous setter at the other
location (the function spanning lines 116-124) so the pattern is consistent
across modules.
- Around line 145-152: The current hooks_verified() stores a single boolean
under hooks_verified_key(e), which incorrectly short-circuits future
verify_required_hooks(required) calls with different required sets; fix by
keying the cache by the specific hook set (e.g., canonicalize the required list
by sorting and joining or compute a stable hash) and use that canonicalized-set
key in hooks_verified_key/when writing the persistent entry (and extend TTL with
MODULE_TTL_THRESHOLD / MODULE_EXTEND_AMOUNT as before); alternatively, if you
prefer the simpler API change, restrict verify_required_hooks to a single fixed
set per module and document/enforce that, but do not keep a single global
boolean—update both hooks_verified and the corresponding write path (where the
flag is stored) and any similar logic in the other block referenced (the code
around verify_required_hooks and its storage usage) to use the per-set cache
key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d058ee-4190-484f-b712-1dc8d8b4556a
📒 Files selected for processing (4)
.gitignorepackages/tokens/src/rwa/compliance_modules/common.rspackages/tokens/src/rwa/compliance_modules/mod.rspackages/tokens/src/rwa/mod.rs
| /// Read-only cross-contract client into the Identity Registry Storage. | ||
| /// | ||
| /// Modules that need identity or country resolution store the IRS address | ||
| /// per token and call through this client at check time — mirroring the | ||
| /// T-REX pattern where modules resolve identity via the token's registry. | ||
| #[contractclient(name = "IRSReadClient")] | ||
| pub trait IRSRead { | ||
| /// Returns the on-chain identity address associated with `account`. | ||
| fn stored_identity(e: &Env, account: Address) -> Address; | ||
|
|
||
| /// Returns all country data entries for `account`. | ||
| fn get_country_data_entries(e: &Env, account: Address) -> Vec<CountryData>; | ||
| } |
There was a problem hiding this comment.
@brozorec I'm more inclined to have the library more feature-rich so that we won't need such helpers. We can put #[contractclient] above:
pub trait IdentityRegistryStorage: TokenBinderpub trait CountryDataManager: IdentityRegistryStorage
so that this helper trait won't be necessary.
I acknowledge that this helper is more compact, however, having extra helpers for an already very complex suite (RWA) is a bigger CON compared to having all everything ready (as much as possible) in the original library files.
What do you think?
@pasevin feel free to chime in as well
There was a problem hiding this comment.
#[contractclient] will solve the need to declare such partial traits only to get a client but the problem is that this macro doesn't work when there are associate types which is the case here.
There was a problem hiding this comment.
Ah I see... That's a bummer.
There was a problem hiding this comment.
I agree with the direction of keeping this client surface canonical rather than growing extra helpers in common.rs. The issue is that #[contractclient] is not a drop-in option on IdentityRegistryStorage / CountryDataManager as they stand today, because they use associated types (Self::CountryData), and that breaks the macro path here. So I think the current IRSRead helper is a pragmatic stopgap, not the ideal end state.
If we want to clean this up properly, I’d suggest introducing a canonical non-generic read-only IRS trait in identity_registry_storage/mod.rs for the concrete methods the modules need, and generating the client from that. That would keep the client surface in the original library files without relying on partial helper traits in common.rs.
I will leave this decision up to you :)
There was a problem hiding this comment.
The options we have:
- The approach proposed here, ie. declare a trait with the concrete type and only the methods that will be called. Downsides: partial code duplication, but as the interfaces sit in different places and are decoupled, if the main one changes, failing to change all the duplicates will result in issues => code smell.
- The approach in
Verifierhere where we have the same issue: we keep both interfaces in the same file, but at the cost of code duplication. - We avoid using assoc types at all and directly use the type from the reference implementation in
storage.rs. Downsides: we make interfaces opinionated and tightly coupled to the ref implementation. - If we still want to allow some flexibility for the types of the params passed to methods, we declare those params as
Val. Downsides: difficulty for downstream clients to treat those opaque type => bad UX.
@ozgunozerk wdyt?
There was a problem hiding this comment.
We can also open an issue to soroban_sdk about this limitation (assoc types with contractclient macro). Maybe in the future, there will be a workaround, and we can introduce assoc types once again for a more flexible interface.
Because of that, I think we should put in the inline comments why we are not using assoc types, and in which case they can be reintroduced again.
There was a problem hiding this comment.
Having the ability to put #[contractclient] above our traits started to sound non-negotiable to me.
I support this 👍
Concretely, is this how we're moving forward?
IdentityRegistryStorage: we set thestorage::CountryDataas the type to use in that trait, it's an opinionated decision, but we assume it. If someone needs another data type, they conceive their own interface.Verifier: we switch toVal, instead of assoc type
If you're ok, I can start working on those.
There was a problem hiding this comment.
Marking this as resolved based on internal discussions and the subsequent reorg commit 20981b1
There was a problem hiding this comment.
@pasevin sorry, but unresolving here 😅 this discussion is actually more relevant to your question here.
@ozgunozerk wdy about the proposed ways to move forward?
There was a problem hiding this comment.
@brozorec , sounds good to me, we can forward with these decisions
| // --------------------------------------------------------------------------- | ||
| // Identity Registry Storage helpers | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
I'm unsure about this whole section. Do we really need an another set of explicit implementations? Can't we achieve the same this by utilizing what we already have?
There was a problem hiding this comment.
I think the duplication concern is valid, but only for the extra read-only client trait, not for the whole section.
The per-token IRS binding helpers in common.rs still make sense for the modules, but IRSRead is overlapping with the canonical IRS interfaces we already expose in identity_registry_storage/mod.rs (stored_identity / get_country_data_entries). So I’d lean toward reusing the existing IRS client surface and keeping only the module-side binding/resolution helpers in common.rs. WDY?
There was a problem hiding this comment.
Here, if I understand correctly the individual modules need access to IRS and that's why we should add a dedicated setter and getter. Regarding the access to an IRSClient check my point here.
There was a problem hiding this comment.
Thanks, that makes sense. I think this confirms the split here:
- the per-token IRS binding/resolution helpers in
common.rsare still needed for the modules - the overlapping part is the extra read-only client trait, not the whole IRS helper section
I had initially hoped we could reuse the canonical IRS traits directly, but your linked point clarifies why that is not a drop-in fix: IdentityRegistryStorage / CountryDataManager rely on associated types, so #[contractclient] does not fit that surface cleanly today.
So my current view is that the right long-term cleanup would be to expose a canonical non-generic read-only IRS trait from identity_registry_storage/mod.rs for the concrete methods modules need, and generate the client from that. That would let us keep the module-side setter/getter helpers while avoiding an extra ad hoc client trait in common.rs.
There was a problem hiding this comment.
@ozgunozerk @brozorec I think this was only partially addressed by the upstream reorg and by my migration.
The part that was cleaned up is the compliance-side duplication: we no longer keep a separate hook-check client in common.rs, and the post-reorg structure under rwa/compliance/modules is definitely more coherent than the old split. That aligns with the general cleanup direction discussed above.
What is still not fully addressed is the IRS side of the concern. We still keep an explicit read-only helper trait/client in common.rs (IRSRead / IRSReadClient) instead of reusing a canonical IRS client surface directly. The reason seems to still be the same one discussed in-thread: the current IRS traits use associated types, so #[contractclient] is not a drop-in fit for them. In other words, the reorg improved structure, but it did not by itself eliminate the need for that stopgap.
So my view is: this is not a live bug, but it is still an open design question. The cleaner end state would be to expose a canonical, non-generic read-only IRS trait from the identity registry layer and generate the client from there, instead of declaring the partial IRS read trait in common.rs. The reorg commit moved things in the right direction structurally, but I would not consider this specific concern fully closed just from that alone.
So the question remains, what should we do?
There was a problem hiding this comment.
I think this discussion will be addressed in here: https://github.com/OpenZeppelin/stellar-contracts/pull/607/changes#r2923570826
pasevin
force-pushed
the
feat/rwa-compliance-01-base
branch
from
3c3ebd1 to
ab45658
Compare
Introduce the compliance_modules sub-module under rwa with shared helpers (common.rs), the ComplianceModuleError enum (390-397 range), and TTL constants. This provides the foundation for individual compliance module implementations in follow-up PRs.
Use a dedicated contract error for missing required hook wiring and add focused tests for compliance helper verification, IRS lookup, and math failure paths.
Update the few remaining files whose formatting drifted from the current nightly rustfmt output so the base branch matches CI again.
Short-circuit cached hook verification, extend compliance TTL during verification, and use stable contract errors for duplicate compliance binding. Clarify the singleton storage-key comment and cover the new helper behavior with focused tests.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Make shared compliance auth helpers reject unbound module calls so default module entrypoints cannot rely on unauthenticated pre-bind fallback behavior.
Normalize shared compliance helper docs to use `# Errors` and inline return prose so the base branch matches the upstream review convention and downstream stacked PRs inherit it.
Adjust the shared compliance helper docs to match nightly rustfmt wrapping so format checks pass consistently across the stacked RWA branches.
Define `ComplianceModuleError` in `rwa::compliance` so the shared module error type lives alongside the compliance contract types while preserving existing re-exports.
Drop the temporary re-exports for `ComplianceModuleError` and point shared compliance helpers at the enum's canonical home under `rwa::compliance`.
Move the shared compliance-module TTL constants into `rwa::compliance` and update the base compliance helper code to use that canonical location.
Rename the shared compliance math helpers to make their panic-on-overflow behavior explicit instead of using the misleading `checked_*` convention.
Remove the duplicate hook-check client from the shared module helpers and use the canonical compliance client directly for required-hook verification.
Replace the ad hoc singleton symbol helpers in common.rs with a typed ComplianceModuleStorageKey enum to make module storage access explicit.
Move the module-level compliance address singleton into instance storage and remove per-key TTL handling for that bounded contract-wide state.
Require the compliance address to be configured before exposing it through the shared getter and update module auth guidance to use require_compliance_auth.
Drop the shared require_compliance_auth wrapper now that the compliance getter is fail closed and callers can require auth directly on the configured address.
Move the module-wide hook verification cache into instance storage and stop extending TTL for that bounded singleton flag.
Re-export the module trait entrypoints from the compliance modules root so downstream example contracts keep compiling against the shared library surface.
Expose only the shared base helper on the foundation branch so nightly fmt does not resolve module exports before the downstream module branches introduce them.
Fold the IRS registry key into the shared compliance storage enum and remove redundant write-time TTL extension while aligning the helper docs with the upstream review feedback.
Treat verify_required_hooks as a post-bind validation step and panic when compliance is unset instead of silently succeeding, while updating the shared helper docs and regression test to match the stricter behavior.
Call the shared compliance getter from verify_required_hooks so the helper follows the canonical fail-closed access path instead of duplicating the instance-storage lookup inline.
Move the shared compliance module helpers into rwa::compliance::modules, remove the old compliance_modules root, and keep the rebased base branch focused by dropping formatter-only residue outside the RWA surface.
pasevin
force-pushed
the
feat/rwa-compliance-01-base
branch
from
2dfca98 to
9a4340e
Compare
Move the compliance common helper tests into a dedicated test module so the implementation file stays focused on shared logic while preserving coverage.
| .unwrap_or_else(|| panic_with_error!(e, ComplianceModuleError::MathUnderflow)) | ||
| } | ||
|
|
||
| /// Allocates a Soroban [`String`] from a static `&str` for use as a |
There was a problem hiding this comment.
This doesn't belong to Amount Validation section.
I think, we can get rid of these inline sections, and have QUERY STATE, CHANGE STATE, and HELPERS if needed, as in our other examples. For the formatting, take a look at any storage.rs file in the token packages. This will be more inline with the library's conventions
There was a problem hiding this comment.
common.rs can be renamed to storage.rs imo. Also, the test.rs can be moved higher to modules folder.
All the changes done to compliance/mod.rs for the modules should be moved to compliance/modules/mod.rs due to now we have a separate mod.rs for modules.
In other words, mod.rs, storage.rs and test.rs can be replicated into compliance/modules folder, as in we have in other parts of the library.
After that, and after we resolve the remaining discussions, it is good to go from my end!
Well done, and thanks a ton @pasevin 🚀
3 participants
Summary
Introduces the compliance module foundation under
packages/tokens/src/rwa/compliance_modules/for the stacked RWA compliance modules work (PR #606 follow-up).Changes
rwa/mod.rs: Addpub mod compliance_modules;,ComplianceModuleErrorenum (390–397 range), andMODULE_EXTEND_AMOUNT/MODULE_TTL_THRESHOLDconstants.compliance_modules/mod.rs: Parent module withpub mod common;and re-exports.compliance_modules/common.rs: Shared helpers — compliance address management (one-time lock), IRS client, hook verification, safe math, country extraction..gitignore: Add.cursor/and.claude/.No new workspace crates; purely additive library code. Later PRs will add the seven module traits + storage and their example crates.
Stack
This is PR 1 of 6. Branch
feat/rwa-compliance-02-countrywill stack on this one.Summary by CodeRabbit
New Features
Chores