search

ethpm / py-ethpm

This library is deprecated. ethPM python tooling is now located in web3.py
MIT License
24 stars 13 forks source link

Remove bytecode compilation if bytecode field not in manifest #42

Closed njgheorghita closed 6 years ago

njgheorghita commented 6 years ago

What was wrong?

Compiling bytecodeis being deprecated until a compilation strategy is decided upon Also removed pm module which was a prototype for https://github.com/ethereum/web3.py/pull/897

How was it fixed?

Removed automatic compilation of bytecode if 'bytecode' field not present in manifest. Compilation utils have been left in, in case they will be useful once a compilation strategy is made

Cute Animal Picture

image

davesque commented 6 years ago

Just noticed this, sorry. So does this basically just revert some changes you made to get bytecode from source? Why not just throw a meaningful exception if the task can't be completed with the information available in the package manifest?

njgheorghita commented 6 years ago

@davesque Yeah that's a good idea. Will do. On further inspection - it looks like utils/contract/validate_minimal_contract_data_present is incorrect. Right now it only checks that one of (abi/bytecode/runtime_bytecode) is present. Just to confirm, to generate a contract factory we need at least abi and bytecode, correct? ABI and runtime_bytecode would be insufficient to generate a factory, correct? As would either bytecodes without the ABI?

pipermerriam commented 6 years ago

To generate a factory, the absolute minimum is ABI. If it is to be deployable, then bytecode is also required.

njgheorghita commented 6 years ago

@pipermerriam @davesque Not sure if this is the right call - but I made the assumption that right now a user would only want to generate a deployable contract factory (seems like the most common use case for me) - thereby the validation util requires both abi and deployment_bytecode. If it's fairly common that a user would want to generate a non-deployable factory (i.e. with only the abi) - then I can add that feature in a later pr.

davesque commented 6 years ago

This looks okay for now. Although pretty soon I'll be wanting to allow for manifests without the "deployment_bytecode" key for packages which only include deployed instances.