Improve UnwrapOrRevert interface #493
Conversation
* Add Revertible trait * unwrap_or_revert accepts revertible - reduces ContractEnv cloning
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (2)
WalkthroughThe codebase was primarily refactored to introduce a new Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Benchmark report
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- core/src/contract_env.rs (4 hunks)
- core/src/list.rs (4 hunks)
- core/src/mapping.rs (4 hunks)
- core/src/module.rs (2 hunks)
- core/src/named_keys.rs (3 hunks)
- core/src/sequence.rs (3 hunks)
- core/src/unwrap_or_revert.rs (1 hunks)
- core/src/var.rs (5 hunks)
- examples/src/features/cross_calls.rs (1 hunks)
- examples/src/features/livenet.rs (1 hunks)
- examples/src/features/module_nesting.rs (1 hunks)
- modules/src/cep18/storage.rs (2 hunks)
- modules/src/cep18_token.rs (3 hunks)
- modules/src/cep78/metadata.rs (2 hunks)
- modules/src/cep78/reverse_lookup.rs (2 hunks)
- modules/src/cep78/token.rs (1 hunks)
- modules/src/wrapped_native.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- modules/src/cep78/token.rs
Additional comments not posted (31)
core/src/unwrap_or_revert.rs (2)
9-9: The changes tounwrap_or_revert_witheffectively integrate theRevertibletrait, enhancing the flexibility and reusability of error handling across different contexts.Also applies to: 16-17, 26-27
12-12: The update tounwrap_or_reverteffectively utilizes theRevertibletrait, simplifying the interface and reducing dependencies on specific environment instances.Also applies to: 20-21, 30-31
examples/src/features/cross_calls.rs (1)
21-21: The modification to useselfdirectly in theunwrap_or_revertcall streamlines the interaction with theRevertibletrait, enhancing code clarity and error handling consistency.examples/src/features/livenet.rs (1)
43-43: The update to theunwrap_or_revertcall inpop_from_stackto useselfinstead of&self.env()aligns with the newRevertibletrait usage, enhancing code simplicity and reducing unnecessary references.core/src/sequence.rs (1)
25-31: The implementation of theRevertibletrait forSequence<T>is correctly done, utilizing the existingenv.revertmethod to handle errors, which maintains consistency and leverages existing functionality.core/src/var.rs (5)
4-10: Consolidation of imports enhances readability and maintainability.
20-24: Implementation ofRevertibleforVar<T>correctly delegates therevertaction toContractEnv. This aligns with the PR's goal of reducing the need for cloningContractEnv.
60-60: The use ofselfdirectly inunwrap_or_revert_withis a good application of the newRevertibletrait, reducing the need for passingContractEnvexplicitly.
87-87: Usingselfinunwrap_or_revertwithin theaddmethod follows the new trait's design and improves code clarity by removing unnecessary references.
100-100: Similarly, thesubtractmethod's use ofselfwithunwrap_or_revertis consistent and correct, adhering to the new interface design.modules/src/cep18/storage.rs (2)
55-55: The updatedunwrap_or_revertusage inCep18BalancesStorage.addmethod is a direct application of theRevertibletrait, which simplifies the error handling by eliminating the explicit reference toContractEnv.
64-64: InCep18BalancesStorage.subtract, the use ofselfwithunwrap_or_revert_withis correctly implemented and aligns with the new trait's design to simplify and standardize error handling.core/src/module.rs (2)
9-9: Inclusion ofOdraErrorin the import statement is necessary for the newRevertibletrait implementation.
30-46: The introduction of theRevertibletrait and its implementation forRc<ContractEnv>andT: Moduleare central to this PR's objectives. The implementation correctly uses the environment'srevertmethod, which should ensure consistent error handling across modules.core/src/mapping.rs (3)
6-12: The updated imports and inclusion ofRevertiblein the import list are necessary for the new functionality introduced in this file.
33-37: Implementation ofRevertibleforMapping<K, V>correctly delegates the revert responsibility toContractEnv, which is consistent with the trait's usage in other parts of the codebase.
Line range hint
93-113: The use ofselfinunwrap_or_revertwithin theaddandsubtractmethods ofMapping<K, V>is correctly implemented. It leverages theRevertibletrait to handle errors directly, enhancing code clarity and reducing dependencies.core/src/named_keys.rs (2)
19-19: Change aligns with PR objectives to utilizeRevertiblefor error handling. Consider adding documentation on howRevertibleis used here for clarity.
72-88: Refactoring to useRevertiblein thekeymethod enhances error handling. Ensure thatbase64::prelude::EngineandBASE64_STANDARDare the most efficient choices for this task.examples/src/features/module_nesting.rs (1)
64-64: Change to useselfdirectly inunwrap_or_revertaligns with PR objectives to reduceContractEnvcloning. Check all related method calls across the codebase for consistency.modules/src/wrapped_native.rs (1)
31-31: Change to useselfdirectly inunwrap_or_revertaligns with PR objectives. Consider adding a comment explaining whyselfis used here for future maintainers.core/src/list.rs (1)
Line range hint
80-95: Refactoring to useRevertibleinreplaceandpopmethods simplifies error handling. Verify the behavior with unit tests to ensure correct functionality.Verification successful
The search results indicate that there are indeed unit tests covering both the
replaceandpopmethods in the codebase. Therefore, the refactoring to use theRevertibletrait for error handling in these methods should be verified through these existing tests.
replacemethod is tested infn test_replace().popmethod is tested infn test_pop().These tests will ensure that the new error handling paths are correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure unit tests cover the new error handling paths in `replace` and `pop` methods. # Test: Search for unit tests related to `replace` and `pop`. rg --type rust $'List.*(replace|pop).*test'Length of output: 44
Script:
#!/bin/bash # Description: Search for direct usage of `replace` and `pop` methods in test files. # Find all test files and search for `replace` method. fd --type f --extension rs --exec rg 'replace' {} # Find all test files and search for `pop` method. fd --type f --extension rs --exec rg 'pop' {}Length of output: 2099
modules/src/cep78/reverse_lookup.rs (2)
90-90: Update to useunwrap_or_revertwith self-reference is consistent with PR goals.
239-239: Ensure the error handling logic withunwrap_or_revertis tested thoroughly as it directly impacts the robustness of the contract.core/src/contract_env.rs (2)
6-6: The import ofRevertibleis aligned with the PR's objective to enhance error handling by leveraging the new trait.
309-314: Proper error handling inget_named_argmethod is crucial. Ensure that the deserialization errors are correctly converted toOdraErrorand tested.modules/src/cep78/metadata.rs (2)
103-103: The use ofunwrap_or_reverthere is appropriate to ensure that the contract fails early if the JSON schema is invalid. This aligns with the PR’s goals of robust error handling.
304-304: Ensure that the custom validated metadata schema parsing is covered by unit tests to prevent runtime errors.Verification successful
Ensure that the custom validated metadata schema parsing is covered by unit tests to prevent runtime errors.
- The
CustomMetadataSchemais referenced inmodules/src/cep78/tests/mod.rs, indicating that it is indeed covered by tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify unit tests cover JSON schema parsing in `metadata.rs`. # Test: Search for tests covering JSON schema parsing in `metadata.rs`. rg --type rust 'test' -A 20 modules/src/cep78/metadata.rs | rg 'from_str::<CustomMetadataSchema>'Length of output: 97
Script:
#!/bin/bash # Description: Check for unit tests in `metadata.rs`. # Search for unit test definitions in `metadata.rs`. rg --type rust '#\[cfg\(test\)\]' -A 10 modules/src/cep78/metadata.rsLength of output: 71
Script:
#!/bin/bash # Description: Check for unit tests in the broader codebase. # Search for unit test definitions across the codebase. rg --type rust '#\[cfg\(test\)\]' -A 10Length of output: 51831
Script:
#!/bin/bash # Description: Check for `CustomMetadataSchema` usage in test files. # Search for `CustomMetadataSchema` in test files. rg 'CustomMetadataSchema' --type rust --glob 'tests/**/*.rs'Length of output: 209
Script:
#!/bin/bash # Description: Check for `CustomMetadataSchema` usage in all Rust files. # Search for `CustomMetadataSchema` in all Rust files. rg 'CustomMetadataSchema' --type rustLength of output: 992
modules/src/cep18_token.rs (3)
99-99: The update to useunwrap_or_revert_withwith self-reference improves the readability and consistency of error handling across the contract.
237-237: Usingunwrap_or_revert_withfor handling insufficient allowance errors ensures that the contract's behavior is predictable and secure. This is crucial for maintaining trust in the token's operations.
257-257: The use ofunwrap_or_revert_within themintfunction aligns with the PR’s objectives and ensures that only authorized users can mint new tokens.
| impl<T> Revertible for List<T> { | ||
| fn revert<E: Into<OdraError>>(&self, e: E) -> ! { | ||
| self.env.revert(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The recent shell script results indicate that there are no direct tests for the revert method associated with List and no specific references to List in the test files. However, the revert method is widely used across the codebase, suggesting its importance in error handling.
To ensure the robustness of the List implementation, it is crucial to add unit tests specifically targeting the revert method to cover possible error scenarios.
- Ensure unit tests cover scenarios where
List::revertmight be triggered. - Add specific tests for the
revertmethod inListto ensure its functionality and reliability.
Analysis chain
Implementation of Revertible for List enhances error handling. Ensure unit tests cover scenarios where revert might be triggered.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify unit tests cover the new `revert` method in `List`.
# Test: Search for unit tests related to `List` and `revert`.
rg --type rust $'List.*revert.*test'
Length of output: 37
Script:
#!/bin/bash
# Description: Check for any usage of the `revert` method for `List` in the codebase.
# Search for the usage of the `revert` method in the codebase.
rg --type rust 'List.*\.revert\('
Length of output: 35
Script:
#!/bin/bash
# Description: Check for any existing tests for the `List` structure.
# Search for tests related to the `List` structure.
rg --type rust 'List.*test'
Length of output: 29
Script:
#!/bin/bash
# Description: Check for any usage of the `revert` method for `List` in the codebase.
# Search for the usage of the `revert` method in the codebase.
rg --type rust 'revert\('
Length of output: 22307
Script:
#!/bin/bash
# Description: Check for any existing tests for the `List` structure.
# List all test files to manually inspect for any tests related to `List`.
fd --type f --extension rs --exec rg --files-with-matches 'test'
Length of output: 4054
Benchmark report
|
kpob
deleted the
feature/unwrap_or_revert_with-should-accept-module-as-argument
branch
Summary by CodeRabbit
New Features
Revertibletrait with arevertmethod to handle contract execution reversals.Bug Fixes
unwrap_or_revertmethods to improve reliability.Refactor
&self.env()and instead useselffor better consistency and cleaner code.Documentation
Revertibletrait and associated method changes.