Hash linker #5145
Hash linker #5145
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5145 +/- ##
============================================
- Coverage 87.92% 28.66% -59.26%
============================================
Files 321 318 -3
Lines 31979 31775 -204
Branches 3783 3770 -13
============================================
- Hits 28118 9109 -19009
- Misses 2576 21986 +19410
+ Partials 1285 680 -605
|
|
It seems this needs an implementation of the linker in solc-js and for that we need a keccak256 dependency. |
|
Needs ethereum/solc-js#266 |
|
I think standard json output should contain the hash for the unlinked references or at least the documentation be extended to explain that. |
|
@axic standard json contains offset references - I don't think we need to list the hashes in addition to that. |
|
This looks very good. I'd consider some more testing around compiling and linking multiple libraries. Even though conflicts should not happen anymore in theory. I feel like having some kind of proof with regards to the audit. @axic I also think that we do not need to add extra hashes for unlinked references to the standard json output. But I'd test the bytecode format more extensively and then cover the offsets. |
|
Added documentation - I actually already had it, just forget to check it in. |
docs/using-the-compiler.rst
Outdated
| If your contracts use :ref:`libraries <libraries>`, you will notice that the bytecode contains substrings of the form ``__LibraryName______``. You can use ``solc`` as a linker meaning that it will insert the library addresses for you at those points: | ||
| If your contracts use :ref:`libraries <libraries>`, you will notice that the bytecode contains substrings of the form ``__53aea86b7d70b31448b230b20ae141a537e5__``. These are placeholders for the actual library addresses. | ||
| The placeholder is a 36 character prefix of the hex encoding of the keccak256 hash of the fully qualified library name. | ||
| The bytecode file will also contain lines of the form ``// <placeholder> -> <library name>`` at the end to help |
There was a problem hiding this comment.
Shouldn't this be // <placeholder> -> <library path>:<library name>?
There was a problem hiding this comment.
I will change it to "fully qualified library name".
solc/CommandLineInterface.cpp
Outdated
| @@ -905,8 +905,10 @@ void CommandLineInterface::handleCombinedJSON() | |||
| if (requests.count("metadata")) | |||
| contractData["metadata"] = m_compiler->metadata(contractName); | |||
| if (requests.count(g_strBinary)) | |||
| // Do we want to change it here or not? | |||
There was a problem hiding this comment.
What do you mean with this comment? Is this a left-over?
There was a problem hiding this comment.
It is a question to the reviewers.
There was a problem hiding this comment.
Well, I wasn't sure :) I'd prefer to add such questions as regular comments to the pull request. In that case, I'd say yes, let's be consistent and also add the reference hints to the combined-json output.
There was a problem hiding this comment.
Ok, perhaps I should give the reason why I did not add it here: I would assume some tools to be confused when a json string suddenly starts containing newlines, and several tools might not be prepared for that.
|
Restarted the failing Linux test. |
|
The driveby-bugfixes inside this PR are getting out of hand... |
chriseth
force-pushed
the
hashLinker
branch
2 times, most recently
from
1e5b321 to
a017c02
Compare
Fixes #3918
Fixes #4429