Add support for option --allow-paths #1010

tglaeser · 2018-02-16T17:58:35Z

Following through on my proposal as outlined in ticket #1006.

….19 ... adding new function property stateMutability (not yet used)

coveralls · 2018-02-16T18:33:19Z

Coverage increased (+0.2%) to 56.811% when pulling ccbadb4 on afide:develop into ca6d8f3 on ethereum:develop.

tglaeser · 2018-02-26T22:30:29Z

@mkalinin @zilm13 Any objections to get those changes merged into develop branch?

zilm13

Great improvement, but I'd change several things

zilm13 · 2018-02-27T14:11:48Z

ethereumj-core/src/main/java/org/ethereum/solidity/compiler/SolidityCompiler.java

        return getInstance().compileSrc(sourceDirectory, false, combinedJson, options);
    }

-    public enum Options {
+    public static final class Options {


Could you, please, add comment here, that it's here for backward compatibility

zilm13 · 2018-02-27T14:15:59Z

ethereumj-core/src/main/java/org/ethereum/solidity/compiler/SolidityCompiler.java

+        String getName();
+    }
+
+    public static class ListOption implements Option {


This code currently designed only for --allow-paths, maybe make it just AllowPathsOption or at least make AllowPathsOption extends ListOption or any other shortcut, so user will not need to write ListOption("allow-paths",

You are absolutely right; I'll do the necessary changes.

zilm13 · 2018-02-27T14:34:37Z

ethereumj-core/src/test/java/org/ethereum/core/ImportLightTest.java

@@ -372,15 +372,15 @@ public void createContractFork() throws Exception {
                "}";

        StandaloneBlockchain bc = new StandaloneBlockchain();
-        SolidityContract parent = bc.submitNewContract(contractSrc, "Parent");
+        SolidityContract parent = bc.submitNewContract(contractSrc, "<stdin>:Parent");


I think it's a bad thing with all this <stdin>'s.
We are making interface to proxy user from solc and add some simplicity and programming-language interface to interact with. Also all method declarations call it "Contract name" already. Plus we want it to work with several versions of solc.
I'd suggest modify CompilationResult.parse to remove "<stdin>:" part when it exists in the beginning and revert all method calls in tests. What do you think?

Well, I didn't spent a lot of thoughts on this, simply adjusted the tests to work with the new Solidity compiler version.

To me it seems that the creators of Solidity, with version 0.4.9, added some form of a poor men's namespace support; that is, requiring to prefix each contract with ${filename}:. Only if the contract is read from standard input instead, the prefix becomes <stdin>:. Please follow the link above for the release notes.

I don't see how the new contract's namespace information could be ignored in the production code. Thoughts/concerns?

Ok, what I suggest:

Adding public String path into ContractMetadata. If there is anything on first line before :, it's saved there (and removed from contract name)

public Map<String, ContractMetadata> contracts becomes public Map<String, List<ContractMetadata>> contracts in CompilationResult

What do you think?
We are braking backward compatibility but I see only weird ways to preserve it.
Anyway remembering path which is specific to filesystem to get contract from compilation is not a good thing, I think, and we need to get it over.

I understand your suggestion, but I believe that would not be as designed by the creators of Solidity.

To me it seems, since Solidity version 0.4.9, that the compiler returns a contract key, not contract name anymore, which will always be prefixed with ${filename}: or <stdin>:, that's a breaking change at the side of the Solidity compiler. You cannot ditch this information and use the contract name instead of the contract key as the key in Map<String, ContractMetadata>. This breaking change was introduced to allow contracts of the same name but different location to coexists, e.g. map entries with keys /foo/file1.sol:test, /foo/bar/file2.sol:test.

We need to agree on this first before we can think about how to better address this change.

Look, now contract name just contains more information: path (or <stdin>) and contract name. I just suggest to store it separately in our Java model. Contract name is not {filename}:realContractName. It's just new part of information, they've added in output in such way. You are not able to call one contract from another with such prefix, so it's not a part of contract name, yeah?

Yeah, I've called @mkalinin for his opinion, he will reply later.
I don't like paths (or combined key as originally comes from solc) as a first key because paths are even different on different filesystems and we make life of all users more difficult for rare cases when someone needs to compile the same names at once. Let's see what Michael will say.

Yeah, I noticed, our messages somehow overlapped. I can relate to the discontent towards java.io.File, so maybe Map<URI, ?> would actually better reflect reality.

One more thing, doing contracts.get("MyContract").getValue(0) as suggested by @zilm13 above would allow keeping the current tests more or less the same, however any general provider of this functionality would still have to check the entries in LinkedMap<String, ContractMetadata> in order to make sure that he doesn't miss any contract. It just unnecessarily complicates the code IMHO.

Both of you have thoughts meaningful to me. I agree with @zilm13 that contract name is just a name and it's incorrect to prefix it with contract source, cause that will definitely confuse the user. On the other hand @tglaeser is right pointing that CompilationResult contains solidity output where key in contracts map is not a contract name but its identifier. So, what would I do: hide CompilationResult.contracts and provide getters to access it. I.e. something like getContract(String name) for general usage which returns first matching contract with no matter for path, getContract(String name, String path) - this method requires additional work in path transformation depending on the OS (need to check how solc handles that). Also, this change requires additional getter which returns just the first contract from that map (I saw that this case is also can take place). That is my opinion guys and that solution combines both of your suggestions, hence it should work :)

Thanks for the input. I'll go forward and make the necessary changes and update the pull request...

…ll supported options under SolidityCompiler.Options

… using new access to the contract metadata

…ting this to change anyway

tglaeser · 2018-03-03T02:33:31Z

@zilm13 @mkalinin Please let me know in case you have additional thoughts/suggestions...

tglaeser · 2018-03-08T16:00:01Z

@zilm13 @mkalinin Please advise.

zilm13

Sorry for delay. Looks almost complete. Just a bit of polishing.

zilm13 · 2018-03-08T20:58:59Z

ethereumj-core/src/main/java/org/ethereum/solidity/compiler/CompilationResult.java

+                return entry.getValue();
+            }
+        }
+        throw new UnsupportedOperationException("Source contains more than 1 contact. Please specify a valid contract name. Available keys (" + getContractKeys() + ").");


There should be something with "not found"
Contract %s was not found in compilation result or something like this.

zilm13 · 2018-03-08T21:02:17Z

ethereumj-core/src/main/java/org/ethereum/solidity/compiler/CompilationResult.java

+        }
+    }
+
+    @JsonIgnore public ContractMetadata getContract(String contractName) {


Could you add javadoc to getters, especially to this one. There should be mentioned, that if there are more than 1 contract with the same name, only first will be returned, so you should use {@link getContract(Path contractPath, String contractName)} for this cases.

zilm13 · 2018-03-08T21:23:25Z

ethereumj-core/src/test/java/org/ethereum/solidity/CompilerTest.java

+        CompilationResult.ContractMetadata a = result.getContract(source, "test3");
+        CallTransaction.Contract contract = new CallTransaction.Contract(a.abi);
+        System.out.print(contract.functions[0].toString());
+    }


could you assert here that there are 2 contracts totally and second is not null too and could be queried by name.

tglaeser · 2018-03-09T16:36:10Z

@zilm13 Made the requested changes.

zilm13 · 2018-03-09T21:43:22Z

@tglaeser Thank you!

tglaeser · 2018-03-10T03:21:59Z

@zilm13 Thank you for reviewing. Since the Solidity compiler options changed quite a bit with the upgraded version, there is more work to do in this area. If I get a chance I'll come back and do the remaining work...

mkalinin · 2018-03-10T05:00:43Z

guys, have you an idea of what went wrong? ether-camp/ethereum-harmony#63

zilm13 · 2018-03-10T07:49:25Z

@mkalinin I'll fix it

tglaeser and others added 7 commits February 14, 2018 10:19

Replacing the enum type 'Options' with more flexible interface 'Option'

0510240

Upgrading module org.ethereum:solcJ-all to version 0.4.10

79f4159

Preparing for upgrade of module org.ethereum:solcJ-all to version 0.4…

52fa9ff

….19 ... adding new function property stateMutability (not yet used)

Adding support for option --allow-paths

34caf03

Upgrading module org.ethereum:solcJ-all to version 0.4.19

8e6721b

Making Option and inner interface

5e1d607

Make objects of type Option serializable

76c6ac4

zilm13 suggested changes Feb 27, 2018

View reviewed changes

tglaeser added 6 commits February 27, 2018 13:03

Merge remote-tracking branch 'upstream/develop' into develop

c924799

Adding new option types CombinedJson and AllowPaths ... registering a…

c8b8097

…ll supported options under SolidityCompiler.Options

Reverting the fully qualified contract name back to contract name ...…

5b063b1

… using new access to the contract metadata

Adding new methods to access the contract metadata

6f29521

Returning the contracts and keys as type List

351b0c2

Removing this part of the test ... not platform independent ... expec…

c1294de

…ting this to change anyway

zilm13 suggested changes Mar 8, 2018

View reviewed changes

Adding JavaDoc ... more assertions with test

ccbadb4

zilm13 approved these changes Mar 9, 2018

View reviewed changes

zilm13 merged commit 65ef5a1 into ethereum:develop Mar 9, 2018

tglaeser mentioned this pull request Mar 14, 2018

Add support for option --allow-paths #1006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for option --allow-paths #1010

Add support for option --allow-paths #1010

tglaeser commented Feb 16, 2018

coveralls commented Feb 16, 2018 •

edited

Loading

tglaeser commented Feb 26, 2018

zilm13 left a comment

zilm13 Feb 27, 2018

tglaeser Feb 27, 2018

zilm13 Feb 27, 2018

tglaeser Feb 27, 2018

zilm13 Feb 27, 2018 •

edited

Loading

tglaeser Feb 27, 2018

zilm13 Feb 28, 2018 •

edited

Loading

tglaeser Feb 28, 2018

zilm13 Feb 28, 2018

zilm13 Feb 28, 2018

tglaeser Feb 28, 2018

tglaeser Feb 28, 2018

mkalinin Mar 1, 2018

tglaeser Mar 1, 2018

tglaeser commented Mar 3, 2018

tglaeser commented Mar 8, 2018

zilm13 left a comment

zilm13 Mar 8, 2018

zilm13 Mar 8, 2018

zilm13 Mar 8, 2018 •

edited

Loading

tglaeser commented Mar 9, 2018

zilm13 commented Mar 9, 2018

tglaeser commented Mar 10, 2018

mkalinin commented Mar 10, 2018

zilm13 commented Mar 10, 2018

Add support for option --allow-paths #1010

Add support for option --allow-paths #1010

Conversation

tglaeser commented Feb 16, 2018

coveralls commented Feb 16, 2018 • edited Loading

tglaeser commented Feb 26, 2018

zilm13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tglaeser commented Mar 3, 2018

tglaeser commented Mar 8, 2018

zilm13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

tglaeser commented Mar 9, 2018

zilm13 commented Mar 9, 2018

tglaeser commented Mar 10, 2018

mkalinin commented Mar 10, 2018

zilm13 commented Mar 10, 2018

coveralls commented Feb 16, 2018 •

edited

Loading

zilm13 Feb 27, 2018 •

edited

Loading

zilm13 Feb 28, 2018 •

edited

Loading

zilm13 Mar 8, 2018 •

edited

Loading