feat: validate token name localizations #2468
Conversation
WalkthroughThis change introduces localization support and validation for token configurations in the data contract module. A new accessor module (with a Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/accessors/v0/mod.rs (1)
1-7: LGTM! Consider enhancing the documentation.The trait is well-designed with clear method signatures and consistent naming patterns. The methods appropriately handle both singular and plural forms with default language fallback.
Consider adding documentation to clarify:
- What is the default language code?
- What happens when the requested language code is not found?
/// Accessor trait for getters of `TokenConfigurationConventionV0` pub trait TokenConfigurationConventionV0Getters { - /// Returns the localized token name in singular form + /// Returns the localized token name in singular form. + /// If the requested language code is not found, falls back to English ('en'). fn singular_form_by_language_code_or_default(&self, language_code: &str) -> &str; - /// Returns the localized token name in plural form + /// Returns the localized token name in plural form. + /// If the requested language code is not found, falls back to English ('en'). fn plural_form_by_language_code_or_default(&self, language_code: &str) -> &str; }packages/rs-dpp/src/errors/consensus/basic/token/missing_default_localization.rs (1)
1-24: LGTM! Consider adding documentation.The error type is well-designed with appropriate trait implementations and clear error message.
Consider adding documentation to explain the error's purpose and when it's raised:
+/// Error raised when a token configuration is missing the required English ('en') localization. +/// This error occurs during validation of token configurations to ensure that a default localization +/// is always available. #[derive( Error, Debug, Clone, PartialEq, Eq, Encode, Decode, PlatformSerialize, PlatformDeserialize, )] #[error("Missing english ('en') localization which is using by default")] #[platform_serialize(unversioned)] pub struct MissingDefaultLocalizationError {}packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/validate_localizations/mod.rs (1)
9-26: LGTM! Consider adding documentation.The method is well-structured with proper version handling and error management.
Consider adding documentation to explain the method's purpose and return values:
+ /// Validates the localizations of a token configuration based on the platform version. + /// + /// # Arguments + /// * `platform_version` - The platform version to determine which validation rules to apply + /// + /// # Returns + /// * `Ok(SimpleConsensusValidationResult)` - The validation result + /// * `Err(ProtocolError)` - If the platform version is not supported pub fn validate_localizations( &self, platform_version: &PlatformVersion, ) -> Result<SimpleConsensusValidationResult, ProtocolError> {packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/validate_localizations/v0/mod.rs (1)
5-22: Consider enhancing the error message.The implementation correctly validates the presence of English localization. However, consider adding a more descriptive error message to help users understand why English localization is required.
- MissingDefaultLocalizationError::new().into(), + MissingDefaultLocalizationError::new() + .with_message("English (en) localization is required as the default language") + .into(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/accessors/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/accessors/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/validate_localizations/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/validate_localizations/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/v0/mod.rs(3 hunks)packages/rs-dpp/src/errors/consensus/basic/basic_error.rs(2 hunks)packages/rs-dpp/src/errors/consensus/basic/token/missing_default_localization.rs(1 hunks)packages/rs-dpp/src/errors/consensus/basic/token/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v2.rs(1 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/mod.rs
- packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (13)
packages/rs-dpp/src/errors/consensus/basic/token/mod.rs (1)
7-7: LGTM!The new module and its public export are correctly added in alphabetical order, maintaining the file's organization.
Also applies to: 16-16
packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/methods/validate_localizations/v0/mod.rs (1)
1-4: LGTM!The imports are correctly organized and include all necessary dependencies.
packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/accessors/mod.rs (2)
1-8: LGTM!The module structure and imports are well-organized, with clear separation of concerns.
9-25:❓ Verification inconclusive
Consider handling potential panic cases.
The implementation correctly delegates to v0 methods, but consider handling cases where the requested language code doesn't exist to prevent potential panics.
Could you provide the implementation of
singular_form_by_language_code_or_defaultandplural_form_by_language_code_or_defaultfrom the v0 module to ensure they handle missing language codes gracefully?
🏁 Script executed:
#!/bin/bash # Description: Check implementation of getter methods in v0 module ast-grep --pattern $'impl TokenConfigurationConventionV0 { $$$ fn singular_form_by_language_code_or_default($$$) { $$$ } $$$ fn plural_form_by_language_code_or_default($$$) { $$$ } $$$ }'Length of output: 210
Manual Verification Required for v0 Getter Implementations
The current delegation in the
TokenConfigurationConventionV0Gettersimplementation passes calls directly to the v0 methods without any additional safeguard. However, we couldn’t confirm from an automated search whether the underlying implementations ofsingular_form_by_language_code_or_defaultandplural_form_by_language_code_or_defaultin the v0 module handle cases of missing language codes gracefully.Please manually verify that in the v0 module:
- When an unknown
language_codeis provided, the implementations either return a proper default value without panicking or have explicit error handling.- If no such handling exists, consider refactoring these methods to ensure they do not trigger panics when the requested language code is missing.
packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v2.rs (1)
22-22: LGTM!The addition of
validate_localizationsfield with initial version 0 is consistent across v1 and v2, correctly reflecting the introduction of localization validation as a breaking change.packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v1.rs (1)
22-22: LGTM!The addition of
validate_localizationsfield with initial version 0 is consistent across v1 and v2, correctly reflecting the introduction of localization validation as a breaking change.packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/mod.rs (1)
24-24: LGTM!The new field
validate_localizationsis correctly added to theDataContractValidationVersionsstruct, following the same pattern as other validation fields.packages/rs-dpp/src/data_contract/associated_token/token_configuration_convention/v0/mod.rs (3)
26-26: LGTM!The constant
ENGLISH_ISO_639is well-named and uses the standard ISO 639 language code.
33-36: LGTM!The
localizationsfield is well-designed with:
- Ordered storage using
BTreeMap- Clear documentation specifying ISO 639 2-chars language code requirement
- Default attribute for serde
63-77: LGTM!The implementation of
TokenConfigurationConventionV0Gettersis efficient and idiomatic:
- Uses references to avoid unnecessary cloning
- Properly handles missing language codes by defaulting to English
- Uses
mapandunwrap_or_elsefor clean error handlingpackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs (1)
121-133: LGTM!The localization validation is well-integrated:
- Follows the existing pattern of validation checks
- Properly handles validation errors through
ConsensusValidationResult- Correctly integrates with the bump identity nonce action
packages/rs-dpp/src/errors/consensus/basic/basic_error.rs (1)
473-474: LGTM!The
MissingDefaultLocalizationErrorvariant is well-integrated:
- Follows the existing pattern of error variants
- Uses transparent error propagation
- Is placed at a logical position in the enum
packages/wasm-dpp/src/errors/consensus/consensus_error.rs (1)
69-69: LGTM! Error handling for token localization validation is properly implemented.The addition of
MissingDefaultLocalizationErroris well-integrated into the error handling system and aligns with the PR's objective to enforce token name localization requirements. The implementation follows the established error handling patterns and is correctly placed within the token-related error section.Also applies to: 693-695
Issue being fixed or feature implemented
Currently, you can create a token without name. Also, there is no standard locatization defined.
What was done?
en) localization is presentHow Has This Been Tested?
Updated existing tests
Breaking Changes
It's a breaking change for the
v2.0-devbranchChecklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Chores