-
Notifications
You must be signed in to change notification settings - Fork 48
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: make Contract generic for Compiler and add metadata to CompilerOutput #224
Conversation
Co-authored-by: Nisheeth Barthwal <[email protected]>
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 see, imo this seems reasonable if this simplifies things for zksync a lot.
the additional type/generic isn't too bad
left some doc nits,
needs another review by @klkvr
It'd be great if you could open a pr to foundry where the compilers deps a patched to this branch so we can ensure that this still works or what we need to tune
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] | ||
#[serde(transparent)] | ||
pub struct VersionedContracts(pub FileToContractsMap<Vec<VersionedContract>>); | ||
pub struct VersionedContracts<C: CompilerContract>( | ||
pub FileToContractsMap<Vec<VersionedContract<C>>>, | ||
); | ||
|
||
impl<C> Default for VersionedContracts<C> | ||
where | ||
C: CompilerContract, | ||
{ | ||
fn default() -> Self { | ||
Self(BTreeMap::new()) | ||
} | ||
} |
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.
these trait bounds can be relexed, we only need this for the impl block
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.
removed the CompilerContract
bound: 2be52cf
@@ -267,6 +276,42 @@ pub trait Language: | |||
const FILE_EXTENSIONS: &'static [&'static str]; | |||
} | |||
|
|||
pub trait CompilerContract: Serialize + Send + Sync + Debug + Clone + Eq + Sized { |
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.
could we add some docs here?
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.
Added, took them from original Contract
: 0a36f57
#[serde(default)] | ||
pub metadata: BTreeMap<String, 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.
are these really just strings or are those json objects?
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.
Changed value to json: c2a6c68
#[serde(default)] | ||
pub metadata: BTreeMap<String, 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.
I'd like to make this an option and skip if missing because now this will always be present for solc
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.
Skipped serializing if empty: c2a6c68 is that good enough?
@mattsse thanks, added draft PR on foundry that compiles: foundry-rs/foundry#9423 |
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.
very nice! thanks for working on this @elfedy
just have a small nit
I think it might also be useful to abstract the source eventually and have something like
trait CompilerOutput {
type Error;
type Contract;
type Source;
}
but this can definitely be refactored later once we have a nice way to support generic contracts in foundry
Co-authored-by: Arsenii Kulikov <[email protected]>
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.
lgtm!
Right now the compiler abstraction has two couplings that force foundry-zksync to implement it's own fork of compilers:
Contract
is specific tosolc
/EVM contracts. Era VM contracts, while requiring different fields, still could use most of the functionality of thecompilers
pipeline.CompilerOutput
hassolc
specific fields.zksolc
compilation has relevant information that is useful to have later on, for example when storingBuildInfo
This PR implements changes to address this. If implemented, it would allow
foundry-zksync
(and potentially other non EVM implementations of foundry) to get rid of all overrides and only maintain ZKsync specific data structures/trait implementations. See sample PR. Changes include:Compiler
generic overContract
(usingCompilerContract
as a trait type).metadata
field toCompilerOutput
in order to add arbitrary data to compilation output.