Fix and test for duplicated fields (#680)

[kostind]

What does this PR do?

Changes SolidityFunctionWrapper in order to don't use upper case for function names which will be duplicated.

Where should the reviewer start?

SolidityFunctionWrapper
SolidityFunctionWrapperGeneratorTest

Why is it needed?

To fix bug with code generation for contracts with duplicate fields.

Extra

Java class generated from DuplicateField contract will have the following constants:
FUNC_name
FUNC_DECIMALS
FUNC_Name
FUNC_NAME
FUNC_SYMBOL

[iikirilov]

Thanks for the submission but I disagree with the approach. I don't think the issue is with web3j. It is more an issue of the naming convention from the Solidity developer. I think it is bad practice to have the same name with a different case for different variables.

I would suggest that the Solidity code is modified e.g. DEFAULT_NAME and Name rather than NAME and Name for variable names.

[kostind]

I completely agree with you about the naming convention.
The issue was marked as a bug, so I've tried to solve it and investigate code generation in web3j.

[iikirilov]

fixes #680

[iikirilov]

@kostind thanks for taking a look - I did not understand the issue but with your code it is obvious :) I will get a colleague to review this and give their opinion.

[kostind]

@iikirilov I'm happy to help :-) Thanks.

[antonydenyer]

I guess the more interesting question is as a user of web3j what do you expect to happen. Should we keep to the normal Java naming conventions or bend them to match Solidity?

Personally, I think we should keep the naming convention we have but throw a compile error when you have duplicates.

[kostind]

I usually avoid such situation with function/variable names.
An error can be thrown in case of duplicated fields (!duplicateFunctionNames.isEmpty()), it's very simple to change.

The only concern here if we need to interact with a smart contract which has already been deployed and can't be upgraded.

[antonydenyer]

I usually avoid such situation with function/variable names.
An error can be thrown in case of duplicated fields (!duplicateFunctionNames.isEmpty()), it's very simple to change.

I think that's the way to go with this.

The only concern here if we need to interact with a smart contract which has already been deployed and can't be upgraded.

That's true, I think what would be nice would be a halfway house between a fully generated wrapper and eth.call. Perhaps we could have something like contract.call("MethodName", args) ?

[kostind]

That's true, I think what would be nice would be a halfway house between a fully generated wrapper and eth.call. Perhaps we could have something like contract.call("MethodName", args) ?

Did you mean that we can always build methods for working with duplicated functions by hand and don't use generated wrappers for them?
Something like:

 public RemoteCall<String> NAME() {
        final Function function = new Function("NAME", 
                Arrays.<Type>asList(), 
                Arrays.<TypeReference<?>>asList(new TypeReference<Utf8String>() {}));
        return executeRemoteCallSingleValueReturn(function, String.class);
    }

[antonydenyer]

The problem is that executeRemoteCallSingleValueReturn is protected so you would need to extend your generated class. Just wondering if there's something nice we can. Perhaps not.

[antonydenyer]

With this PR I think we should prevent code generation for anything that will cause a clash. When clashes are found output a warning to the user.

[kostind]

With this PR I think we should prevent code generation for anything that will cause a clash. When clashes are found output a warning to the user.

Thank you for your suggestions.
I'm going to update it next week.
I will also change testDuplicateField in order to read a message from System.err and probably prevent System.exit.

[iikirilov]

The only concern here if we need to interact with a smart contract which has already been deployed and can't be upgraded.

Good point.

Just to clarify the resolution is to successfully generate a wrapper without the duplicate fields (i.e. it must compile) and to output warnings on the command line for the user.

[kostind]

@antonydenyer Thank you for the approval.
@iikirilov Please take a look.