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

Adds Typescript definitions. #205

Closed
wants to merge 1 commit into from
Closed

Conversation

MicahZoltu
Copy link
Contributor

This isn't complete, but it is way better than nothing and gives a place for people to add additional typescript definitions in the future. Recommend switching over to TypeScript for this library so this file is generated automatically. :)

@coveralls
Copy link

coveralls commented Mar 5, 2018

Coverage Status

Coverage decreased (-0.2%) to 57.267% when pulling c609aea on MicahZoltu:master into b1a098a on ethereum:master.

index.d.ts Outdated
interface CompilerOutputContracts {
[globalName: string]: {
[contractName: string]: {
abi: Abi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these are optional. I'm not sure it is a good idea to have forced description the JSON, it is not actually part of solc-js, but part of Solidity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean abi and evm? The rest are already marked as optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything in the JSON output is optional as it depends on the JSON input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anywhere that this is documented? When you say "everything is optional" I'm pretty sure you don't actually mean everything as (for example) I don't believe that you can configure the output to omit type in the AbiFunction structure. Perhaps you just mean "each of the top level keys under [contractName: string] are optional"? Are all of them actually optional or are some of them optional in the sense that they may be empty objects or empty arrays? Looking at the docs, the example shows several as having empty arrays or empty objects and I vaguely remember when I was working with these a while back encountering some situations where rather than being omitted an empty object or array was included (but I could be misremembering).

My goal here is to accurately represent the structure of the object returned by the Solidity compiler. My inquiries are because I really have no idea exactly what it can spit out, so I need some help specifying it.

If you really want to go above and beyond to help out, a comment on each non-optional thing that can be optional would be ideal. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over the docs again, I have added a few more optional indicators (locally). However, it is unclear from the docs whether evm.bytecode will be included if all of the members of evm.bytecode are excluded? Or perhaps it will just be an empty object in this case? Similar question for ewasm section, is it an empty object if ewasm.wast and ewasm.wasm are filtered out or is it omitted entirely if they are filtered out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question for the top level things, will the contracts key be omitted entirely if everything is filtered out from it, or will it always be present with an empty object?

Copy link
Member

@axic axic Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of it is documented here: http://solidity.readthedocs.io/en/develop/using-the-compiler.html#compiler-input-and-output-json-description

The ABI is documented here (but that is again outside the scope of the JSON compiler interface, it is a specification shared between languages and clients): http://solidity.readthedocs.io/en/develop/abi-spec.html#json

I think the JSON compiler interface documentation needs to be refined way more than it is currently, but the assumption is everything can be filtered out, based on the input selection. The truth current may be different, specific to the actual implementation, but we should just specify it properly in the documentation and follow that on the consumer side.

@MicahZoltu
Copy link
Contributor Author

I'm not sure it is a good idea to have forced description the JSON, it is not actually part of solc-js, but part of Solidity.

As a user, I depend on solcjs, not Solidity (I get that transitively). Therefore, I should be able to get the typescript definitions through solcjs. Specifically, I call functions in the solcjs library and it takes parameters/returns values that have a type and if I am using TypeScript those types should be exposed to me without needing to include some additional library.

I don't believe there is any easy way to bundle TypeScript definitions with Solidity such that I can get them transitively via solcjs, which is why I have added them here. I'm certainly open to other mechanisms for making the definition files available, but this seemed like the best option from a usability standpoint.

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Mar 7, 2018

The two links you provided (@axic) in your comment how I created this file in the first place. If you believe I have mis-interpreted the documentation or there is otherwise an error in this definition I encourage you to point it out (please be specific). I have updated the PR with a couple changes for where I believe I had a mistake previously (non-optional fields), but the documentation is not particularly clear on what happens when a node in the object graph would be an empty array or empty object; will the node be omitted or will it be empty?

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Mar 7, 2018

Examples of what is unclear are the top three keys (errors, sources, contracts) and contracts.[globalName].[contractName].evm, .ewasm, and .evm.bytecode. According to the documentation, these things cannot be filtered out, yet they may reasonably be empty based on filters and compilation results (e.g., no errors may be present, or all of the bytecode fields may be filtered out).

@danqing
Copy link

danqing commented May 19, 2018

Any progress on this? Would love to have TypeScript defs.

@cruhl
Copy link

cruhl commented Jun 22, 2018

It seems like this is ready to go... any updates? Thanks in advance!

@axic
Copy link
Member

axic commented Jul 11, 2018

@MicahZoltu @danqing @cruhl sorry for the delays, we just need to understand Typescript better since otherwise we cannot maintain it and merging means we have to maintain it.

Would it be possible to add tests which use the typescript definition?

index.d.ts Outdated
userdoc?: any;
devdoc?: any;
ir?: string;
evm: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even evm is optional, not only the fields below. Practically everything is optional in the response JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I'm not a fan of responses where everything is optional, it makes the receiver have to litter their code with null coalescing which is painful in JavaScript. However, that is out of scope of this PR which is just trying to match reality.

@MicahZoltu
Copy link
Contributor Author

There isn't really a way to "test" definition files unless you author your actual tests in TypeScript (which I generally recommend, but is out of scope for this PR). It is a matter of whether the definition files line up with reality, and testing that means writing a test that tests actual functionality in TypeScript. If there is a mismatch, either the test will fail to compile or the test will fail.

@andys8
Copy link

andys8 commented Aug 8, 2018

Is this definition also relevant for the user with solc/wrapper?

I'm trying to match the definitions with the actual use or the code in the README.md. E.g. this line:

console.log(contractName + ': ' + output.contracts[contractName].bytecode)

But CompilerContracts doesn't have a field bytecode on top level.

solc-js/index.d.ts

Lines 112 to 148 in bab0fef

interface CompilerOutputContracts {
[globalName: string]: {
[contractName: string]: {
abi?: Abi;
metadata?: string;
userdoc?: any;
devdoc?: any;
ir?: string;
evm?: {
assembly?: string;
legacyAssembly?: any;
bytecode: CompilerOutputEvmBytecode;
deployedBytecode?: CompilerOutputEvmBytecode;
methodIdentifiers?: {
[methodName: string]: string;
};
gasEstimates?: {
creation: {
codeDepositCost: string;
executionCost: string;
totalCost: string;
};
external: {
[functionSignature: string]: string;
};
internal: {
[functionSignature: string]: string;
};
};
};
ewasm: {
wast?: string;
wasm?: string;
}
}
};
}

@MicahZoltu
Copy link
Contributor Author

Last I checked contracts[contractName] doesn't have a bytecode field on it directly. Perhaps this has changed?

@andys8
Copy link

andys8 commented Aug 9, 2018

Looking at the Readme of the repository and the definitions I would assume it will not work. Also it did not work for me in a local project with this specific example.

As far as I know, the Typescript people write tests which work with their definitions and run the Typescript compiler on them. Could be an option.

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Aug 9, 2018

At the point in time when I submitted this PR it worked. npm install solc and dump this in your project's definitions folder. This project compiles and runs: https://github.com/AugurProject/augur-core/blob/2d9cbac69e33be2f07a02ad91fe51574994be066/source/libraries/ContractCompiler.ts#L154

The definition file that project uses can be found here: https://github.com/AugurProject/augur-core/blob/master/typings/solc/index.d.ts

Uses solc 0.4.20

@ghost
Copy link

ghost commented Aug 19, 2018

Is this still being worked on? Would love to have TypeScript definitions for solc-js!

@MicahZoltu
Copy link
Contributor Author

I'm not actively working on it, but would still like to get it merged. I'm unclear if there is anything realistic that we could do to get this merged. To maintainers: Can you decide yay or nay on this rather than limbo? If you decide nay it can be added to DefinitelyTyped so people can still benefit (though not as cleanly).

@ToJen
Copy link

ToJen commented Nov 17, 2018

Any update on this PR?

@axic
Copy link
Member

axic commented Nov 18, 2018

The API with 0.5.0 changed significantly. It only supports strings externally and as a result there's no need to define the JSON (including ABI JSON).

@MicahZoltu can you update the PR? I'd like to merge it after.

@axic
Copy link
Member

axic commented Nov 18, 2018

To clarify: if you could create a PR which only adds types to the public methods on wrapper (e.g. without the standard JSON/ABI) that could be merged quickly. The rest of this PR is more complicated.

@MicahZoltu
Copy link
Contributor Author

Hmm, I haven't followed Solidity 5.0, and I don't understand what you mean by "it only supports strings externally".

@axic
Copy link
Member

axic commented Nov 18, 2018

The only important entry point is compile(string, callback) -> string.

This isn't complete, but it is _way_ better than nothing and gives a place for people to add additional typescript definitions in the future.  Recommend switching over to TypeScript for this library so this file is generated automatically.  :)
@MicahZoltu
Copy link
Contributor Author

I have created ea new PR that only contains the compile callback definition: #298

While I'm happy to see that PR get merged as it is a good starting point for getting TypeScript definitions included, I don't feel that it is nearly as useful as getting the JSON input/output well defined.

I'm also curious why the compile function takes a string and returns a string when what it really wants/provides is a very specifically defined JSON object. In normal JS the difference is minimal, but in TypeScript the difference is pretty huge as you can get strong type checking and hinting if it takes a well defined object while "string" results in basically no type checking.

@r0qs
Copy link
Member

r0qs commented Sep 28, 2022

Hi, thank you very much for this PR. However, part of what was discussed here was already implemented or is under development in other PRs (e.g. #630), so I will be closing this for now.

Please, feel free to reopen it if necessary.

@r0qs r0qs closed this Sep 28, 2022
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.

8 participants