-
Notifications
You must be signed in to change notification settings - Fork 7
feat: permissioned evm #100
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
base: main
Are you sure you want to change the base?
Conversation
|
Claude finished @tac0turtle's task —— View job Code Review CompleteI've thoroughly reviewed this PR implementing the permissioned EVM contract deployment feature. Here's my comprehensive analysis: Review Tasks:
SummaryThis PR successfully implements an optional permissioned contract deployment system with excellent code quality and comprehensive testing. The implementation is clean, well-documented, and follows Rust best practices. Code Quality ✅Strengths:
Security Analysis ✅Proper Security Measures:
Security Limitations (by design):
Performance Considerations ✅Optimizations:
Implementation Quality ✅Handler Integration (
Configuration (
Deploy Settings (
Test Coverage ✅Comprehensive Testing:
Minor Suggestions
Addressing Gemini's FeedbackThe implementation already addresses the performance concerns raised by Gemini Code Assist:
RecommendationAPPROVE - This is a high-quality implementation that:
The permissioned deployment feature is well-scoped, properly implemented, and ready for production use in scenarios requiring deployment restrictions. |
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements an optional permissioned contract deployment system for EVM chains. It allows chain operators to define an allowlist of EOAs that are permitted to deploy new contracts, providing a mechanism for more controlled chain environments. The feature is configurable via the chainspec and integrates seamlessly into the existing EVM execution flow, ensuring that only authorized top-level contract creations are processed while maintaining existing contract-to-contract interaction capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an optional permissioned system for contract deployment, which is a great feature for private or consortium chains. The implementation is well-structured, with changes cleanly plumbed through the EVM execution flow, from configuration parsing in the node to enforcement in the EVM handler. The addition of comprehensive unit and end-to-end tests is excellent and ensures the feature works as expected.
I have a couple of suggestions for performance improvements in the allowlist validation and lookup logic. These are not critical but would improve efficiency, especially as the allowlist grows.
crates/ev-revm/src/deploy.rs
Outdated
|
|
||
| /// Returns true if the caller is in the allowlist. | ||
| pub fn is_allowed(&self, caller: Address) -> bool { | ||
| self.allowlist.contains(&caller) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of is_allowed uses a linear scan (contains) on a slice, which has O(n) time complexity. With an allowlist of up to 1024 entries, this could become a performance concern as it's checked for every contract creation transaction.
Consider sorting the allowlist upon creation and using a binary search for lookups. This will improve the lookup time to O(log n).
To make this work, you'll also need to sort the allowlist in the new function:
pub fn new(mut allowlist: Vec<Address>, activation_height: u64) -> Self {
debug_assert!(!allowlist.is_empty(), "deploy allowlist must not be empty");
// The list is validated for duplicates before this is called.
allowlist.sort_unstable();
Self {
allowlist: Arc::from(allowlist),
activation_height,
}
}| self.allowlist.contains(&caller) | |
| self.allowlist.binary_search(&caller).is_ok() |
crates/node/src/config.rs
Outdated
| for i in 0..allowlist_len { | ||
| let addr = self.deploy_allowlist[i]; | ||
| if addr.is_zero() { | ||
| return Err(ConfigError::InvalidDeployAllowlist( | ||
| "deployAllowlist contains zero address".to_string(), | ||
| )); | ||
| } | ||
| for j in (i + 1)..allowlist_len { | ||
| if addr == self.deploy_allowlist[j] { | ||
| return Err(ConfigError::InvalidDeployAllowlist( | ||
| "deployAllowlist contains duplicate entries".to_string(), | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation for checking duplicate addresses in deploy_allowlist uses a nested loop, which has a time complexity of O(n²). While this validation only runs at startup, it can be made more efficient and readable by using a HashSet. This would reduce the complexity to O(n).
let mut seen = std::collections::HashSet::with_capacity(allowlist_len);
for addr in &self.deploy_allowlist {
if addr.is_zero() {
return Err(ConfigError::InvalidDeployAllowlist(
"deployAllowlist contains zero address".to_string(),
));
}
if !seen.insert(*addr) {
return Err(ConfigError::InvalidDeployAllowlist(
"deployAllowlist contains duplicate entries".to_string(),
));
}
}
Description
This pr adds a permissioned system that is optional. Users may want to deploy their chain not as a general chain, this allows it.