rm VMAdapter #4191
rm VMAdapter #4191
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StarcoinVM
participant TransactionProcessor
User->>StarcoinVM: Submit Transaction
StarcoinVM->>TransactionProcessor: Process Transaction
TransactionProcessor-->>StarcoinVM: Return Result
StarcoinVM-->>User: Send Transaction Output
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- vm/vm-runtime/src/parallel_executor/vm_wrapper.rs (1 hunks)
- vm/vm-runtime/src/starcoin_vm.rs (6 hunks)
- vm/vm-runtime/src/vm_adapter/mod.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- vm/vm-runtime/src/parallel_executor/vm_wrapper.rs
Additional comments not posted (8)
vm/vm-runtime/src/vm_adapter/mod.rs (4)
6-8: LGTM!The changes to the public use declarations are approved. Directly importing specific types from
starcoin_vm_typesinstead of the entireadapter_commonmodule suggests a more focused approach to managing dependencies.
10-14: LGTM!The new enum
PreprocessedTransactionis a great addition. It enhances type safety and clarity of the transaction processing logic by clearly encapsulating different transaction types.
16-26: LGTM!The new function
preprocess_transactionis a valuable addition. It improves the handling of different transaction types and streamlines the processing logic by clearly delineating how each transaction is treated.
Line range hint
1-26: Overall, the changes in this file are excellent!The modifications reflect a more structured approach to transaction management within the module, enhancing both clarity and functionality. The removal of the
adapter_commonmodule from the public use declarations, the introduction of thePreprocessedTransactionenum, and the addition of thepreprocess_transactionfunction all contribute to a cleaner and more focused design.Great work on improving the transaction processing logic!
vm/vm-runtime/src/starcoin_vm.rs (4)
81-91: LGTM!The code changes are approved.
93-102: LGTM!The code changes are approved.
1491-1513: LGTM!The code changes are approved.
1515-1526: LGTM!The code changes are approved.
| let sender = txn.sender().to_string(); | ||
| let (vm_status, output) = self.execute_user_transaction(data_cache, *txn.clone()); | ||
| // XXX FIXME YSG | ||
| // let gas_unit_price = transaction.gas_unit_price(); think about gas_used OutOfGas |
There was a problem hiding this comment.
Address the TODO comment.
The TODO comment indicates that the gas_used logic needs to be fixed to handle OutOfGas scenarios.
Do you want me to open a GitHub issue to track this task? I can also help brainstorm potential solutions if needed.
|
|
||
| pub(crate) fn should_restart_execution(output: &TransactionOutput) -> bool { | ||
| // XXX FIXME YSG if GasSchedule.move UpgradeEvent | ||
| for event in output.events() { |
There was a problem hiding this comment.
Address the TODO comment.
The TODO comment indicates that the logic to determine if execution should be restarted needs to be fixed to handle GasSchedule changes.
Do you want me to open a GitHub issue to track this task? I can also help brainstorm potential solutions if needed.
| Ok(match txn { | ||
| PreprocessedTransaction::UserTransaction(txn) => { | ||
| let sender = txn.sender().to_string(); | ||
| let (vm_status, output) = self.execute_user_transaction(data_cache, *txn.clone()); |
There was a problem hiding this comment.
Address the TODO comment.
The TODO comment indicates that the sender string logic needs to be fixed.
Do you want me to open a GitHub issue to track this task? I can also help brainstorm potential solutions if needed.
| } | ||
| PreprocessedTransaction::BlockMetadata(block_meta) => { | ||
| let (vm_status, output) = | ||
| match self.process_block_metadata(data_cache, block_meta.clone()) { |
There was a problem hiding this comment.
Address the TODO comment.
The TODO comment indicates that the match logic needs to be fixed.
Do you want me to open a GitHub issue to track this task? I can also help brainstorm potential solutions if needed.
nkysg
force-pushed
the
remove_adapter
branch
from
8d4cea0
to
9b757f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
vm/vm-runtime/src/starcoin_vm.rs (2)
1491-1513: Overall structure looks good, but address the TODO comments.The overall structure of the function, with matching on the transaction type and executing accordingly, looks good.
However, please address the following TODO comments:
- Review the gas unit price logic to handle
OutOfGasscenarios.- Fix the sender string logic.
1515-1526: Overall logic looks good, but address the TODO comment.The overall logic of the function, with checking for specific events and their creator address to determine if execution should be restarted, looks good.
However, please address the following TODO comment:
- Review and fix the logic for checking the
GasScheduleandUpgradeEvent.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- vm/vm-runtime/src/parallel_executor/vm_wrapper.rs (1 hunks)
- vm/vm-runtime/src/starcoin_vm.rs (6 hunks)
- vm/vm-runtime/src/vm_adapter/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- vm/vm-runtime/src/parallel_executor/vm_wrapper.rs
Additional comments not posted (5)
vm/vm-runtime/src/vm_adapter/mod.rs (3)
5-6: LGTM!The changes to the public use declarations are approved. Removing the
adapter_commonmodule and directly importing specific types fromstarcoin_vm_typessuggests a more focused approach to handling transaction types.
10-14: LGTM!The new enum
PreprocessedTransactionis approved. It enhances the type safety and clarity of the transaction processing logic by encapsulating theUserTransactionandBlockMetadatavariants.
16-26: LGTM!The new function
preprocess_transactionis approved. It improves the handling of different transaction types and streamlines the processing logic by clearly delineating how each transaction is treated.vm/vm-runtime/src/starcoin_vm.rs (2)
81-91: LGTM!The code changes are approved.
93-102: LGTM!The code changes are approved.
Benchmark for d5c3c19Click to view benchmark
|
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
New Features
PreprocessedTransactionenum for better type safety.StarcoinVM.Bug Fixes
Refactor
Documentation