Rwa fixes

[ozgunozerk]

Fixes #549

PR Checklist

• Tests
• Documentation

@brozorec take your time on inspecting this one, this is important :)

Reminder to myself: we should merge this after the release

Summary by CodeRabbit

• Improvements
  • Strengthened security with enhanced authorization verification for token transfers
  • Reorganized core transfer logic to improve code structure and operational consistency
  • Comprehensive compliance checks now uniformly applied across all transfer operation types
  • Better separation of authorization concerns for improved code reliability

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors RWA token transfer logic by extracting core transfer checks into a new non-authorized helper function and restructuring transfer and transfer_from as authorization wrappers that delegate to this centralized logic, improving code organization and maintainability.

Changes

Cohort / File(s)	Summary
RWA Transfer Logic Refactoring packages/tokens/src/rwa/storage.rs	Introduced new public transfer_no_auth() helper performing all transfer validations and execution; reworked transfer() to enforce sender authorization and delegate to helper; reworked transfer_from() to enforce spender authorization, manage allowance spending, and delegate to helper; added comprehensive documentation clarifying authorization and non-authorization paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A transfer most clever, refactored with care,
Auth checks now gathered, authorization fair,
The wrapper and helper, in harmony dance,
One path with permission, one path askance,
RWA tokens now flow through a well-ordered lane! 🌙

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Rwa fixes' is too vague and generic, failing to convey specific information about the authorization changes made to transfer and transfer_from functions.	Revise title to be more descriptive, such as 'Refactor RWA transfer authorization logic' or 'Fix RWA transfer and transfer_from authorization', to clearly indicate the main change.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description references issue #549 and includes PR checklist items, but lacks detailed explanation of the actual changes made to the transfer authorization logic and why they were necessary.
Linked Issues check	✅ Passed	The PR refactors RWA authorization by introducing transfer_no_auth helper and reworking transfer/transfer_from wrappers to enforce proper authorization [#549], addressing the linked issue's objective to improve RWA auth.
Out of Scope Changes check	✅ Passed	All changes in the PR (transfer_no_auth introduction, transfer/transfer_from refactoring, documentation) are directly related to resolving RWA authorization issues outlined in #549.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch rwa-fixes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.11%. Comparing base (82f9711) to head (ca67977).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
+ Coverage   96.07%   96.11%   +0.04%     
==========================================
  Files          54       54              
  Lines        5201     5206       +5     
==========================================
+ Hits         4997     5004       +7     
+ Misses        204      202       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @packages/tokens/src/rwa/storage.rs:
- Line 625: The doc comment for the event data currently uses incorrect quote
formatting around the code snippet; replace /// * data - `["to_muxed_id:
Option<u64>, amount: i128"]` with a properly formatted inline code or list, e.g.
/// * data - [`to_muxed_id: Option<u64>, amount: i128`] or /// * data -
`to_muxed_id: Option<u64>, amount: i128`, so the identifiers to_muxed_id and
amount render correctly in the documentation.
- Around line 596-633: The helper function transfer_no_auth is marked pub but
intentionally bypasses authorization, creating a security risk if called
externally; change its visibility from pub to pub(crate) so only crate-internal
code can call it, update any external callers to use the authorized wrappers
(transfer / transfer_from) or add crate-level accessors if needed, and adjust
the function's docstring/comments to reflect the reduced visibility (retain the
security warning).

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82f9711 and babf1c1.

📒 Files selected for processing (1)

• packages/tokens/src/rwa/storage.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: clippy-fmt-test (ubuntu-latest)

🔇 Additional comments (2)

packages/tokens/src/rwa/storage.rs (2)

677-694: LGTM! Clean refactoring with proper authorization.

The function now correctly enforces authorization on the from address and delegates the transfer logic to the centralized helper. The comprehensive documentation clearly outlines all the additional checks performed compared to the base implementation.

---

702-706: Excellent fix for the authorization bug!

This change correctly addresses the authorization issue. By calling transfer_no_auth instead of transfer, the function now properly enforces authorization only on the spender (who must be approved) without incorrectly requiring additional authorization from the from address. This is the standard and correct behavior for allowance-based transfers.

[brozorec]

This works but I'd organize it differently. What about:

• adding validate_transfer() with all the pause and freeze checks + identity verifications + the compliance check and hook
• calling validate_transfer() and Base::update and emitting events in transfer() and transfer_from()?

[ozgunozerk]

I like validate_transfer() idea, will implement and then re-ping for review

[ozgunozerk]

@brozorec here is some subjective design choice I had to make, and open to discussion about it:

• after following validate_transfer() suggestion (which I liked very much), now we shouldn't do call the transferred hook in the validate_transfer(). We were previously calling that hook inside transfer_no_auth().
• and this transferred() hook needs to be called on the ComplianceClient.
• the creation of the ComplianceClient had already took place in the validate_transfer() function
• so, I returned it, hence transfer and transfer_from can re-use that and call the transferred hook
• but, returning the client from validate_function, although very useful from the code perspective, does not align with my design perspective. I wouldn't expect validate_function to return me the client. However, I see this pattern quite a lot on other places, so it wouldn't be super surprising. In fact, it would signal me that: "probably, I'm expected to use this ComplianceClient now, hmmm...", which can be a good thing
• if we provide ComplianceClient to validate_transfer instead, this makes great sense. However, there are 2 minor problems with this:
  • we have duplication of code of ComplianceClient creation in both transfer and transfer_from
  • then, we also have to do the same for IdentityVerifierClient... We have to provide it to validate_transfer
  • then, suddenly, we have extra 2 arguments for the validate_transfer, and we also have additional 4 lines of code in each transfer and transfer_from
  • this can be optimized by having a separate transfer_inner function, that can eliminate the duplication of code across transfer and transfer_from, but at this point, I don't know if it will worth against just returning this ComplianceClient from the validate_transfer
• if we do not return ComplianceClient, and create them in transfer and transfer_from instead, this is duplication of code, but design wise, it is the cleanest imo

This is a very subjective choice, wondering your opinions on this

[brozorec]

This is a very subjective choice, wondering your opinions on this

Would be re-naming validate_transfer to smth like validate_and_hook an option so that we can have this fn does all that stuff?

[ozgunozerk]

I don't like that approach because we emit transfers in the function, and that is for validation. Having validation and hook together does not make sense to me tbh

[brozorec]

I don't like that approach because we emit transfers in the function, and that is for validation. Having validation and hook together does not make sense to me tbh

Then I'd go with recreating the client in transfer and transfer_from:

ComplianceClient::new(e, &compliance_addr).transferred(from, to, &amount, &e.current_contract_address())

It has no impact on perf/cost and looks cleaner to me as well.

[ozgunozerk]

that is what I'm inclining towards also for cleanness. Thanks for the input!

[ozgunozerk]

The reason I did not follow the chain syntax, is cargo format is turning it into:

So, I went with intermediate variables: