feat: blocked addrs #73
Conversation
|
Warning Rate limit exceeded@beer-1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 50 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update introduces a new boolean parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GoAPI
participant AccountService
participant Database
User->>GoAPI: Request Account Info
GoAPI->>AccountService: GetAccountInfo(address)
AccountService->>Database: FetchAccountData(address)
Database-->>AccountService: AccountData(found, account_number, sequence, account_type, is_blocked)
AccountService-->>GoAPI: Return(found, account_number, sequence, account_type, is_blocked)
GoAPI-->>User: Response(found, account_number, sequence, account_type, is_blocked)
sequenceDiagram
participant Admin
participant BlockService
participant AccountService
participant Database
Admin->>BlockService: BlockAccount(address)
BlockService->>Database: UpdateAccountStatus(address, is_blocked=true)
Database-->>BlockService: Confirmation
BlockService-->>Admin: Account Blocked
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/bindings.h (1 hunks)
- api/callbacks.go (5 hunks)
- api/callbacks_cgo.go (2 hunks)
- api/mocks.go (2 hunks)
- crates/compiler/src/mocks.rs (1 hunks)
- crates/e2e-move-tests/src/test_utils/mock_chain.rs (4 hunks)
- crates/natives/src/account.rs (8 hunks)
- libmovevm/bindings.h (1 hunks)
- libmovevm/src/api.rs (5 hunks)
- precompile/modules/initia_stdlib/sources/account.move (7 hunks)
- precompile/modules/initia_stdlib/sources/coin.move (3 hunks)
- precompile/modules/initia_stdlib/sources/fa/fungible_asset.move (9 hunks)
- precompile/modules/initia_stdlib/sources/fa/primary_fungible_store.move (2 hunks)
- precompile/modules/initia_stdlib/sources/managed_coin.move (1 hunks)
- precompile/modules/minitia_stdlib/sources/account.move (7 hunks)
- precompile/modules/minitia_stdlib/sources/coin.move (3 hunks)
- precompile/modules/minitia_stdlib/sources/fa/fungible_asset.move (9 hunks)
- precompile/modules/minitia_stdlib/sources/fa/primary_fungible_store.move (2 hunks)
- precompile/modules/minitia_stdlib/sources/managed_coin.move (1 hunks)
Files not reviewed due to errors (2)
- precompile/modules/initia_stdlib/sources/account.move (no review received)
- precompile/modules/initia_stdlib/sources/coin.move (no review received)
Additional comments not posted (32)
crates/compiler/src/mocks.rs (1)
66-68: The addition ofis_blockedto theget_account_infofunction aligns with the PR's objectives. Consider adding a comment explaining the significance of theis_blockedvalue in this context.api/callbacks_cgo.go (1)
14-14: The addition ofis_blockedtocGetAccountInfoand its gateway function is crucial for supporting the new account block status feature. Consider adding documentation for theis_blockedparameter to clarify its usage and impact.Also applies to: 45-46
precompile/modules/initia_stdlib/sources/managed_coin.move (1)
41-68: The implementation ofsudo_mintandcheck_sudofunctions are well-constructed with necessary security checks. Consider enhancing the documentation to explain the security checks and the conditions under which these functions should be used.precompile/modules/minitia_stdlib/sources/managed_coin.move (1)
45-48: Ensure proper access control for thecheck_sudofunction.api/bindings.h (1)
195-195: The addition of theis_blockedparameter toget_account_infoin the API bindings is correctly implemented. Ensure that all callers of this function are updated to handle this new parameter.libmovevm/bindings.h (1)
195-195: This change appears to be a duplicate of the one inapi/bindings.h. Ensure there is no duplication of functionality or conflicting implementations between these files.libmovevm/src/api.rs (1)
37-37: The integration of theis_blockedparameter in theget_account_infofunction is well-implemented. Ensure that the handling of this parameter is consistent across all API functions and that error handling is robust.Also applies to: 106-106
Verification successful
The integration of the
is_blockedparameter is consistent across all relevant files and functions.
libmovevm/src/api.rscrates/natives/src/account.rscrates/e2e-move-tests/src/test_utils/mock_chain.rscrates/compiler/src/mocks.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent handling of `is_blocked` across all API functions. # Test: Search for `is_blocked` usage in API functions. Expect: Consistent handling. rg --type rust $'is_blocked'Length of output: 1798
crates/natives/src/account.rs (5)
38-38: Addition ofis_blockedparameter toAccountAPItrait.This change enables the system to handle account status checks directly within the account info retrieval, enhancing security and control flows.
53-56: Expansion oftest_accountsinNativeAccountContextto includeis_blocked.This extension is necessary for accurately testing the handling of blocked accounts within the system.
89-92: Methodset_account_infonow handlesis_blocked.Proper handling and testing of the blocked status of accounts are crucial, and this change facilitates that.
Line range hint
121-157: Updatednative_get_account_infoto handleis_blocked.The inclusion of
is_blockedin account information retrieval aligns with the new requirements and enhances the system's capability to manage account statuses effectively.
Line range hint
300-315: Enhancement ofnative_test_only_set_account_infoto includeis_blocked.This change is vital for testing environments to accurately reflect the new account blocking functionality.
precompile/modules/minitia_stdlib/sources/account.move (2)
22-27: Addition ofis_blockedtoAccountInfostruct.This change is crucial for integrating the account blocking functionality into the system, enhancing security and control mechanisms.
34-35: Updated account creation functions to consideris_blocked.These updates are necessary to ensure that new accounts respect the blocked status and that the system's integrity is maintained.
Also applies to: 43-46, 58-58
precompile/modules/minitia_stdlib/sources/coin.move (3)
7-8: New imports for error handling and signer operations are correctly added.
23-24: The addition ofminitia_std::managed_coinas a friend module supports new functionalities.
54-79: The additions ofEUNAUTHORIZED,sudo_transfer, andsudo_depositcorrectly implement the new sudo functionalities for secure coin management.crates/e2e-move-tests/src/test_utils/mock_chain.rs (2)
258-258: Addition ofis_blockedparameter toget_account_infoenhances testing capabilities to reflect new account status features.
Line range hint
309-337: UpdatedMockAccountAPIstructure and methods to includeis_blockedstatus, correctly supporting new account management features.api/callbacks.go (1)
16-16: Correctly addedis_blockedparameter to API function signatures to handle new account status checks.Also applies to: 31-31, 363-399
precompile/modules/initia_stdlib/sources/fa/primary_fungible_store.move (1)
39-39: Addedinitia_std::coinas a friend module to allow access to restricted functions. Ensure this change is aligned with the security model, as it grants significant privileges.precompile/modules/minitia_stdlib/sources/fa/primary_fungible_store.move (3)
39-39: Friend declaration forminitia_std::coinis appropriate for accessing restricted functions in thecoinmodule.
260-272: Ensure proper permissions for thesudo_depositfunction.Verification successful
Ensure proper permissions for the
sudo_depositfunction.
- The
sudo_depositfunction is marked aspublic(friend), which restricts its usage to specific modules within the same package. This visibility ensures that only trusted modules can call this function.- The search results show that
sudo_depositis only called within the expected contexts in the Move files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `sudo_deposit` function can only be called by the chain. # Test: Search for calls to `sudo_deposit` outside of expected contexts. rg --type move $'sudo_deposit'Length of output: 58
Script:
#!/bin/bash # Description: Verify that the `sudo_deposit` function can only be called by the chain. # Test: Search for calls to `sudo_deposit` within Move files. rg 'sudo_deposit' --glob '*.move'Length of output: 1864
274-294: The implementation ofsudo_transferappears correct but ensure that it can only be called by the chain.precompile/modules/initia_stdlib/sources/fa/fungible_asset.move (4)
63-64: The introduction of the constantECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNTis aligned with the PR's objective to handle blocked accounts.
601-607: Repeating the logic to prevent deposits to blocked accounts indeposit_with_refensures consistency across different deposit methods. Double-check that this does not introduce redundancy that could be refactored.
[REFACTOR_SUGGESTion]
Consider abstracting the blocked account check into a separate function to avoid code duplication across different deposit functions.
457-462: The addition of a check to prevent deposits to blocked accounts is crucial for security and compliance. Ensure that the functionis_blocked_store_addris correctly implemented and tested.
637-647: Similarly,sudo_depositallows for administrative control over deposits, which is a powerful feature that needs to be tightly controlled.Verify that this function can only be called by authorized entities to prevent potential misuse.
Verification successful
The
sudo_depositfunction is defined with thepublic(friend)visibility modifier in multiple files, which restricts its access to modules within the same address space. This visibility ensures that only authorized modules can call this function.
precompile/modules/initia_stdlib/sources/coin.moveprecompile/modules/minitia_stdlib/sources/coin.moveprecompile/modules/minitia_stdlib/sources/fa/primary_fungible_store.moveprecompile/modules/minitia_stdlib/sources/fa/fungible_asset.moveprecompile/modules/initia_stdlib/sources/fa/primary_fungible_store.moveprecompile/modules/initia_stdlib/sources/fa/fungible_asset.moveGiven this context, the
public(friend)visibility modifier already ensures that only authorized entities within the same address space can callsudo_deposit.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `sudo_deposit` can only be called by authorized entities. # Test: Search for permission checks around the usage of `sudo_deposit`. rg --type move $'sudo_deposit'Length of output: 58
Script:
#!/bin/bash # Description: Verify that `sudo_deposit` can only be called by authorized entities. # Test: Search for the definition of `sudo_deposit` and its usage across the codebase. rg 'sudo_deposit' --glob '*.move'Length of output: 1864
Script:
#!/bin/bash # Description: Extract the definitions of the `sudo_deposit` function to understand their context. # Extract the `sudo_deposit` function definitions from the identified files. ast-grep --lang move --pattern 'public(friend) fun sudo_deposit($_, $_) { $$$ }' --jsonLength of output: 198
Script:
#!/bin/bash # Description: Search for the `sudo_deposit` function definitions with the `public(friend)` visibility modifier. # Search for the `sudo_deposit` function definitions with the `public(friend)` visibility modifier. rg 'public\(friend\) fun sudo_deposit' --glob '*.move'Length of output: 1023
precompile/modules/minitia_stdlib/sources/fa/fungible_asset.move (4)
63-64: The addition of the constantECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNTis aligned with the PR's goal to handle blocked accounts. Ensure that this constant is used consistently across all relevant functions.
1102-1122: The test casetest_deposit_blocked_accountchecks the behavior of depositing into a blocked account. It's crucial to ensure that this test case and similar ones are part of a continuous integration pipeline to prevent regressions in the handling of blocked accounts.
1124-1144: Similarly,test_deposit_blocked_account_with_refshould be part of regular testing routines. This ensures that even with reference-based operations, blocked accounts are handled correctly.
623-635: Thesudo_transferfunction is introduced to allow privileged transfers by the chain. This is a critical function, and it's important to ensure that it is thoroughly tested, especially since it bypasses certain checks like frozen status.
| /// Deposit fungible asset `fa` to the given account's primary store. | ||
| /// | ||
| /// This function is only callable by the chain. | ||
| public(friend) fun sudo_deposit(owner: address, fa: FungibleAsset) acquires DeriveRefPod, ModuleStore { | ||
| let metadata = fungible_asset::asset_metadata(&fa); | ||
| let store = ensure_primary_store_exists(owner, metadata); | ||
| fungible_asset::sudo_deposit(store, fa); | ||
|
|
||
| // create cosmos side account | ||
| if (!account::exists_at(owner)) { | ||
| let _acc_num = account::create_account(owner); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The sudo_deposit function correctly implements the logic to deposit assets into an account's primary store. However, consider handling errors or exceptions that may arise from account::create_account or fungible_asset::sudo_deposit.
To improve robustness, wrap these operations in a try-catch block and handle potential exceptions appropriately.
| /// Transfer `amount` of fungible asset from sender's primary store to receiver's primary store. | ||
| /// | ||
| /// This function is only callable by the chain. | ||
| public(friend) entry fun sudo_transfer<T: key>( | ||
| sender: &signer, | ||
| metadata: Object<T>, | ||
| recipient: address, | ||
| amount: u64, | ||
| ) acquires DeriveRefPod, ModuleStore { | ||
| let sender_store = ensure_primary_store_exists( | ||
| signer::address_of(sender), | ||
| metadata | ||
| ); | ||
| let recipient_store = ensure_primary_store_exists(recipient, metadata); | ||
| fungible_asset::sudo_transfer( | ||
| sender, | ||
| sender_store, | ||
| recipient_store, | ||
| amount | ||
| ); | ||
| } |
There was a problem hiding this comment.
The sudo_transfer function is well-implemented, ensuring both sender and recipient stores exist before executing the transfer. However, consider adding logging for audit purposes, especially since this operation involves asset transfers which are critical operations.
Add logging statements before and after the transfer to facilitate tracking and auditing of these operations.
| /// Transfer an `amount` of fungible asset from `from_store`, which should be owned by `sender`, to `receiver`. | ||
| /// Note: it does not move the underlying object. | ||
| /// | ||
| /// This function is only callable by the chain. | ||
| public(friend) fun sudo_transfer<T: key>( | ||
| sender: &signer, | ||
| from: Object<T>, | ||
| to: Object<T>, | ||
| amount: u64, | ||
| ) acquires FungibleStore { | ||
| let fa = withdraw(sender, from, amount); | ||
| sudo_deposit(to, fa); | ||
| } |
There was a problem hiding this comment.
The introduction of sudo_transfer as a chain-specific administrative function is a significant addition. It allows bypassing certain checks which could be dangerous if not properly secured.
Ensure that appropriate permissions checks are in place to prevent unauthorized use of this function.
| // cannot deposit to blocked account | ||
| let store_addr = object::object_address(store); | ||
| assert!( | ||
| !is_blocked_store_addr(store_addr), | ||
| error::invalid_argument(ECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNT) | ||
| ); |
There was a problem hiding this comment.
The logic to prevent deposits to blocked accounts is correctly implemented. However, consider adding a detailed comment explaining the is_blocked_store_addr function for clarity.
+ // Check if the store address is associated with a blocked account
assert!(
!is_blocked_store_addr(store_addr),
error::invalid_argument(ECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNT)
);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // cannot deposit to blocked account | |
| let store_addr = object::object_address(store); | |
| assert!( | |
| !is_blocked_store_addr(store_addr), | |
| error::invalid_argument(ECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNT) | |
| ); | |
| // cannot deposit to blocked account | |
| let store_addr = object::object_address(store); | |
| // Check if the store address is associated with a blocked account | |
| assert!( | |
| !is_blocked_store_addr(store_addr), | |
| error::invalid_argument(ECANNOT_DEPOSIT_TO_BLOCKED_ACCOUNT) | |
| ); |
| @@ -118,7 +118,7 @@ func (m MockAPI) Query(request types.QueryRequest, gasBalance uint64) ([]byte, u | |||
| return m.QueryAPI.Query(request, gasBalance) | |||
| } | |||
|
|
|||
| func (m MockAPI) GetAccountInfo(addr types.AccountAddress) (bool, uint64, uint64, uint8) { | |||
| func (m MockAPI) GetAccountInfo(addr types.AccountAddress) (bool, uint64, uint64, uint8, bool) { | |||
There was a problem hiding this comment.
The updated return values of GetAccountInfo correctly include the is_blocked status. Ensure there are unit tests covering scenarios where an account is blocked and not blocked.
Would you like me to help create these unit tests?
Also applies to: 156-162
| /// Deposit `amount` of the fungible asset to `store`. | ||
| /// | ||
| /// This function is only callable by the chain. | ||
| public(friend) fun sudo_deposit<T: key>(store: Object<T>, fa: FungibleAsset) acquires FungibleStore { | ||
| assert!( | ||
| !is_frozen(store), | ||
| error::invalid_argument(ESTORE_IS_FROZEN) | ||
| ); | ||
|
|
||
| deposit_internal(store, fa); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search did not return any results indicating that the sudo_deposit function is covered by tests. This suggests that there may be insufficient test coverage for this function.
- Ensure that the
sudo_depositfunction is covered by comprehensive tests.
Analysis chain
The sudo_deposit function complements sudo_transfer by allowing deposits in a privileged context. Ensure that this function is also covered by comprehensive tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify thorough testing of `sudo_deposit` function.
# Test: Search for test cases involving `sudo_deposit`. Expect: Multiple test cases covering various scenarios.
rg --type move $'sudo_deposit'
Length of output: 58
Script:
#!/bin/bash
# Description: Verify thorough testing of `sudo_deposit` function.
# Test: Search for test cases involving `sudo_deposit`. Expect: Multiple test cases covering various scenarios.
rg 'sudo_deposit'
Length of output: 1848
Script:
#!/bin/bash
# Description: Verify thorough testing of `sudo_deposit` function.
# Step 1: Identify test files (assuming test files contain 'test' in their filename or are in a 'tests' directory)
test_files=$(fd --type f --name '*test*.move' || fd --type f --glob 'tests/**/*.move')
# Step 2: Search for `sudo_deposit` within identified test files
if [ -n "$test_files" ]; then
rg 'sudo_deposit' $test_files
else
echo "No test files found."
fi
Length of output: 407
Script:
#!/bin/bash
# Description: Verify thorough testing of `sudo_deposit` function.
# Step 1: Identify test files (assuming test files contain 'test' in their filename or are in a 'tests' directory)
test_files=$(fd --type f --glob '**/*test*.move' || fd --type f --glob 'tests/**/*.move')
# Step 2: Search for `sudo_deposit` within identified test files
if [ -n "$test_files" ]; then
rg 'sudo_deposit' $test_files
else
echo "No test files found."
fi
Length of output: 3002
| public entry fun sudo_mint( | ||
| account: &signer, | ||
| dst_addr: address, | ||
| metadata: Object<Metadata>, | ||
| amount: u64, | ||
| ) acquires Capabilities { | ||
| check_sudo(account); | ||
|
|
||
| let account_addr = signer::address_of(account); | ||
| assert!(object::is_owner(metadata, account_addr), error::not_found(EUNAUTHORIZED),); | ||
|
|
||
| let object_addr = object::object_address(metadata); | ||
| assert!(exists<Capabilities>(object_addr), error::not_found(ENO_CAPABILITIES),); | ||
|
|
||
| let capabilities = borrow_global<Capabilities>(object_addr); | ||
| let fa = coin::mint(&capabilities.mint_cap, amount); | ||
| coin::sudo_deposit(dst_addr, fa); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that there are no direct or indirect test cases covering the sudo_mint function in the codebase. Comprehensive testing, especially for error paths, is essential to ensure the robustness of this function.
- Please add test cases for the
sudo_mintfunction, covering both successful and error scenarios.
Analysis chain
The implementation of sudo_mint appears correct and adheres to the principles of access control and resource management. However, ensure comprehensive testing, especially for error paths.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify comprehensive testing for `sudo_mint`.
# Test: Search for test cases involving `sudo_mint`. Expect: Tests covering both successful and error paths.
rg --type move $'sudo_mint'
Length of output: 55
Script:
#!/bin/bash
# Locate all .move files to understand the directory structure.
fd --extension move
Length of output: 7985
Script:
#!/bin/bash
# Search for test cases involving `sudo_mint` in identified .move files.
rg 'sudo_mint' precompile/modules/tests/sources/
Length of output: 48
Script:
#!/bin/bash
# Search for references to `sudo_mint` in all .move files to identify any indirect test coverage.
rg 'sudo_mint' --glob '**/*.move'
Length of output: 216
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- precompile/modules/initia_stdlib/sources/coin.move (3 hunks)
- precompile/modules/initia_stdlib/sources/staking.move (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- precompile/modules/initia_stdlib/sources/coin.move
Additional comments not posted (2)
precompile/modules/initia_stdlib/sources/staking.move (2)
678-679: Consider using a well-defined constant for the module address instead of hardcoding@relayer.
[REFACTOR_SUGGESTion]
Using a constant for addresses like@relayerincreases code maintainability and reduces the risk of errors if the address needs to be changed in the future. You could define a constant at the beginning of your module:const RELAYER_ADDRESS: address = @relayer;Then replace
@relayerwithRELAYER_ADDRESSthroughout your code.
908-910: Ensure that the use ofsudo_depositadheres to security best practices.The
sudo_depositfunction appears to perform a critical operation related to asset transfer. It's crucial to ensure that this operation is secured against unauthorized access. Consider implementing additional checks to verify that the@relayeraddress has the appropriate permissions to perform this operation.
is_blockedSummary by CodeRabbit
New Features
is_blockedparameter to account information retrieval, adding the ability to check if an account is blocked.sudo_transfer,sudo_deposit,sudo_mint) in coin and fungible asset modules, with associated permission checks.Enhancements
AccountInfoobjects.is_blockedstatus, improving account management and security.Bug Fixes
These updates enhance account management, introduce new administrative functions, and improve security by handling blocked accounts appropriately.