- 
                Notifications
    You must be signed in to change notification settings 
- Fork 240
Display gas report table for fork contracts #3876
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: 3660-gas-report
Are you sure you want to change the base?
Conversation
| This change is part of the following stack: 
 Change managed by git-spice. | 
|  | ||
| #[derive(Debug, Clone, PartialOrd, PartialEq, Ord, Eq)] | ||
| pub enum ContractId { | ||
| TestContract(ContractName), | 
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.
nit: wdyt?
| TestContract(ContractName), | |
| LocalContract(ContractName), | 
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.
I prefer local
|  | ||
| #[derive(Debug, Clone, PartialOrd, PartialEq, Ord, Eq)] | ||
| pub enum ContractId { | ||
| TestContract(ContractName), | 
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.
I prefer local
| let name = match self { | ||
| ContractId::TestContract(name) => format!("{name} Contract"), | ||
| ContractId::ForkedContract(class_hash) => { | ||
| format!( | ||
| "forked contract\n(class hash: {})", | ||
| shorten_felt(class_hash.0) | ||
| ) | 
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.
Capitalization is inconsistent between these
| ContractId::ForkedContract(class_hash) => { | ||
| format!( | ||
| "forked contract\n(class hash: {})", | ||
| shorten_felt(class_hash.0) | 
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.
I wonder if it should be configurable to show full contract urls, maybe let's just add an issue for that, maybe it can be integrated if we ever add a -v flag to foundry.
| pub(super) fn shorten_felt(felt: Felt) -> String { | ||
| let padded = format!("{felt:#066x}"); | ||
| let first = &padded[..4]; | ||
| let last = &padded[padded.len() - 4..]; | ||
| format!("{first}…{last}") | ||
| } | 
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 first 3 characters area always 0x0, maybe we should also show 4 after that? It's not very useful to show that beginning otherwise.
Towards #3660