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

chore: upgrade revm dependency #1074

Merged
merged 1 commit into from
Jun 11, 2024
Merged

chore: upgrade revm dependency #1074

merged 1 commit into from
Jun 11, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner June 11, 2024 20:49
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

2

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

API Changes:
The PR includes changes that might not be backward compatible such as the removal of CreateScheme::Create and the change in method invocation from take_instruction_table().unwrap() to take_instruction_table(). Ensure that these changes are compatible with the rest of the codebase and that they do not introduce any bugs.

Method Invocation Change:
The change from value.bytecode.0.into() to value.bytecode().clone().into() in src/eth/primitives/bytes.rs might introduce unnecessary cloning, impacting performance. Verify if cloning is indeed necessary or if a reference could be used instead.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add error handling for potential None value from take_instruction_table

Consider handling the case where handler.take_instruction_table() returns None.
Previously, there was an unwrap() which would panic if None was encountered. If this
behavior is still desired, explicitly handle the None case to avoid silent failures or
unexpected behavior.

src/eth/evm/revm.rs [85]

-let instructions = handler.take_instruction_table();
+let instructions = handler.take_instruction_table().expect("Expected instruction table not found");
 
Suggestion importance[1-10]: 9

Why: This suggestion addresses a potential bug by ensuring that the code explicitly handles the case where handler.take_instruction_table() returns None, preventing silent failures or unexpected behavior.

9
Performance
Optimize the conversion from RevmBytecode to Bytes to avoid unnecessary cloning

Ensure that the cloning of bytecode() in the conversion from RevmBytecode to Bytes is
necessary. If the bytecode data is large, this could lead to performance issues. Consider
using a reference or improving the data handling to avoid unnecessary data cloning.

src/eth/primitives/bytes.rs [84]

-Self(value.bytecode().clone().into())
+Self(value.bytecode().as_ref().into())  # Use as_ref to avoid cloning if possible
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a potential performance issue by recommending the use of references instead of cloning, which can be beneficial if the bytecode data is large.

8
Possible issue
Verify and adjust the transaction creation behavior if necessary

Since CreateScheme::Create was removed and replaced with TransactTo::Create, ensure that
this change aligns with the expected behavior in your transaction environment setup. If
different types of creation schemes are still needed, consider reintroducing an
enumeration or handling different scenarios explicitly.

src/eth/evm/revm.rs [132]

-None => TransactTo::Create,
+None => TransactTo::Create, // Ensure this matches the intended transaction creation behavior
 
Suggestion importance[1-10]: 7

Why: This suggestion is important for verifying that the change in transaction creation behavior aligns with the intended functionality, but it does not provide a concrete improvement or fix.

7
Enhancement
Review and update the asm-keccak feature requirement for the new revm version

Ensure that the feature asm-keccak is still required with the upgrade to revm = "=9.0.0".
If this feature is no longer compatible or necessary, consider removing it or replacing it
with relevant features that are compatible with the new version.

Cargo.toml [112]

-revm = { version = "=9.0.0", features = ["asm-keccak"]}
+revm = { version = "=9.0.0" }  # Review necessity of "asm-keccak" feature
 
Suggestion importance[1-10]: 6

Why: This suggestion is useful for ensuring compatibility and necessity of features with the new version of the revm dependency, but it is more of a reminder to review rather than a concrete code improvement.

6

@dinhani-cw dinhani-cw merged commit eb8dd1a into main Jun 11, 2024
25 checks passed
@dinhani-cw dinhani-cw deleted the revm branch June 11, 2024 20:55
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.

1 participant