Add types for compiler input & output #630
Conversation
There was a problem hiding this comment.
Before we dive into something like this we should first agree on a general direction. This has potential to be pretty hard to maintain. Please see my comments below.
@axic, curious what's your opinion on this. Is it even feasible to have a precise schema for Standard JSON in solc-js given that it's not consistent between compiler versions? This kinda gets into the territory of ethereum/solidity#2276.
| } | ||
|
|
||
| export interface CondensedCompilationInput { | ||
| language: 'Solidity' | 'Vyper' | 'lll' | 'assembly' | 'yul' |
There was a problem hiding this comment.
What is this based on? Technically, solc supports only Solidity and Yul (case-sensitive). And in a broader context (i.e. including any possible compilers supporting Standard JSON) I'd say that the values are not limited to some predefined choices - other compilers are free to support any language identifiers.
| modelChecker?: CompilerModelChecker | ||
| } | ||
|
|
||
| export interface CompilationInput { |
There was a problem hiding this comment.
These definitions are pretty detailed. There are some problems though:
- If we keep them here, they will be hard to keep in sync with the compiler. Maybe there are better ways to go about this? For example defining some JSON schema (not tied specifically to TypeScript syntax) that would be kept in the main compiler repo and only imported here?
- solc-js is meant to support all the old compiler versions and Standard JSON changed over time. So these definitions won't be right for all the compiler binaries that you can use. So we either need separate definitions for different versions (at least breaking ones) or some superset that won't exactly fit any particular version.
|
@cameel thanks for the review. I created these type for I think this PR could still be a good place to discuss type naming, how to deal with the different output based on the input config, ... like that we can have a base for generating types on a second step. What do you think ? |
The problem here is that it's a lot of work to get that right for all the versions. Unfortunately currently this is basically defined by the implementation of the C++ compiler and not some more generic spec. Docs cover it on a high level but small details would have to be checked in the source. My general feeling is that it would be easier to get some kind of schema into the main compiler repo first and get this working for the current version. Then worry about older ones later. The downside is that this way the typing for older versions would be unavailable or wrong for the time being. On the other hand having precise definitions for all versions would definitely be appreciated by people working on tooling so there is some value in that if we could pull that off. This is far from our current roadmap though so I can't promise that we could focus on that enough to get that through. As is, the current Typescript migration is already progressing much slower than it otherwise could because we're bottlenecked by people available to review the PRs.
Maybe. But I really don't want you to put too much extra work into something that would end up being rejected so I think it would be best to start with some general discussion first, before focusing on details. I can definitely give you some feedback here but I'm not sure how much this will matter if in the end it turns out it's not the solution we want. Also @axic will definitely have some opinions here, so we should also wait for some input from him :) |
|
Ok no problem. I can continue the output types on an external library for now, and if you consider it would make sense to add it into solc-js at some point I can make another PR 🙂 |
Feat #287
This Pull Request adds types & inline documentation for compiler input & output based on the Compiler Input & Output documentation and the ABI Spec documentation.
Some types are not strictly typed (Yul AST for example) because I didn't find the right documentation. Don't hesitate to point me to some doc / code to improve it.
Note: I included a commit that demonstrates how to use the output type with the update method. I can remove the commit from the PR for separation of concern.