ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
22.99k stars 5.69k forks source link

[CLI] Drop --combined-json option and direct users to standard json #10278

Open axic opened 3 years ago

axic commented 3 years ago

Closes #10276.

chriseth commented 3 years ago

I think it has some features that standard json lacks.

axic commented 3 years ago

Can you list those? We should add it to standard json.

chriseth commented 3 years ago

Maybe I confused this with non-json mode.

axic commented 3 years ago

Frameworks:

Tools:

I'm sure there are more frameworks, just I'm unaware of them.

axic commented 3 years ago

@iamdefinitelyahuman could you explain whether you rely on --combined-json and if yes, why?

@MrChico likewise, could you explain if moving to --standard-json would be debilitating? :)

cameel commented 3 years ago

Buidler/Hardhat: uses only --standard-json

axic commented 3 years ago

@cameel thanks, added to the list above (please feel free to edit my comment if you check other frameworks)

iamdefinitelyahuman commented 3 years ago

Brownie uses --standard-json exclusively . py-solc-x is just wrapping around solc, so it provides access to --combined-json for no reason other than that combined json exists :)

Brownie would not break anywhere if you removed --combined-json, so :+1: from me.

axic commented 3 years ago

@iamdefinitelyahuman thanks for the response! Do you know of any py-solc-x user who makes use of --combined-json?

MrChico commented 3 years ago

I mainly prefer --combined-json for it's easy commandline use – I can get all of the compiler outputs of interest with a relatively simply one liner. As far as I can foresee there would be nothing fundamentally limiting to migrate dapptools to use --standard-json besides the obvious labor involved in architecturing everything to support the new format. Many features that have been introduced into --standard-json have been neglected in --combined-json, so if that pattern is to continue we would probably migrate anyway. My preference would of course be to simply get --combined-json up to feature parity, as requested in https://github.com/ethereum/solidity/issues/10274.

axic commented 3 years ago

The thing is we have three outputs to maintain:

  1. standard-json
  2. individual options to solc
  3. the legacy combined-json (which predates standard-json)

We can barely keep feature parity between individual solc options and standard-json, and since combined-json is so sparsely used (seemingly only by dapphub), it is hard to rationalise keeping it maintained.

See the various open issues regarding CLI issues.

iamdefinitelyahuman commented 3 years ago

@iamdefinitelyahuman thanks for the response! Do you know of any py-solc-x user who makes use of --combined-json?

I took a look through some of the listed dependents, most are using the standard JSON interface. The notable exception is NuCypher. @KPrasch @cygnusv - care to weigh in here?

vzotova commented 3 years ago

@iamdefinitelyahuman @KPrasch and I are working on that, probably will finish moving to standard JSON interface soon

axic commented 3 years ago

@MrChico could you point me to the places in dapptools where --combined-json is used? I am wondering whether the output parsing or the input preparation seems like the bigger hurdle.

The output should be much more nicely formatted (compared to --combined-json), but at the cost of more levels of objects.

MrChico commented 3 years ago

@axic, it is being called in dapp build and parsed in a few different places, such as dapp create or (most frequently) in Solidity.hs of hevm.

axic commented 3 years ago

@MrChico Looking at dapp build, wouldn't that be better off using the CLI options directly, would they output proper files (e.g. #10275)?

MrChico commented 3 years ago

It does use the CLI options directly if the --extract flag is passed. Not everything that is available under --combined-json is available as a direct CLI flag however, for example--srcmap does not exist. Most of the time, it's quite convenient to have all artifacts in one file though.

MrChico commented 3 years ago

Not everything that is available under --combined-json is available as a direct CLI flag however, for example--srcmap does not exist.

Which I get is part of your original point of the maintenance burden. I can investigate migrating to the --standard-json format when I have some more time (in about 2 weeks).

MrChico commented 3 years ago

OK, having started the process of supporting stdjson, this seems doable. I really miss the pretty printed error messages. I know they are present in the json, but without the nice coloring. To me it would seem appropriate to also display them (pretty printed) in stderr

axic commented 3 years ago

I really miss the pretty printed error messages. I know they are present in the json, but without the nice coloring.

That is coming in 0.8.0.

MrChico commented 3 years ago

@axic nice! Will color be added to the json or output to stderr?

axic commented 3 years ago

The new formatting replaces the old in the formattedMessage field. However the colours are disabled, as of now.

What do you mean by stderr? No error/warning should be reported by the compiler apart from things encoded in the JSON response.

MrChico commented 3 years ago

By stderr I mean the linux standard IO stream, where errors are outputted in any other invocation of solc that doesn't involve --standard-json.

No error/warning should be reported by the compiler apart from things encoded in the JSON response.

I find this a bit unfortunate, but I understand.

axic commented 3 years ago

By stderr I mean the linux standard IO stream, where errors are outputted in any other invocation of solc that doesn't involve --standard-json.

On the regular non-standard-json invocations the stderr was coloured for quite a bit already. That does not change.

MrChico commented 3 years ago

As of https://github.com/dapphub/dapptools/pull/569, dapptools now no longer relies on combined-json.

axic commented 3 years ago

Thanks @MrChico!

cameel commented 3 years ago

The saddle framework is using --combined-json.

EDIT: Added to @axic's list above.

axic commented 2 years ago

Thanks @cameel, opened an issue for them: https://github.com/compound-finance/saddle/issues/43

axic commented 2 years ago

@iamdefinitelyahuman is py-solc-x still dependent on --combined-json?

cameel commented 2 years ago

@axic I have just noticed a problem here. I think that solc --import-ast only accepts input produced by --combined-json ast. The format of --ast-compact-json is different. We should probably provide a replacement before we drop --combined-json.

cameel commented 2 years ago

gasol is another tool that expects input in the --combined-json asm format (and also outputs that format).

Looks like evm.legacyAssembly is basically the same format if you look at each individual contract but contracts are laid out a bit differently in JSON and it's a part of a bigger JSON structure so it needs at least a simple transformation to be usable.