fix for linker wrt binaries generated with import statements

[VoR0220]

I ran into a problem and found out that it was actually a bug with how we've set up our linker. I also am not quite sure how we should test for this, so any ideas for that are also welcome. The problem can be outlined like this:

suppose that I have an import statement in one of my contracts that leads to a library. Suppose I also use the -o option to get .bin files (solc -o binaries --bin myfile.sol). Currently, that will generate a placeholder in the .bin file in the form of __theFileThisLibraryIsFrom.sol:LibraryName______. Couple this with the way linking CURRENTLY works (we search for the first :) and you can see that this leads to a problem when coming into linking as no matter what you do, you are bound to hit that first : and thus your library name gets included as your address along with an additional :. This fixes that by looking for : in reverse. Very simple fix. But not very sure how it should be tested. Any suggestions and I will write them up.

Signed-off-by: VoR0220 rj@erisindustries.com

[roadriverrail]

((facepalm)) Good catch. I was 90% sure that I'd tested out the scenario you're describing, but it might have slipped through in one of the final tweaks that got made.

I thought we had stronger linker tests than we appear to, too. Even just grepping the tests for "link" shows there isn't very much; I suspect this is because what linking gets done is done while the test is running. I'm not sure there's a good way to test this in the future without writing a test that does exactly what you did to find the bug...that is, to emit output binary files with the symbols in them and then try a final linking. I might have missed something, though.

[VoR0220]

I was thinking perhaps we could better structure the CLI tests but it seems those could get nasty very quickly. Hence why I'm looking for a bit of guidance here as to wth to do.

[VoR0220]

another potential fix would be that we just don't include filenames in the library linking for binaries...that could also be a potential fix. This one is easier for now, but I know for a fact of usecases where the the filename needn't be shared and where it would be preferable for it not to be.

[roadriverrail]

I'll link the previous PR's conversation here for posterity, because really, that sort of decision is above my pay grade and maybe should involve @chriseth : #1397

Dropping filenames trades one issue off for another; namely, you can now have a link-level collision on name. I don't know which is more or less preferable an issue to have, which is why I won't comment. The most logical thing I could think, off the top of my head, is that if filenames should be hidden, then it's more appropriate to obfuscate them, turning the linker symbol from <filename>:<contract> into, say, <filename-hash>:<contract>.

As for the testing framework, I'll also confess that I'm not really clear on how to achieve what we'd be looking for.

[VoR0220]

perhaps we could have both or rather than filename-hash, just be <contract-code-hash>...that's more legit imo, because I agree collision is something that ought to be avoided. Could maybe add it to the compilation output options...that'd be something I'd be interested in doing.

[roadriverrail]

Seems pretty legit to me. Again, I'm mostly a roving janitor grabbing stuff marked "soon" in this project, but it looks like valid work to me.

…

On Mon, Jan 30, 2017 at 6:21 PM RJ Catalano ***@***.***> wrote: perhaps we could have both, because I agree collision is something that ought to be avoided. Could maybe add it to the compilation output options...that'd be something I'd be interested in doing. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlhYAc_fLsMy2hvG_P1_2FFq3Fi1oB_ks5rXnCQgaJpZM4Lx2Kd> .

[chriseth]

The main issue is that we use a non-structured encoding for the libraries option. Using something like

libraries = {'my/file/name.sol': {'MyLibrary': '0x123'}}

would not have this problem. But it's coming!