Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Add support for option --allow-paths #1010

Merged
merged 14 commits into from
Mar 9, 2018
Merged

Add support for option --allow-paths #1010

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0510240
Replacing the enum type 'Options' with more flexible interface 'Option'
tglaeser Feb 14, 2018
79f4159
Upgrading module org.ethereum:solcJ-all to version 0.4.10
tglaeser Feb 14, 2018
52fa9ff
Preparing for upgrade of module org.ethereum:solcJ-all to version 0.4…
tglaeser Feb 14, 2018
34caf03
Adding support for option --allow-paths
tglaeser Feb 14, 2018
8e6721b
Upgrading module org.ethereum:solcJ-all to version 0.4.19
tglaeser Feb 15, 2018
5e1d607
Making Option and inner interface
Feb 15, 2018
76c6ac4
Make objects of type Option serializable
tglaeser Feb 15, 2018
c924799
Merge remote-tracking branch 'upstream/develop' into develop
tglaeser Feb 27, 2018
c8b8097
Adding new option types CombinedJson and AllowPaths ... registering a…
tglaeser Feb 27, 2018
5b063b1
Reverting the fully qualified contract name back to contract name ...…
tglaeser Mar 2, 2018
6f29521
Adding new methods to access the contract metadata
tglaeser Mar 2, 2018
351b0c2
Returning the contracts and keys as type List
tglaeser Mar 3, 2018
c1294de
Removing this part of the test ... not platform independent ... expec…
tglaeser Mar 3, 2018
ccbadb4
Adding JavaDoc ... more assertions with test
tglaeser Mar 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethereumj-core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ dependencies {

compile "org.ethereum:leveldbjni-all:1.18.3" // native leveldb components

compile "org.ethereum:solcJ-all:0.4.8" // Solidity Compiler win/mac/linux binaries
compile "org.ethereum:solcJ-all:0.4.19" // Solidity Compiler win/mac/linux binaries

compile "com.cedarsoftware:java-util:1.8.0" // for deep equals
compile "org.javassist:javassist:3.15.0-GA"
Expand Down
8 changes: 8 additions & 0 deletions ethereumj-core/src/main/java/org/ethereum/core/CallTransaction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ public String getType() {
}
}

public enum StateMutabilityType {
pure,
view,
nonpayable,
payable
}

public enum FunctionType {
constructor,
function,
Expand All @@ -101,6 +108,7 @@ public static class Function {
public Param[] inputs = new Param[0];
public Param[] outputs = new Param[0];
public FunctionType type;
public StateMutabilityType stateMutability;

private Function() {}

Expand Down
124 changes: 99 additions & 25 deletions ethereumj-core/src/main/java/org/ethereum/solidity/compiler/SolidityCompiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,24 @@
*/
package org.ethereum.solidity.compiler;

import com.google.common.base.Joiner;
import org.ethereum.config.SystemProperties;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

import java.io.*;
import java.io.BufferedOutputStream;
import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.Serializable;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import static java.util.stream.Collectors.toList;

@Component
public class SolidityCompiler {

Expand All @@ -38,38 +47,96 @@ public SolidityCompiler(SystemProperties config) {
solc = new Solc(config);
}

public static Result compile(File sourceDirectory, boolean combinedJson, Options... options) throws IOException {
public static Result compile(File sourceDirectory, boolean combinedJson, Option... options) throws IOException {
return getInstance().compileSrc(sourceDirectory, false, combinedJson, options);
}

public enum Options {
public static final class Options {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

public static final OutputOption AST = OutputOption.AST;
public static final OutputOption BIN = OutputOption.BIN;
public static final OutputOption INTERFACE = OutputOption.INTERFACE;
public static final OutputOption ABI = OutputOption.ABI;
public static final OutputOption METADATA = OutputOption.METADATA;
public static final OutputOption ASTJSON = OutputOption.ASTJSON;
}

public interface Option extends Serializable {
String getValue();
String getName();
}

public static class ListOption implements Option {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private String name;
private List values;

public ListOption(String name, List values) {
this.name = name;
this.values = values;
}

@Override public String getValue() {
StringBuilder result = new StringBuilder();
for (Object value : values) {
if (OutputOption.class.isAssignableFrom(value.getClass())) {
result.append((result.length() == 0) ? ((OutputOption) value).getName() : ',' + ((OutputOption) value).getName());
} else if (Path.class.isAssignableFrom(value.getClass())) {
result.append((result.length() == 0) ? ((Path) value).toAbsolutePath().toString() : ',' + ((Path) value).toAbsolutePath().toString());
} else if (File.class.isAssignableFrom(value.getClass())) {
result.append((result.length() == 0) ? ((File) value).getAbsolutePath() : ',' + ((File) value).getAbsolutePath());
} else if (String.class.isAssignableFrom(value.getClass())) {
result.append((result.length() == 0) ? value : "," + value);
} else {
throw new UnsupportedOperationException("Unexpected type, value '" + value + "' cannot be retrieved.");
}
}
return result.toString();
}
@Override public String getName() { return name; }
@Override public String toString() { return name; }
}

private enum NameOnlyOption implements Option {
OPTIMIZE("optimize"),
VERSION("version");

private String name;

NameOnlyOption(String name) {
this.name = name;
}

@Override public String getValue() { return ""; }
@Override public String getName() { return name; }
@Override public String toString() {
return name;
}
}

private enum OutputOption implements Option {
AST("ast"),
BIN("bin"),
INTERFACE("interface"),
ABI("abi"),
METADATA("metadata"),
METADATA("metadata"),
ASTJSON("ast-json");

private String name;

Options(String name) {
OutputOption(String name) {
this.name = name;
}

public String getName() {
return name;
}

@Override
public String toString() {
@Override public String getValue() { return ""; }
@Override public String getName() { return name; }
@Override public String toString() {
return name;
}
}

public static class Result {
public String errors;
public String output;
private boolean success = false;
private boolean success;

public Result(String errors, String output, boolean success) {
this.errors = errors;
Expand Down Expand Up @@ -125,11 +192,11 @@ public void run() {
}
}

public static Result compile(byte[] source, boolean combinedJson, Options... options) throws IOException {
public static Result compile(byte[] source, boolean combinedJson, Option... options) throws IOException {
return getInstance().compileSrc(source, false, combinedJson, options);
}

public Result compileSrc(File source, boolean optimize, boolean combinedJson, Options... options) throws IOException {
public Result compileSrc(File source, boolean optimize, boolean combinedJson, Option... options) throws IOException {
List<String> commandParts = prepareCommandOptions(optimize, combinedJson, options);

commandParts.add(source.getAbsolutePath());
Expand All @@ -156,24 +223,33 @@ public Result compileSrc(File source, boolean optimize, boolean combinedJson, Op
return new Result(error.getContent(), output.getContent(), success);
}

private List<String> prepareCommandOptions(boolean optimize, boolean combinedJson, Options[] options) throws IOException {
private List<String> prepareCommandOptions(boolean optimize, boolean combinedJson, Option... options) throws IOException {
List<String> commandParts = new ArrayList<>();
commandParts.add(solc.getExecutable().getCanonicalPath());
if (optimize) {
commandParts.add("--optimize");
commandParts.add("--" + NameOnlyOption.OPTIMIZE.getName());
}
if (combinedJson) {
commandParts.add("--combined-json");
commandParts.add(Joiner.on(',').join(options));
ListOption combinedJsonOption = new ListOption("combined-json", getElementsOf(OutputOption.class, options));
commandParts.add("--" + combinedJsonOption.getName());
commandParts.add(combinedJsonOption.getValue());
} else {
for (Options option : options) {
for (Option option : getElementsOf(OutputOption.class, options)) {
commandParts.add("--" + option.getName());
}
}
for (Option option : getElementsOf(ListOption.class, options)) {
commandParts.add("--" + option.getName());
commandParts.add(option.getValue());
}
return commandParts;
}

public Result compileSrc(byte[] source, boolean optimize, boolean combinedJson, Options... options) throws IOException {
private static <T> List<T> getElementsOf(Class<T> clazz, Option... options) {
return Arrays.stream(options).filter(clazz::isInstance).map(clazz::cast).collect(toList());
}

public Result compileSrc(byte[] source, boolean optimize, boolean combinedJson, Option... options) throws IOException {
List<String> commandParts = prepareCommandOptions(optimize, combinedJson, options);

ProcessBuilder processBuilder = new ProcessBuilder(commandParts)
Expand Down Expand Up @@ -205,7 +281,7 @@ public Result compileSrc(byte[] source, boolean optimize, boolean combinedJson,
public static String runGetVersionOutput() throws IOException {
List<String> commandParts = new ArrayList<>();
commandParts.add(getInstance().solc.getExecutable().getCanonicalPath());
commandParts.add("--version");
commandParts.add("--" + NameOnlyOption.VERSION.getName());

ProcessBuilder processBuilder = new ProcessBuilder(commandParts)
.directory(getInstance().solc.getExecutable().getParentFile());
Expand All @@ -231,12 +307,10 @@ public static String runGetVersionOutput() throws IOException {
throw new RuntimeException("Problem getting solc version: " + error.getContent());
}



public static SolidityCompiler getInstance() {
if (INSTANCE == null) {
INSTANCE = new SolidityCompiler(SystemProperties.getDefault());
}
return INSTANCE;
}
}
}
28 changes: 14 additions & 14 deletions ethereumj-core/src/test/java/org/ethereum/core/ImportLightTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Collaborator

@zilm13 zilm13 Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@zilm13 zilm13 Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, what I suggest:

  1. Adding public String path into ContractMetadata. If there is anything on first line before :, it's saved there (and removed from contract name)
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Block b1 = bc.createBlock();
Block b2 = bc.createBlock();
parent.callFunction("createChild", 100);
Block b3 = bc.createBlock();
byte[] childAddress = (byte[]) parent.callConstFunction("child")[0];
parent.callFunction("createChild", 200);
Block b2_ = bc.createForkBlock(b1);
SolidityContract child = bc.createExistingContractFromSrc(contractSrc, "Child", childAddress);
SolidityContract child = bc.createExistingContractFromSrc(contractSrc, "<stdin>:Child", childAddress);
child.callFunction("sum");
Block b4 = bc.createBlock();
Assert.assertEquals(BigInteger.valueOf(100 + 333 + 100 + 444), child.callConstFunction("c")[0]);
Expand All @@ -407,9 +407,9 @@ public void createContractFork1() throws Exception {
StandaloneBlockchain bc = new StandaloneBlockchain();
Block b1 = bc.createBlock();
Block b2 = bc.createBlock();
SolidityContract a = bc.submitNewContract(contractSrc, "A");
SolidityContract a = bc.submitNewContract(contractSrc, "<stdin>:A");
Block b3 = bc.createBlock();
SolidityContract b = bc.submitNewContract(contractSrc, "B");
SolidityContract b = bc.submitNewContract(contractSrc, "<stdin>:B");
Block b2_ = bc.createForkBlock(b1);
Assert.assertEquals(BigInteger.valueOf(333), a.callConstFunction("a")[0]);
Assert.assertEquals(BigInteger.valueOf(111), b.callConstFunction(b2_, "a")[0]);
Expand Down Expand Up @@ -441,11 +441,11 @@ public void createValueTest() throws IOException, InterruptedException {
" }\n" +
"}";
StandaloneBlockchain bc = new StandaloneBlockchain().withAutoblock(true);
SolidityContract a = bc.submitNewContract(contract, "A");
SolidityContract a = bc.submitNewContract(contract, "<stdin>:A");
bc.sendEther(a.getAddress(), BigInteger.valueOf(10_000));
a.callFunction(10, "create");
byte[] childAddress = (byte[]) a.callConstFunction("child")[0];
SolidityContract b = bc.createExistingContractFromSrc(contract, "B", childAddress);
SolidityContract b = bc.createExistingContractFromSrc(contract, "<stdin>:B", childAddress);
BigInteger val = (BigInteger) b.callConstFunction("valReceived")[0];
Assert.assertEquals(20, val.longValue());
}
Expand Down Expand Up @@ -502,7 +502,7 @@ public void operateNotExistingContractTest() throws IOException, InterruptedExce
StandaloneBlockchain bc = new StandaloneBlockchain()
.withGasPrice(1)
.withGasLimit(5_000_000L);
SolidityContract a = bc.submitNewContract(contractA, "A");
SolidityContract a = bc.submitNewContract(contractA, "<stdin>:A");
bc.createBlock();

{
Expand All @@ -514,7 +514,7 @@ public void operateNotExistingContractTest() throws IOException, InterruptedExce

// checking balance of not existed address should take
// less that gas limit
Assert.assertEquals(21508, spent);
Assert.assertEquals(21532, spent);
}

{
Expand Down Expand Up @@ -558,7 +558,7 @@ public void deepRecursionTest() throws Exception {
"}";

StandaloneBlockchain bc = new StandaloneBlockchain().withGasLimit(5_000_000);
SolidityContract a = bc.submitNewContract(contractA, "A");
SolidityContract a = bc.submitNewContract(contractA, "<stdin>:A");
bc.createBlock();
a.callFunction("recursive");
bc.createBlock();
Expand Down Expand Up @@ -647,7 +647,7 @@ public void selfdestructAttack() throws Exception {
StandaloneBlockchain bc = new StandaloneBlockchain()
.withGasLimit(1_000_000_000L)
.withDbDelay(0);
SolidityContract a = bc.submitNewContract(contractSrc, "A");
SolidityContract a = bc.submitNewContract(contractSrc, "<stdin>:A");
bc.createBlock();
a.callFunction("f");
bc.createBlock();
Expand Down Expand Up @@ -770,7 +770,7 @@ public void suicideInFailedCall() throws Exception {
"}";

StandaloneBlockchain bc = new StandaloneBlockchain().withGasLimit(5_000_000);
SolidityContract a = bc.submitNewContract(contractA, "A");
SolidityContract a = bc.submitNewContract(contractA, "<stdin>:A");
bc.createBlock();
final BigInteger[] refund = new BigInteger[1];
bc.addEthereumListener(new EthereumListenerAdapter() {
Expand Down Expand Up @@ -803,7 +803,7 @@ public void logInFailedCall() throws Exception {
"}";

StandaloneBlockchain bc = new StandaloneBlockchain().withGasLimit(5_000_000);
SolidityContract a = bc.submitNewContract(contractA, "A");
SolidityContract a = bc.submitNewContract(contractA, "<stdin>:A");
bc.createBlock();
final List<LogInfo> logs = new ArrayList<>();
bc.addEthereumListener(new EthereumListenerAdapter() {
Expand Down Expand Up @@ -831,14 +831,14 @@ public void ecRecoverTest() throws Exception {
" mstore(0x120, v)" +
" mstore(0x140, r)" +
" mstore(0x160, s)" +
" callcode(0x50000, 0x01, 0x0, 0x100, 0x80, 0x200, 0x220)" + // call ecrecover
" let ret := callcode(0x50000, 0x01, 0x0, 0x100, 0x80, 0x200, 0x220)" + // call ecrecover
" return(0x200, 0x20)" +
" }" +
" }" +
"}";

StandaloneBlockchain bc = new StandaloneBlockchain().withGasLimit(5_000_000);
SolidityContract a = bc.submitNewContract(contractA, "A");
SolidityContract a = bc.submitNewContract(contractA, "<stdin>:A");
bc.createBlock();

ECKey key = ECKey.fromPrivate(BigInteger.ONE);
Expand Down
8 changes: 4 additions & 4 deletions ethereumj-core/src/test/java/org/ethereum/core/TransactionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,14 +576,14 @@ public void multiSuicideTest() throws IOException, InterruptedException {
ECKey sender = ECKey.fromPrivate(Hex.decode("3ec771c31cac8c0dba77a69e503765701d3c2bb62435888d4ffa38fed60c445c")).compress();
System.out.println("address: " + Hex.toHexString(sender.getAddress()));

if (cres.contracts.get("PsychoKiller") != null) {
if (cres.contracts.get("<stdin>:PsychoKiller") != null) {
Transaction tx = createTx(blockchain, sender, new byte[0],
Hex.decode(cres.contracts.get("PsychoKiller").bin));
Hex.decode(cres.contracts.get("<stdin>:PsychoKiller").bin));
executeTransaction(blockchain, tx);

byte[] contractAddress = tx.getContractAddress();

CallTransaction.Contract contract1 = new CallTransaction.Contract(cres.contracts.get("PsychoKiller").abi);
CallTransaction.Contract contract1 = new CallTransaction.Contract(cres.contracts.get("<stdin>:PsychoKiller").abi);
byte[] callData = contract1.getByName("multipleHomocide").encode();

Transaction tx1 = createTx(blockchain, sender, contractAddress, callData, 0l);
Expand Down Expand Up @@ -662,7 +662,7 @@ public void receiptErrorTest() throws Exception {
contract.getBytes(), true, SolidityCompiler.Options.ABI, SolidityCompiler.Options.BIN);
System.out.println(res.errors);
CompilationResult cres = CompilationResult.parse(res.output);
Transaction tx = createTx(blockchain, sender, new byte[0], Hex.decode(cres.contracts.get("GasConsumer").bin), 0);
Transaction tx = createTx(blockchain, sender, new byte[0], Hex.decode(cres.contracts.get("<stdin>:GasConsumer").bin), 0);
TransactionReceipt receipt = executeTransaction(blockchain, tx).getReceipt();
receipt = new TransactionReceipt(receipt.getEncoded());

Expand Down
Loading