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 refactors the identity registry storage module to use Soroban SDK's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 58 58
Lines 5960 5964 +4
=======================================
+ Hits 5744 5748 +4
Misses 216 216 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (2)
packages/tokens/src/rwa/identity_verification/identity_registry_storage/mod.rs (1)
507-523:⚠️ Potential issue | 🟠 MajorThese default getters still lock the trait to the reference
CountryDatastorage.The whole point of the
Valrefactor is to let implementers use a custom country-data shape, but both default read paths still deserialize throughstorage::get_country_data[_entries], which is hard-wired to the referenceIdentityProfile/CountryDatalayout. That means a contract with customValcountry data will compile yet return the wrong thing unless it remembers to override both methods.I’d make these required trait methods, or move the current bodies into explicit “reference implementation” helpers so the abstraction can’t be accidentally violated.
Possible fix
pub trait CountryDataManager: IdentityRegistryStorage { @@ - fn get_country_data_entries(e: &Env, account: Address) -> Vec<Val> { - Vec::from_iter( - e, - get_country_data_entries(e, &account).iter().map(|entry| entry.into_val(e)), - ) - } + fn get_country_data_entries(e: &Env, account: Address) -> Vec<Val>; @@ - fn get_country_data(e: &Env, account: Address, index: u32) -> Val { - storage::get_country_data(e, &account, index).into_val(e) - } + fn get_country_data(e: &Env, account: Address, index: u32) -> Val; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tokens/src/rwa/identity_verification/identity_registry_storage/mod.rs` around lines 507 - 523, The default getters get_country_data_entries and get_country_data currently hard-wire deserialization via storage::get_country_data and storage::get_country_data_entries (tied to the reference IdentityProfile/CountryData), which breaks custom Val implementations; change the trait so these methods are required (remove default bodies) or instead extract the current bodies into explicit helper functions (e.g., reference_get_country_data_entries and reference_get_country_data) and keep the trait methods abstract; update references to get_country_data_entries and get_country_data to call the new helpers only when the implementer wants the reference behavior.packages/tokens/src/rwa/identity_verification/identity_registry_storage/storage.rs (1)
83-109:⚠️ Potential issue | 🟡 MinorUpdate the
CountryDataManagerexample to the newValsignature.This example still implements
add_country_data_entrieswithVec<CountryData>, but the trait now takesVec<Val>. As written, the snippet no longer matches the public API and will fail for anyone copy-pasting it. The example should acceptVec<Val>and convert each entry toCountryDatabefore running the duplicate check and calling the reference helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tokens/src/rwa/identity_verification/identity_registry_storage/storage.rs` around lines 83 - 109, The example implements CountryDataManager::add_country_data_entries with the old Vec<CountryData> signature; update the function to accept Vec<Val> (matching the trait) and convert each Val into CountryData before duplicate-checking and forwarding to the helper. Specifically, change the parameter type to Vec<Val>, map or iterate over those Vals converting with the appropriate conversion helper (e.g., CountryData::try_from_val or Val::try_into::<CountryData>() / from_val pattern used in this crate), handle conversion failures by returning or panicking with the crate's error macro (e.g., panic_with_error!(e, Error::InvalidInput)), perform the existing duplicate country checks on the converted CountryData items, and pass a reference to the converted Vec<CountryData> into rwa::identity_verification::identity_registry_storage::add_country_data_entries.
🤖 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/identity_verification/identity_registry_storage/mod.rs`:
- Around line 329-331: Update the event docs to correctly reflect the ABI: for
the contract events CountryDataAdded, CountryDataRemoved, and
CountryDataModified, only the account field is emitted as a topic (it is tagged
with #[topic]); the country_data field is emitted in the event data payload (as
a map under the default #[contractevent] data_format = "map"), not as a topic,
so change the documentation at the affected comment blocks to list topics as
["country_removed", account: Address] (or the appropriate event name + account)
and place country_data under the data payload description rather than topics.
---
Outside diff comments:
In
`@packages/tokens/src/rwa/identity_verification/identity_registry_storage/mod.rs`:
- Around line 507-523: The default getters get_country_data_entries and
get_country_data currently hard-wire deserialization via
storage::get_country_data and storage::get_country_data_entries (tied to the
reference IdentityProfile/CountryData), which breaks custom Val implementations;
change the trait so these methods are required (remove default bodies) or
instead extract the current bodies into explicit helper functions (e.g.,
reference_get_country_data_entries and reference_get_country_data) and keep the
trait methods abstract; update references to get_country_data_entries and
get_country_data to call the new helpers only when the implementer wants the
reference behavior.
In
`@packages/tokens/src/rwa/identity_verification/identity_registry_storage/storage.rs`:
- Around line 83-109: The example implements
CountryDataManager::add_country_data_entries with the old Vec<CountryData>
signature; update the function to accept Vec<Val> (matching the trait) and
convert each Val into CountryData before duplicate-checking and forwarding to
the helper. Specifically, change the parameter type to Vec<Val>, map or iterate
over those Vals converting with the appropriate conversion helper (e.g.,
CountryData::try_from_val or Val::try_into::<CountryData>() / from_val pattern
used in this crate), handle conversion failures by returning or panicking with
the crate's error macro (e.g., panic_with_error!(e, Error::InvalidInput)),
perform the existing duplicate country checks on the converted CountryData
items, and pass a reference to the converted Vec<CountryData> into
rwa::identity_verification::identity_registry_storage::add_country_data_entries.
🪄 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: 157dd887-382c-49e9-aa46-561e7da05e91
📒 Files selected for processing (3)
packages/tokens/src/rwa/identity_verification/identity_registry_storage/mod.rspackages/tokens/src/rwa/identity_verification/identity_registry_storage/storage.rspackages/tokens/src/rwa/identity_verification/storage.rs
packages/tokens/src/rwa/identity_verification/identity_registry_storage/mod.rs
Outdated
Show resolved
Hide resolved
| //! Because the trait accepts `Val` for country data, implementors can define | ||
| //! their own privacy-preserving types while keeping the same contract | ||
| //! interface. Below are examples of alternative country data types: |
There was a problem hiding this comment.
I think we can expand the examples below. They are defining some struct with #[contracttype], and nothing else is shown on the examples. We should explain/show how this defined struct is getting connected to the trait.
3 participants
PR Checklist
Summary by CodeRabbit